Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor to add 3.4 Multiple Items in Form Results #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LightZam
Copy link
Contributor

@LightZam LightZam commented Jun 6, 2015

XEP-0004, 3.4 Multiple Items in Form Results
http://xmpp.org/extensions/xep-0004.html#protocol-results

refactor :
add Fileld::parse to refactor QXmppDataForm::parse
add Fileld::toXml to refactor QXmppDataForm::toXml
add Media::parse to refactor QXmppDataForm::parse
add Media::toXml to refactor QXmppDataForm::toXml

new feature :
add Item, reported class to achieve XEP-0004 3.4
unit test for XEP-0004 3.4 Multiple Items in Form Results

@jlaine
Copy link
Contributor

jlaine commented Jun 7, 2015

On the whole this looks good, though I will need to read it more in depth.

Georg: any thoughts on the non-const accessors? I understand the motivation, but we do not use them much in the rest of the API.

@LightZam
Copy link
Contributor Author

LightZam commented Jun 7, 2015

but it will be needed in some of XEP Protocol like XEP-0055 Jabber Search

@LightZam
Copy link
Contributor Author

LightZam commented Sep 1, 2016

it might not be super useful for other API,
but this PR is no harm for project, and it can be use on other protocol in the futures.
As I said, XEP-0055 Jabber Search was an example, and i will implement it.

ref: https://xmpp.org/extensions/xep-0055.html

QSharedDataPointer<QXmppDataFormItemPrivate> d;
};

class QXMPP_EXPORT Reported
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not entirely sure I understand the purpose of the Item / Reported classes. Unless I am mistaken, they both seem to describe a list/vector of Fields?

Copy link
Contributor Author

@LightZam LightZam Sep 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, both of them described a list of fields, the difference between them is reported stand for "table haed" and item stand for "talbe cell".

reference:
the definition
http://xmpp.org/extensions/xep-0004.html#protocol-results
the example
https://xmpp.org/extensions/xep-0055.html#example-9

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I guess my question was: do we need two separate classes for this?

Copy link
Contributor Author

@LightZam LightZam Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a little different xml tag name and element between reported, item

if you would like it to use only item, it is possible, but there will be some specific logic for this kind purpose. it looks wired to me.

like below

 "<x xmlns=\"jabber:x:data\" type=\"result\">"
            "<reported>"
                "<field type=\"jid-single\" label=\"JID\" var=\"jid\"/>"
                "<field type=\"text-single\" label=\"Username\" var=\"Username\"/>"
                "<field type=\"text-single\" label=\"Name\" var=\"Name\"/>"
                "<field type=\"text-single\" label=\"Email\" var=\"Email\"/>"
            "</reported>"
            "<item>"
                "<field type=\"text-single\" var=\"jid\">"
                    "<value>[email protected]</value>"
                "</field>"
                "<field type=\"text-single\" var=\"Username\">"
                    "<value>Zam</value>"
                "</field>"
                "<field type=\"text-single\" var=\"Name\">"
                    "<value>Zam</value>"
                "</field>"
                "<field type=\"text-single\" var=\"Email\">"
                    "<value>[email protected]</value>"
                "</field>"
            "</item>"
        "</x>"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jlaine
if you are having any considering or suggestion, please shoot to me.
it would be great to have that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK so "reported" has "label" but no "value", right? Let's keep the classes separate then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it is

@jlaine jlaine force-pushed the master branch 4 times, most recently from 81b12d9 to cd81054 Compare June 24, 2018 08:26
@jlaine
Copy link
Contributor

jlaine commented Jan 3, 2019

Could you please address my question: can't we use "Item" class for the "reported" member ?

@tehnick
Copy link
Member

tehnick commented Jan 3, 2019

Georg: any thoughts on the non-const accessors? I understand the motivation, but we do not use them much in the rest of the API.

@0xd34df00d ^^^

@jlaine
Copy link
Contributor

jlaine commented Jan 15, 2019

Could we re-push this branch to get coverage to run please? Also does doc/xep.doc need updating?

@0xd34df00d
Copy link
Member

@tehnick re non-const getters — I'm generally fine with them if they're providing direct access to some d-proxied field (and if that field is not involved in any invariants of the class), but, indeed, the consistency with the rest of the API is also important. I'd probably drop them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants