Closed Bug 954839 Opened 10 years ago Closed 10 years ago

Add "Your account is reconnected" system message

Categories

(Chat Core :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 8 obsolete files)

*** Original post on bio 1404 at 2012-04-25 23:14:00 UTC ***

Currently when an account is disconnected, this is reported in open conversations by a system message, "Your account is disconnected". However, the user is given no indication when the account has successfully been reconnected. 

This means you will often come back to a tab and find the disconnection message, and you have to check the account manager to discover whether the connection is down or not. This is not a step a novice user would necessarily take - they'll just think the connection is still down.

What makes this a little tricky (but even more important) is that there may be conversations that can't be reconnected (for IRC channels until auto-reconnect is implemented, and possibly for rooms requiring passwords).
*** Original post on bio 1404 at 2012-06-15 20:53:33 UTC ***

We then could also drop displaying the topic a second time when rejoining an IRC channel.
*** Original post on bio 1404 at 2012-06-15 20:59:52 UTC ***

Will intersect with manually rejoining a channel (/join) after /part, and bug 953828 (bio 385).
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1404 as attmnt 1617 at 2012-06-16 15:29:00 UTC ***

This patch adds a simple "Your account has been reconnected" message. I think it's better than the status quo. It will not show when /joining after /part. It will show in channels that have not been reconnected, but that merely highlights bug 953828 (bio 385) and doesn't make things worse: at least the user then knows he can just /join again and there is no longer a connection problem.
Attachment #8353374 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Blocks: 954950
Comment on attachment 8353374 [details] [diff] [review]
Patch

*** Original change on bio 1404 attmnt 1617 at 2012-06-19 02:10:16 UTC ***

