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

added support for XEP-0030 with tests #736

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

andresinaka
Copy link
Contributor

No description provided.

@andresinaka andresinaka force-pushed the andres.XEP-0030ServiceDiscovery branch from 24bd4cf to 46ff0b0 Compare May 27, 2016 15:44
@chrisballinger
Copy link
Collaborator

chrisballinger commented May 27, 2016

@andresinaka This looks good, however, I wonder if this is duplicate functionality from the XMPPCapabilities module - (void)fetchCapabilitiesForJID:(XMPPJID *)jid;

- (void)fetchCapabilitiesForJID:(XMPPJID *)jid;

Which internally calls - (void)sendDiscoInfoQueryTo:(XMPPJID *)jid withNode:(NSString *)node ver:(NSString *)ver

- (void)sendDiscoInfoQueryTo:(XMPPJID *)jid withNode:(NSString *)node ver:(NSString *)ver

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.

@andresinaka
Copy link
Contributor Author

andresinaka commented May 27, 2016

@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: [<item jid="muc.my-server.com"/>, <item jid="muclight.my-server.com"/>, <item jid="pubsub.my-server.com"/>, <item jid="vjud.my-server.com"/>]

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

@chrisballinger
Copy link
Collaborator

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.

@chrisballinger
Copy link
Collaborator

Instead of a full module perhaps you could make a category on XMPPIQ+XEP_0030.h to handle generation and parsing of #items responses and defer the work to the client implementation.

@andresinaka
Copy link
Contributor Author

@chrisballinger thanks! I think the XMPPIQ+XEP_0030.h makes way more sense than adding the full XEP-0030! I'll work on that when I have some time!

👍 👍 👍 👍 👍 👍 👍

@andresinaka
Copy link
Contributor Author

@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?

@andresinaka andresinaka reopened this Jul 7, 2016
@andresinaka andresinaka force-pushed the andres.XEP-0030ServiceDiscovery branch 2 times, most recently from bd99f21 to ca5f449 Compare July 7, 2016 20:19
NSXMLElement *query = [iq elementForName:@"query" xmlns: XMLNS_DISCO_ITEMS];
if(query) {
return [query elementsForName:@"item"];
}
Copy link
Collaborator

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

@chrisballinger
Copy link
Collaborator

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.

@andresinaka
Copy link
Contributor Author

@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...

@chrisballinger
Copy link
Collaborator

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.

@andresinaka
Copy link
Contributor Author

Perfect! I'll do that. I like the idea of keeping the parser/generators separate! Thanks!!

@chrisballinger
Copy link
Collaborator

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!

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.

2 participants