950455 - Make thread-pane "snap to last page" behavior optional

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Andrew Buehler]

Attachment: thunderbird-threadpane-seeking-nosnap-v1.diff

In most cases, when selecting a new message with the "next message" ('f'), "previous message" ('b'), or "next unread message" ('n') commands, the thread pane will scroll far enough to make sure that the newly selected message is visible; if the message would be visible without scrolling, the thread pane will often not scroll at all..

In cases where the newly selected message is near the bottom of the thread pane (within one visible "page"), however, the thread pane will instead scroll all the way to the bottom - even if the newly selected message would have still been visible even with no scrolling at all.

At the very least, this violates the principle of least surprise. In practice, it's also a problem for me for the same reasons as given under bug 920510: it interferes with page-by-page message counting when reading large numbers of new messages. It's not as big a problem as the one fixed under that bug was, because it doesn't come up as often, but it's still enough to be irritating.

The attached patch makes this behavior optional.

Comment 1

[Jim Porter (:squib)]

Should we really make this an option? Maybe what we actually want to do here is to decide on the ideal behavior and just switch to that without any options. I worry about adding more and more options to the thread pane, since it just increases the likelihood that users will find a broken combination of prefs and get upset/expect us to fix it.

Comment 2

[Andrew Buehler]

I'd have no problem with disabling this "snap to last page" behavior entirely, but I imagine it was presumably added for a reason, and it's been there for a while now...

Comment 3

[Andrew Buehler]

When you say "adding more and more options to the thread pane", what existing options did you have in mind?

The options added under bug 920510 would obviously qualify.

The option added under bug 495946 (also my doing), which is related to thread sorting, might or might not be considered to qualify.

There are also several thread-sorting-related options which already existed, which I think cross the Thunderbird/Seamonkey divide; I'm not sure whether those would be counted in as well.

Are there any others you were thinking of?


I've done somewhat extensive testing of the options from this bug, bug 920510, and bug 495946 - both individually and in various combinations - and I haven't found a way to break anything; further, looking at the code, I don't think I see a way they could even potentially interact.

(Well, if you use unreasonably large values for the padding prefs from bug 920510, you can get broken behavior - but in that bug, I originally included checks to put sane caps on those prefs, and got asked to remove them before the patch would be accepted. In any case, the brokenness doesnt seem to be any worse with snap_to_last_page disabled than without this patch.)

This should be the final option I'm likely to need to add in the thread pane for my own purposes, and judging by the commit log, there doesn't seem to be much of anyone else trying to add such options - so the odds of future options adding further, potentially more broken or breakable, combinations seem low.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Sorry for the delay.  I'm a hopefully well-known opponent of adding options where we don't need to, so I would vote for the no-option approach, and just implement the new behaviour.

Comment 5

[Andrew Buehler]

If I don't get a comment disagreeing with that perspective in a day or two, I'll do the trivial patch to implement the no-option behavior.

There's a chance there might be pushback against that change (once released) from people who've gotten used to the current behavior, but although I'm generally in favor of adding options rather than removing existing behavior, I'm willing to take that chance if that's what the decision is.

Comment 6

[Andrew Buehler]

Attachment: thunderbird-threadpane-seeking-nosnap-v2.diff

This patch removes the "snap to last page of message list" behavior. As best I can tell, this restores the TB2 behavior.

Rudimentary testing confirms that it works as expected, but more extensive testing (e.g. for interaction with other things) has not been done with this iteration of the patch.

Comment 7

[Jim Porter (:squib)]

Comment on attachment 8359207 [details] [diff] [review]
thunderbird-threadpane-seeking-nosnap-v2.diff

Review of attachment 8359207 [details] [diff] [review]:
-----------------------------------------------------------------

This code looks fine, but it needs some UI discussion first. I'm setting a ui-review? with no reviewer for the moment to track this fact. If you could post something to [e-mail] and [e-mail] to get feedback on the UX change, that'd be useful.

Comment 8

[Andrew Buehler]

It's been a week now. To summarize the feedback:


Gervase Markham likes the "look-ahead" effect of the "snap to last page" behavior, but that effect can also be achieved more generally - that is, across the entire thread pane rather than in just the last page - with the mail.threadpane.padding prefs. There are issues in some use cases, including his, due to the lack of sanity checking; I've written a fix for that already, but there's no bug for it.


Andrew Sutherland - who apparently wrote the "snap to last page" behavior in the first place - has explained part of the reasoning behind it, and has said that he agrees that the current behavior is inconsistent.

Essentially, the current behavior was intended to be the first step of a more complex "stay snapped to the bottom until the user moves away from it" solution, to show newly-arrived messages when in a sort order that places them at the bottom; that solution itself was intended to be a stopgap measure against the creation of a more general, and possibly more robust, "new messages have arrived" indication. However, since neither has been implemented in the intervening 4.5 years, there seems little value in the "snap to last page" behavior itself. Also, to quote Andrew Sutherland directly, "[t]hreaded display is a big complication the original solution and game-plan did not deal with (well)".


No other objections have been raised.

Is that sufficient?

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8359207 [details] [diff] [review]
thunderbird-threadpane-seeking-nosnap-v2.diff

Empty request, putting to Blake's queue.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

So, I tried to ui-review this today, but couldn't due to an empty folder pane, which should be getting tracked in bug 912782.  Sorry about that!

Comment 11

[Andrew Buehler]

I'm a bit confused. I don't see anything in the linked bug which looks like it should affect most profiles, or which indicates that it's presently being worked on. Are you now (presumably as a recent onset) being affected by that bug? I would have expected to see a comment on that bug from you, in that case, and I don't see one there.

Note that this patch would conflict with whichever patch is currently active for bug 964824, on the sanity-checking behavior (which has expanded into something more); if something gets landed for that bug before this one, we'll need to refresh the patch for this bug. (And vice versa, of course.)

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8359207 [details] [diff] [review]
thunderbird-threadpane-seeking-nosnap-v2.diff

Okay, now that I got Thunderbird building again, and have had a chance to test out your patch, I like it!

Thank you so much for your patience!

Comment 13

[Andrew Buehler]

As this patch now has both r+ and ui-r+, I believe it's ready for check-in. Setting checkin-needed on that basis; if I'm premature in my assessment, anyone may feel free to remove that keyword (preferably with an explanation of what I missed or did wrong).

Comment 14

[Magnus Melin [:mkmelin]]

You're good to go! :)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/2980ef9bd2d0