365049 - Clean up logic for scrolling and selecting when entering folder

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Karsten Düsterloh]

When entering a folder, it's not always very clear for human beings which message - if any - will be selected and where the threadpane will be scrolled to, despite a UI pref "remember the last selected folder" (mailnews.remember_selected_message) and a hidden pref (mailnews.scroll_to_new_message).

With the default pref settings (both prefs default to 'true'), entering a folder with new messages (eg. a newsgroup) will _scroll_ to the first (= usually the oldest!) new message, probably opening the repective thread, but not selecting any message, regardless of the remember_selected_message setting! (See also TB's bug 273693).
Only if no new messages exist, the last selected message will be selected.

With scroll_to_new_message set to false, message selection doesn't happen at all...

Relevant code: msgMail3PaneWindow.js, ll.117+, ll.181+.

Comment 1

[Karsten Düsterloh]

Furthermore, when using cross folder navigation like "go to next unread" do in fact select the new message across folder boundaries, but the message is not scrolled into view...

If cross folder navigation happens, it should have priority.
If not, a remembered last message should be selected.
Else, scroll to the first (oldest!) new message, but don't select/open it (security!).
Else, in case we have a sort order, use that.
If all this fails, select the first message.

Comment 2

[Karsten Düsterloh]

Attachment: sanitize scrolling logic

This implements the logic laid out in comment #1, effectively doing "if we can _select_ something, do it, else just _scroll_ to somewhere sane".

Comment 3

[[e-mail]]

Comment on attachment 255960 [details] [diff] [review]
sanitize scrolling logic

I'm not sure that we should switch the priority around. But then I never remember the last selected message anyway.

Comment 4

[Karsten Düsterloh]

Comment on attachment 255960 [details] [diff] [review]
sanitize scrolling logic

Landed on trunk to collect real world complains. ;-)

Comment 5

[Mike Cowperthwaite]

(In reply to comment #1)
> If cross folder navigation happens, it should have priority.
  -- meaning, select the oldest unread [?]

> If not, a remembered last message should be selected.
> Else, scroll to the first (oldest!) new message, but don't select/open it
> (security!).
> Else, in case we have a sort order, use that.
  -- meaning scroll to, or select?  If the sort order is, say, by subject,
     are you scrolling to (or selecting) the 'A' end or the 'Z' end?

> If all this fails, select the first message.
  -- select?  Why not just scroll to?  Doesn't the 'security' consideration
     apply here as well?


I noticed running this trunk version (1.5a-0307) that the thread pane doesn't always have a 'current' item, as happens in Thunderbird due to bug 280153.  (Because not using toolkit?  I don't know what the status is for toolkit integration on trunk.)  If you select a single message, that will be the current; but, it's possible to set the current without making a selection.

In the case where you're scrolling to a new message that you don't want to select, if you set that as current as well, then the user could tab (or Ctrl+Tab) to the thread pane and type space to select it.  Without setting the current in this case, getting to the new message is less accessible.

In the case where you're scrolling to one end or the other but neither selecting nor trying to get a specific message view, it would be nice if the current could be set at the end of the sort (as requested in bug 305140 for TB).

Comment 6

[Karsten Düsterloh]

> > If cross folder navigation happens, it should have priority.
>   -- meaning, select the oldest unread [?]

Kind of yes, eg when using N with threaded view, the next unread is in the oldest thread with unread messages.
-> select

> > If not, a remembered last message should be selected.

-> select

> > Else, scroll to the first (oldest!) new message, but don't select/open it
> > (security!).

-> just scroll

> > Else, in case we have a sort order, use that.
>   -- meaning scroll to, or select?

-> just scroll

>  If the sort order is, say, by subject,
>      are you scrolling to (or selecting) the 'A' end or the 'Z' end?

See unchanged code in patch:
     // if we still haven't scrolled,
     // scroll to the newest, which might be the top or the bottom
     // depending on our sort order and sort type
AIUI, with A at the top this should _scroll_ to Z.

> > If all this fails, select the first message.
>   -- select?

