495946 - thunderbird show threaded, sort by date - randomly sorts the dates

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Dave T]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4) Gecko/20090427 Fedora/3.5-0.20.beta4.fc11 Firefox/3.5b4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4) Gecko/20090427 Fedora/3.5-0.20.beta4.fc11 Firefox/3.5b4

Thread dates are not displayed in the chosen order. Regression from 2.0 series.

Reproducible: Always

Steps to Reproduce:
1. 3 pane view
2. View|Sort by threads
3. View|Sort by date
4. (to make it easier to see issue) View|Threads|Collapse All Threads
Actual Results:  
The thread starters are shown with the thread icon at left.
The starters are however not order by date, even though the date header is showing the date sort arrow.

Expected Results:  
Threads sorted by date of the initial thread starter in each thread.

Comment 1

[Dave T]

Attachment: visual description of issue.

see dates are nowhere near in order.
Used to be OK, in last major release.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Dave, which build are you using? (It'll be good to test on Thunderbird 3 Beta 2) Also, can you attach a testcase mailbox with a few messages (stripped of private data) illustrating the problem?

Comment 3

[Dave T]

(In reply to comment #2)
> Dave, which build are you using? (It'll be good to test on Thunderbird 3 Beta
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b5pre) Gecko/20090513 Fedora/3.0-2.4.b3pre.hg.6a6386c16e98.fc11 Thunderbird/3.0b3pre

[Maybe I did help about from firefox-, but I thought I entered it from thunderbird whoops]

Comment 4

[Dave T]

Attachment: test case mail folder bzip2 archive

email copied from a folder where filters move mails sent to [e-mail]
Public list, no editing performed.

Comment 5

[Vince C.]

I can confirm that too. It has always occurred for me since I've been using Thunderbird.

I usually click the date column until messages are sorted by date in descending order (i.e. newer first); then I check the Display menu "Group by conversation". Conversations are indeed grouped but dates are no longer sorted in descending order and new messages are always at the end of the thread.

It happens on both Gentoo Linux (my main platform) and Windows. I'm using Thunderbird 2.0.0.22 on both Gentoo Linux and Windows.

Comment 6

[Jim Porter (:squib)]

Doesn't this sort by the *latest* date in the thread, but since the threads are collapsed, it's showing the date of the original message (i.e. the earliest date)?

Comment 7

[Dave T]

My experience is that this sorting issue has been introduced with beta 3 (on Fedora 11, was OK on thunderbird 2x on F10).

(In reply to comment #5)
> I can confirm that too. It has always occurred for me since I've been using
> Thunderbird.
If you have some folders (without private information), showing this issue, that you could screen capture (png) and attach to this bug, this would help to determine whether we are seeing the same issue.

(In reply to comment #6)
> Doesn't this sort by the *latest* date in the thread, but since the threads
> are collapsed, it's showing the date of the original message (i.e. the
> earliest date)?
My impression was that whether the sort is ascending or descending, the date of the first message in the thread is used in the ordering. 

This issue continues to be visible with:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1) Gecko/20090814 Fedora/3.0-2.6.b3.fc11 Thunderbird/3.0b3

Comment 8

[Jim Porter (:squib)]

> My impression was that whether the sort is ascending or descending, the date of
> the first message in the thread is used in the ordering.

For me, in Thunderbird 2 and 3, sorting by the Date column with threads sorts by the last date in the thread. This is kinda confusing, since for collapsed threads, you don't see the date being used to sort by, but it seems to me like a more useful way to sort.

One option would be to have collapsed threads show data for the entire thread in the message list, as opposed to showing data for the original message. For example, the Sender would be a list of people who replied (plus the original sender), the Date would be the date of the last message, the Size would be the total size, etc.

Comment 9

[Vince C.]

I think I start to see clearer. I also need to clarify my comment. I think understand why messages in a thread are sorted with the latest message last. Grouping messages by threads creates a tree with the latest message being the last leaf "node". This wasn't what I was talking about -- although I should have thought of that.

My comment was about various messages with the exact same subject, for instance messages you receive from a daily report (Nagios, apticron, logwatch, aso). These send reports with the exact same subject, hence excellent candidates for thread grouping. The only thing is, in *that* particular case, the latest messages appear last in the thread regardless of the date sort order that was selected previously.

So, messages on the same level in a thread should be sorted using the sorting order that was previously selected. Well, I start to wonder if it's possible at all given how particular that case is...

Comment 10

[Dave T]

Just an update to say that this is the status quo in 
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.5) 
Gecko/20091209 Fedora/3.0-4.fc12 Thunderbird/3.0

Since we have had this alternate sorting order foisted upon us, could we please get the original (sane) sorting order back as at least an option or config ?
Especially for those where you have actually threaded emails like email lists and stuff. You just don't want stuff your reading suddenly jumping around the list as new mail is received that happens to be part of the thread you are reading.

Comment 11

[Andrew Buehler]

Seconding the request from comment #10.

As I understand things, the reason why Thunderbird 2 appeared to sort threads by "date of topmost message in the thread hierarchy" is that it actually was sorting threads by "Order Received", which usually though not always seems to approximate the other criterion. "Order Received" is still available in Thunderbird 3; unfortunately, in the specific case I have at hand, it doesn't produce the desired results.

I was going to file a standalone feature-request bug report for a "date of topmost message in the thread hierarchy" sorting criterion when in threaded mode, but this bug report seems close enough that I'll piggyback on top of it for the time being.

Comment 12

[Andrew Buehler]

No activity on this at all? This (the inability to sort threads by the date of the *oldest* message) is the more important of the two issues which still block me from upgrading from Thunderbird 2.

Comment 13

[Vince C.]

(In reply to Andrew Buehler from comment #12)
> No activity on this at all? This (the inability to sort threads by the date
> of the *oldest* message) is the more important of the two issues which still
> block me from upgrading from Thunderbird 2.

I must confess I stopped caring. Most [if not all] of the mail services I now use provide a web interface and using a heavy mail client has become the exception.

Comment 14

[Andrew Buehler]

All of the mail services I use (that I recall offhand) do have a Web interface. However, A: there's no way to access *all* of them through the same interface, and B: none of them provide an interface that matches what I actually want to use.

(For starters, try finding *any* Web-mail interface which provides proper hierarchical threading, as opposed to the relatively flat "conversations" provided by e.g. Gmail... that's one of the fundamental UI conventions which is absolutely indispensable IMO, and very little besides Thunderbird seems to provide it nowadays.)

Comment 15

[Andrew Buehler]

Attachment: Let thread sorting use the topmost message in the thread, depending on a pref

The attached patch makes this behavior configurable, with separate prefs for "sort by date" and "sort by date received". It leaves the default as "use newest message in thread", but allows using the topmost for people who want that.

Since this behavior is handled in C++ code, I'm reasonably sure this couldn't reasonably be done in an add-on.

I'm not sure how to run this past the test suite, much less update any tests which might need updating; if that is necessary, information would be appreciated.

I have, however, tested this manually in nearly as exhaustive a manner as I could think of, and everything seems to work as expected and desired. The "nearly" qualifier is because I ran into problems setting up a local test IMAP server and getting Thunderbird to talk to it, and so I haven't been able to test what happens when a new message arrives.

I haven't touched the "we want the newest message" comments, since I'm not sure whether they should be removed, or if not, what they should be changed to.

Note that although this does appear to work exactly as I wanted it to, this is literally my first attempt at this patch, and I've never made a nontrivial patch that did everything right on the first try; there's a first time for everything, but the odds are that there is a problem somewhere.

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 773269 [details] [diff] [review]
Let thread sorting use the topmost message in the thread, depending on a pref

Setting an actual reviewer, please feel free to forward this on if necessary.

Comment 17

[Andrew Buehler]

Attachment: Show 8 context lines and the containing function name, for easier review.

Comment 18

[Andrew Buehler]

I'm looking over the code again based on one comment from neil in IRC, and I"m a little confused by the existing code for the "by date received" case.

Is it supposed to sort threads by dateReceived when possible (falling back to message date only when the other is not available), or is it only supposed to try dateReceived when not in threaded view?

The former is what I would expect from sorting "by date received" while in threaded view, but the latter is what the code appears to do.


How to handle that case for this patch depends on which set of semantics should be followed.

Option 1: Use message date when in threaded view. This would be simple, and would match what the code currently appears to do, but doesn't seem to fit with what the user who selected "by date received" requested.

Option 2: Use dateReceived for threaded view when this patch's new received_uses_newest pref is false, and message date for threaded view otherwise. This would be nearly as simple, but would involve mild code duplication (at least in the simplest approach), and would mean that someone setting received_uses_newest to false because they want the topmost-message behavior would also get an unrelated and potentially undesired behavior change.

Option 3: Use dateReceived for all three cases (threaded with / without the pref, and unthreaded regardless of the pref). This would be seem the most correct, and would let us avoid the code duplication from option 2 - but it would also be a considerably more invasive change, and would seem to extend beyond the scope of this bug.

The current version of the patch uses option 1. What's the right thing to do here? Stick with option 1 for this patch, file a separate bug about the byReceived case, and convert to option 3 under that?

Comment 19

[[e-mail]]

Comment on attachment 780968 [details] [diff] [review]
Show 8 context lines and the containing function name, for easier review.

>       // in the thread
>       if (m_viewFlags & nsMsgViewFlagsType::kThreadedDisplay
>         && ! (m_viewFlags & nsMsgViewFlagsType::kGroupBySort))
>       {
>         nsCOMPtr <nsIMsgThread> thread;
>         rv = GetThreadContainingMsgHdr(msgHdr, getter_AddRefs(thread));
>         if (NS_SUCCEEDED(rv))
>         {
>-          thread->GetNewestMsgDate(result);
>+          bool useNewest = true;
>+          nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
>+          prefs->GetBoolPref("mailnews.thread_sorting.date_uses_newest", &useNewest);
>+          if (useNewest)
>+          {
>+            thread->GetNewestMsgDate(result);
>+          }
>+          else
>+          {
>+            nsCOMPtr <nsIMsgDBHdr> threadRoot;
>+            thread->GetRootHdr(nullptr, getter_AddRefs(threadRoot));
>+            threadRoot->GetDateInSeconds(result);
>+          }
>           break;
>         }
>       }
>       rv = msgHdr->GetDateInSeconds(result);
>       break;
So, in pseudocode, you're writing
if (threaded && !grouped) {
  if (msgHdr.thread) {
    if (useNewest)
      result = msgHdr.thread.newest;
    else
      result = msgHdr.thread.root.date;
    break;
  }
}
result = msgHdr.date;
break;

Now as it happens, if threaded is true, then we're only sorting the thread roots, and the other messages get inserted under the roots when you expand them in the thread pane. So msgHdr.thread.root == msgHdr in this case, simplifying that line of code to result = msgHdr.date; which is just the value you would have got if you weren't threaded or couldn't find a thread.

An unrelated problem is that this function is also used when new mail arrives. In theory it could get confused if you changed the preference in between getting two lots of new mail. It might be worth getting the preference once when the view is originally opened (compare news.show_size_in_lines, which has a similar problem).

Comment 20

[Andrew Buehler]

> Now as it happens, if threaded is true, then we're only sorting the thread roots, and the
> other messages get inserted under the roots when you expand them in the thread pane. So
> msgHdr.thread.root == msgHdr in this case, simplifying that line of code to result =
> msgHdr.date; which is just the value you would have got if you weren't threaded or couldn't
> find a thread.

Yeah, I figured that out, though not the details of why. I've cleaned up the byDate case on that basis, but when trying to clean up the byReceived case similarly, I ran into the problem I referred to in comment 18.


I mentioned earlier that I wasn't able to test the "when new mail arrives" case (without risking mail from an actual live account which I care about), since I couldn't get Thunderbird in my testing VM to talk to a test IMAP server set up on the host machine. If there were going to be problems I hadn't noticed, that's where I'd expect to find them.

I'll look into checking the pref or prefs elsewhere as (I think) you suggest.

Comment 21

[Andrew Buehler]

Attachment: thunderbird-sortorder-configurable-globalvariable-v2.diff

Now uses a single pref for both byDate and byReceived, and checks that pref when opening the folder rather than at sort time.

The pref is also now reversed from the previous patch; enabling the new behavior is now a matter of setting one pref to true, rather than setting one or both of two prefs to false. The new pref name may make it less discoverable than its reversed version, but I'm not sure if that's a concern.

I've gone with option 1 from comment 18, as the simplest approach.

Seems to work as expected in my testing, but again, I'm not in a position to test "incoming mail" without risking data I actually care about. I'm looking into ways to change that, but I'm not there yet.

Comment 22

[[e-mail]]

Comment on attachment 781981 [details] [diff] [review]
thunderbird-sortorder-configurable-globalvariable-v2.diff

>+      bool temp;
>+      rv = prefs->GetBoolPref("mailnews.sort_threads_by_root", &temp);
>+      if (NS_SUCCEEDED(rv))
>+        mSortThreadsByRoot = temp;
Looking into this, it seems that at one point mShowSizeInLines used to be a different type and couldn't be passed directly to GetBoolPref, but things have since changed and you can now GetBoolPref directly without requiring a temporary. Seeing as you're changing these lines already, would you mind adjusting the calls appropriately?

>+      rv = prefs->GetBoolPref("news.show_size_in_lines", &temp);
I think we should have an entry in mailnews/mailnews.js for this.

Although this seems to work fine, I don't know whether we have any way of automatically testing this, so I'll only grant f+ for now, and then maybe you can find out whether tests are applicable?

Comment 23

[Andrew Buehler]

Attachment: thunderbird-sortorder-configurable-globalvariable-v3.diff

> Looking into this, it seems that at one point mShowSizeInLines used to be a
> different type and couldn't be passed directly to GetBoolPref, but things
> have since changed and you can now GetBoolPref directly without requiring a
> temporary. Seeing as you're changing these lines already, would you mind
> adjusting the calls appropriately?

Done.

> I think we should have an entry in mailnews/mailnews.js for this.

Done (assuming you meant for the new pref, not for show_size_in_lines which already has one). I wasn't entirely sure where to put it, but looking at the existing layout of the file I'm not sure it matters much. I also wasn't sure whether to include a comment (some existing prefs seem to have them but others don't), so I put one in just to be on the safe side.

I notice that the mailnews.js entry for show_size_in_lines defaults to true, but mSizeInLines is initialized to false. I'd have expected them both to be the same, providing the same default in different ways, which is what I've done for the new pref; any idea what's going on there?

> Although this seems to work fine, I don't know whether we have any way of
> automatically testing this, so I'll only grant f+ for now, and then maybe you
> can find out whether tests are applicable?

I'm not sure how to tell that, to be honest.

None of the existing comm-central mozmill tests seem to be concerned with sorting, so far as I can see (though I've only just today learned more about the Mozilla testing suites than that they exist and there's more than one).

I think I see a path towards creating some tests that would check sort order, based on what seems to be possible given what the existing tests do - but I'm not sure having such a test for just this one case without also having a broader "normal sorting" framework as a backdrop would be really useful, and creating that larger backdrop would be far beyond the scope of this bug.

Also, the hoops I presently have to jump through in order to build and test (due to bug 890877) make trying to deal with the automated testing framework enough of a pain that I'd really rather not try to learn how to write tests until that is fixed, at the very least.

Comment 24

[[e-mail]]

Maybe aceman knows whether there are any relevant tests?

Comment 25

[:aceman]

I would expect some tests for folder sorting being in mail/test/mozmill/folder-display/ .

Comment 26

[Andrew Buehler]

There don't seem to be any there as far as I can tell.

'find mail/test/mozmill/ -type f | grep -i sort' returns only 26 results from 10 files; filtering out the ones in comments reduces that to 11, from only 4 files (3 in folder-display, the other being the folder-display-helpers).

I've looked at everything in the 4 files, and anything that looked potentially relevant from the 26, and anything using the one folder-display-helper which touches message/thread sorting, and none of it seems to be sorting tests. There are only 3-5 places where it looks like it changes sort order at all, and all of them are in the service of testing something else. (All of them pass with the patch applied, for what that's worth.)

'find mail/test/mozmill/ -type f | grep -i thread' returns quite a few more results, but although I haven't dug through them as closely, none of them look any more relevant.


Just to clarify: are we trying to find out whether there are already tests which monitor sorting behavior (which would then need to be updated and/or extended to account for this pref), or whether it would be possible/practical to implement such tests (and, from there, whether we should)?

Comment 27

[[e-mail]]

(In reply to Andrew Buehler from comment #23)
> I notice that the mailnews.js entry for show_size_in_lines defaults to true,
> but mSizeInLines is initialized to false. I'd have expected them both to be
> the same, providing the same default in different ways, which is what I've
> done for the new pref; any idea what's going on there?
For news we default the pref to showing lines, but for mail we never show lines, so that needs to default to false.

Comment 28

[Andrew Buehler]

Yeah, that makes sense.

What's the next step here? The documentation I've found suggests adding the checkin-needed keyword, but given that the tree appears to be closed due to breakage at the moment (though I haven't personally encountered any), I'm not sure what the correct procedure would be.


Just to note: I'd like to see this included in the TB24 release, since that will presumably be the basis of the next Thunderbird ESR, which will form the basis of what will be provided by Debian until the ESR after that; if it isn't included, I'll have to either maintain it as a local patch against the distro version, or run a non-distro-provided Thunderbird for the interim.

I understand that TB24 inclusion may not be an option at this relatively late date, but if it is, what would be the path for getting this change included? For that matter, what would the odds of having that happen be?

Comment 29

[:aceman]

It seems the patch does not change the current default behaviour so should not break anything. That is positive.
There is chance to get it into TB24 if you follow these steps:
1. get it into the current trunk so that it can be used by people for several days.
2. then, set the approval-comm-aurora? request on the patch.

But it would be good if actually any TB people commented on the patch before that.

Comment 30

[Andrew Buehler]

I presumed Neil would qualify as "TB people", given that he's so consistently in #maildev with operator status, and knew the code well enough to be willing/able to review this. If he doesn't, then should we even be requesting checkin without review from someone who does?

I certainly don't want to delay this any more than necessary, but I would also rather not jump over any appropriate procedures, or risk stepping on any toes if that can be avoided.

Comment 31

[:aceman]

See https://wiki.mozilla.org/Modules. Neil can review your patch here because it changed only files in /mailnews. But I think is main interest is Seamonkey so he will probably not comment on TB releases or what the default of the pref on TB should be.

Comment 32

[Andrew Buehler]

Understood, I think. I hadn't looked at that list in some while (I think I'd forgotten it was there), but it does help clarify some things.

I'll leave this as-is for now, and if there's no movement in a reasonable time-frame, request review from one of the TB-specific people again.

Comment 33

[:aceman]

I have set checkin-needed for you, as this can go into trunk (it will be checked in once the tree reopens [there are bustages in tests, not in TB build itself)]. You can then continue with step 2 from my comment 29 later.

Comment 34

[Magnus Melin [:mkmelin]]

Does it fix the visual issue? I'd expect the date shown for the thread to be the one sorted by. Besides that, sounds good to me.

Comment 35

[Andrew Buehler]

Magnus: yes. The date shown for a thread is the date of the message at the top, or "root", of the thread, which is usually (though not always) the oldest one; with this pref set, the threads will be sorted based on that message, rather than on the newest one.

Comment 36

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/946329921731

Comment 37

[Andrew Buehler]

Comment on attachment 782177 [details] [diff] [review]
thunderbird-sortorder-configurable-globalvariable-v3.diff

[Approval Request Comment]
Regression caused by (bug #): None or unknown; the original issue was introduced during the Thunderbird 3 beta period, but although this resolves that issue, it is technically new functionality.

User impact if declined: In environments which only provide or only allow the ESR - which, given that for Thunderbird the ESR is the only official "full release" presented to the public, is many of them - the functionality won't be available until the TB31 release. (In my specific case, this will impede me from upgrading from TB2 until that point.)

Testing completed (on c-c, etc.): mozmill tests pass; xpcshell tests which touch message-sorting behavior pass; manual testing shows no issues. No existing automated testing appears to cover thread-sorting behavior at all. While I could potentially create tests for this, the process would not be quick or (due to consequences of bug 890877) easy.

Risk to taking this patch (and alternatives if risky): None if the new pref is not set; none I can see even with it. However, as no existing tests seem to cover the behavior this changes, it is hard to be absolutely certain.


Setting both approval-comm-aurora and approval-comm-beta as suggested on tb-planning, since I'm not certain which is applicable for the TB24 line at present, and either should be acceptable for my purposes.

Comment 38

[David :Bienvenu]

view tests are here - http://mxr.mozilla.org/comm-central/source/mailnews/base/test/unit/test_nsMsgDBView.js

Comment 39

[Andrew Buehler]

I found that, yes, and those tests pass with the fix applied.

I'm planning to add thread-sorting tests to that file, but it's unclear how long that will take. If there's no chance of this making it into the TB24 ESR release line anyway, I'd prefer to take my time with creating the tests, and make absolutely certain they come out right, in addition to prioritizing this among other tasks; if there is a chance of this making it in if I can make an appropriate deadline, I'll have to try to get the tests done as quickly as possible.

Comment 40

[Mark Banner (:standard8)]

Comment on attachment 782177 [details] [diff] [review]
thunderbird-sortorder-configurable-globalvariable-v3.diff

Given that this is disabled by default, I'm happy to take it. We should still get tests for the case where the preference is enabled, to ensure it doesn't regress in future.

Comment 41

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-beta/rev/63e923339f85

Comment 42

[Andrew Buehler]

Thank you very much!

I am indeed working on tests for thread-sorting order (for at least the by-date case when this is enabled vs. when it's disabled, including toggling between them), but I've been working heavy overtime for the past two weeks, so progress has stalled.

When I have tests which I think are ready, should I post them here, or open a new bug for them?

Comment 43

[Mark Banner (:standard8)]

I think its best to open a new bug, patches on closed bugs can easily get lost (and can confuse branch tracking)

Comment 44

[Andrew Buehler]

Acknowledged; I'll plan to proceed on that basis.

Comment 45

[Andrew Buehler]

Tests filed as bug 911465.