219620 - Rationalize Thread Messages menu items

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Mike Cowperthwaite]

Carried over from bug 72493 comment 81:

With the fix for 72493, the menu item   View | Sort by | Thread   still exists, 
tucked into the middle of the list as if it were a sort order.  This is in 
addition to the 'new' menu item   View | Messages | Threaded.

The two menu items currently behave differently:
  Messages|Threaded   leaves the sort order alone.
  SortBy|Thread       sets the sorting to Order Received.

Now that the effort has been made to apply the user's sort order to the threads, 
I think only one menu item, which doesn't change the sort order, is called for.  
If the user actually wants to sort by Order Received, that option is still 
present.

If the menu design were up to me, I would eliminate 'Threaded' from 
 View | Messages   and then move 'Thread' in the SortBy submenu into a separate 
menu group (as 'Threaded' is currently), but at the BOTTOM of the submenu.  I 
would also move Order Received to the end of the first menu group, and otherwise 
reorder to put the more-likely-used sort orders at the top, i.e.
  Date
  Subject
  Sender
  Recipient
  Priority
  Label   
  Size    
  Flag    
  Read    
  Reply Status       [this is currently just 'Status']
  Junk Status
  Order Received 
  --------------                      
  Threaded


Note that selecting an option from the  View | Threads  submenu also selects 
Order Received as the sort criterion.  I am undecided whether these menu items 
also should maintain whatever sort order was previously selected.

Comment 1

[Mike Cowperthwaite]

Note that, since selecting  Sort By|Thread  sets the selection criterion to 
Order Received, the Thread item in that submenu cannot be checked except as a 
carryover from a profile created with an earlier version of Mozilla -- all the 
more reason to move it out from among the rest of the items in the submenu.

Also, in case it wasn't clear in my original report, I would expect that the 
Sort By | Thread item behave as the current  Messages | Threaded  item does now: 
a checked (rather than radio) item, that does not affect the Sort By criterion.

Comment 2

[Peter Lairo]

I like most of the ideas in this bug. Message threading is currently a mess. 

