Currently automatic reconnection of MUCs only works for IRC. This patch moves the reconnection to imConversations.js. To use this feature, protocols must store the data needed to rejoin a chat in a chatRoomFields variable, which should also be useful for session restore, when that is implemented. As a demo, XMPP MUCs are now reconnected.

I'm unsure whether the nullptr does the right thing here.

Comment on attachment 8426909 [details] [diff] [review] 1014472.patch Review of attachment 8426909 [details] [diff] [review]: ----------------------------------------------------------------- This is pretty awesome! I r+ed it assuming the answers to my comments are what they think they are, + 1 nit. ::: chat/modules/jsProtoHelper.jsm @@ +532,5 @@ > get isChat() true, > > + // Stores the prplIChatRoomFieldValues required to join this channel > + // to enable later reconnections. If absent, the MUC will not be reconnected > + // automatically after disconnections. I think you mean "If null, the MUC"... ::: chat/protocols/irc/ircBase.jsm @@ +344,5 @@ > // Check if any of our buddies are online! > this.sendIsOn(); > > + // Done! > + this.reportConnected(); Moving this is just because it makes sense to wait till the end of this function? I.e. Is this actually necessary or just more sane? ::: chat/protocols/irc/ircCommands.jsm @@ +207,1 @@ > // Otherwise, make use of it (e.g. if the user was kicked). This comment kind of makes me think we should be keeping track separately if we should be rejoining a room automatically or not. Any thoughts? (This would allow someone to /part a channel, NOT be automatically reconnected, but still just type /join on it.)

Oh and as Florian already said over IRC this should be a readonly property. :)

>> + // Done! >> + this.reportConnected(); > >Moving this is just because it makes sense to wait till the end of this >function? I.e. Is this actually necessary or just more sane? It's more sane, and it keeps the channel joins (which can be slow) happening at the end. >This comment kind of makes me think we should be keeping track separately if >we should be rejoining a room automatically or not. Any thoughts? (This would >allow someone to /part a channel, NOT be automatically reconnected, but still >just type /join on it.) But is this use case (which noone has ever complained about being absent) worth tracking yet another boolean?

