User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10 Build Identifier: Mozilla Thunderbird version 0.8 (20040913) Sort by thead fails to sort again after a new message arrives. Reproducible: Always Steps to Reproduce: 1. Switch to View->Sort By->Date,Threaded,Ascending 2. Send yourself a mail in an old thread. 3. Wait for the mail to arrive Actual Results: 4. Note that the new mail has been threaded with the old thread but the thread is not the last in the inbox. 5. Click on some other folder and then click on the inbox. The mails are sorted now and the thread is the last thread in the inbox. Expected Results: The threads should have been sorted by date as soon as the new message arrives. (note: the threads are sorted by the date of the latest message). The sort works correctly if you switch folders or resort manually.

The program is working as designed; personally, I would find it disconcerting to have threads jumping around in the thread pane while I'm trying to read a message. But obviously you others (bug 207766, bug 260135) think it would be a good idea. The simple workaround is to resort: switch between Ascending and Descending, and back; or select another folder and then return.

Concerning comment #1: for me, the idea behind moving threads with activity to the head/bottom of the list when sorted on date/ID is to easily see what new mails arrived when coming back to my desk. If I'm out for a meeting and have 10 messages when I come back, scanning my whole folder to find the concerned threads is really cumbersome. About the thread pane jumping around, I agree it could move a bit. But polling for new mail only happens every 20-30 minutes in my situation, so it is not something that happens all the time. In any case, since the currently selected mail remains visible even when new mail arrives, the current thread won't jump.

I learned only yesterday about the mailnews.thread_pane_column_unthreads preference, which makes the threaded view useable (thanks for the tip!). However, after one day in threaded view, I really think there should be an option (user_pref?) to enable automatic re-sort on incoming mail. I see the mailbox name turn bold to indicate new messages, but clicking every time on ascending/descending to find new mails really is cumbersome.

I agree that this would be a good fix. I also understand the view stated in comment #1. However, I think if you resort the header pane, without changing the active message, or neading to redraw the message pane at all (if you have a message pane), would be only a minor inconveinence. You can keep reading, it'll just glitch the display. Which while I agree might be annoying, it will only happen in the cases that an existing thread receives new mail. Right now, it probabaly happens anyway if you're near the end of your messages, and a new thread starts up, right? Why would this be worse?

*** Bug 249808 has been marked as a duplicate of this bug. ***

I've noticed similar behavior when dealing with unthreaded sorted columns. I'm not sure if this is related or should be logged as a new bug. Here's my observations. Sort By: Date, Ascending, Unthreaded. New mail arrives via manual checking or scheduled checks, the message is added to the bottom of the list. The window does not scroll to display this new message. Resorting via click on the Date tab will proceed to reverse sorting. Clicking back to return does not display the new messages in the window. You must scroll the window in order to see new mail. I understand the concerns about forced sorting with threads, but I believe with unthreaded sorts, you're preference is to have the window listing display based on sorted criteria (such as date or order received) at all times. It forces another step for the user to access their new mail and is frustrating especially when only one new message arrives which is not displayed in the list by default. Please advise if this would be better submitted as a new bug, other please note this behavior of unthreaded sorts in addition.

About the original report: I don't consider this an enhancement, I think it's a bug. When I return to my computer I often don't even notice that I have got a new mail, which I find very annoying. I hoped that upgrading to version 1.0 would fix it, but obviosly it still works like before. I heard something about voting for bugs to get fixed, is that possible?

