Closed Bug 372372 Opened 14 years ago Closed 8 years ago

Search folder for NEW items does not display correct counts

Categories

(MailNews Core :: Search, defect)

x86
Windows XP
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sean115, Assigned: Bienvenu)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 2.0.50727; InfoPath.1; .NET CLR 1.1.4322)
Build Identifier: version 2.0pre (20070228)

I have a search folder that searches for messages with status NEW. The count keeps incrementing (dont know what it relates to). This is not correct - see the attachment, all messages have been READ, but still it displays an UNREAD count

Reproducible: Always

Steps to Reproduce:
1. Create UNREAD search folder
2. Receive messages and work with TB
3.
Actual Results:  
UNREAD count incorrect

Expected Results:  
UNREAD couont should update
Attached image screenie
Does deleting the .msf file for that saved search help? Find it in your profile folder <http://kb.mozillazine.org/Profile_folder>

Version: unspecified → 2.0
Yes this does help _ i sent three test messages and counts were displayed correctly. I dont know if this solves the issue for the long run.

Furthermore, this is a workaround. It shouuld work directly

ps. I also saw that my old account folders still exist, even though I have deleted these IMAP accounts
1 day ahead - samen problem

(no new messages in folders, except 1 in trash)

total unread count = 3
If you search for unread msgs using the normal search, does it find any, except the one?
All of my search folders count unread messages twice.

If I delete all msf files in my mail folder, no unread message count is shown until I click on the search folder. (BTW, deleting an msf file for a search folder destroys the search criteria, which have to be manually entered again). Then the correct number is shown, but it doesn't update automatically if new unread messages appear. Restarting TB twice brings things back to "normal": automatic updates are shown but the amount is incorrect (twice the real count).

This makes search folders pretty useless.
Summary: Search folder for UNREAD items doesn not display correct count → Search folder for UNREAD items does not display correct count
I have exactly the same problem, I have many search folders and I leave my Thunderbird running in the background. The first time it got mails, the counts are correct, but after some while it get some other mails, the counts become more than real. The longer Thunderbird running, the more counts show.
Restart Thunderbird will fix it, but it's not what I want.

To make it clear, here's my reading progress:
1. Thunderbird notify me that I have new mails
2. See the counts
3. Read the new mails, so the counts are all cleared
4. Wait some while, some new mails coming, back to step 1.
update: this seems to be the same as:
https://bugzilla.mozilla.org/show_bug.cgi?id=392321#c3

If I just don't close the Thunderbird main window, it doesn't happen.