I'm not sure the "Threaded" item should be moved from View|Messages to
View|SortBy, because the Threaded status is Yes/No (it's not a "sort criteria").
There is no "sort order" in the state called "Threaded".

Also, maybe the menu-item View|Threads should be grayed out if
View|Messages|Threaded is de-selected.

BTW, I stil miss the ability to be able to click on the collumn headers
"threaded" and "subject|sender|date|etc." *separately* to turn on/off threading
and sort, independently from each other - as I had mentioned in bug 72493
comment 28.

Comment 3

[Mike Cowperthwaite]

> I'm not sure the "Threaded" item should be moved from View|Messages to
> View|SortBy, because the Threaded status is Yes/No (it's not a "sort
> criteria").  There is no "sort order" in the state called "Threaded".

As I noted in the original bug, I expected you to make that objection; and as I 
noted there, threading is more akin to sorting than it is to filtering, which is 
what the View|Messages and View|Threads menus are controlling.  Threading a 
bunch of messages is in fact sorting them into groups, altho that sort is not 
ordered (ascending/descending).

Also: since there is a Thread column in the three pane and (as argued in bug 
219787) it should toggle between flat and threaded, it makes sense for the 
corresponding menu item to be grouped with the items corresponding to the other 
column headers.

> Also, maybe the menu-item View|Threads should be grayed out if
> View|Messages|Threaded is de-selected.

I would not do this, because then selecting a particular thread-filter becomes a 
two-step process.

Comment 4

[Peter Lairo]

> threading is more akin to sorting than it is to filtering, 
> which is what the View|Messages and View|Threads menus are controlling.

Fine, however, if it is to be listed under "View | Sort By", then it should be
*separated* from the "sort" criteria by a dividing line, similar to the
"ascending "and "descending" items. It should then look something like one of
these (my preference: "C"):

                        -A-                   -B-                   -C-
View | Sort By | +----------------+ or +----------------+ or +----------------+ 
                 |x Threaded      |    | Date           |    | Date           |
                 |----------------|    | Flag           |    | Flag           |
                 | Date           |    | Order Received |    | Order Received |    
                 | Flag           |    | ...            |    | ...            |
                 | Order Received |    |----------------|    |----------------|
                 | ...            |    |x Threaded      |    |  Ascending     |
                 |----------------|    |----------------|    |o Descending    |
                 |  Ascending     |    |  Ascending     |    |----------------|    
                 |o Descending    |    |o Descending    |    |x Threaded      |
                 +----------------+    +----------------+    +----------------+

> I would not do this [gray out "View|Threads"], because then 
> selecting a particular thread-filter becomes a two-step process.

I would still gray it out, because otherwise, selecting an item there could
cause the (possibly unselected) "Threaed" item (in *another* submenu!) to change
state. Also, if it were to *not* cause the unselected "Threaded" state to change
state, then it would be confusing if the "View|Threads" selection did *nothing*
in this case. But this is a minor issue, IMO.

PS. I just want to be able to click on the "threaded" and "sorting" collumn
headers *separately* (clicking thread-state does *not* affect sorting, clicking
sorted-state does *not* affect thread-state). So they would look/act like this:

Initial (example) state:
 | threaded   | subject | sender   | date /\| size |

click date collumn:
 | threaded   | subject | sender   | date \/| size |

click thread-state collumn:
 | UNthreaded | subject | sender   | date \/| size |

click sender collumn:
 | UNthreaded | subject | sender \/| date   | size |

click thread-state collumn:
 | threaded   | subject | sender \/| date   | size |

click sender collumn:
 | threaded   | subject | sender /\| date   | size |

click date collumn:
 | threaded   | subject | sender   | date \/| size |

click thread-state collumn:
 | UNthreaded | subject | sender   | date \/| size |

etc.

Comment 5

[Mike Cowperthwaite]

An additional issue that I just noticed: if one of the MailViews is selected 
under View|Messages, the View|Messages|Threaded item is enabled and checkable. 
Since MailViews are currently not threadable, this causes a bit of confusion.

If a MailView is selected from a Threaded view, the Threaded menu item is 
automatically unchecked; and then automatically re-checked when the view is 
restored to All, and the messages are shown in their earlier Threaded view.  If 
the menu item is checked while in the MailView, then on restoring the view to 
All, the menu item remains checked.

If a MailView is selected from a flat view, then the Threaded menu item is 
manually checked, it is unchecked automatically if the view is restored to All 
and the messages are shown flat.

Since checking the menu item makes no difference in this context, when a Mail 
View is selected, the Threaded menu item should be disabled.  In fact, the 
View|SortBy|Thread item *is* disabled in that situation.

Comment 6

[[e-mail]]

CC'ing some folks who might be able to provide some input. At the very least the
view/messages/threaded option should be disabled for unthreadable views,
assuming that there isn't another bug already reported.

Comment 7

[Stefan Borggraefe]

There seems to be a general consent about the removal of View|Sort By|Thread. I
agree to this to.

I think the menu item "View|Messages|[x] Threaded" should be replaced by two new
menu items in the Sort By menu below "Descending":

|   .....     |
|-------------|
|(*) Threaded |
|( ) Flat     |
+-------------+

Reasons:
 - like "Ascending" and "Descending", "Threaded" is an additonal property of the
other search criteria (except "Thread", which is another reason to remove it).
 - when "Threaded" is moved out of the "Messages" menu it will be identical to
the "View"-dropdown in 3-pane. So we gain consistency.
 - I think the option is more clear to the user when the alternative "Flat" is
written out. ("Ascending" could be a menu item with a checkmark too, but isn't
for the same reason).

I think Mike and I have pretty much the same idea about this except my
additional idea to have two radio-menu items "Threaded" and "Flat".


Furthermore I think the menu items in the "Threads" menu shouldn't change the
"Threaded" option. This is already true for all menu items in this submenu
except for "Threads with Unread" and "Watched Threads with Unread" (this was
confused in the discussion so far).
Perhaps we should move this aspect and the other ideas stated in this bug to
other bugs and just concentrate on the removal of "Sort By|Thread" and the
movement of "Threaded" in this bug to make it easier to handle the discussion.

Comment 8

[Greg Colyer]

It would be even more like "Ascending/Descending" if "Flat" were instead
"Unthreaded".

Comment 9

[Mike Cowperthwaite]

I think changing 'Threaded' from a single check-item to dual radio-items is 
perfectly fine.

per Stefan's comment 7:  I did not realize that  View|Threads|Unread  and  
View|Threads|All  did not affect the threadedness.  In fact, I thought that 
turning off threading somehow disabled all those View|Threads 'filters'; but I 
now see that you can, for instance,  View|Threads|Threads with Unread  and then 
turn threading off and see the same set of messages, only flat.  That's very 
interesting, but I now think that interface even more confusing.  As Stefan 
said, that is an issue for another bug.

For this bug, I see the following:
(1) remove  Sort By|Thread;
(2) morph  Messages|Threaded  into  Sort By|Threaded/Unthreaded (above or below 
the Ascending/Descending pair?  I would say below);
(3) disable  Sort By|Threaded/Unthreaded  when a MessageView is selected (per 
comment 5).

The other issue is whether to re-order the items in the Sort By submenu, along 
the lines suggested in the original report.  Perhaps that's a separate bug too.

Comment 10

[Stefan Borggraefe]

Attachment: Patch

Patch that implements exactly the three items from comment 3 for Seamonkey and
Thunderbird.

Comment 11

[Stefan Borggraefe]

Attachment: Documentation update

I will ask for reviews for this when/if the patch got r+ and sr+. Though if
someone wants to proofread this right now, I won't stop him. ;-)

Comment 12

[Stefan Borggraefe]

(In reply to comment #10)
> Patch that implements exactly the three items from comment 3 for Seamonkey and
> Thunderbird.

I meant comment 9 of course.

Comment 13

[[e-mail]]

Comment on attachment 142941 [details] [diff] [review]
Patch

> function MsgToggleThreaded()
> {
>-  var dbview = GetDBView();
>-  dbview.viewFlags ^= nsMsgViewFlagsType.kThreadedDisplay;
>-  dbview.sort(dbview.sortType, dbview.sortOrder); // resort
>-  UpdateSortIndicators(dbview.sortType, dbview.sortOrder);
>+    MsgSortThreaded(!(GetDBView().viewFlags & nsMsgViewFlagsType.kThreadedDisplay));
>+}
>+
>+function MsgSortThreaded(threaded)
>+{
>+    var dbview = GetDBView();
>+    var alreadyThreaded = (dbview.viewFlags & nsMsgViewFlagsType.kThreadedDisplay);
>+
>+    if (alreadyThreaded != threaded)
alreadyThreaded is an int. threaded is a bool. oops.

>+        dbview.viewFlags ^= nsMsgViewFlagsType.kThreadedDisplay;
>+
>+    dbview.sort(dbview.sortType, dbview.sortOrder); // resort
>+    UpdateSortIndicators(dbview.sortType, dbview.sortOrder);
> }
This looks ugly - surely if you want to share code you would do something along
the lines of
function MsgSortUnthreaded()
{
    if (GetDBView().viewFlags & nsMsgViewFlagsType.kThreadedDisplay)
	MsgToggleThreaded();
}

>+    var threaded =
>+        ((gDBView.viewFlags & nsMsgViewFlagsType.kThreadedDisplay) != 0);
Can't see why you wrapped this...

> function InitViewMessageViewMenu()
> {
>   var viewFlags = gDBView ? gDBView.viewFlags : 0;
>-
>-	var threadedMenuItem = document.getElementById("viewThreaded");
>-	if (threadedMenuItem)
>-		threadedMenuItem.setAttribute("checked", (viewFlags & nsMsgViewFlagsType.kThreadedDisplay) != 0);
>-
This is the only part of that function that uses viewFlags...

Comment 14

[Stefan Borggraefe]

Attachment: Patch V1.1

Addressed all review comments, otherwise unchanged.

Comment 15

[[e-mail]]

Comment on attachment 142981 [details] [diff] [review]
Patch V1.1

Nit: you changed the indent level in threadPane for some reason, it should be
two spaces, not four.

Comment 16

[Stefan Borggraefe]

Comment on attachment 142981 [details] [diff] [review]
Patch V1.1

I indented my changes to threadPane.js with four spaces because of the emacs
modeline in this file.

Comment 17

[Scott MacGregor]

Comment on attachment 142981 [details] [diff] [review]
Patch V1.1

thanks stefan!

Comment 18

[[e-mail]]

Comment on attachment 142942 [details] [diff] [review]
Documentation update

>+<p><strong>Tip</strong>: The thread button automatically sorts the threads
>+after the age of their parent messages. If you want to use another
This is untrue, the thread button maintains the sort... also, sort by date now
uses the date of the newest message.

>+sort criterion for the threads, open the View menu and select the desired
>+option from the Search by submenu.</p>
I assume you meant Sort by submenu ;-)

Comment 19

[[e-mail]]

Or is it? I've forgotton what the default preference is...

Comment 20

[Mike Cowperthwaite]

Per bug 219787:
If mailnews.thread_pane_column_unthreads is true (default, "complex"):
  clicking Thread column header goes into threaded mode and sorts by Order 
Received; subsequent clicks reverse sort-order (still on Order Received).
  clicking any other column header goes into flat mode and sorts on that column; 
subsequent clicks reverse sort-order.

If mailnews.thread_pane_column_unthreads is false ("simple")
  clicking Thread column header toggles between threaded and flat, does not 
affect sort order
  clicking any other column header sorts on that column (maintaining current 
flat/thread mode); subsequent clicks reverse sort-order.

Comment 21

[[e-mail]]

Comment on attachment 142942 [details] [diff] [review]
Documentation update

Sorry, I had the pref set to opposite of default :-[
Just fix Sort by.

Comment 22

[Stefan Borggraefe]

Attachment: Documentation Update V1.1

Changes Search to Sort. Don't know how I overlooked this. ;-)

Comment 23

[Stefan Borggraefe]

Comment on attachment 143575 [details] [diff] [review]
Documentation Update V1.1

<NeilAway> right, Search -> Sort ... and it might be an idea to request review
from rj_keller ([e-mail]) just in case he has any questions

Ok, doing so.

Comment 24

[Mike Cowperthwaite]

See bug 237164.

Comment 25

[R.J. Keller]

>  <ul>
> -  <li><span class="ui">Threaded</span>: Choose this option to view messages
> -    organized into threads.</li>

Why is this removed? I still see it in the menu.

Comment 26

[Stefan Borggraefe]

(In reply to comment #25)
> >  <ul>
> > -  <li><span class="ui">Threaded</span>: Choose this option to view messages
> > -    organized into threads.</li>
> 
> Why is this removed? I still see it in the menu.

This is one effect of attachment 142981 [details] [diff] [review] in this bug which is not checked in yet.
I will checkin these both together, if I get approval before the  	Localization
string freeze.

Comment 27

[Stefan Borggraefe]

Comment on attachment 142981 [details] [diff] [review]
Patch V1.1

Asking for approval for 1.7b. This makes the menu items for threaded message
display less confusing. Low risk.

Comment 28

[R.J. Keller]

Comment on attachment 143575 [details] [diff] [review]
Documentation Update V1.1

I'm sorry. I didn't realize the patch to remove the menuitem was in this bug.
r=[e-mail] on the help patch.

Comment 29

[chris hofmann]

Comment on attachment 142981 [details] [diff] [review]
Patch V1.1

a=chofmann for 1.7b

Comment 30

[Stefan Borggraefe]

Comment on attachment 142981 [details] [diff] [review]
Patch V1.1

I just checked this in. I'm leaving the bug open for the Documentation update.

Comment 31

[chris hofmann]

Comment on attachment 143575 [details] [diff] [review]
Documentation Update V1.1

a=chofmann for 1.7b...	need to get this in quickly

Comment 32

[Stefan Borggraefe]

Documentation update checked in.

Marking FIXED.

Comment 33

[Mike Cowperthwaite]

Verified: changes are as expected (see comment 9)

Comment 34

[Malcolm Smith]

This is not fixed in Thunderbird 0.5/Linux. Will it find its way there
automatically, or should another bug be submitted?

Comment 35

[Stefan Borggraefe]

(In reply to comment #34)
> This is not fixed in Thunderbird 0.5/Linux. Will it find its way there
> automatically, or should another bug be submitted?

Thunderbird 0.5 was released on 5th February and this bug was fixed on 12th
March. It would be kind of scary if you found this fix in Thunderbird 0.5. ;-)

If you would like to help Mozilla QA in Bugzilla, you should better use a
current nightly build.