This looks good, note that without the fix for bug 953828 (bio 385) this looks a bit funky (as it will tell you "This account has reconnected." on a chat that you haven't rejoined). The message isn't incorrect, per say, but it seems a bit out of place.
Attachment #8353374 - Flags: review?(clokep) → review+
No longer blocks: 954950
Whiteboard: [checkin-needed]
Whiteboard: [checkin-needed]
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1404 as attmnt 1652 at 2012-06-21 00:55:00 UTC ***

- If the channel is parted, suppress both the disconnection and reconnection message.
- If the channel is not a chat, show the reconnection message as part of the next buddy status system message.
Attachment #8353409 - Flags: review?(florian)
Comment on attachment 8353374 [details] [diff] [review]
Patch

*** Original change on bio 1404 attmnt 1617 at 2012-06-21 00:55:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353374 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1404 as attmnt 1653 at 2012-06-21 00:58:00 UTC ***

Fixed typo.
Attachment #8353410 - Flags: review?(florian)
Comment on attachment 8353409 [details] [diff] [review]
Patch

*** Original change on bio 1404 attmnt 1652 at 2012-06-21 00:58:05 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353409 - Attachment is obsolete: true
Attachment #8353409 - Flags: review?(florian)
*** Original post on bio 1404 at 2012-06-21 09:17:22 UTC ***

Comment on attachment 8353410 [details] [diff] [review] (bio-attmnt 1653)
Patch

> # LOCALIZATION NOTE (statusChangedFromUnknown[WithStatusText]):
> #  special case of the previous 2 strings for when the status was
> #  previously unknown. These 2 strings should not mislead the user
> #  into thinking the person's status has just changed.
> statusChangedFromUnknown=%1$S is %2$S.
> statusChangedFromUnknownWithStatusText=%1$S is %2$S: %3$S.

When are we displaying these strings then?
*** Original post on bio 1404 at 2012-06-21 10:13:03 UTC ***

(In reply to comment #7)
> > statusChangedFromUnknown=%1$S is %2$S.
> > statusChangedFromUnknownWithStatusText=%1$S is %2$S: %3$S.
> 
> When are we displaying these strings then?

I couldn't find an example, but since Unknown is used as a fallback value, I wasn't absolutely certain disconnection was the only circumstance in which a conversation could have a buddy with Unknown status. E.g. XMPP sets status to Unknown when there is no preferred resource for a buddy - but does that imply the accounts are disconnected?
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1404 as attmnt 1661 at 2012-06-22 21:43:00 UTC ***

This patch assumes we have a reconnection if the status changes from unknown. Since the strings still change, we need to add new strings anyway. So my suggestion is to try this, but to not delete the old strings. If it ever turns out there are circumstances where "You have been reconnected..." is shown in error, the strings will still be there to add them back.

Minor issue with this patch: No reconnection message is shown if a channel was parted by the user, but is on the autojoin list, so reconnects anyway. I think the problem here is the autojoin behaviour.
Attachment #8353418 - Flags: review?(florian)
Comment on attachment 8353410 [details] [diff] [review]
Patch

*** Original change on bio 1404 attmnt 1653 at 2012-06-22 21:43:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353410 - Attachment is obsolete: true
Attachment #8353410 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1404 as attmnt 1662 at 2012-06-22 21:51:00 UTC ***

Current patch attached this time.
Attachment #8353419 - Flags: review?(florian)
Comment on attachment 8353418 [details] [diff] [review]
Patch

*** Original change on bio 1404 attmnt 1661 at 2012-06-22 21:51:40 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353418 - Attachment is obsolete: true
Attachment #8353418 - Flags: review?(florian)
Comment on attachment 8353419 [details] [diff] [review]
Patch

*** Original change on bio 1404 attmnt 1662 at 2012-06-25 22:00:23 UTC ***

>diff --git a/chrome/en-US/locale/en-US/chat/conversations.properties b/chrome/en-US/locale/en-US/chat/conversations.properties
>index 5244494..9465b2b 100644
>--- a/chrome/en-US/locale/en-US/chat/conversations.properties
>+++ b/chrome/en-US/locale/en-US/chat/conversations.properties
>@@ -18,20 +18,26 @@ statusChanged=%1$S is now %2$S.
> statusChangedWithStatusText=%1$S is now %2$S: %3$S.
> # LOCALIZATION NOTE (statusChangedFromUnknown[WithStatusText]):
> #  special case of the previous 2 strings for when the status was
> #  previously unknown. These 2 strings should not mislead the user
> #  into thinking the person's status has just changed.
> statusChangedFromUnknown=%1$S is %2$S.
> statusChangedFromUnknownWithStatusText=%1$S is %2$S: %3$S.
> 
>+# LOCALIZATION NOTE (reconnect.statusChangedFromUnknown[WithStatusText]):
>+# This variant will be displayed when an account has just been reconnected.
>+reconnect.statusChangedFromUnknown=Your account has been reconnected (%1$S is %2$S).
>+reconnect.statusChangedFromUnknownWithStatusText=Your account has been reconnected (%1$S is %2$S: %3$S).
>+

This looks like it would want to be:
statusKnown=Your account has been reconnected (%S).

and %S replaced with either statusChangedFromUnknown or statusChangedFromUnknownWithStatusText

> # LOCALIZATION NOTE (statusUnknown):
> #  %S is the display name of the contact.
> statusUnknown=Your account is disconnected (the status of %S is no longer known).
> accountDisconnected=Your account is disconnected.
>+accountReconnected=Your account has been reconnected.

>diff --git a/components/imConversations.js b/components/imConversations.js

>-    this.systemMessage(bundle.GetStringFromName("accountDisconnected"));
>+    if (!this.isChat || !this.left)
>+      this.systemMessage(bundle.GetStringFromName("accountDisconnected"));
>+    else
>+      this._wasLeft = true;

I think reversing the test would make this easier to read:

if (this.isChat && this.left)
  this._wasLeft = true;
else
  ...

Overall, I don't feel very good about this code, but I don't see a real reason to reject it either, so with the 2 changes mentioned above, I would take it :).
Attachment #8353419 - Flags: review?(florian) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1404 as attmnt 1693 at 2012-06-25 22:29:00 UTC ***

(In reply to comment #11)
> >+reconnect.statusChangedFromUnknown=Your account has been reconnected (%1$S is %2$S).
> >+reconnect.statusChangedFromUnknownWithStatusText=Your account has been reconnected (%1$S is %2$S: %3$S).
> >+
> 
> This looks like it would want to be:
> statusKnown=Your account has been reconnected (%S).

I first thought "great idea!" but it makes the code more complicated (two wasUnknown checks rather than one) and would lead to ugly double periods, e.g.
"Your account has been reconnected (flo is Available.)."

> I think reversing the test would make this easier to read:
done.

> Overall, I don't feel very good about this code, but I don't see a real reason
> to reject it either, so with the 2 changes mentioned above, I would take it 

_wasLeft could be eliminated at a later stage when/if the _chatRoomFields variable on the MUC which determines auto-reconnection is lifted from JS-IRC to imConversations.
Attachment #8353450 - Flags: review?(florian)
Comment on attachment 8353419 [details] [diff] [review]
Patch

*** Original change on bio 1404 attmnt 1662 at 2012-06-25 22:29:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353419 - Attachment is obsolete: true
*** Original post on bio 1404 at 2012-06-25 22:31:49 UTC ***

> I first thought "great idea!" but it makes the code more complicated (two
> wasUnknown checks rather than one) and would lead to ugly double periods, e.g.
> "Your account has been reconnected (flo is Available.)."

The only alternative I can think of is concatenation:
"Your account has been reconnected. flo is Available."

But then the statusUnknown string would also have to be changed to match. And it might cause problems with R-to-L languages?
*** Original post on bio 1404 at 2012-06-25 23:37:00 UTC ***

Another option would be to stop fretting about unknown edge cases and simply change the existing statusChangedFromUnknown strings to include "Your account has been reconnected". I suppose they'd have to be called statusChangedFromUnknown2 and statusChangedFromUnknown2WithStatusText then, or whatever the convention for changed strings is...
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1404 as attmnt 1702 at 2012-06-26 14:37:00 UTC ***

How's this?

- Introduce statusKnown[WithStatusText] strings, which follows the existing naming convention while being (hopefully) clearer about how they fit in.
- The strings can be translated as a whole (no nesting), which should give translators maximum flexibility.
- Long l10n comment.
- Add your suggested change to statusUnknown to make the causality clearer. I didn't change the stringID as this doesn't seem a large enough change to require that.

There's still going to be a double period at the end of the default away message when it's displayed in statusXXX strings, but I don't see a good solution for that.
Attachment #8353459 - Flags: review?(florian)
Comment on attachment 8353450 [details] [diff] [review]
Patch

*** Original change on bio 1404 attmnt 1693 at 2012-06-26 14:37:05 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353450 - Attachment is obsolete: true
Attachment #8353450 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1404 as attmnt 1703 at 2012-06-26 14:56:00 UTC ***

Undo change in statusUnknown for consistency.
Attachment #8353460 - Flags: review?(florian)
Comment on attachment 8353459 [details] [diff] [review]
Patch

*** Original change on bio 1404 attmnt 1702 at 2012-06-26 14:56:53 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353459 - Attachment is obsolete: true
Attachment #8353459 - Flags: review?(florian)
Attached patch PatchSplinter Review
*** Original post on bio 1404 as attmnt 1704 at 2012-06-26 17:10:00 UTC ***

It is possible to receive messages from an XMPP contact who is not on the buddy list. In this case you have an open conversation with a status=unknown buddy, whose status should change once we accept their buddy request, subscribing to presence info. This currently does not work properly, but I've added back the justReconnected flag so this doesn't display erroneous "you have been reconnected" messages when it does.
Attachment #8353461 - Flags: review?(florian)
Comment on attachment 8353460 [details] [diff] [review]
Patch

*** Original change on bio 1404 attmnt 1703 at 2012-06-26 17:10:05 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353460 - Attachment is obsolete: true
Attachment #8353460 - Flags: review?(florian)
Comment on attachment 8353461 [details] [diff] [review]
Patch

*** Original change on bio 1404 attmnt 1704 at 2012-06-26 21:32:38 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353461 - Flags: review?(florian) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 1404 at 2012-06-26 23:51:31 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/8fb82edf3731

Thanks for making this behavior better. :)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.