(In reply to comment #7) > I heard something about voting for bugs to get fixed, is that possible? It's not (yet?) possible to vote for Thunderbird bugs.

*** Bug 278865 has been marked as a duplicate of this bug. ***

*** Bug 207766 has been marked as a duplicate of this bug. ***

*** Bug 288476 has been marked as a duplicate of this bug. ***

*** Bug 309504 has been marked as a duplicate of this bug. ***

Finally found this, and noting by the number of duplicates, it's apparently easy to miss. It seems voting is enabled, so I cast one for this bug (RFE?), and I hope others will too. The option to auto re-sort when checking for new messages would be extremely useful, and though comment #1 disagree's, it seems clear there is a strong desire for the ability.

*** Bug 312467 has been marked as a duplicate of this bug. ***

*** Bug 323201 has been marked as a duplicate of this bug. ***

I think you get this kind of order when you sort by date and then activate the threading. But you have to activate the threading over the menu entry view -> sort by... -> Threaded not over the caption in the ThreadPane.

*** Bug 327960 has been marked as a duplicate of this bug. ***

(In reply to comment #1) > The program is working as designed; Then the design is flawed. > personally, I would find it disconcerting to have threads jumping around in > the thread pane while I'm trying to read a message. How is this any different from the automatic reordering that occurs every time a new message is delivered to a folder in a non-threaded view? Sort by date or sender, for example, and new messages will appear in their correctly sorted order immediately upon delivery. That the view is threaded should make no difference. Lacking any external event (i.e., message delivery) the fact that merely applying the *same sort*, whether by switching to a different folder and back or toggling ascending/descending, causes the display to change should be considered a bug. The message delivery itself should apply the sort criteria consistently, or not at all.

*** Bug 345871 has been marked as a duplicate of this bug. ***

I have to concur with this "bug/feature". The same thing happens in newsgroups. View-sort by- tag (TB2alpha) works but when new threads come in, they are placed above the original sort preferences. This means I have to go back to view-sort-by tags again to reset the preferences. Having to do this every time I go back into the newsgroup is a pain.

I wrote an extension last night that fixes this. Basically when new mail comes in, it just resorts by date and then scrolls to show the new messages. Anyone is welcome to use it: http://micropipes.com/code/threadbubble/ Let me know if it doesn't work for you.

(In reply to comment #21) > I wrote an extension last night that fixes this. Basically when new mail comes > in, it just resorts by date and then scrolls to show the new messages. Wil, thanks for this extension but isn't it possible for you to provide a native patch so it could be fixed directly in Thunderbird/SeaMonkey?

(In reply to comment #22) > (In reply to comment #21) > > I wrote an extension last night that fixes this. Basically when new mail comes > > in, it just resorts by date and then scrolls to show the new messages. > > Wil, thanks for this extension but isn't it possible for you to provide a > native patch so it could be fixed directly in Thunderbird/SeaMonkey? > While that would provide another exciting chapter in my brief thunderbird adventure, I don't think I've got the time to learn how to do that right now. I think that would be a lot more code intensive than this short extension.

(In reply to comment #21) > I wrote an extension last night that fixes this. Basically when new mail comes > in, it just resorts by date and then scrolls to show the new messages. > > Anyone is welcome to use it: http://micropipes.com/code/threadbubble/ > This is great, any chance you could release a version that works with the TB 2.0 betas?

Setting mailnews.thread_pane_column_unthreads to 'false' seems to trigger another bug. The expand/collapse (x/-) icon shows up for threads with only one message. Can anyone confirm this, so we can post a new bug report?

Thunderbird needs to get this fixed, this is a serious issue. I am completely appalled that Thunderbird has not fixed it yet. This is a basic feature that all email clients should support. Email messages are meant to be grouped into conversations (threads) and when a new message is added to a conversation/thread -- the timestamp for that thread should be the timestamp of its newest message, and the inbox MUST have to be resorted whenever a new message comes. Turns out -- Thunderbird does update the timestamp for a thread with its newest message, but the inbox itself is not resorted. The "workaround" of clicking another mailbox and coming back to the inbox is barely usuable. I feel the severity of this bug should be raised.

> This is great, any chance you could release a version that works with the TB > 2.0 betas? I don't have the 2.0 betas installed, so I can't test it. If I get extra time to install them, I'll be happy to update the extension. Or, if you want to email me, I can change the maxVersion and you can test it for me? (In reply to comment #25) > Setting mailnews.thread_pane_column_unthreads to 'false' seems to trigger > another bug. The expand/collapse (x/-) icon shows up for threads with only one > message. Can anyone confirm this, so we can post a new bug report? > I'm seeing that now too, but mailnews.thread_pane_column_untreads is true for me. I was wondering if it was a side effect of my threadbubble extension, but now I'm thinking no? Did you install threadbubble?

The expand/collapse widget ("twisty") showing up unnecessarily next to an address list is bug 348395 for TB, bug 367294 for SM. See also bug 355662.

(In reply to comment #27) > I'm seeing that now too, but mailnews.thread_pane_column_untreads is true for > me. I was wondering if it was a side effect of my threadbubble extension, but > now I'm thinking no? Did you install threadbubble? I dont think I've seen the expand/collapse bug since the day I wrote comment 25. So it might have been random. I remember seeing it a long time ago, before changing mailnews.thread_pane_column_untreads too. And no, I don't have threadbubble.

If I click the "threaded" column icon in the message list pane the threads appear in expanded mode and the latest emails are at the bottom of the list. Each thread has the earliest message as the thread "anchor" and later messages in the thread are intended below the anchor message. This necessitates I scroll to the bottom to see the latest emails. And if the threads are collapsed, the latest messages in the thread are hidden. If I try clicking the "date" column header, the threading disappears. This also happens if select "Descending" from the sorting menu. This is the exact opposite of how I would like my mail list to behave. Gmail does this properly by allowing the latest message in a thread to be the anchor message and governing the sorting by date. I don't need it to be the default behavior, but please offer the option to reverse the behavior for those of us who want to work this way. As for other criteria, size doesn't matter! but simply allow the sorting to be reversed if the user so chooses... thanks, please fix this, otherwise it's impossible for me to stay organized. ;-)

It looks like i am unable to mark dups in bugzilla but this bug and bug https://bugzilla.mozilla.org/show_bug.cgi?id=254159

Sorry, I missed bug 254159 comment 36 => Reopening

I'm assuming bienvenu will want this one. David, is b1 realistic?

shooting for b1 is realistic, yes. The complication is that I have the view code all taken apart for grouping/threading saved searches and that may not happen by b1. But this isn't that hard to do by itself.

this basically changes the way we insert threads into sorted, threaded views. It looks for the right insertion point, based on the current sort, and deals with expanded threads. And for newly added headers into threaded views, it checks if the newest msg in the thread has changed, and if so, moves the new thread, maintaining the selection, if any. The one ugly thing is that we're not using a comptr for the nsIMsgFolder pointer, but we do that to save some space during the synchronous sort, so we end up releasing the nsIMsgFolder pointer we've just gotten.

Comment on attachment 336751 [details] [diff] [review] proposed fix >+ while (highIndex > lowIndex) >+ { >+ nsMsgViewIndex tryIndex = (lowIndex + highIndex - 1) / 2; >+ // need to adjust tryIndex if it's not a thread. >+ while (m_levels[tryIndex] && tryIndex) >+ tryIndex--; >+ if (tryIndex <= lowIndex) >+ { >+ highIndex = tryIndex; >+ break; >+ } This doesn't work if only the first thread is expanded, since then the try index may be in the middle of the first thread although you want to try the second thread. >+ // if we're looking for an existing thread, check if these hdrs >+ // are the same, and if so, break out. >+ if (tryHdr == msgHdr) >+ { >+ NS_ASSERTION(PR_FALSE, "didn't expect header to already be in view"); I'm confused, is this expected or not? (And never use PR_FALSE, use one of the other debug macros that always warns.) >+ highIndex = tryIndex; >+ while (m_levels[highIndex] != 0 && highIndex) >+ highIndex--; Haven't you already checked that this is a thread header? >+ prevTryIndex = tryIndex; You never read this variable. >+ PRBool moveThread = PR_FALSE; > nsMsgViewIndex threadIndex = ThreadIndexOfMsg(newKey, nsMsgViewIndex_None, &threadCount, &threadFlags); >+ nsCOMPtr <nsIMsgThread> threadHdr; >+ m_db->GetThreadContainingMsgHdr(newHdr, getter_AddRefs(threadHdr)); >+ if (threadHdr && m_sortType == nsMsgViewSortType::byDate) >+ { >+ PRUint32 newestMsgInThread = 0, msgDate = 0; >+ threadHdr->GetNewestMsgDate(&newestMsgInThread); >+ newHdr->GetDateInSeconds(&msgDate); >+ moveThread = (msgDate == newestMsgInThread); Shouldn't this be >? (Actually you probably want to check the next thread...) >+ mTreeSelection->SetSelectEventsSuppressed(PR_TRUE); SaveAndClearSelection already does this. Also, you might not have a selection, e.g. in a standalone message window. >+ PRUint32 saveFlags = m_flags[threadIndex]; >+ PRBool threadIsExpanded = !(saveFlags & MSG_FLAG_ELIDED); >+ if (threadIsExpanded) >+ { >+ CollapseByIndex(threadIndex, &childCount); >+ } >+ nsMsgDBView::RemoveByIndex(threadIndex); >+ nsMsgViewIndex newIndex; >+ AddHdr(threadHdr, &newIndex); >+ if (threadIsExpanded) >+ { >+ OrExtraFlag(newIndex, MSG_FLAG_ELIDED); >+ ExpandByIndex(newIndex, &childCount); >+ } >+ m_flags[newIndex] = saveFlags; This seems an inefficient way to move these rows around in the array, especially as the caller went to the trouble of inserting the new message in the correct place in the thread... >+ RestoreSelection(preservedKey, preservedSelection); >+ mTreeSelection->SetSelectEventsSuppressed(PR_FALSE); See above. >+ EnableChangeUpdates(); >+ NoteChange(threadIndex, childCount + 1, nsMsgViewNotificationCode::changed); >+ NoteChange(newIndex, childCount + 1, nsMsgViewNotificationCode::changed); This doesn't actually invalidate the rows between the two threads...

Comment on attachment 336751 [details] [diff] [review] proposed fix I'll attach a new patch later addressing some of the comments. Re this, + // if we're looking for an existing thread, check if these hdrs + // are the same, and if so, break out. I had originally written the code to be useful if you wanted to find an existing thread, but changed my mind w/o removing the comment, so it should not happen.

Re the possibility of there not being a selection, we don't seem to check that in other places where we save and restore selection, and it doesn't seem to break anything if there's not, so as long as I'm not referencing mTreeSelection, I should be OK. Re >+ moveThread = (msgDate == newestMsgInThread); >Shouldn't this be >? (Actually you probably want to check the next thread...) The context is that a new header has arrived in the view. All I'm trying to do is check if this new header has become the newest message in the thread, which means "newestMsgInThread" has changed for this thread, which means we need to move it. It's already been added to the thread, so there's no way it's date will be > than the date of the newest message in the thread. I changed moveThread to remember the expanded thread keys, flags, and levels, and just insert them - it will be slightly faster than re-expanding the moved thread, but it's a bit more code. I fixed the case of the first thread being expanded by changing the <= to <, here: >+ if (tryIndex < lowIndex) >+ { >+ highIndex = tryIndex; >+ break; I always worry that I'm going to get stuck in an infinite loop, so this will require a bit more testing.

(In reply to comment #42) > It's already been added to the thread, so there's no way it's date > will be > than the date of the newest message in the thread. Ah, that makes sense, thanks.

Comment on attachment 337066 [details] [diff] [review] patch addressing Neil's comments >+ while (highIndex > lowIndex) >+ { >+ nsMsgViewIndex tryIndex = (lowIndex + highIndex - 1) / 2; >+ // need to adjust tryIndex if it's not a thread. >+ nsMsgViewIndex nextThreadIndex = tryIndex + 1; Not sure what this variable is doing, it's not used, and I'm still not convinced you'll find the right index in a case like this: - Thread 1 - Thread 1 - Thread 1 - Thread 1 - Thread 1 + Thread 2 + Thread 3 + Thread 4 >+ nsAutoTArray<nsMsgKey, 1> threadKeys; >+ nsAutoTArray<PRUint32, 1> threadFlags; >+ nsAutoTArray<PRUint8, 1> threadLevels; The parameter here is the number of elements that you expect to contain in the array, which saves an allocation if you turn out not to need more, so in the case of a selection you typically have one message selected and thus we save it in an nsAutoTArray<nsMsgKey, 1>. However here I don't see there being a typical number, so you might as well use a plain nsTArray instead.

Here's what happens in your test case, when Thread 1.5 comes in: tryIndex = 3, we back it up to 0. It's not less than lowIndex, retStatus of the compare is 1, so lowIndex gets set to 1, and we advance lowIndex to the next thread, at the bottom of the loop, so lowIndex gets set to 5, and tryIndex is 6. retStatus > 0, so highIndex gets set to 6, and tryIndex is 5. retStatus < 0, so highIndex is set to 5, and we break out of the loop, because highIndex == lowIndex. Or, in table form: 1st pass - lowIndex = 0, highIndex = 8, tryIndex = 3->0 2nd pass - lowIndex = 1->5 highIndex = 8, tryIndex = 6 3rd pass - lowIndex = 5 highIndex = 6 tryIndex = 5 4th pass - lowIndex = 5 highIndex = 5 break; There may be scenarios where we jump around, but I haven't constructed any yet.

Comment on attachment 337066 [details] [diff] [review] patch addressing Neil's comments >+ nsMsgViewIndex tryIndex = (lowIndex + highIndex - 1) / 2; I don't think you want to subtract 1 here. In the case of an odd number of rows it makes no difference; in the case of an even number of rows your code slightly decreases the chances of an equal split. >+ nsMsgViewIndex nextThreadIndex = tryIndex + 1; (Not used.) >+ if (tryIndex < lowIndex) >+ { >+ highIndex = tryIndex; >+ break; >+ } This makes no sense because lowIndex is a thread. (You could assert and break but I wouldn't set highIndex to the potentially bogus tryIndex.) >+ EntryInfo2.id = m_keys[tryIndex]; >+ GetFolderForViewIndex(tryIndex, &EntryInfo2.folder); >+ EntryInfo2.folder->Release(); This looks so ugly, but I guess we don't have anything better. >+ if (fieldType == kCollationKey) >+ { >+ PR_FREEIF(EntryInfo2.key); >+ rv = GetCollationKey(tryHdr, m_sortType, &EntryInfo2.key, &EntryInfo2.dword, colHandler); >+ NS_ASSERTION(NS_SUCCEEDED(rv),"failed to create collation key"); >+ } >+ else if (fieldType == kU32) >+ { >+ if (m_sortType == nsMsgViewSortType::byId) >+ EntryInfo2.dword = EntryInfo2.id; >+ else >+ GetLongField(tryHdr, m_sortType, &EntryInfo2.dword, colHandler); >+ } >+ retStatus = (*comparisonFun)(&pValue1, &pValue2, &comparisonContext); I wonder why this was never written if (fieldType == kCollationKey) { PR_FREEIF(EntryInfo2.key); rv = GetCollationKEy(...); NS_ASSERTION(...); retStatus = FnSortIdKeyPtr(&pValue1, &pValue2, &comparisonContext); } else if (fieldType == kU32) { ... retStatus = FnSortIdDWord(&pValue1, &pValue2, &comparisonContext); } >+ while (m_levels[lowIndex] && lowIndex < GetSize()) Need to compare lowIndex against the size first! sr=me with this fixed. > m_levels.InsertElementAt(insertIndex, level); >+ if (resultIndex) >+ *resultIndex = insertIndex; > > // the call to NoteChange() has to happen after we add the key > // as NoteChange() will call RowCountChanged() which will call our GetRowCount() > NoteChange(insertIndex, 1, nsMsgViewNotificationCode::insertOrDelete); I wonder whether we can arrange for insertIndex to have the correct value at all times and then set *resultIndex and call NoteChange in common code. >+ threadKeys.SetLength(childCount); >+ threadFlags.SetLength(childCount); >+ threadLevels.SetLength(childCount); >+ for (nsMsgViewIndex index = threadIndex + 1; >+ index < GetSize() && m_levels[index]; index++) >+ { >+ nsMsgViewIndex saveThreadIndex = index - threadIndex - 1; >+ threadKeys[saveThreadIndex] = m_keys[index]; >+ threadFlags[saveThreadIndex] = m_flags[index]; >+ threadLevels[saveThreadIndex] = m_levels[index]; >+ } It seems strange that there's no easy way to append a copy of part of an array, but you could possibly simplify the code by setting the capacity of the array (optionally via the nsTArray constructor) and then appending each element so you don't have to keep track of the save index seprately. >+ m_flags[newIndex] = saveFlags; Out of interest, which flags get lost?

carrying forward Neil's sr, requesting r= for the new patch. This addresses all of Neil's comments, I think. Re the flags that we're saving and restoring, I think they're MSG_VIEW_FLAG_HASCHILDREN and MSG_VIEW_FLAG_ISTHREAD. And since we're saving/restoring the flags, we don't need to OrExtraFlag(...ELIDED), and in fact, we shouldn't be setting that flag at all. Oh, the reason I'm not initializing the size of the nsTArrays in the constructor is that I don't know if the thread is expanded or not.

pinging for review...

fix checked in.

I'm using icedove 3.0.7 (debian/testing) with this view options: View->Sort by->Date/Descending/Treaded The latest message showed the last in the thread. ThreadBubble can't help anymore as it doesn't work in Thunderbird 3.