Closed Bug 495020 Opened 11 years ago Closed 10 years ago

Unread Folder population causes focused folder to change

Categories

(Thunderbird :: Folder and Message Lists, defect, minor)

x86_64
Windows 7
defect
Not set
minor

Tracking

(thunderbird3.1 beta2-fixed, thunderbird3.0 .5-fixed)

RESOLVED FIXED
Tracking Status
thunderbird3.1 --- beta2-fixed
thunderbird3.0 --- .5-fixed

People

(Reporter: johnlgalt, Assigned: Bienvenu)

References

Details

(Keywords: regression)

Attachments

(6 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1pre) Gecko/20090526 Firefox/3.5b4pre (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1pre) Gecko/20090526 Shredder/3.0b3pre

I participated in the discussion of https://bugzilla.mozilla.org/show_bug.cgi?id=466644 extensively, until a patch was written to fix the issue.

However, after the patch was introduced into the current nightly codebase, a new error has emerged, and even though I am almost absolutely sure this is because of said patch, I was instructed to create a new bug since this is a new issue.

cf: https://bugzilla.mozilla.org/show_bug.cgi?id=466644#c42

Using all builds since this fix was added to the codebase, including the most
current one, Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1pre)
Gecko/20090525 Shredder/3.0b3pre:

If I have a folder selected in the unread folders view, and Tb starts checking
mail, and folders appear above the currently selected folder in the tree, the
view remains the same.  However, if folders appear after the currently selected
folder in the tree, then for some reason the currently selected folder moves on
its own accord to the next one lower in the tree.

This is reproducible 100% on my machine.

Stats:  Windows 7 64bit RC1, Tb build as noted above, running on a Quad Core
6600 OC'd to 3.21 GHz w/ 4 GB RAM (1000MHz, PC8000) installed.

Thunderbird is running a combination of IMAP, POP and Webmail for access to 12 different accounts (Not 12 different user profiles, 12 accounts for the *same* user profile).

Reproducible: Always

Steps to Reproduce:
1.  Run Tb, change to unread folders view, select a folder randomly (easiest manifestation is to select the top folder, as this will then reproduce a lot more quickly).
2.  Check mail.  
3.  
Actual Results:  
If any folders appear *above the currently selected folder in the tree, focus will not change.  If any folders appear *below* the currently selected folder, the selection focus will move to the next folder in the tree - and not only does the selection focus move, the actually view in the Preview changes to accommodate the currently selected *message* in the newly selected folder.

Expected Results:  
The selection focus should not change upon checking mail in the unread folder view.  I can think of one *possible* exception, and that would be if you have marked everything in the currently selected folder as 'Read' - in which case, upon a new mail check Tb could intelligently realize that the folder no longer applies to the current view and would remove it from view.....

I do not use a theme, and this does not cause a 'crash'.  Obviously, as noted from bug 466644 above, this did not occur before, b/c Tb 3 was simply not updating the user folders view at all.  So, this only started occurring after the patch from 466644 was introduced into the codebase about a week ago or so.
cc'ing all cc'ed folks from bug 466644
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1pre) Gecko/20090525 Lightning/1.0pre Shredder/3.0b3pre

I can confirm the same behavior.
Blocks: 466644
Keywords: regression
still see this in beta 3?
 http://www.mozillamessaging.com/en-US/thunderbird/early_releases/
Version: unspecified → 3.0
I am.  

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.1) Gecko/20090715 Lightning/1.0pre Thunderbird/3.0b3

In my case, all accounts are IMAP, with server-side filtering.  A new IMAP message shows up in a folder further down the list, and off we go.
Sorry for the late reply - yes, this is still, absolutely 100% reproducible - regardless of which Tb I use (currently loading:

Shredder 3.1a1 - Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20090903 Shredder/3.1a1pre;

also testing in Tb 3.0b3 - Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.1) Gecko/20090715 Thunderbird/3.0b3;

also testing in Shredder 3.0b4pre - Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.4pre) Gecko/20090903 Shredder/3.0b4pre

I have a supposition, though.  I am currently allowing Tb to manage my filtering, even on IMAP accounts, and I have just started foraying into the world of GMail filters right in the app itself - I am going to try to run Tb tomorrow after moving all filters to server side to see if it makes a difference in terms of how the unread folder operates - perhaps it is not the appearance of the messages themselves, but the quick appearance and then filtration of messages from the Inbox to other locations that may be causing this....
>  I am going to try to run Tb
> tomorrow after moving all filters to server side to see if it makes a
> difference in terms of how the unread folder operates

Unlikely - see my comment (#4).  *All* of my filters are server-side, and I see this too.
Hmmm - I only killed the filters for the 3 GMail accounts that I had in Thunderbird, and even in spite of your comment, Paul, this time it did not happen at all.

All filters for other accounts are still in place.  However, I was up pretty late last night building server side filters, and am only partially completed, so a lot of my previously filtered mail was dumped in the inbox, so it could also be the fact that not all my filters are in place in the first place.  I'll continue to build the filters and hopefully by late Sat I should be able to verify again on my end.
I don't recall ever seeing anything causes a focus change in folders, other than explicit selection. Do you see this started in safe mode? http://support.mozilla.com/en-US/kb/Safe+Mode
Yes, I just tested it in safe mode.  Same result.
I am unsure as to why this is marked unconfirmed - is it possible that Paul and I are the only ones seeing this b/c of some ironic configuration setting?

I have converted all of my GMail account to server side filtering and this still occurs on a regular basis....
I can confirm this same behaviour on:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.5pre) Gecko/20091026 Shredder/3.0pre

This makes the current builds unusable, as emails vanish from the window while I'm reading them. And if it was the last unread email in that folder, I now have to go back to the full folder list and navigate back to where I was.
I can confirm it too on:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.5) Gecko/20091204 Thunderbird/3.0

This is the released image!
gmail users, can anyone confirm this scenario which might be related?  and sid0, any thoughts ...

1. Be in All Folders
2. get a new gmail message
3. folder view changes to "Smart Folders"
Wayne, LTNS - I'll get back to the queue in the next couple of days....

As to this particular scenario:

Are you referring to being in the <b>All Mail</b> folder?  If so, I currently have those disabled from shwoing in Thunderbird (<b>Settings --> Labels --> All Mail --> <i>uncheck </i>Show in IMAP</b>)

If that is what you meant, I'll enable and test.
I mean that the folder pane's header is All folders, and not smart folders.
Oh - no, this is in *Unread* Folders.
(In reply to comment #16)
> Oh - no, this is in *Unread* Folders.

NM.  I answered before I remembered what was going on - I'll check out this and see if I can reproduce.
One of three. A single message in "folder 1" is selected and previewed,
Adding a simple message, via Panda IMAP's dmail command, which is what normally delivers my mail to the folders.
Another account's new folder didn't seem to trigger the change in the meantime
Tossed irrelevant stuff, just grepped for "folder 2" with 7 lines of context before/after.  All the notifications/etc. are there.
Whiteboard: [has protocol logs]
I have this issue too in 3.0.3 on Linux.
Any new details on this?  This is the most annoying bug I have run into in Thunderbird - I basically have to let it completely synchronize all of my IMAP accounts before I can start processing mail.  Otherwise, as I am attempting to click on an unread mail, TB is changing the focus from one folder to another and in the process I either open a different email entirely, or else it freezes as it tries to sort out just where exactly I am trying to (erroneously) go....
Also, currently using v 3.0.3 final, plus 3.1 beta *and* 3.2a1 - all three are exhibiting this behavior.  OS and other information have to changed from above (other than yet another re-install to allow for upgraded hardware).
confirming
Assignee: nobody → bienvenu
Status: UNCONFIRMED → NEW
Ever confirmed: true
A little more detail - when a folder is newly marked as having unread, that causes a complete rebuild of the unread folders view, which removes all the folders that no longer have unread. rebuilding attempts to maintain the selected folder, but it seems to be failing miserably.
OK, I think this code is the issue - 
      if (oldCount !== null)
          this._tree.rowCountChanged(0, this._rowMap.length - oldCount);

It basically updates the row count after rebuilding  by saying that rows were added or removed at row 0, which is not usually the case, and completely messes up the tree's selected row.

The easiest way to fix this is to avoid getting into the rebuild code in this case. We can simply add the newly unread folder to the view. This would mean that folders no longer with unread messages would stay in the unread view, which is analogous to what we do for unread messages only views. Or, we could update the view by iterating over the folders and removing folders no longer with unread messages one by one, instead of rebuilding the view. Or we could make _rebuild just save and restore the selected folder instead of letting the tree do it, and lying to the tree about what changed.
imap logs irrelevant, so I've obsoleted it. This patch fixes the bug. I'll try to come up with a mozmill test for this, though mozmill tests can't get new mail.
Attachment #427864 - Attachment is obsolete: true
I'm tempted to mark this blocking since I don't know how anyone would live in that view with this bug, but I'll just leaved it at wanted for now, since there haven't been dupes of this, afaik.
Status: NEW → ASSIGNED
I live with it because
1) I REALLY like this view
2) I set the update time pretty large to minimize this bug's effect
3) It's only REALLY annoying when I've just selected the last unread message in a folder, and the bug removes the folder and the open message from the view before I can read it.

If there are still unread messages, the folder stays in the view and if I re-select it the message I was reading comes back.

Still, I'm glad it's fixed and look forward to the next release.
Attached patch fix with mozmill test (obsolete) — Splinter Review
this adds a mozmill test that fails without the folderPane.js change, and succeeds with it.
Attachment #434925 - Attachment is obsolete: true
Attachment #434963 - Flags: review?(sid.bugzilla)
Berry, I haven't seen your #3 issue, but that could be because I have TB set to remember the last loaded message in a folder (which is the default).
David, If I go back to the "All Folders" view and select the folder I was in, I do get the last loaded message back; having to do that and then go back to "Unread Folders" is the annoying part.

