-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
added support for XEP-0030 with tests #736
base: master
Are you sure you want to change the base?
added support for XEP-0030 with tests #736
Conversation
24bd4cf
to
46ff0b0
Compare
@andresinaka This looks good, however, I wonder if this is duplicate functionality from the XMPPCapabilities module
Which internally calls
It looks like according to XEP-0115 it should be used instead of XEP-0030 to prevent the "disco/version flood", although in this case the stanzas look nearly identical so I'm not sure how it differs unless the hashed capabilities storage is used. |
@chrisballinger I totally agree with you! I added the disco#info to this because I needed to disco#items and I couldn't find a way of doing it. So I googled and found that XEP-0030 so I implemented the whole thing but I think the only missing feature is the disco#items. Is there an other way to find out the items from a server?? (this is what i needed: Maybe I can add this to some other place? What do you think? (btw, don't merge this as is because I found a little but, but first let's discuss about info#items) edit: I'm rethinking this and i think it makes no sense at all |
Ahh I see, the #items thing is what you really need. It would be kind of convoluted to use XMPPCapabilities for just the #info part within the XEP-0030 module, and #items is definitely not part of XEP-0115 so it doesn't really belong over there either. I guess just a mention in the docs that if you're just discovering capabilities you might want to use XMPPCapabilities instead because hashed caps are faster. |
Instead of a full module perhaps you could make a category on |
@chrisballinger thanks! I think the 👍 👍 👍 👍 👍 👍 👍 |
@chrisballinger I'm back to this. So I created this category that has a method to create the iq for the disco items and a method that parses the response to get the items. What do you think? |
bd99f21
to
ca5f449
Compare
NSXMLElement *query = [iq elementForName:@"query" xmlns: XMLNS_DISCO_ITEMS]; | ||
if(query) { | ||
return [query elementsForName:@"item"]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if !query? Also violates nonnull
Oh interesting... looks like this is already sort-of implemented inside the file transfer module, albeit tied to the internals:
I now see how it could be useful as a module because of the PID tracker, but the standalone IQ parser/generators are still a good thing. Hmmmmmm. |
@chrisballinger I like the module solution more because of the PID tracker too. Also we would have the whole XEP-0030 implemented. Let me know what you prefer, if you think the module is better I'll reset hard (i know it's not happy but reverting is going to add an ugly commit and I'm pretty sure I'm the only one working on this) to the module work, fix an small bug and PR it again. If you think is better to go with the category I'll fix those problems you found and let you know... |
Given more thought the module seems better especially if it will be growing in size and using the PID tracker, but I'd still think keeping the IQ parser/generators separate would still be good and you could keep the tests you already wrote. |
Perfect! I'll do that. I like the idea of keeping the parser/generators separate! Thanks!! |
I fixed a few warnings and nullability issues, and discovered some OS X issues as well: 991b5dc Please check that there's no warnings or test failures for both iOS and the new macOS test projects. Thanks! |
ca5f449
to
12db9b7
Compare
No description provided.