XMPPFramework icon indicating copy to clipboard operation
XMPPFramework copied to clipboard

"MUC light" implementation

Open Nyco opened this issue 8 years ago • 9 comments

We (Erlang Solutions, makers of the MongooseIM XMPP server) would like to contribute "MUC light".

Proposed version: http://xmpp.org/extensions/inbox/muc-light.html

Latest version: https://github.com/fenek/xeps/blob/muc_light/inbox/muc-light.xml

Doc: http://mongooseim.readthedocs.io/en/latest/developers-guide/mod_muc_light_developers_guide/ http://mongooseim.readthedocs.io/en/latest/modules/mod_muc_light/

Here is preliminary implementation, covering only the basic use cases: https://github.com/robbiehanson/XMPPFramework/compare/master...esl:andres.XMPPMUCLight?expand=1 We are expanding this implementation to cover all the use cases.

What do you think of this?

Nyco avatar Jun 10 '16 12:06 Nyco

So I created XMPPRoomLight and XMPPMUCLight.

  • XMPPRoomLight handles sending/receiving messages, creating a room, fetching the members list, leaving a room and adding users to the room. All the basic actions.
  • XMPPMUCLight monitors the active XMPPRoomLight instances, discover rooms, and listens for MUC light affiliation changes.

Also I added a the same persistent layer as the one implemented for XMPPRoom, I created a XMPPRoomLightCoreDataStorage that knows how to handle incoming and outgoing messages. Also I created a custom XMPPRoomLight core data model to store the messages.

A lot of the code from XMPPRoomLightCoreDataStorage is similar/the same as the one in XMPPRoomCoreDataStorage but I thought that creating a different class was better because MUC Light is way simpler so I thought that having a shorter class that just encapsulates the behaviour of MUC Light was better than trying to reuse XMPPRoomCoreDataStorage and start mixing concepts. But let me know what's your thought on this...

tl;dr: I didn't touch XMPPMUC, XMPPRoom and all the other classes that currently implement XEP-0045

andresinaka avatar Jun 10 '16 13:06 andresinaka

Overall it's looking pretty good to me! Some small typos I noticed on the first pass:

  • +- (void)xmppRoomLight:(XMPPRoomLight *)sender didCreatRoomLight:(XMPPIQ *)iq;
  • +#import "XMPPRoomLightCoreDataSTorage.h"

Some other improvements:

  • I would rename jid to roomJID to be consistent with the method below it: +- (id)initWithJID:(XMPPJID *)jid roomname:(NSString *) roomname;
  • Add some general header documentation about how this differs from traditional MUC, server support, links to the proto XEP, etc.
  • Bonus: Use instancetype, nullability annotations, and Obj-C generics where possible.
  • +@property(nonatomic, strong, readonly) NSMutableSet *rooms; there could be threading issues by exposing this as a nonatomic mutable set. It looks like this should never be mutated by a consumer of the class, and it may be mutated by multiple threads internally by - (void)xmppStream:(XMPPStream *)sender didRegisterModule:(id)module { and unregister. These mutations should probably be done on the moduleQueue, and replacing the public property with an accessor method that returns an NSSet copy that synchronizes access to the object on the moduleQueue.

chrisballinger avatar Jun 10 '16 17:06 chrisballinger

@chrisballinger I worked on the typos and on the other improvements, I'm not a 100% sure if my changes are doing what you meant in the last bullet, if you can confirm that's what you were thinking of would be great.

andresinaka avatar Jun 13 '16 19:06 andresinaka

Is rooms meant to be publicly accessible and mutable to other modules or classes that may be operating on different threads? Although it would be an edge case, if two threads mutate the object at the same time, or one is reading while another is mutating, it may cause a crash.

chrisballinger avatar Jun 13 '16 21:06 chrisballinger

The idea of rooms is to be publicly accesible but not mutable, it can only be mutated by didRegisterModule and willUnregisterModule.

`if two threads mutate the object at the same time, or one is reading while another is mutating, it may cause a crash.

Do you think this is still possible? As now adding and removing objects from rooms is now dispatched dispatch_async(moduleQueue, block);, so it would be only changed in the same thread right?

andresinaka avatar Jun 14 '16 14:06 andresinaka

Looks good! I think your changes should prevent any issues and makes the intention of the API more clear.

Ready for a PR?

chrisballinger avatar Jun 14 '16 15:06 chrisballinger

hey thanks for checking it out so fast!! I'm working on adding the missing functionalities to XMPPRoom like destroyRoom, changeSubject and more... do you think is better to wait for a BIG PR with all the goodies implemented or just merge this and PR again when I finish adding stuff?

andresinaka avatar Jun 14 '16 20:06 andresinaka

Probably best to finish it all and submit a big PR to avoid potential confusion about a partial implementation

chrisballinger avatar Jun 16 '16 19:06 chrisballinger

Is Muc Light implementation complete?

niqt avatar Jun 28 '20 07:06 niqt