Closed Bug 955552 Opened 7 years ago Closed 7 years ago

Chat rooms on hold should stay on hold after a restart

Categories

(Instantbird :: Conversation, enhancement)

x86
Other
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: florian)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 2114 at 2013-08-18 00:08:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2114 as attmnt 2731 at 2013-08-18 00:08:00 UTC ***

This is a very simplified version of session restore that only restores whether a conversation was on hold or not.

I restricted this to only chatrooms as I don't really see how a private conversation could start on hold.
Attachment #8354500 - Flags: review?(aleth)
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 2114 as attmnt 2732 at 2013-08-18 00:37:00 UTC ***

As discussed on IRC, I'm removing a small touch of code duplication; and this should also improve readability :).
Attachment #8354501 - Flags: review?(aleth)
Comment on attachment 8354500 [details] [diff] [review]
Patch

*** Original change on bio 2114 attmnt 2731 at 2013-08-18 00:37:21 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354500 - Attachment is obsolete: true
Attachment #8354500 - Flags: review?(aleth)
Comment on attachment 8354501 [details] [diff] [review]
Patch v2

*** Original change on bio 2114 attmnt 2732 at 2013-08-18 18:06:21 UTC ***

>+  _isConversationHidden: function(aConv) {
>+    let accountId = aConv.account.id;
>+    return aConv.isChat && accountId in this._hiddenConversations &&
>+           aConv.normalizedName in this._hiddenConversations[accountId];

Shouldn't you use hasOwnProperty here? (I don't trust normalizedNames to be sanitized for that)

A conversation on hold should be removed from the hidden list if it is closed due to user interaction (rather than on shutdown). Otherwise it will be opened on hold when it is next joined, which is not the expected behaviour imho.
Attachment #8354501 - Flags: review?(aleth) → review-
*** Original post on bio 2114 at 2013-08-18 20:07:37 UTC ***

(In reply to comment #2)

> A conversation on hold should be removed from the hidden list if it is closed
> due to user interaction (rather than on shutdown). Otherwise it will be opened
> on hold when it is next joined, which is not the expected behaviour imho.

So if a channel is autojoined and on hold, and I close it, at the next restart you think the expected behavior is to show it in a tab?
Attached patch Patch v3Splinter Review
*** Original post on bio 2114 as attmnt 2751 at 2013-08-22 23:26:00 UTC ***

This new patch addresses the review comments in comment 2 (interdiff: http://pastebin.instantbird.com/301877).

However I didn't address "I was also wondering if there needs to be something which cleans up the pref over time, so it doesn't accumulate a list of channels which were joined as a one-off, closed automatically on shutdown, but are not autojoined" from http://log.bezut.info/instantbird/130818/#m430

I agree that it would be nice to cleanup the preference (and to not include in the first place channels that aren't auto-joined), but I don't see a correct way to do it. The account manager code touches directly the auto-join preferences, there's no easy way to check if a channel is currently auto-joined or not.
I don't really want to have imWindows.jsm go touch these account preferences to guess if a channel is auto-joined or not (it would be only a guess, as there could be normalization issues; especially as the user can enter any string in the auto-join field).

I think the correct solution is to either get rid of the auto-join feature completely and replace it by a more complete session restore implementation; or to include a boolean attribute in the prplIConvChat interface (and maybe make it easily settable from a tab's context menu). Either way, I think it's out of the scope of this bug.
Attachment #8354520 - Flags: review?(aleth)
Comment on attachment 8354501 [details] [diff] [review]
Patch v2

*** Original change on bio 2114 attmnt 2732 at 2013-08-22 23:26:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354501 - Attachment is obsolete: true
Comment on attachment 8354520 [details] [diff] [review]
Patch v3

*** Original change on bio 2114 attmnt 2751 at 2013-08-23 11:19:14 UTC ***

>> A conversation on hold should be removed from the hidden list if it is closed
>> due to user interaction (rather than on shutdown). Otherwise it will be opened
>> on hold when it is next joined, which is not the expected behaviour imho.
>
>So if a channel is autojoined and on hold, and I close it, at the next restart
>you think the expected behavior is to show it in a tab?

I think we are less likely to close autojoined conversations (which we want to reopen on hold, at least if we had a certain add-on installed) than we are to close channels which we opened a while back by hand, put on hold by force of habit or in case we are pinged, and then decide a bit later to get rid of when the list of convs on hold looks too long.

Really this runs into what you mention in your last comment - that this is a halfway house to what ultimately should be session restore (where expectations are clear). But it should be a big improvement. Let's try this in nightlies :)
Attachment #8354520 - Flags: review?(aleth) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 2114 at 2013-08-23 21:47:37 UTC ***

http://hg.instantbird.org/instantbird/rev/e0942f4cdc06
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
Depends on: 955577
Depends on: 955680
Depends on: 955681
You need to log in before you can comment on or make changes to this bug.