Closed Bug 219787 Opened 21 years ago Closed 21 years ago

Clicking thread icon in column header should toggle threaded/flat view (and NOT affect the sort order of the other collumns)

Categories

(SeaMonkey :: MailNews: Message Display, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: u69748, Assigned: sspitzer)

References

Details

(Whiteboard: Please read comment 8)

Attachments

(4 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja-JP; rv:1.5b) Gecko/20030912 Firebird/0.6.1+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ja-JP; rv:1.5b) Gecko/20030912 Firebird/0.6.1+

clicking thread icon in column header should switch thread view and flat view.

I often switch thread view and flat view.
After fix of bug 72493, we have no easy way to
switch from thread view to flat view.

Mozilla 1.4:
flat -> thread : click thread icon in column header
thread -> flat : click column headers (ex Subject, Date, ,..)
I can switch by one click in both cases.

Mozilla 1.5rc1:
flat -> thread : click thread icon in column header
thread -> flat : we have only View | Message | Threaded


Reproducible: Always

Steps to Reproduce:
> Mozilla 1.5rc1:
> flat -> thread : click thread icon in column header
> thread -> flat : we have only View | Message | Threaded

Sorry, I'm using trunk 20030919

Mozilla trunk 20030919:
flat -> thread : click thread icon in column header
thread -> flat : we have only View | Message | Threaded

This bug is what I had boped bug 72493 would achieve. I hope the intentionof
this bug is to be able to toggle the threaded view on/off *without* affecting
the sort order of the other collums (subject, date, etc.). If so, I would
suggest to clarify this in the summary:

"Clicking thread icon in column header should toggle threaded/flat view (and NOT
affect the sort order of the other collumns)"
At least Thunderbird already seems to do what this bug is requesting (can't test
Mozilla right now). Unfortunately, when going back to flat view, TB reverts back
to the "default" sort order (date:ascending), and not the sort order that the
user had chosen before entering (or while in) threaded view.

