Closed
Bug 955552
Opened 11 years ago
Closed 11 years ago
Chat rooms on hold should stay on hold after a restart
Categories
(Instantbird Graveyard :: Conversation, enhancement)
Tracking
(Not tracked)
RESOLVED
FIXED
1.5
People
(Reporter: florian, Assigned: florian)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 2114 at 2013-08-18 00:08:00 UTC ***
*** Due to BzAPI limitations, the initial description is in comment 1 ***
Assignee | ||
Comment 1•11 years ago
|
||
*** 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)
Assignee | ||
Comment 2•11 years ago
|
||
*** 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)
Assignee | ||
Comment 3•11 years ago
|
||
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 4•11 years ago
|
||
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-
Assignee | ||
Comment 5•11 years ago
|
||
*** 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?
Assignee | ||
Comment 6•11 years ago
|
||
*** 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)
Assignee | ||
Comment 7•11 years ago
|
||
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 8•11 years ago
|
||
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+
Updated•11 years ago
|
Whiteboard: [checkin-needed]
Comment 9•11 years ago
|
||
*** Original post on bio 2114 at 2013-08-23 21:47:37 UTC ***
http://hg.instantbird.org/instantbird/rev/e0942f4cdc06
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
You need to log in
before you can comment on or make changes to this bug.
Description
•