So I'm not sure if it's the same bug with seansan, is there a close-but-not-quit mode in Windows?
(In reply to comment #4)
> (no new messages in folders, except 1 in trash)
> total unread count = 3

How many search target folders are specified for the search folder?
Same phenomenon as (2) 3. of Bug 351867 Comment #22?
When I tested in the past, click the folder(re-open the folder) corrected count.
( See (2) 4. of Bug 351867 Comment #22 ) 
Is this also true when your case?

(In reply to comment #3)
> ps. I also saw that my old account folders still exist, even though I have deleted these IMAP accounts

One problem per a bug is rule of bugzilla.mozilla.org.
I saw a bug for similar phenomenon. Search bugzilla.mozill.org well, please.  

(In reply to comment #6)
> If I delete all msf files in my mail folder, no unread message count is shown
> until I click on the search folder.

This is independent problem. See Bug 377239 Comment #4.
If you think important problem, open new bug, please.
(No one has probably opened bug for this phenomenon)

> (BTW, deleting an msf file for a search folder destroys the search criteria, which have to be manually entered again).

IMAP folder case? Or Local mail folder(POP3) case? Or both?
Criteria is saved in file of virtualFolders.dat in profile directory.
Was content of this file lost too when "delete .msf and Restart"?  

> Then the correct number is shown, but it doesn't update automatically if new
> unread messages appear. Restarting TB twice brings things back to "normal":
> automatic updates are shown but the amount is incorrect (twice the real count).

Won't "click other folder then click the folder again"(re-open the folder) correct count?
> > If I delete all msf files in my mail folder, no unread message count is shown
> > until I click on the search folder.
> 
> This is independent problem. See Bug 377239 Comment #4.
> If you think important problem, open new bug, please.
> (No one has probably opened bug for this phenomenon)
Oh, no, this is not an "important" problem. Little problems like these become important when there are too many. I just stopped using Thundirbird for this reason. 

> > (BTW, deleting an msf file for a search folder destroys the search criteria, which have to be manually entered again).
> 
> IMAP folder case? Or Local mail folder(POP3) case? Or both?
Local folder.

> Criteria is saved in file of virtualFolders.dat in profile directory.
Great, but it still keeps happening, so maybe we have another nice little bug here (and counting).

> Was content of this file lost too when "delete .msf and Restart"?  
No.

 
> > Then the correct number is shown, but it doesn't update automatically if new
> > unread messages appear. Restarting TB twice brings things back to "normal":
> > automatic updates are shown but the amount is incorrect (twice the real count).
> 
> Won't "click other folder then click the folder again"(re-open the folder)
> correct count?
No. 

The problem is so serious and so solid I can play around as much as I want, mark items read/unread, delete the search folder the recreate it, make as man nbew search folders, anything. I always get a consistent double count in search folders, no matter what. I might even record a little animation if you want (by clicking on the green button for read/unread, the double count updates consistently).

As I said, I've quit using Thunderbird. I'm terribly sad but I had to go back to Outlook. This bug has been here for months now, nobody seems to have paid attention to it and I have a business to run. Too bad. :-(
> deleting an msf file for a search folder destroys the search criteria
I've opened Bug 393352 for this issue.
My (2) 3. of Bug 351867 Comment #22 was problem when View/Message/Unread of Seamonkey. Because Thunderbird doesn't have View/Message/Unread (View/Threads/unread only), this bug and my issue was different problem.
Sorry for my confusion.

> Summary : Search folder for UNREAD items does not display correct count

Count display only problem?
Did you observe both unread count & total count in folder pane?
It is similar issue to Bug 351867, isn't it?
(invalid count is simply a result of internal view setting problem or folder pane/thread pane refresh problem)

With my quick test(Tb 2.0.0.6 and trunk 2007/8/22 build), following problems seems to still remain.
(1) View/Thread/Unread setting of a search looks to be reset after several click of other folder(especiall when usual folder click)
(2) When clicking among search folders only, phenomenon seems to be different from (1).
(3) Thread pane was sometimes cleared when View/Threads/All is set for a search folder, after problem (1) or (2).
These are similar problems to Bug 351867, but different problems because Bug 351867 is already fixed1.8.1(fixed by 2.0.0.1).

To seansan(bug opener):

If you continue to test Tb, test with 2.with 2.0.0.6 and trunk, please.
And, if you find problems like Bug 351867, please report detailed test procedure and detailed result at each step (count display, thread pane display, View/Thread setting after each operation, etc.)
If you already stopped to test Tb, close as INCOMPLETE, please. 
Severity: normal → minor
My current method and experience follows:

* defined Search Folder named "Today" as subfolder of Inbox
  - Age In Days is less than 1
  - Junk Status isn't Junk
  - Status is New

It works, but has quirks.

1. Unread count stays at X, even after several messages inside the Search Folder have been marked Read. There have been two ways to fix this, or basically force reset: select "All" from the View toolbar menu (does not matter if it's already selected before), or switch to another folder and back. Both actions correctly remove the Unread count from my Search Folder. It works, but it's rather annoying.

2. With regards to being forced to often click on other folders there have been several occasions for me where  the thread pane is not updated at all with the newly selected folder's contents. I will have to do additional measuring and take notes on this, but the problem definitely is real. For now I would say that perhaps the thread pane remains un-updated only when I am switching away from my special Search Folder. I have never noticed similar behavior switching between regular folders, such as Sent and Inbox or any of my custom ones. My Search Folder idea above is relatively recent, so for now I am tending to suggest it is the culprit here. Further notes to follow.
Let's focus this bug on the issue with saved searches involving New, since they have a single root cause in the code. I think I'm going to fix this as part of bug 428427, but first let me document what I think is the problem.

At http://mxr.mozilla.org/seamonkey/source/mailnews/base/src/nsMsgAccountManager.cpp#2462 we have:

if (oldMatch != newMatch || (oldMatch && (aOldFlags & MSG_FLAG_READ) != (aNewFlags & MSG_FLAG_READ)))

This is where the virtual folder decides whether to look for count changes - but it does not check for changes in New. This root issue is a little different than the original case for bug 428427, but close enough to fix at the same time.

I can confirm the original bug description, but not necessarily all of the variants in the comments.
Status: UNCONFIRMED → ASSIGNED
Component: Mail Window Front End → MailNews: Search
Ever confirmed: true
Product: Thunderbird → Core
Version: 2.0 → Trunk
Changing description to mention NEW, changing assignee and QA contact.
Assignee: mscott → kent
Status: ASSIGNED → NEW
QA Contact: front-end → search
Summary: Search folder for UNREAD items does not display correct count → Search folder for NEW items does not display correct counts
Unfortunately this is more complex than I had hoped, so I probably will not fix it as part of bug 428427 - though the same approach is needed. Let me explain the issues for future fixers (including possibly a braindead me who will forget all of this in a matter of hours).

The OnHdrChanged function for the virtual folders, located in nsMsgAccountManager.cpp, needs to determine if the filter matched before and after the change. (Note I am renaming OnHdrChanged to OnFlagsChanged). It tries to do that by trying to recreate the status before the change - by writing the old flags to the nsIMsgDBHdr. Unfortunately in the case of NEW, that status is not simply maintained in the database, so the SetFlags does not really restore the NEW status. That is, from the search terms perspective, the NEW flags it is reading during the search are not affected by the nsIMsgDBHdr->SetFlags. As a result, the oldMatch is not accurate, and the counting can fail.

One solution is to follow the approach I am taking in bug 428427, namely call OnHdrChanged twice, once before and once after the change, and let it store status. That requires moving the entire notification up a level though, since you need the list of listeners to do that.
Status: NEW → ASSIGNED
Blocks: 438257
Product: Core → MailNews Core
I think this issue of mine is relevant to this one. If not, or is duplicate of another bug I somehow missed, please let me know.

When you create a new subfolder under a folder that is part of a search folder's criteria, there is no prompt to ask if you want to include it to any search folder that has it's parent folder as a criteria. As a mater of fact, I believe that it would be much better if each time a new folder is created, a list of all the existing search folders is presented to the user, with the choice to include the newly created folder to any search folder's criteria or not (check boxes next to each search folder).

Also, in the case you rename a an old folder that is part of a search folder's criteria, there is no warning that it will be removed from the search folder. Now that I think about it, it should not just warn you, but offer to either remove from the criteria or update them instead.
This little patch may be able to solve this issue with the NEW counts, in combination with my current patch for bug 272963 - which allows the stored value of the NEW flag in the nsMsgHdr object to be preserved temporarily. For now, I'm going to test for awhile, but I will mark the dependency.
Depends on: 272963
Whiteboard: [has patch] [blocked by 272963]
Adding dependency which will be needed in the unit test.
Depends on: 498154
Attached patch added unit test (obsolete) — Splinter Review
Added unit test, but it requires the POP3pump from bug 498154, so that will delay review of this bug until that bug lands.

I've been running this patch for a few days now. It makes the counts in a New search folder usable for the first time, but it does not solve all of the issues. But it is a step in the right direction, and solves the first-order problem. The other issues (which I have not developed STR for yet) are probably cases where NEW flag is being changed without proper notifications.
Attachment #382383 - Attachment is obsolete: true
Whiteboard: [has patch] [blocked by 272963] → [has patch] [blocked by 498154]
One case where I notice this does not work is RSS. One of the better use cases of a New search is to track a high-volume feed like Mozilla Planet, where you want to note new incoming items but not necessarily read them. Unfortunately, the New count does not change in a New search folder, even with my latest patch above, when I exit the RSS folder containing the New items. Also, we might want to clear New items from the New search folder when we exit the new search folder itself, but that is not currently done (but probably should be).
I've been running with this for awhile, and I think it is a vast improvement. So let's review and land it if possible. Not a priority though.

There are two additional things that need to be done to make counts reasonably correct for NEW flag virtual folders:

1) fix missing notification in a variety of cases. Since previously the notifications were essentially no-ops, it is not surprising that several are missing.

2) deal with the issue that, on restart, the NEW flags are all cleared, yet the virtual folder is not updated. Either we need to tell the virtual folder about that, or persist the NEW flags on restart. I would prefer the latter, as almost everyone I have ever asked thinks that they need to be persisted between restarts.
Attachment #384161 - Attachment is obsolete: true
Attachment #386089 - Flags: superreview?(bienvenu)
Attachment #386089 - Flags: review?(bienvenu)
Whiteboard: [has patch] [blocked by 498154] → [needs r/sr bienvenu] [blocked by 498154]
Whiteboard: [needs r/sr bienvenu] [blocked by 498154] → [needs r/sr bienvenu]
Comment on attachment 386089 [details] [diff] [review]
changed interface uid, updated for latest POP3pump (partial fix)

ugh, I don't like making this part of the msg hdr state - let me think if I can see any way around that...
Kent, I'm not seeing any problems with a saved search based on New - is it possible your other patches fixed the issue and made the patch in this bug unneeded? If not, are there any simple STR for the remaining issue(s)?
Current STR:

1) Create a saved search with Status Is New on a local inbox.
2) Send a new message to the inbox (Note both inbox and New virtual folder show 1 unread message, and have new decoration)
3) Select the inbox, and then select another local folder (not the New virtual folder). (Note inbox loses its New decoration)

Expected results:

New virtual folder loses its new decoration, its bold showing one unread message, and the count goes to zero.

Actual results:

New virtual sees no change on leaving the Inbox, and continues to display as bold, with 1 unread message in it, and with new decoration. When the New virtual folder is selected, folder is updated to have no messages, and no messages are displayed. When New is left, new decoration disappears.
those STR work fine for me w/o this patch. My "new" saved search is just over a single folder, and just shows "New" messages.
actually, the saved search still has the new decoration and bold, but it has an unread count of 0 as soon as I leave the inbox...
I should say, that happened once - usually everything gets cleared when I leave the inbox...
My STR are quite reproducible for me. Are you *sure* you're not varying your steps somehow, or doing something else? My understanding is that with current trunk, changes to the new flag do not get propogated to the header, so there is no way for the virtual folder listener to see them.

As to alternate approaches, the root cause is the requirement that the new set stay sorted. We could resort it if needed when the new flag is changed, though that might cause serious issues on a folder with thousands of new messages, and a new flag virtual folder. My approach in bug 441932 also addressed the issue, though I know now one flaw in that was that I assumed that the folder object persists, but now I know that certain operations (such as a folder rename) destroy it. That was a fairly complex change as well, though it was aimed at larger issues than just this new flag. Perhaps the new flag portion of that could be separated as an alternate approach.

The new virtual folder can be useful, BTW. I add to it not only the inbox that I care about, but also certain RSS feeds that I want to track. It sort of becomes a kind of favorites folder.
I'll set a breakpoint on the virtual folder change listener and attach a stack trace when I get a chance.
I looked at the code again, and it's not as clear as I thought, so I'm not
quite as confident of my statement "changes to the new flag do not get
propogated to the header" as I was - though I still think that is true, and the
STR are still reproducible for me. But if not for you, then I can check my
setup as well.
nsMsgDatabase::ClearNewList notifies all listeners of the change in the new flag for the new hdrs...but I did see it fail once, so there may be cases that don't work (perhaps having to do with whether the hdr is cached or not?). The time it failed for me, I was already in the inbox when the new msg arrived, though I'm pretty sure I've seen it work in that case as well.
I now see that it varies as you say. For my STR, POP mail is setup to not automatically download messages, but on demand. If I have selected an alternate subfolder of the POP3 server when I request a download, then go to the inbox, then return to the alternate subfolder, then the New counts look OK. In all of my previous reports, I had the Inbox folder selected when I downloaded messages, and the New subfolder came out incorrectly.

It must have to do with when the database gets opened or something. Perhaps there is some way to make it always work the way that gives the correct counts.
What I've observed this weekend is that junk messages mess up the counts in the "new" saved search. I haven't been able to debug it yet, but I'll do so as soon as I can.
(In reply to comment #34)
> junk messages mess up the counts in the "new" saved search.

FYI.
Bug 272125 is for "new mail count issue of new mail alert due to Junk mail".
Bug 496254 is an spin-off of the bug for a clear case.
Comment on attachment 386089 [details] [diff] [review]
changed interface uid, updated for latest POP3pump (partial fix)

clearing review request for further investigation, in particular, the junk issue, which is the one I can recreate...
Attachment #386089 - Flags: superreview?(bienvenu)
Attachment #386089 - Flags: review?(bienvenu)
Here's the bug I was able to reproduce:

1. Setup a saved search on a pop3 inbox, that just matches new messages.
2. Send yourself a new message.
3. the saved search should show a count of 1, and be bold in the folder pane.
4. Collapse message pane w/ f8
5. Move the new message to an other pop3 folder.

Prior to the patch, that would leave the unread count on the folder at 1, and leave it bold. With the patch, the count goes to 0 and the folder loses the bold status. I'm reasonably sure this will fix the issue I saw with junk mail as well.

As the comment in the bug says, the notifier will pass in the new flag if the deleted message had been new, and the virtual folder listener should have been paying (more) attention to it.

Collapsing the message pane is important, and doing move intead of delete is important, because the front end is marking messages read before deleting them, and the "new" saved search was adjusting its counts correctly when the msg is marked read first.
Assignee: kent → bienvenu
Attachment #393014 - Flags: superreview?(neil)
Attachment #393014 - Flags: review?(kent)
I reviewed and ran your patch, and it works fine. One question though. The same upstream code that stores the New flag, and then passes the flags to the listener, also does the same thing with thread parent. Perhaps you could finish fixing the error, and do the same thing with thread parent in the listener? I don't think there is any way that the base code now uses the thread parent in a virtual folder, but with the new custom searches, it would be possible for an extension to define a saved search that somehow used the thread parent.

As for my original patch, I' a little befuddled right now, as I cannot reproduce the STR in my comment 25, and the unit test passes with or without my patch. So I'll abandon that for now, as the changes from bug 272963 seem to be mostly solving the issue.

However, the same STR that I gave in comment 25 for a "local inbox" do still work for me if that is an imap inbox, and neither your patch nor mine fix that. (Yours is not aimed at that case, so it should move forward anyway.) I'll investigate the IMAP case myself when I get a chance.
I'm not sure I understand what you want to do with the thread parent. There's this code:

  if (notify)
  {
    (void)msg->GetFlags(&flags);
    msg->GetThreadParent(&threadParent);
  }

and this code:

    if (hdrWasNew)
      flags |= nsMsgMessageFlags::New;

but I'm not tweaking the thread parent in any way that I can see...and I'm not sure what state of the parent needs to be adjusted because a new child was deleted.

Or were you referring to the thread object itself?
Whiteboard: [needs r/sr bienvenu]
In nsMsgDatabase, there is the comment:

  //Save off flags and threadparent since they will no longer exist after we remove the header from the db.

So my understanding is that the message header that gets passed to NotifyHdrDeletedAll does not have the correct thread parent - just like the NEW flag. In the virtual change listener, your change corrects the new flag, and I am suggesting that you also correct the thread parent. That is,

  // Since the notifier went to the trouble of passing in the msg flags
  // and thread parent, we should use them when doing the match.
  PRUint32 msgFlags;
  aHdrDeleted->GetFlags(&msgFlags);
  aHdrDeleted->SetFlags(aFlags);
  nsMsgKey threadParent;
  aHdrDeleted->GetThreadParent(&threadParent);
  aHdrDeleted->SetThreadParent(aParentKey);
  rv = m_searchSession->MatchHdr(aHdrDeleted, msgDB, &match);
  aHdrDeleted->SetThreadParent(threadParent);
  aHdrDeleted->SetFlags(msgFlags);

I don't really understand why the changes to the thread parent and new flag
are done in nsMsgDatabase before the notification. All of this would not be needed if the notification was done before the changes, so that the header was correct. But that could have lots of interesting side effects.
ah, ok, restoring the thread parent - that makes sense.

Re doing notifications before or after, you pick your poison at the start, basically, and then it's very hard to change, because, as you say, there could be lots of interesting side effects. I don't remember the reasons anymore...
Comment on attachment 393014 [details] [diff] [review]
proposed fix for issues I could recreate... - checked in.

I like Kent's idea but I'm OK with this version too.
Attachment #393014 - Flags: superreview?(neil) → superreview+
Comment on attachment 393014 [details] [diff] [review]
proposed fix for issues I could recreate... - checked in.

I can also accept either this version, or one with the additional changes that I proposed.
Attachment #393014 - Flags: review?(kent) → review+
Comment on attachment 393014 [details] [diff] [review]
proposed fix for issues I could recreate... - checked in.

I believe the thread parent is not in need of repair, since it is cached. Calling GetThreadParent where we do makes sure it gets cached. So I checked in this patch as is.
Attachment #393014 - Attachment description: proposed fix for issues I could recreate... → proposed fix for issues I could recreate... - checked in.
Duplicate of this bug: 506953
(In reply to David :Bienvenu from comment #44)
> Comment on attachment 393014 [details] [diff] [review]
> proposed fix for issues I could recreate... - checked in.

But still open after checkin. Anything left to do?
Flags: needinfo?(mozilla)
Flags: needinfo?(kent)
"Anything left to do?"

At this point, years later with a complex subject and no subsequent comments, I think we can close it and ask for a new bug with any remaining issues.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: needinfo?(kent)
Resolution: --- → FIXED
Flags: needinfo?(mozilla)
You need to log in before you can comment on or make changes to this bug.