Based on this, you might want to consider rewording the summary. Then I could
"confirm" this bug. ;)
(from Comment #2)

Peter, you are right.
I have changed the summary as your suggestion.
Summary: clicking thread icon in column header should switch thread view and flat view. → Clicking thread icon in column header should toggle threaded/flat view (and NOT affect the sort order of the other collumns)
Status: UNCONFIRMED → NEW
Ever confirmed: true
this works fine for me - you haven't set "mailnews.thread_pane_column_unthreads"
to false, have you?
I have tried both true/false for "mailnews.thread_pane_column_unthreads"
on Mozilla-trunk-2003092409 and Thunderbird 0.3RC2.
But it dose not affect the behavior of clicking thread icon.

(0) View|Mesages|Threaded (switch to flat view)
(1) 1st click: switch to thread view
(2) 2nd click: reverse order
(3) 3rd click: normal order: same as (1)
(4) 4th click: same as (2)
...
oh, sorry, I see what you want. You want the tri-state action, or something like
that. Clicking on the thread column when in a flat sort does switch to threaded
view, but you want it to switch out of threaded view as well, which it doesn't do.
> (2) 2nd click: reverse order

Please NO! Clicking the "thread icon" should *only* toggle between threaded and
non-threaded. The sort oder is *only* managed by clicking the other column
headers. That's exactly why I asked you to edit the summary before I would
verify this bug.

Please carefully read bug 72493 comment 15 (and the link mentioned in that comment).
I understand your position - I just don't agree with it.
> I just don't agree with it.

Why ?

I don't understand why clicking thread icon is *1 WAY ACTION* to
switch from flat view to thread view.
Why it dose not have function to switch back to flat view?
imho, the current behavior is far from intuitive. or it's just bizarre.

i do not think people might ever expect clicking on the icon change
its _sort order_ while fist clicking changes its _view_.

if it changes its view, it should stick with changing the view.
what if clicking the icon eventually performs like delete button? no way.

swtiching the view between thread and flat is the behavior that
people natually expect.
Whiteboard: Please read comment 8
I'm not sure that people *expect* toggling between flat/threaded; all the other 
column headers, when clicked, first sort according to the criterion, then 
subsequently reverse the sort order.  In that sense, the current behavior is 
consistent.

However, the sort order of the threads is in fact controlled by some other 
field.

Try this: configure the thread pane to show Order Received; and set
 mailnews.thread_pane_column_unthreads  to false.
Start with a flat display; then click the Thread header (twice).  There is now a 
little sort arrow in the Order Received header.  NOW, click the Thread header: 
the Order Received sort arrow reverses direction.
If some other field (e.g. date) is used as the sort criterion, and then the 
Thread header changes the sort criterion to Order Received.

I think the Thread header should not change the sort order at all; it should 
behave exactly as View|Messages|Threaded currently does.  (Instead, it behaves 
as View|Sort by|Thread does -- which behavior, as I argue in bug 219620, should 
be eliminated.)

However, if that change is not made, then I would say that clicking the Thread 
header the *first* time should put the sort arrow into the Order Received 
header, and its failure to do so is a (rather minor) bug.
Also: if the thread column header is a threaded/flat toggle, then we could do 
away with the thread_pane_column_unthreads preference, essentially forcing it to 
false.

Then the column headers never act differently from the SortBy menu items (that 
is, neither method unthreads), but unthreading is easy to do from the column 
headers by clicking the Thread header.
Re comment #12: No, I seriously doubt people "expect" an incorrect behavior (I
sure don't). Have you carefully read bug 72493 comment 15? Have you carefully
read http://www.jwz.org/doc/threading.html ? If you did, you would understand
that threading and sorting (by subject, sender, date, ...) are completely
separate actions. A quote from that page:

"Threading is the act of presenting parent/child relationships, whereas sorting
is the act of ordering siblings."

I put the following comment into bug 219620, but it is more applicable here:

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.

This may be (very) difficult to implement (to thread by something other than
date), but it would be the correct way to go. 

As an interim solution, *if* necessary, we could make it so if the user causes
the threaded state, then the message is threaded by date (received?), clicking
the thread collumn again would go back to flat state (sorted by date? or by the
sort collumn selected before the threading?). However, clicking the date collumn
should *not* cause a threaded display to become unthreaded.
Re: Comment #12

maybe people first have no paticular expectation on clicking the icon.
but when people click on it, they find it change its look to thread view.
and they understand it change the _view_, not sort order.

note that other columns only change sort order, not view.

and if you would not know what would be the purpose of the icon and
happen to click on it (and its view was changed to thread view), how
would you expect to get back to flat view?

you will never click on the same icon and go to find the command in
menu first?  if so, you are too smart. i doubt ordinally people ever
do that.

i would try clicking on the icon again and again in order to get it
back to flat view and end up finding it never will (and probably scream).

if clicking on the icon only changes sort order to `Order Received' and
toggle ascending and descending and does not change its view to thread
like other columns do not, i would agree with you that its behavior is
consistent enough.

in short, changing two different things (view and sort order)
simultaneously is definitely confusing.
In comment 12, I noted:

> I would say that clicking the Thread 
> header the *first* time should put the sort arrow into the Order Received 
> header, and its failure to do so is a (rather minor) bug.

This symptom, and a related one with View|Messages|Threaded, is now discussed in 
depth in bug 199217.
Attached patch Proposed patch (obsolete) — Splinter Review
First take at a patch for this (using 1.6a-1003 as the basis)
Uses mailnews.thread_pane_column_unthreads to switch between complex
(old-style) and simple columns.
 - Complex (pref true): Clicking thread-column header always leaves messages in
threaded mode, will reverse sort if necessary; tweaked so that now it ALWAYS
actually sorts by "order received" (aka "ID").	Clicking any other column
header unthreads, if appropriate, and resorts/reverses, as has always happened.

 - Simple (pref false): Clicking thread-column header always toggles between
threaded and unthreaded without affecting sort criterion or order.  Clicking
any other column header simply resorts/reverses without unthreading.

This fix also addresses the first symptom listed at (the end of) bug 199217
comment 7.  For completeness, two additional minor fixes address the other two
symptoms listed there.
Comment on attachment 132681 [details] [diff] [review]
Proposed patch

The above patch works with 1.5 Final (altho the symptom of bug 218656 is only
fixed in 1.6a).

I imagine a different name for the pref is desirable, but I'm not sure how
many/which files need to be hacked up for that.
Attachment #132681 - Flags: review?(bienvenu)
Mike, this generally looks OK - I'm going to apply the patch (I hope tomorrow)
and try it out.
Attached patch diff against the trunk (obsolete) — Splinter Review
I tried this - it seems to work well. I had one instance where clicking on the
thread icon when in unthreaded mode didn't do anything, but I can't recreate it
(I might have been in quick search mode).

Re the name of the pref - it's trivial to change - it occurs in threadPane.js,
and mailnews.js to give it a default.
Comment on attachment 132681 [details] [diff] [review]
Proposed patch

IMHO the code is confusing, do you find this any more straightforward?

if (thread column header clicked) {
   /* check threading support here */
   if (simple columns) {
     MsgToggleTheaded();
   } else if (already threaded) {
     MsgReverseSortThreadPane();
   } else {
     /* turn threading on here */
     MsgSortThreadPane(byId);
   }
} else {
  if (!simple columns) {
    /* unthread here */
  }
  if (same sort type) {
    MsgReverseSortThreadPane();
  } else {
    MsgSortThreadPane(sortType);
  }
}
OK, Neil, I see where you're going with that.  I was trying to minimally disrupt 
the existing code, since I'm hardly an adept in this domain.

If we're going for pure straightforwardness, what I'd like to see is having the 
threadedness of the sort controlled either by a method of dbview, or else by a 
parameter of dbview.sort(); it should not be a direct bit manipulation of some 
flagword member of dbview.  (I'm not going to submit code towards that end 
because I'm not going to deal with any code that's not being downloaded as part 
of the Mozilla install package -- I'm on a 28.8 dialup, and I refuse to deal 
with CVS.)

Also: if the logic in my patch is accepted, then as far as I know dbview.sort() 
is never called with the sorttype set to byThread -- if I'm correct about that, 
then any support that still exists for byThread in dbview should be removed and 
that method be streamlined accordingly.


As far as the change in logic to my patch: MsgReverseSortThreadPane() is only 
called from the HandleColumnClick() method; MsgToggleThread(), I think, is 
currently not called from anywhere.  My preference would be to eliminate these 
methods, and directly call dbview.sort() at the end of HandleColumnClick(), with 
local variables that have been set according to the logic as currently exists.

This is actually a different direction than Neil's suggestion, but those two 
methods have no use outside of HandleColumnClick() and everything eventually 
percolates down to a dbview.sort() call followed by an UpdateSortIndicators() 
call.  Having well-named local members should be no more confusing that having 
calls to well-named methods.  I will try to create a patch according to Neil's 
schematic, and one according to what I've outlined here; whichever one is found 
acceptable can be checked in -- I'm more interested in getting the thing 
working, and I'm sure I'll always be at odds with at least *some* of the 
decisions in the Mozilla codebase.  

(Like the loathsome brace style, f'rinstance.  :)


Regarding the pref name: David, did you want me to submit a diff file to change 
that?  Since I'm not hooked in to CVS, you'd still end up having to reintegrate 
that diff to the trunk.  We could name the pref 
  mailnews.complex_column_sorting
to maintain the same current true/false sense; or else
  mailnews.simple_column_sorting
which would require a reversal of the current patch's sense.  

Does the initialization code require checking for a current instance of 
mailnews.thread_pane_column_unthreads and setting the value of the new pref to 
match (either straight, or toggling, per the decision above).

My preference, of course, is to make the *default* be Simple.
I'll try to rework this today to incorporate Neil's suggestions.
Status: NEW → ASSIGNED
Attachment #132681 - Attachment is obsolete: true
Attachment #134536 - Attachment is obsolete: true
+  var threadingToggled = false;

This var is now unused.



+    else // sort by thread - use MsgToggleThreaded when bug 223970 is fixed
+      MsgSortThreadPane(sortType); 

There is nothing in this clause to turn threading on, nor to change the sorting 
criterion to 'byID'; you'll need to change this to:
+    else { // sort by thread - use MsgToggleThreaded when bug 223970 is fixed
+      dbview.viewFlags &= ~nsMsgViewFlagsType.kThreadedDisplay;
+      MsgSortThreadPane(nsMsgViewSortType.byId); 
+    }

Also: I'm not 100% sure what's going to change with bug 223970, but: is calling 
MsgToggleThreaded() here going to do the trick?  You need to Sort Ascending, and 
change to the sort criterion, to get complex columns to work as they should.
(I know that currently, the first click to the thread column does NOT reset the 
criterion to byID, but it should; the current logic is not quite right here:
-  if (sortType == nsMsgViewSortType.byThread &&
             (dbview.viewFlags & nsMsgViewFlagsType.kThreadedDisplay))
-    sortType = nsMsgViewSortType.byId;
That test for "am I threaded now" is superfluous.



+   if (!simpleColumns) 
+      // complex (old-style) columns: unthread for all non-thread columns
+      dbview.viewFlags &= ~nsMsgViewFlagsType.kThreadedDisplay;
+    if (dbview.sortType == sortType) 
+      MsgReverseSortThreadPane();

I discovered a small bug in my original patch in this area: with complex 
columns, if the display is threaded and you click on the OrderReceived column, 
it unthreads as expected, but the order of the column is reversed from the 
current sort order, rather than being forced to Ascending -- that is because the 
dbview.sortType *is* sortType.  I fixed this tweak originally by adding code to 
set threadingToggled; it can be fixed in the current version by changing the 
first lines to:
+   if (!simpleColumns) {
+      // tweak dbview.sortType to force ascendant sort:
+      dbview.sortType = nsMsgViewSortType.byThread;
+      // complex (old-style) columns: unthread for all non-thread columns
+      dbview.viewFlags &= ~nsMsgViewFlagsType.kThreadedDisplay;
+   }
Here is my version of the patch which goes in the opposite direction from that
suggested by Neil -- rather than call MsgSortThreadPane(), MsgToggleThreaded(),
or MsgReverseSortThreadPane() directly, it maintains the three sort parameters
locally and sets dbview accordingly at the end of the method.  To my mind this
is a cleaner approach, but I understand the opposing arguments and have no
problem with the basic structure of Neil's approach.


One additions thing that this patch does is change UpdateSortIndicators(). One
of the changes in my original patch, maintained in David's, is:
-  UpdateSortIndicators(dbview.sortType, nsMsgViewSortOrder.ascending);
+  UpdateSortIndicators(dbview.sortType, dbview.sortOrder);

This was probably due to code-by-copy-and-paste -- but really,
UpdateSortIndicators() should NEVER disagree with with dbview.	In this patch,
UpdateSortIndicators is passed only the dbview argument, and directly uses its
sortType and sortOrder members within the method.

Alternately, you could pass no argument at all and call GetDBView() internally,
but every single method that calls UpdateSortIndicators() has *already* called
GetDBView(), so I thought I'd save the extra function call.  (Maybe that's not
necessary if the call optimizes to a member fetch.)
I fixed the unused var, thx.

>+    else // sort by thread - use MsgToggleThreaded when bug 223970 is fixed
>+      MsgSortThreadPane(sortType); 

>There is nothing in this clause to turn threading on, nor to change the sorting 
>criterion to 'byID'; you'll need to change this to:...

Since sortType is byThread, MsgSortThreadPane(sortType) does the right thing. 

>-  if (sortType == nsMsgViewSortType.byThread &&
>             (dbview.viewFlags & nsMsgViewFlagsType.kThreadedDisplay))
>-    sortType = nsMsgViewSortType.byId;

I'm not sure what this comment refers to - this code has been removed and I
don't see any other superfluous remaining checks for isThreaded...

I've added the code you suggested for doing a flat sort by order received. 

I'll attach a new patch in a minute.
Actaully the sort by thread isn't to fix bug 223970, that's caused by
unthreading... although sort by thread will use a saved threaded sort.
> Since sortType is byThread, MsgSortThreadPane(sortType) does the right thing. 

Not quite:  In the current builds, if you click the Thread column once, no 
column shows a sort-arrow, but subsequent clicks put the arrow in OrderReceived 
-- which makes a lot of sense to me.  I thought that the arrow in OrderReceived 
for byThread sorting was desirable.

Therefore, in the patch, you have to change byThread to byId before calling 
sort(): otherwise the indicator on the OrderReceived column does not update on 
the first click.  And therefore, you have to set the threading flag externally 
before calling sort() as well.
Here's a better idea: instead of putting in the explicit code to thread, replace 
the call of   MsgSortThreadPane(sortType)   with   MsgSortByThread().
That will do the right thing, and you'll probably want to put whatever fix is 
necessary for the saved-thread-view issue into MsgSortByThread().
To see if I've understood it correctly :-)
Actaully Mike's UpdateSortIndicators changes don't take two things into account:
1. Two other js files use UpdateSortIndicators, commandglue.js and searchBar.js
2. UpdateSortIndicators already uses gDBView, so it should be able to figure out
the indicators without being passed anything - which raises another question:
should it really be using GetDBView() instead?
Actually Mike's approach paves the way for a future extension:
dbview.Sort(sortType, sortOrder, sortFlags);
which I hope could solve bug 223970.
David, I took the liberty of updating attachment 134992 [details] [diff] [review] to what I think covers
all the subsequent comments.
Comment on attachment 135020 [details] [diff] [review]
Updated patch based on my comments

looks good to me.
Attachment #135020 - Flags: superreview+
That patch addresses everything I wanted to see; thanks!

I did miss those external calls to UpdateSortIndicators(); they could be changed 
in the same way as my suggestion (Attachment 134997 [details]), since, again, each of the 
two calls has an instance of a db-view object to pass in.  I just think it would 
be more maintainable to query the object directly for that data rather than 
relying on programmers to pass 'em in correctly.


Is it important to change the preference name before proceeding?


:::BY THE WAY:::
When are Windows builds going to show up in the nightly/latest-trunk directory 
again?  They've been stuck at 28-Oct since, um, sometime earlier this week. (I 
have a 30-Oct Windows build that I d/l'd on the weekend.)  I'd download tomorrow 
to verify this patch, but I don't have any reason to expect a new build.
Comment on attachment 135020 [details] [diff] [review]
Updated patch based on my comments

Scott, can we get an r= for this? I'd really like to get this checked in.
Attachment #135020 - Flags: review?(mscott)
Attachment #135020 - Flags: review?(mscott) → review+
This fix is present in:
  Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031114

The only thing that might be hanging at this point is whether the preference 
name needs to be changed.  But I'm happy!  Thanks, David & Neil.
marking fixed. I don't have any good ideas about a new name for the pref. I
think we might want to have a prefs UI for the hidden pref, but that can be a
new bug.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
*** Bug 225416 has been marked as a duplicate of this bug. ***
*** Bug 231184 has been marked as a duplicate of this bug. ***
Attachment #132681 - Flags: review?(bienvenu)
This is not fixed in Thunderbird 0.5/Linux. Will it find its way there
automatically, or should another bug be submitted?
(In reply to comment #42)
> This is not fixed in Thunderbird 0.5/Linux. Will it find its way there
> automatically, or should another bug be submitted?

Um, I can't guarantee this, but this was a JavaScript-level fix and it is 
present in the Windows version of TB 0.5 that I'm running, so I think it must be 
in the Linux version as well.  It is necessary to add this line to prefs.js:
  user_pref("mailnews.thread_pane_column_unthreads", false);
*** Bug 224030 has been marked as a duplicate of this bug. ***
*** Bug 265615 has been marked as a duplicate of this bug. ***
Adding a hidden preference which defaults to the broken behaviour is NOT a fix,
as the continued duplicate reports show.
PING

The default behavior is very confusing. Threaded views are virtually unusable if
you like to sort by clicking on the columns, because when you return to the
threaded view your recent messages are squirreled away in hard to find threads.

At the very least, clicking the thread icon should default to "by date" instead
of "by order received".

Adam
I am trying out that hidden preference and it indeed is the correct behavior.

The default needs to be changed.  Seriously.  The current behavior is inexplicable.

But if you must leave that variable as is, then at least change the default sort
to Date when turning on threads.  Then copy all sent messages into your Inbox
and you are in email reading heaven.

The only remaining bug is: I have to constantly leave the folder and come back
to get the correct sort order.  That should at least be a hidden preference.

So ... can we re-open this bug?  I'd do it but I don't what to **** anybody off.
Product: Browser → Seamonkey
(In reply to comment #48)
> The default needs to be changed.  Seriously.  The current behavior is
> inexplicable.

But historical.  However, there is a UI for this, implemented in bug 234690.


> The only remaining bug is: I have to constantly leave the folder and come back
> to get the correct sort order.  That should at least be a hidden preference.

I don't understand what this comment has to do with this bug.  Do you mean you 
want the threads to resort every time a new message arrives?  That is a desire 
that *I* find inexplicable; however, bug 262319 requests exactly that.
*** Bug 251423 has been marked as a duplicate of this bug. ***
*** Bug 311210 has been marked as a duplicate of this bug. ***
*** Bug 318313 has been marked as a duplicate of this bug. ***
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: