If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Only remove unread ruler when switching away from a tab if tab has been visible for a certain time

RESOLVED FIXED in 1.2

Status

Instantbird
Conversation
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

4 years ago
*** Original post on bio 1450 at 2012-05-20 21:23:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
(Assignee)

Comment 1

4 years ago
Created attachment 8353245 [details] [diff] [review]
WIP

*** Original post on bio 1450 as attmnt 1492 at 2012-05-20 21:23:00 UTC ***

I would be grateful for any feedback on this tweaked unread ruler behaviour:

- Does this remove (most of) the instances where the ruler is removed, but the user hasn't actually read any messages (i.e. the tab was only selected "accidentally" or "in passing")?

- The minimum visibility time in the patch is set to 3s. I wonder if it should be even shorter/a bit longer.
Attachment #8353245 - Flags: feedback?
(Assignee)

Comment 2

4 years ago
Created attachment 8353246 [details] [diff] [review]
WIP

*** Original post on bio 1450 as attmnt 1493 at 2012-05-20 21:32:00 UTC ***

- small bugfix
Attachment #8353246 - Flags: feedback?
(Assignee)

Comment 3

4 years ago
Comment on attachment 8353245 [details] [diff] [review]
WIP

*** Original change on bio 1450 attmnt 1492 at 2012-05-20 21:32:59 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353245 - Attachment is obsolete: true
Attachment #8353245 - Flags: feedback?
(Assignee)

Updated

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

Comment 4

4 years ago
Created attachment 8353247 [details] [diff] [review]
Patch

*** Original post on bio 1450 as attmnt 1494 at 2012-05-20 21:46:00 UTC ***

- method rename for clarity
- reduce time constant to 2s
Attachment #8353247 - Flags: feedback?
(Assignee)

Comment 5

4 years ago
Comment on attachment 8353246 [details] [diff] [review]
WIP

*** Original change on bio 1450 attmnt 1493 at 2012-05-20 21:46:36 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353246 - Attachment is obsolete: true
Attachment #8353246 - Flags: feedback?
Comment on attachment 8353247 [details] [diff] [review]
Patch

*** Original change on bio 1450 attmnt 1494 at 2012-05-20 21:58:52 UTC ***

Seems to make sense.
Would we want to handle differently switching between tabs (which seems guaranteed to be caused by the user) and the window being deactivated (any popup caused by other applications could do it, right?).
Attachment #8353247 - Flags: feedback? → feedback+
(Assignee)

Comment 7

4 years ago
*** Original post on bio 1450 at 2012-05-20 22:05:02 UTC ***

(In reply to comment #3)
> Would we want to handle differently switching between tabs (which seems
> guaranteed to be caused by the user) and the window being deactivated (any
> popup caused by other applications could do it, right?).

I'm not sure. I've certainly been annoyed once or twice by the situation where I click on a particular (not current) tab when selecting the IB conversation window (e.g. because I have been pinged) and the ruler in the current tab is removed, though I've not looked at that conversation. Also it seems more consistent to me.

Needs testing!
(Assignee)

Comment 8

4 years ago
Comment on attachment 8353247 [details] [diff] [review]
Patch

*** Original change on bio 1450 attmnt 1494 at 2012-05-20 22:44:09 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353247 - Flags: review?(florian)
(Assignee)

Comment 9

4 years ago
*** Original post on bio 1450 at 2012-05-20 23:20:04 UTC ***

Should the time constant be a pref (that could then also be set to -1 for "never remove the ruler due to switching away from tabs")?

I think I'd prefer to find a value that "just works" but we shall see.
(Assignee)

Comment 10

4 years ago
Created attachment 8353249 [details] [diff] [review]
Patch

*** Original post on bio 1450 as attmnt 1496 at 2012-05-20 23:35:00 UTC ***

- Add comment and rename method for clarity.
Attachment #8353249 - Flags: review?(florian)
(Assignee)

Comment 11

4 years ago
Comment on attachment 8353247 [details] [diff] [review]
Patch

*** Original change on bio 1450 attmnt 1494 at 2012-05-20 23:35:57 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353247 - Attachment is obsolete: true
Attachment #8353247 - Flags: review?(florian)
*** Original post on bio 1450 at 2012-05-21 09:55:00 UTC ***

Comment on attachment 8353249 [details] [diff] [review] (bio-attmnt 1496)
Patch

>+      <method name="switchingToTab">

>+            delete this._wasVisible;
>+            this._visibleTimer = setTimeout(function() {
>+              if (this) {

Is this test really what you meant? If this was null I think |delete this._wasVisible;| would throw.

>+                this._wasVisible = true;
>+                delete this._visibleTimer;
>+              }
>+            }.bind(this), 2000);
(Assignee)

Comment 13

4 years ago
*** Original post on bio 1450 at 2012-05-21 12:34:11 UTC ***

(In reply to comment #7)
> Comment on attachment 8353249 [details] [diff] [review] (bio-attmnt 1496) [details]
> Patch
> 
> >+      <method name="switchingToTab">
> 
> >+            delete this._wasVisible;
> >+            this._visibleTimer = setTimeout(function() {
> >+              if (this) {
> 
> Is this test really what you meant? If this was null I think |delete
> this._wasVisible;| would throw.

The test is intended to check that the tab hasn't been closed by the time the timeout is over and the handler is called. Doesn't it do this?
(Assignee)

Comment 14

4 years ago
Created attachment 8353251 [details] [diff] [review]
Patch

*** Original post on bio 1450 as attmnt 1499 at 2012-05-21 14:14:00 UTC ***

Right. Removed the unnecessary check.
Attachment #8353251 - Flags: review?(florian)
(Assignee)

Comment 15

4 years ago
Comment on attachment 8353249 [details] [diff] [review]
Patch

*** Original change on bio 1450 attmnt 1496 at 2012-05-21 14:14:32 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353249 - Attachment is obsolete: true
Attachment #8353249 - Flags: review?(florian)
(Assignee)

Comment 16

4 years ago
*** Original post on bio 1450 at 2012-05-21 14:19:34 UTC ***

Personally I think an even shorter time period might be even better (1s maybe?) but I'd like some feedback on that.
(Assignee)

Comment 17

4 years ago
Created attachment 8353338 [details] [diff] [review]
Patch

*** Original post on bio 1450 as attmnt 1581 at 2012-06-09 20:45:00 UTC ***

Fiddling with this again, went for 1200ms as a compromise
Attachment #8353338 - Flags: review?(florian)
(Assignee)

Comment 18

4 years ago
Comment on attachment 8353251 [details] [diff] [review]
Patch

*** Original change on bio 1450 attmnt 1499 at 2012-06-09 20:45:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353251 - Attachment is obsolete: true
Attachment #8353251 - Flags: review?(florian)
Comment on attachment 8353338 [details] [diff] [review]
Patch

*** Original change on bio 1450 attmnt 1581 at 2012-07-14 14:45:44 UTC ***

r=me without enthusiasm, but I see nothing wrong in the code and I definitely agree that the best way to know if this is an improvement or annoying is to test it in nightlies for a while.
Attachment #8353338 - Flags: review?(florian) → review+
*** Original post on bio 1450 at 2012-07-15 03:18:39 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/c03c096f856b

Let's see how this works. :)
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.