Thanks for fixing this.  I looked for folderPane.js on my machine and couldn't find it.  I admit I know almost nothing about the innards of Thunderbird though, since I stopped hacking mail clients after exmh.  If it's easy to find (in some zip file or something) and I can patch it easily let me know, otherwise I'll wait for the next release.
David, thanks for the replies and confirmation.  Glad to know that a new patch is in the works.

Like Berry, I'd like to know if there is a way to easily test this patch without having to compile a new personal build by myself - not currently set up for compiling builds.

If not, I, too, will wait on the next release.
pinging for review. Re testing, there's no easy way to test this patch w/o having your own build. You can repackage the jar file with a changed version of folderPane.js, but it's not trivial.
Duplicate of this bug: 540289
Comment on attachment 434963 [details] [diff] [review]
fix with mozmill test

This is an r- because at startup, the getSelectedFolders() call has somewhat hazy semantics. One way to fix it would be for getSelectedFolders() to return [] if no selection is available (and please also fix this._treeElement.view.selection to be this.selection!)

>diff --git a/mail/base/content/folderPane.js b/mail/base/content/folderPane.js
>--- a/mail/base/content/folderPane.js
>+++ b/mail/base/content/folderPane.js
>@@ -1135,32 +1135,44 @@ let gFolderTreeView = {
>       newRowMap = this._modes[this.mode].generateMap(this);
>     } catch(ex) {
>       Components.classes["@mozilla.org/consoleservice;1"]
>                 .getService(Components.interfaces.nsIConsoleService)
>                 .logStringMessage("generator " + this.mode + " failed with exception: " + ex);
>       this.mode = "all";
>       newRowMap = this._modes[this.mode].generateMap(this);
>     }
>+    let selectedFolders = gFolderTreeView.getSelectedFolders();

this.getSelectedFolders();

>+    if (this.selection)
>+      this.selection.clearSelection();

This is an nsITreeView (so the tree element sets the selection attribute on it), but I'd really like to see a selection: null somewhere.

>+    // restore selection.
>+    for (let i = 0; i < selectedFolders.length; i++) {

Using an iterator for this is the in thing these days.

>+function test_newly_added_folder() {

A doc comment for this would be great.
Attachment #434963 - Flags: review?(sid.bugzilla) → review-
Whiteboard: [has protocol logs] → [has patch for review sid]
Attached patch fix addressing comments (obsolete) — Splinter Review
I think this addresses your comments...
Attachment #434963 - Attachment is obsolete: true
Attachment #436372 - Flags: review?(sid.bugzilla)
Comment on attachment 436372 [details] [diff] [review]
fix addressing comments

Looks good, thanks.

>+    // restore selection.
>+    for each (let folder in selectedFolders){

This should be
for (let folder in Iterator(selectedFolders)) {

since we don't want to iterate over properties that evil extensions might add to Array's prototype, and Iterator protects against that.
Attachment #436372 - Flags: review?(sid.bugzilla) → review+
Whiteboard: [has patch for review sid] → [needs landing]
this is what I'll land on the trunk, then. Thx!
Attachment #436372 - Attachment is obsolete: true
fixed on trunk.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Attachment #436413 - Flags: approval-thunderbird3.0.5?
Comment on attachment 436413 [details] [diff] [review]
patch for checkin

nominating for 3.05
Attached patch bustage fixSplinter Review
Sorry for messing the iterator syntax up -- I always forget that it returns key-value pairs. :(

This will need to be approved along with the main patch for checkin.
Attachment #436439 - Flags: approval-thunderbird3.0.5?
Pushed yet another bustage fix for the remaining test failure: http://hg.mozilla.org/comm-central/rev/f4330a8f5022

This doesn't cleanly apply on 1.9.1, though.
Attachment #436708 - Flags: approval-thunderbird3.0.5?
Attachment #436413 - Flags: approval-thunderbird3.0.5?
Attachment #436439 - Flags: approval-thunderbird3.0.5?
Attachment #436708 - Flags: approval-thunderbird3.0.5? → approval-thunderbird3.0.5+
Sorry if this is a dumb question, but is this bug also fixed in TB 3.1? In the bug list for RC1 I cannot find it:
https://bugzilla.mozilla.org/buglist.cgi?field0-0-0=cf_status_thunderbird31;query_format=advanced;type0-0-0=equals;value0-0-0=rc1-fixed

Maybe it is just because I am unexperienced in using Bugzilla and was summoning up the wrong list. I also do not understand what "status-thunderbird3.1: wanted" means.
This was fixed in beta 2, in fact! Looks like we missed setting the flag, oops.
You need to log in before you can comment on or make changes to this bug.