No, sorry, typo. :(
-> just scroll
See unchanged code in patch:
     // if still we haven't scrolled,
     // scroll to the top.
     if (!scrolled)
       EnsureRowInThreadTreeIsVisible(0);

> I noticed running this trunk version (1.5a-0307) that the thread pane doesn't
> always have a 'current' item,

That I didn't change, it's the main difference between toolkit and xpfe tree.xml. I (and Neil) consider toolkit's notion of always having a current item as broken.

> I don't know what the status is for toolkit integration on trunk.

We didn't sync tree.xml yet because of this very issue.

> In the case where you're scrolling to a new message that you don't want to
> select, if you set that as current as well, then the user could tab (or
> Ctrl+Tab) to the thread pane and type space to select it.  Without setting the
> current in this case, getting to the new message is less accessible.

Good point.
(This does _not_ require the toolkit behaviour of always having a current item!)

> In the case where you're scrolling to one end or the other but neither
> selecting nor trying to get a specific message view, it would be nice if the
> current could be set at the end of the sort (as requested in bug 305140 for
> TB).

This would be the case anyway, if we'd make the sort scroll position current.

Comment 7

[Mike Cowperthwaite]

(In reply to comment #6)
> >  If the sort order is, say, by subject,
> >      are you scrolling to (or selecting) the 'A' end or the 'Z' end?
>
> See unchanged code in patch: [...]
> AIUI, with A at the top this should _scroll_ to Z.

But immediately after that |if| is a switch on the sort type; it scrolls to the 'newest' only for Date, Id or Thread sorting (thread being virtually obsolete now).  That is, the logic in this line of comment 1:
> Else, in case we have a sort order, use that.
is really
* Else, in case we have a chronological sort order, user that.

So, when sorting by any other criterion, it scrolls to top of list.


> I (and Neil) consider toolkit's notion of always having a current
> item as broken.

I'm not sure what's gained by the no-current state, other than aesthetically.  I've come to consider the 'current' marker as like the caret in a text field; it's a visual focus indicator (when scrolled within the widget's bound) as well as an indicator of where the action is.


> > In the case where you're scrolling to a new message that you don't want to
> > select, if you set that as current as well, then the user could tab (or
> > Ctrl+Tab) to the thread pane and type space to select it.  Without setting
> > the current in this case, getting to the new message is less accessible.
> 
> Good point.
> (This does _not_ require the toolkit behaviour of always having a current
> item!)

Right, it does not.  Since you also like the idea, should I open a new RFE for that?


> > In the case where you're scrolling to one end or the other but neither
> > selecting nor trying to get a specific message view, it would be nice
> > if the current could be set at the end of the sort (as requested in
> > bug 305140 for TB).
> 
> This would be the case anyway, if we'd make the sort scroll position current.

That's not how it works in TB now: for an ascending sort, TB scrolls the last message into view, but then sets the current on the item at the top of the pane.  This isn't a huge deal but, if you (e.g.) resize the thread pane smaller, the messages at the end are hidden by the splitter; whereas if the current is at the end, it gets dragged along with the resize.

Comment 8

[Karsten Düsterloh]

> it scrolls to the 'newest' only for Date, Id or Thread sorting
> (thread being virtually obsolete now).
> That is, the logic in this line of comment 1:
> > Else, in case we have a sort order, use that.
> is really
> * Else, in case we have a chronological sort order, user that.
> 
> So, when sorting by any other criterion, it scrolls to top of list.

Yeah, you're right, I did misunderstand you.
So I wondered why this strange switch construct does exist there at all - and all I can guess from the code history is that it was introduced back in 2002, while sorttype.byNone was not introduced until 2003...
I think we can drop that switch and check for sorttype.byNone instead, I'll have a look.

> > I (and Neil) consider toolkit's notion of always having a current
> > item as broken.
> 
> I'm not sure what's gained by the no-current state, other than aesthetically. 

You could do context menu actions to items without selecting/showing other items. You can't do this if you always have a valid current item.
For example, let the tree have a current item without a selection, maybe by negating a current singleline selection via ctrl-space. Now right-click a different item, it'll get selected and the appropriate context will be shown. But after you're done with the context action, the current item will be *selected*, which it wasn't before. (I'm not sure yet if this is an actual tree bug.) Having no current item does of course not select any items after ending the context action.

> I've come to consider the 'current' marker as like the caret in a text field;
> it's a visual focus indicator (when scrolled within the widget's bound) as
> well as an indicator of where the action is.
[...]
> Since you also like the idea, should I open a new RFE for that?

Hmm, after some thinking, I'm not that convinced: 
We shouldn't pick an item without any user actions like remember_last_selected, go_to_next_unread, etc. or explicit clicks.

Comment 9

[Mike Cowperthwaite]

(In reply to comment #8)
> So I wondered why this strange switch construct does exist there at all

I think the reason for that switch (ignoring byThread) is that the only time you can say that the upper end of the sort is "more important" than the lower end is with chronological sorting.  I'm not sure if the internal 'byThread' is achievable via the UI any longer, but the net ordering was essentially the same as Order Received.

Of course, you *could* say the same for some of the other types -- status, tag, star, junk, read,  -- but many of those don't sort naturally anyway: sort-by-star (bug 348350) puts the starred -- more important -- messages at the low end of the sort instead of high (compare to sort by Read/Unread, where the unread -- more important -- messages are at the high end); and sort-by-tag is alphabetical rather than by priority/importance.  Sort-by-Status is, to my eyes, completely hosed, in part because there are three orthogonal pieces of information in a single column -- but at any rate, "New" is at the low end of the sort.


> You could do context menu actions to items without selecting/showing other
> items. You can't do this if you always have a valid current item.
> For example, let the tree have a current item without a selection, maybe by
> negating a current singleline selection via ctrl-space. Now right-click a
> different item, it'll get selected and the appropriate context will be shown.
> But after you're done with the context action, the current item will be
> *selected*, which it wasn't before.

Hm.  I think you're looking at the wrong part of this issue as the bug; it's not the existence of the 'current', it's the failure to reset the selection properly after the right-click -- see bug 304676 comment 2.


> Hmm, after some thinking, I'm not that convinced: 
> We shouldn't pick an item without any user actions like
> remember_last_selected, go_to_next_unread, etc. or explicit clicks.

Well, I posit that you have taken this position because you've bought into this "no 'current' is good" model.  If the no-current state is accepted as invalid, then *some* item needs to be "picked," so why not set it on the most likely candidate for selection?  It's no more harmful to put it there than to put it at one end or another of the sort, and arguably more useful.

Comment 10

[Karsten Düsterloh]

> Hm.  I think you're looking at the wrong part of this issue as the bug; it's
> not the existence of the 'current', it's the failure to reset the selection
> properly after the right-click -- see bug 304676 comment 2.

True.

> > We shouldn't pick an item without any user actions like
> > remember_last_selected, go_to_next_unread, etc. or explicit clicks.
> 
> Well, I posit that you have taken this position because you've bought into
> this "no 'current' is good" model.

I do indeed think that a tree widget should be able to have no current item. I'm still trying to make up my mind, though, whether or not the thread pane could do without...

> If the no-current state is accepted as invalid, then *some* item needs to be
> "picked," so why not set it on the most likely candidate for selection?
> It's no more harmful to put it there than to put it at one end or another of
> the sort, and arguably more useful.

No dissent here. It all depends upon the (in)validity of the current item.

Comment 11

[Robert Kaiser]

Comment on attachment 255960 [details] [diff] [review]
sanitize scrolling logic

Looks good to me, first-a=me for 1.1.2, one needed to go

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Landed on trunk to collect real world complains. ;-)

Consider bug 376171 such a complaint.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 255960 [details] [diff] [review]
sanitize scrolling logic

second-a=me

Comment 14

[Karsten Düsterloh]

Landed on MOZILLA_1_8_BRANCH along with the regression fix for bug 376171.

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 255960 [details] [diff] [review]
sanitize scrolling logic

>@@ -226,17 +227,18 @@ var folderListener = {
>-             scrolled = ScrollToMessageAfterFolderLoad(msgFolder);
>+             if (!scrolled)
>+               ScrollToMessageAfterFolderLoad(msgFolder);

While preparing the port to TB, I noticed that
you removed this |scrolled =|, but left (now useless) |return scrolled;| in |ScrollToMessageAfterFolderLoad()|.

Before I proceed,
can you confirm that the former needs to be removed,
and that the later should be kept ?
Thanks.

Comment 16

[Ian Neal]

I would suggest that |return scrolled;| in |ScrollToMessageAfterFolderLoad()| got missed and should be removed. What is the bug number for your port?

Comment 17

[Serge Gautherie (:sgautherie)]

> What is the bug number for your port?

I was planning on doing it here. Should I rather do it in a separate bug ?

Comment 18

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-SM) additional cleanup [Checkin: Comment 20]

*Removes |return scrolled;| in |ScrollToMessageAfterFolderLoad()|, and its associated comment.
*Restores a comment which was removed in current bug and not put back in bug 376171.
*Fixes 2 whitespace nits: 1 from previous patch, 1 noticed during bug 361248.

Comment 19

[Karsten Düsterloh]

> I would suggest that |return scrolled;| in |ScrollToMessageAfterFolderLoad()|
> got missed and should be removed.

Actually not, I left it in on purpose. 
One can discuss, of course, if returning a result which is ignored by all callers is really useful...

Comment 20

[Reed Loden [:reed]]

Checking in mailnews/base/resources/content/msgMail3PaneWindow.js;
/cvsroot/mozilla/mailnews/base/resources/content/msgMail3PaneWindow.js,v  <--  msgMail3PaneWindow.js
new revision: 1.317; previous revision: 1.316
done

Comment 21

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1-TB) Port to Thunderbird [Checkin: Comment 23]

Merges/Ports the 3 SeaMonkey patches: 2 from this bug, 1 from bug 376171.

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 292092 [details] [diff] [review]
(Cv1-TB) Port to Thunderbird
[Checkin: Comment 23]

No review from Scott since "2007-12-07 09:33:40 PST";
asking Phil.

Comment 23

[Reed Loden [:reed]]

Checking in mail/base/content/msgMail3PaneWindow.js;
/cvsroot/mozilla/mail/base/content/msgMail3PaneWindow.js,v  <--  msgMail3PaneWindow.js
new revision: 1.141; previous revision: 1.140
done