(In reply to aleth [:aleth] from comment #6) > Created attachment 8427167 [details] [diff] [review] > 1014472.patch 2 > > >> + // Done! > >> + this.reportConnected(); > > > >Moving this is just because it makes sense to wait till the end of this >function? I.e. Is this actually necessary or just more sane? > > It's more sane, and it keeps the channel joins (which can be slow) happening > at the end. I agree, just wanted to ensure I understood! > >This comment kind of makes me think we should be keeping track separately if >we should be rejoining a room automatically or not. Any thoughts? (This would >allow someone to /part a channel, NOT be automatically reconnected, but still >just type /join on it.) > > But is this use case (which noone has ever complained about being absent) > worth tracking yet another boolean? Certainly not right now, just a thought. :)

By the way, I'd really like if we could figure out a smarter way to do this for libpurple. I, unfortunately, don't really know the libpurple API. But we could possible ask Florian or EionRobb for more information.

I had a conversation with EionRobb about this on IRC, I think that purple_chat_get_components from blist.h [1] will do what we want. Unfortunately this wants a PurpleChat (as opposed to a PurpleConvChat). Maybe using the new_node UI op would let us access this? {2] Not sure if any of this is helpful, but I was thinking we might be able to lazily get the information when it's requested instead of storing it up front (like we do for the JS prpls). [1] https://hg.mozilla.org/users/florian_queze.net/purple/file/6f9af63e1127/libpurple/blist.h#l1015 [2] https://hg.mozilla.org/users/florian_queze.net/purple/file/6f9af63e1127/purplexpcom/src/purpleInitContacts.cpp#l214 / http://lxr.instantbird.org/instantbird/source/purple/libpurple/blist.c#1338

Unbitrotted.

(In reply to Patrick Cloke [:clokep] from comment #9) > I had a conversation with EionRobb about this on IRC, I think that > purple_chat_get_components from blist.h [1] will do what we want. > Unfortunately this wants a PurpleChat (as opposed to a PurpleConvChat). > Maybe using the new_node UI op would let us access this? {2] Not sure if any > of this is helpful, but I was thinking we might be able to lazily get the > information when it's requested instead of storing it up front (like we do > for the JS prpls). > > [1] > https://hg.mozilla.org/users/florian_queze.net/purple/file/6f9af63e1127/ > libpurple/blist.h#l1015 > [2] > https://hg.mozilla.org/users/florian_queze.net/purple/file/6f9af63e1127/ > purplexpcom/src/purpleInitContacts.cpp#l214 / > http://lxr.instantbird.org/instantbird/source/purple/libpurple/blist.c#1338 At first glance, I don't think this can do what we want. But if we do come up with a clever way, we can do it in a followup.

Comment on attachment 8440750 [details] [diff] [review] 1014472.patch 3 Review of attachment 8440750 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/components/src/imConversations.js @@ +256,5 @@ > + let chatRoomFields = this.target.chatRoomFields; > + if (chatRoomFields) > + this.account.joinChat(chatRoomFields); > + } > + delete this._wasLeft; I don't fully understand why this is getting deleted here, but it matches the previous code, so it seems fine to me! I'd like an explanation, but it's still r+!

(In reply to Patrick Cloke [:clokep] from comment #12) > > + delete this._wasLeft; > > I don't fully understand why this is getting deleted here, but it matches > the previous code, so it seems fine to me! I'd like an explanation, but it's > still r+! It's an imConversations-internal flag that remembers whether a chat was left (parted) before the disconnection or is left due to the disconnection. After a reconnection, it's no longer needed.

Comment on attachment 8427157 [details] [diff] [review] 1014472-purple.patch 2 (purplexpcom changes) Review of attachment 8427157 [details] [diff] [review]: ----------------------------------------------------------------- ::: purplexpcom/src/purpleConvChat.cpp @@ +210,5 @@ > + > +/* readonly attribute prplIChatRoomFieldValues chatRoomFields; > + Just a stub - we don't attempt to change libpurple's default behaviour, > + as no prplConvChat instance exists at the time joinChat returns > + where we could store the chatRoomFieldValues. */ purpleAccount::JoinChat in purpleAccount.cpp could store the value of aComponents somewhere, so that the next purpleConvChat instance created gets it. (Note: this algorithm is a bit naive and could have an annoying edge case if joining the chat fails without us being notified... and later a different MUC is joined because of something received from the server without user interaction.) We agreed that it's fine to land this improvement without handling libpurple, but please don't make the comment sound like it's not possible (I don't think we have _really_ tried to do it).

Comment on attachment 8440750 [details] [diff] [review] 1014472.patch 3 Review of attachment 8440750 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/components/public/prplIConversation.idl @@ +120,5 @@ > /* This is true if we are in the process of joining the channel */ > readonly attribute boolean joining; > > + /* This stores the data required to join the chat with joinChat(). > + If null, the chat will not be reconnected. reconnected when/in which case? ::: chat/components/src/imConversations.js @@ +254,5 @@ > + this.systemMessage(msg); > + // Reconnect chat if possible. > + let chatRoomFields = this.target.chatRoomFields; > + if (chatRoomFields) > + this.account.joinChat(chatRoomFields); the "your account has been reconnected" message should be displayed only _after_ we have called joinChat, right? ::: chat/protocols/irc/irc.js @@ +300,5 @@ > > this._account.sendMessage("PART", params); > > // Remove reconnection information. > + delete this.chatRoomFields; Isn't this preventing a future rejoin of a password projected IRC channel with just "/join"? But anyway, it's not something new in this patch, so if there's an issue here, let's file another bug :-).

(In reply to Florian Quèze [:florian] [:flo] from comment #14) > purpleAccount::JoinChat in purpleAccount.cpp could store the value of > aComponents somewhere, so that the next purpleConvChat instance created gets > it. Yes, this would be the only way to make it work at least "most of the time". I've changed the wording on the comment to make it sound less impossible.

Improved comment.

(In reply to Florian Quèze [:florian] [:flo] from comment #15) > ::: chat/components/src/imConversations.js > @@ +254,5 @@ > > + this.systemMessage(msg); > > + // Reconnect chat if possible. > > + let chatRoomFields = this.target.chatRoomFields; > > + if (chatRoomFields) > > + this.account.joinChat(chatRoomFields); > > the "your account has been reconnected" message should be displayed only > _after_ we have called joinChat, right? We only show the system message if the channel was not left before disconnect, i.e. precisely when we show a "disconnected" system message. > ::: chat/protocols/irc/irc.js > @@ +300,5 @@ > > > > this._account.sendMessage("PART", params); > > > > // Remove reconnection information. > > + delete this.chatRoomFields; > > Isn't this preventing a future rejoin of a password projected IRC channel > with just "/join"? > But anyway, it's not something new in this patch, so if there's an issue > here, let's file another bug :-). This is true and I can't remember if it was by design (to avoid keeping the password around longer than necessary) or not.

I do not think it was done on purpose. I'd prefer if /join still worked in this situation.

Comment on attachment 8447624 [details] [diff] [review] 1014472-purple.patch 3 Review of attachment 8447624 [details] [diff] [review]: ----------------------------------------------------------------- ::: purplexpcom/src/purpleConvChat.cpp @@ +215,5 @@ > +NS_IMETHODIMP purpleConvChat::GetChatRoomFields(prplIChatRoomFieldValues **aComponents) > +{ > + NS_ENSURE_TRUE(mConv, NS_ERROR_NOT_INITIALIZED); > + > + *aComponents = nullptr; Please move the TODO comment here (just before setting *aComponents to null unconditionally).