349714 - new flag not removed from cross-folder saved search when last new message is read

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[David :Bienvenu]

Now that we have an icon for saved search with new messages (yippee!) we need to clear the new flag on the saved search folder when the last new message is read. I'm a little surprised (but happy) that the new flag is getting set in the first place.

Comment 1

[Mike Cowperthwaite]

With 2a1-0821, I just saw this behave as I expected:  I had one single-folder search and one multi-folder search visible in the folder pane, and a message arrived that matched both criteria.  Both folders bolded and flagged New.

I read the new message from the Inbox; and when it was read, all three folders de-bolded (see bug 300487) and lost their New flag.

Is there something else I need to test?

Comment 2

[David :Bienvenu]

yeah, of course, after filing the bug, it works fine for me too.  But I swear, from the time the new theme landed, until today, I almost never saw the new flag getting cleared until I clicked away from the folder. I also would sometimes get in a situation where I'd read all the messages, and delete them, but I'd still see an unread count of 1.

Comment 3

[David :Bienvenu]

Attachment: fix several new flag/unread count issues

This patch does several things:

Share common code in nsMsgDBFolder::UpdateNewMessages

Fix imap code to only count unread non-downloaded headers as new, instead of all headers (this should help with the 0 new messges alert)

Fix filters that mark imap messages as read to not leave them as new, which will help with the new flag not getting set for the folder.

Try to stop new count from getting negative, which causes us to not clear the new flag.

Comment 4

[Wayne Mery (:wsmwk)]

David, any more before marking fixed? (patch checked in 2007-02-08)

Comment 5

[David :Bienvenu]

This seems to have gotten worse, or I've noticed it more. I have a fix now. I need to write a unit test for this, which I'll try to do soon.

Comment 6

[David :Bienvenu]

Attachment: proposed fix

There were a couple issues - if someone has cleared the folder's cached db (and we've gotten pretty good at that), then clearing the new list didn't work, and also the saved search listener wasn't adjusting the new flag in all the right cases...This should fix both issues.

Comment 7

[David :Bienvenu]

Attachment: fix with start of a test

this fix works better, and starts to get a test going, though the test has issues because virtual folders don't seem to have the right listeners attached on creation.

Comment 8

[David :Bienvenu]

Attachment: proposed fix with unit test

The fixes are as described in #c6, plus a couple fixes to deal with unit test failures.

I had to move the account manager's mail session folder listener registration to the top of of the method, before the check for an empty account list. Otherwise, for new profiles, we were not listening for folder events, which broke my unit test, and may have caused various subtle first run bugs.

And I had to check for failure to get the incoming server type - this was to fix an error in the pop3gssapi test, which creates and destroys servers, without having the root folder get initialized, which causes an assertion (it's essentially a unit test artifact, but we should avoid the assertion, since it makes tests fail).

Comment 9

[[e-mail]]

Comment on attachment 459081 [details] [diff] [review]
proposed fix with unit test

>+  if (numNewMessages > 0)
>+    numNewMessages--;
>+  m_virtualFolder->SetNumNewMessages(numNewMessages);
>+  if (!numNewMessages)
>+    m_virtualFolder->SetHasNewMessages(PR_FALSE);
Could this all go in its own block? No point updating the number of new messages if you're not changing it.

Comment 10

[Kent James (:rkent)]

Although attachment 459081 [details] [diff] [review] causes the immediate symptom of this bug as described in the summary, it does not fix the issue of virtual folder new decoration being out of sync with corresponding virtual folders in other cases. In particular, I have the following STR (all described details may not be necessary):

1) Create a filter to move new IMAP messages containing in the subject "test" to an IMAP inbox subfolder, which I call "bird".

2) Create a virtual folder (which I call "testVirtual") as a subfolder of the local folder inbox, which searches folder bird for items with subject contains "test".

3) Send a matching message, which gets moved to "bird", while a different folder is selected. (Note that both bird and testVirtual get the new item decoration).

4) Select folder bird, then select a different folder. Expected results: both bird and testVirtual lose their new decoration. Actual results: bird loses the decoration, but testVirtual keeps it until the mouse is moved over testVirtual. Note that no messages in testVirtual actually have new flag set.

The root cause of this is that the notification being sent to the folderPane is OnItemBoolPropertyChanged (responding to the change in the hasNew status of the folder), but the folder pane has an empty response to this.

I can solve the problem by changing this to:

  OnItemBoolPropertyChanged: function(aItem, aProperty, aOld, aNew) {
    let index = this.getIndexOfFolder(aItem);
    if (index != null)
      this._tree.invalidateRow(index);
  },

I'm not sure what your reaction to this is. It seems to me that if we are going to try to get the virtual folder new decoration correct, then we should be dealing with changes in new in the real folder that are caused by conditions other than just changes in read status. I could see some value in perhaps not changing the new status of the virtual folder, but the condition with the patch (which changes decoration on mouseover) is not that solution.

So I think you should change your patch to include the above code, but I don't feel strongly about it either.

Comment 11

[Kent James (:rkent)]

Oops, causes the immediate symptom of this bug as described in the summary *to go away*

Comment 12

[David :Bienvenu]

I think this is reasonable:

  OnItemBoolPropertyChanged: function(aItem, aProperty, aOld, aNew) {
    let index = this.getIndexOfFolder(aItem);
    if (index != null)
      this._tree.invalidateRow(index);
  },

and I can add it to my patch. I'd want to make sure that aItem is a folder, and that there aren't a bunch of property changes that happen that would cause unneeded redraws, but I doubt that's the case.

Comment 13

[David :Bienvenu]

Attachment: fix incorporating rkent's suggestion

Comment 14

[Kent James (:rkent)]

Comment on attachment 466845 [details] [diff] [review]
fix incorporating rkent's suggestion

I'm a little nervous about moving the adding of the listener to the top of LoadAccounts(), but I can't think of any specific issues with that, so go for it.

Comment 15

[David :Bienvenu]

fixed on trunk.

Moving the listener registration just to fix the xpcshell test bothered me at first, but then it occurred to me that it should fix some potential first run issues as well.