Closed Bug 1014472 Opened 10 years ago Closed 10 years ago

Support automatic MUC reconnection for all protocols

Categories

(Chat Core :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(2 files, 5 obsolete files)

      No description provided.
Attached patch 1014472.patch (obsolete) — Splinter Review
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.
Attachment #8426909 - Flags: review?(clokep)
I'm unsure whether the nullptr does the right thing here.
Attachment #8426910 - Flags: feedback?(florian)
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.)
Attachment #8426909 - Flags: review?(clokep) → review+
Oh and as Florian already said over IRC this should be a readonly property. :)
Attachment #8426910 - Attachment is obsolete: true
Attachment #8426910 - Flags: feedback?(florian)
Attachment #8427157 - Flags: review?(florian)
Attached patch 1014472.patch 2 (obsolete) — Splinter Review
>> +      // 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?
Attachment #8426909 - Attachment is obsolete: true
Attachment #8427167 - Flags: review?(clokep)
(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. :)
Attachment #8427167 - Flags: review?(clokep) → review+
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.
Blocks: 1018771
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
Attached patch 1014472.patch 3 (obsolete) — Splinter Review
Unbitrotted.
Attachment #8427167 - Attachment is obsolete: true
Attachment #8440750 - Flags: review?(clokep)
(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+!
Attachment #8440750 - Flags: review?(clokep) → review+
(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).
Attachment #8427157 - Flags: review?(florian) → review-
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 :-).
Attachment #8440750 - Flags: feedback+
(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.
Attachment #8427157 - Attachment is obsolete: true
Attachment #8447624 - Flags: review?(florian)
Attached patch 1014472.patch 4Splinter Review
Improved comment.
Attachment #8440750 - Attachment is obsolete: true
Attachment #8447625 - Flags: review?(florian)
(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).
Attachment #8447624 - Flags: review?(florian) → review+
Attachment #8447625 - Flags: review?(florian) → review+
https://hg.mozilla.org/comm-central/rev/ab9ae24bbdee
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.