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)
SeaMonkey
MailNews: Message Display
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: u69748, Assigned: sspitzer)
References
Details
(Whiteboard: Please read comment 8)
Attachments
(4 files, 2 obsolete files)
3.35 KB,
patch
|
Details | Diff | Splinter Review | |
5.77 KB,
text/plain
|
Details | |
3.73 KB,
patch
|
Details | Diff | Splinter Review | |
2.85 KB,
patch
|
mscott
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
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
Comment 2•21 years ago
|
||
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)"
Comment 3•21 years ago
|
||
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)
Updated•21 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 5•21 years ago
|
||
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) ...
Comment 7•21 years ago
|
||
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.
Comment 8•21 years ago
|
||
> (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).
Comment 9•21 years ago
|
||
I understand your position - I just don't agree with it.
Reporter | ||
Comment 10•21 years ago
|
||
> 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?
Comment 11•21 years ago
|
||
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.
Updated•21 years ago
|
Whiteboard: Please read comment 8
Comment 12•21 years ago
|
||
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.
Comment 13•21 years ago
|
||
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.
Comment 14•21 years ago
|
||
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.
Comment 15•21 years ago
|
||
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.
Comment 16•21 years ago
|
||
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.
Comment 17•21 years ago
|
||
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 18•21 years ago
|
||
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)
Comment 19•21 years ago
|
||
Mike, this generally looks OK - I'm going to apply the patch (I hope tomorrow) and try it out.
Comment 20•21 years ago
|
||
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 21•21 years ago
|
||
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); } }
Comment 22•21 years ago
|
||
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.
Comment 23•21 years ago
|
||
I'll try to rework this today to incorporate Neil's suggestions.
Status: NEW → ASSIGNED
Comment 24•21 years ago
|
||
Attachment #132681 -
Attachment is obsolete: true
Attachment #134536 -
Attachment is obsolete: true
Comment 25•21 years ago
|
||
+ 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; + }
Comment 26•21 years ago
|
||
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.)
Comment 27•21 years ago
|
||
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.
Comment 28•21 years ago
|
||
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.
Comment 29•21 years ago
|
||
> 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.
Comment 30•21 years ago
|
||
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().
Comment 31•21 years ago
|
||
To see if I've understood it correctly :-)
Comment 32•21 years ago
|
||
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?
Comment 33•21 years ago
|
||
Actually Mike's approach paves the way for a future extension: dbview.Sort(sortType, sortOrder, sortFlags); which I hope could solve bug 223970.
Comment 34•21 years ago
|
||
David, I took the liberty of updating attachment 134992 [details] [diff] [review] to what I think covers all the subsequent comments.
Comment 35•21 years ago
|
||
Comment on attachment 135020 [details] [diff] [review] Updated patch based on my comments looks good to me.
Attachment #135020 -
Flags: superreview+
Comment 36•21 years ago
|
||
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 37•21 years ago
|
||
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)
Updated•21 years ago
|
Attachment #135020 -
Flags: review?(mscott) → review+
Comment 38•21 years ago
|
||
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.
Comment 39•21 years ago
|
||
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
Comment 40•21 years ago
|
||
*** Bug 225416 has been marked as a duplicate of this bug. ***
Comment 41•21 years ago
|
||
*** Bug 231184 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Attachment #132681 -
Flags: review?(bienvenu)
Comment 42•20 years ago
|
||
This is not fixed in Thunderbird 0.5/Linux. Will it find its way there automatically, or should another bug be submitted?
Comment 43•20 years ago
|
||
(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);
Comment 44•20 years ago
|
||
*** Bug 224030 has been marked as a duplicate of this bug. ***
Comment 45•20 years ago
|
||
*** Bug 265615 has been marked as a duplicate of this bug. ***
Comment 46•20 years ago
|
||
Adding a hidden preference which defaults to the broken behaviour is NOT a fix, as the continued duplicate reports show.
Comment 47•20 years ago
|
||
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
Comment 48•20 years ago
|
||
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.
Updated•20 years ago
|
Product: Browser → Seamonkey
Comment 49•20 years ago
|
||
(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.
Comment 50•19 years ago
|
||
*** Bug 251423 has been marked as a duplicate of this bug. ***
Comment 51•19 years ago
|
||
*** Bug 311210 has been marked as a duplicate of this bug. ***
Comment 52•19 years ago
|
||
*** Bug 318313 has been marked as a duplicate of this bug. ***
You need to log in
before you can comment on or make changes to this bug.
Description
•