Add "Your account is reconnected" system message

RESOLVED FIXED in 1.2

Status

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

5 years ago
*** 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).
(Assignee)

Comment 1

5 years ago
*** 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.
(Assignee)

Comment 2

5 years ago
*** 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).
(Assignee)

Comment 3

5 years ago
Created attachment 8353374 [details] [diff] [review]
Patch

*** 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)

Updated

5 years ago
Assignee: nobody → aleth
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
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+
(Assignee)

Updated

5 years ago
No longer blocks: 954950
(Assignee)

Updated

5 years ago
Whiteboard: [checkin-needed]
(Assignee)

Updated

5 years ago
Whiteboard: [checkin-needed]
(Assignee)

Comment 5

5 years ago
Created attachment 8353409 [details] [diff] [review]
Patch

*** 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)
(Assignee)

Comment 6

5 years ago
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
(Assignee)

Comment 7

5 years ago
Created attachment 8353410 [details] [diff] [review]
Patch

*** Original post on bio 1404 as attmnt 1653 at 2012-06-21 00:58:00 UTC ***

Fixed typo.
Attachment #8353410 - Flags: review?(florian)
(Assignee)

Comment 8

5 years ago
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?
(Assignee)

Comment 10

5 years ago
*** 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?
(Assignee)

Comment 11

5 years ago
Created attachment 8353418 [details] [diff] [review]
Patch

*** 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)
(Assignee)

Comment 12

5 years ago
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)
(Assignee)

Comment 13

5 years ago
Created attachment 8353419 [details] [diff] [review]
Patch

*** 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)
(Assignee)

Comment 14

5 years ago
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-
(Assignee)

Comment 16

5 years ago
Created attachment 8353450 [details] [diff] [review]
Patch

*** 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)
(Assignee)

Comment 17

5 years ago
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
(Assignee)

Comment 18

5 years ago
*** 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?
(Assignee)

Comment 19

5 years ago
*** 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...
(Assignee)

Comment 20

5 years ago
Created attachment 8353459 [details] [diff] [review]
Patch

*** 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)
(Assignee)

Comment 21

5 years ago
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)
(Assignee)

Comment 22

5 years ago
Created attachment 8353460 [details] [diff] [review]
Patch

*** 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)
(Assignee)

Comment 23

5 years ago
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)
(Assignee)

Comment 24

5 years ago
Created attachment 8353461 [details] [diff] [review]
Patch

*** 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)
(Assignee)

Comment 25

5 years ago
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+
(Assignee)

Updated

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.2
Depends on: 1018602
You need to log in before you can comment on or make changes to this bug.