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

Multi-User Chat Light (v0.1.0) implementation #81

Closed
wants to merge 1 commit into from

Conversation

ramabit
Copy link
Contributor

@ramabit ramabit commented May 30, 2016

Multi-User Chat Light (v0.1.0) documentation: http://mongooseim.readthedocs.io/en/latest/open-extensions/xeps/xep-muc-light.html

All modules are implemented :)

@ramabit ramabit changed the title Ramabit.muclight Multi-User Chat Light (v0.1.0) implementation May 30, 2016
@Flowdalic
Copy link
Member

Thanks for your contribution. I will work the PRs in FIFO order. But I already noticed that you placed this in smack-extensions while it should be in smack-experimental.

@ramabit
Copy link
Contributor Author

ramabit commented Jun 3, 2016

@Flowdalic Everything moved to smack-experimental

@ramabit ramabit force-pushed the ramabit.muclight branch 3 times, most recently from cc30088 to be195c9 Compare June 8, 2016 18:31
@ramabit
Copy link
Contributor Author

ramabit commented Jun 14, 2016

@Flowdalic In the following days I will be updating this PR implementing the missing functionalities until MUC Light be fully implemented

@ramabit ramabit force-pushed the ramabit.muclight branch 3 times, most recently from 3355664 to 80327e5 Compare June 27, 2016 19:40
@ramabit
Copy link
Contributor Author

ramabit commented Jun 27, 2016

@Flowdalic now the MUC Light implementation is complete

@ramabit
Copy link
Contributor Author

ramabit commented Jul 14, 2016

Detailed documentation added: documentation/extensions/muclight.md

@@ -85,6 +85,7 @@ Experimental Smack Extensions and currently supported XEPs of smack-experimental
| [Internet of Things - Discovery](iot.md) | [XEP-0347](http://xmpp.org/extensions/xep-0347.html) | Describes how Things can be installed and discovered by their owners. |
| Google GCM JSON payload | n/a | Semantically the same as XEP-0335: JSON Containers |
| Client State Indication | [XEP-0352](http://xmpp.org/extensions/xep-0352.html) | A way for the client to indicate its active/inactive state. |
| Multi-User Chat Light | [XEP-xxxx]( https://s3.amazonaws.com/uploads.hipchat.com/15025/1268415/d9ByVbihpLmzxBI/XEP-xxxx.Multi.User.Chat.Light.pdf) | Multi-User Chats for mobile XMPP applications and specific enviroment. |
Copy link
Member

Choose a reason for hiding this comment

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

Can't this point to the XSF's inbox/? Or some other official place where MUC light is provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am investigating that

@Flowdalic
Copy link
Member

Needs conflict resolution

} catch (NoResponseException | XMPPErrorException e) {
removeConnectionCallbacks();
return false;
} catch (InterruptedException e) {
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this part of the multi-catch above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

.discoverItems(connection().getServiceName());
boolean found = false;
for (DiscoverItems.Item item : discoverItems.getItems()) {
if (item.getEntityID().equals(mucLightService)) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry, but what are you doing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Flowdalic Checking if the service received as a parameter is supported by the server (retrieved in discover items)

Copy link
Member

@Flowdalic Flowdalic Jul 30, 2016

Choose a reason for hiding this comment

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

That's not very helpful, please remove this method. If you already have a DomainBareJid then you could just perform a disco#info on it and determine if it announces the muc-light feature. A method

public DomainBareJid discoverService()

which returns the DomainBareJid of the local muc-light service or null if none was found, would be of much more useful.

Copy link
Contributor Author

@ramabit ramabit Aug 1, 2016

Choose a reason for hiding this comment

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

@Flowdalic That method you mention is already implemented too:
public List<DomainBareJid> getXMPPServiceDomains()
Should I remove the method isServiceEnabled anyway?

Copy link
Member

Choose a reason for hiding this comment

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

Ahh good point, then simply remove it.

Also I suggest to

  • s/isFeatureSupportedByServer(DomainBareJid)/isFeatureSupported(DomainBareJid)/
  • s/getXMPPServiceDomains()/getLocalServices()/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Flowdalic
Copy link
Member

I've made a few comments. Please address the issues and append new commits, so it's easier to track which and how they are fixed. I'll tell you when it's time for another round of squashing.

@ramabit
Copy link
Contributor Author

ramabit commented Jul 29, 2016

@Flowdalic Fixes done
The XSF's inbox/ has a previous version (https://xmpp.org/extensions/inbox/muc-light.html v0.0.1). I implemented the v0.1.0. I am trying to upload that XEP somewhere

@Flowdalic
Copy link
Member

The XSF's inbox/ has a previous version (https://xmpp.org/extensions/inbox/muc-light.html v0.0.1). I implemented the v0.1.0. I am trying to upload that XEP somewhere

Well that somewhere somehow scares me. I mean there must be someone feeling responsible for the MUC light development, even if it was not accepted as experimental XEP by the XSF. And that person/entity should make the current state of the XEP official available in some canonical way (and not e.g. an S3 upload).

@ramabit
Copy link
Contributor Author

ramabit commented Jul 29, 2016

Well that somewhere somehow scares me. I mean there must be someone feeling responsible for the MUC light development, even if it was not accepted as experimental XEP by the XSF. And that person/entity should make the current state of the XEP official available in some canonical way (and not e.g. an S3 upload).

@Flowdalic I work for the same company as the responsible of the XEP and we are trying to host it

@ramabit
Copy link
Contributor Author

ramabit commented Jul 29, 2016

@Flowdalic MUC Light XEP hosted ^

@Flowdalic
Copy link
Member

It appears that this PR does not yet register a single provider for MUC light. Please change that.

@ramabit
Copy link
Contributor Author

ramabit commented Nov 1, 2016

@Flowdalic
Copy link
Member

Ahh, right sorry. Squash pls, then it's ready to merge. I'm a little bit worried regarding code duplication with the MUC codebase, but I guess that's fine for now (and probably for the future).

@ramabit
Copy link
Contributor Author

ramabit commented Nov 2, 2016

@Flowdalic Squash done

@ramabit ramabit closed this Nov 7, 2016
@ramabit ramabit reopened this Nov 7, 2016
@Flowdalic
Copy link
Member

Cherry picked as 5372c1b
Assigned SMACK-740

Thanks! :)

@Flowdalic Flowdalic closed this Nov 15, 2016
@ramabit ramabit deleted the ramabit.muclight branch November 17, 2016 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants