Open Bug 519687 Opened 12 years ago Updated 2 years ago

mark all read does not work with virtual folders/smart folders

Categories

(Thunderbird :: Folder and Message Lists, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

People

(Reporter: jkonecny, Assigned: alta88)

References

Details

(Keywords: leave-open)

Attachments

(4 files, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.19) Gecko/20081209 Thunderbird/2.0.0.19 Mnenhy/0.7.6.0

For example...   I have four accounts set up in thunderbird.  Using smart folders, all junk mail appears in the same folder.  That is good.  Then after I review junk, I mark all read (shift-c) but nothing happens.  I have to go to each junk folder and mark all read from there.  I see lots of bugs that sort of reference this with respect to sub folders but none that specifically mention smart folders.  It's unclear to me if subfolders and child smart folders function the same way and thereby are covered by an existing bug.  Sorry if there is on and I missed it but I did search.

Reproducible: Always

Steps to Reproduce:
1.  configure tb with multiple accounts
2.  make sure each account has a junk folder
3.  try to mark all read (shift-c) from a collapsed smart folder
Actual Results:  
None of the folders was marked as read.

Expected Results:  
I expect all folders to be marked as read.
Version: unspecified → 3.0
Duplicate of this bug: 532211
Confirmed here also

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.5) Gecko/20091202 Lightning/1.0b1pre Shredder/3.0.1pre ID:20091202032005

Ludo it happeans also on Mac? Should be All OS related?

a different STR (steaps to reproduce is):
1. set smart folders view;
2. expand Inboxes node
   I suppose for example this situation:
   
   Inbox
     [Account1]
     ...
     [Accountn]
3. set as unread one or more messages in Inbox;
4. select root node Inbox;
5. goto menu-->messages-->mark all as read: don't work.
Status: UNCONFIRMED → NEW
Ever confirmed: true
As mentioned in the duplicate report, it still does it in safe mode and there are no errors in the console.  It's like Thunderbird doesn't even try to do anything.
(In reply to comment #2)
> Confirmed here also
> 
> Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.5) Gecko/20091202
> Lightning/1.0b1pre Shredder/3.0.1pre ID:20091202032005
> 
> Ludo it happeans also on Mac? Should be All OS related?

Can't really test that as I use my status of read/unread to make sure that I didn't miss any email. It's probably all though.
I was searching for a bug to record that mark all unread does not work on cross-folder virtual folders, and ended up here. So this issue is broader than just smart folders, it also affects any virtual folder that has more than one folder to search.
I confirm that 'mark all read' is also not working in "saved searches" folders.
I have just upgraded to V5:

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:5.0) Gecko/20110624 Thunderbird/5.0
Application Build ID: 20110624125636

I use Unified Folders view. Previously (V3.something) I had only a single folder under Junk. I now have and Inbox and a News & Blogs folder. Previously Shift-C worked. Now it doesn't. Also I cannot set to Junk unified folder to display only Unread messages.

This is almost a show-stopper bug for me in terms on usability.
Can confirm it for 9.0.1 on Mac OS X.

I'm collecting all unread items of several folders containing rss feeds in a single folder. Marking all items and selecting "Mark>As read" works as expected, but neither "Mark>All read" with all items selected nor "Mark>All read" when no items are selected.
Component: General → Folder and Message Lists
Summary: mark all read does not work with smart folders → mark all read does not work with virtual folders/smart folders
Depends on: 539526
Attachment #8911585 - Flags: review?(jorgk)
Attached patch markAllRead.patch (obsolete) — Splinter Review
This is really a basic requirement for saved search with mulitple folders to be useful.
Attachment #8911587 - Flags: review?(jorgk)
No longer depends on: 539526
Comment on attachment 8911587 [details] [diff] [review]
markAllRead.patch


The patch needs some refinement. It works as designed in these cases:
1. Mark all read for a parent smart folder, like Trash or Junk or anything else in Unified view, and solves the issue in comment 1.
2. Mark all read for search folders with Match All Messages checked (searchTerm.matchAll = true).

However, a search folder that isn't matchAll, means only the filtered unread items visible for the search should marked, individually, and not the underlying folder..
Attachment #8911587 - Flags: review?(jorgk) → review-
Attachment #8911587 - Flags: review-
Comment on attachment 8911585 [details] [diff] [review]
markAllRead-whitespace.patch [landed comment #13]

Fine. Thank you.
Attachment #8911585 - Flags: review?(jorgk) → review+
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3f2554599137
fix whitespace issues in nsMsgSearchDBView missed before. r=jorgk
Attachment #8911585 - Attachment description: markAllRead-whitespace.patch → markAllRead-whitespace.patch [landed comment #13]
Attached patch markAllRead-whitespace.patch (obsolete) — Splinter Review
whitespace only.
Attachment #8912457 - Flags: review?(jorgk)
Attached patch markAllRead2.patch (obsolete) — Splinter Review
This will mark all read for smart/unified folders and virtual saved search folders. It marks read, in the underlying folder(s), only unread messages in the virtual folder view.  It also implements undo, which is per folder (it would be nicer to see if it could be batched but I don't know if transaction manager truly does batching; it would be a follow up). Debug left in.
Attachment #8911587 - Attachment is obsolete: true
Attachment #8912464 - Flags: review?(jorgk)
It's probably good to return Mark Folder Read (or better wording) to the folderpane context for virtual folders, maybe a UX question. Likely most use Shift C or have a mouse button programmed for it..
Comment on attachment 8912457 [details] [diff] [review]
markAllRead-whitespace.patch

Review of attachment 8912457 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you. I used ^\+.*//.*[^\.]$ to find lines missing a full stop ;-)

Right now, I'm on a bustage fix mission, see C-C treeherder. The other review might take a few days. I'd also like to finish bug 1385573.

Let me fix the nits myself, I have the file open.

::: mailnews/base/src/nsMsgDBView.h
@@ +139,5 @@
>    nsCOMPtr<nsITreeSelection> mTreeSelection;
> +  // We cache this to determine when to push command status notifications.
> +  uint32_t mNumSelectedRows;
> +  // Set when the message pane is collapsed.
> +  bool mSuppressMsgDisplay; 

Trailing space.

@@ +293,5 @@
>                                          nsMsgViewIndex *viewIndex,
>                                          uint32_t *pNumListed);
>    uint32_t GetSize(void) {return(m_keys.Length());}
>  
> +  // Notification api's.

APIs

@@ +437,5 @@
>    nsMsgKey                m_cachedMsgKey;
>  
> +  // We need to store the message key for the message we are currenty
> +  // displaying to ensure we don't try to redisplay the same message just
> +  // because the selection changed (i.e. after a sort)

Full stop.

@@ +487,3 @@
>    nsCOMPtr<nsIStringBundle> mMessengerStringBundle;
>  
> +  // Used for the preference labels

Full stop.
Attachment #8912457 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+2) from comment #17)
> Comment on attachment 8912457 [details] [diff] [review]
> markAllRead-whitespace.patch
> 
> Review of attachment 8912457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you. I used ^\+.*//.*[^\.]$ to find lines missing a full stop ;-)
> 
> Right now, I'm on a bustage fix mission, see C-C treeherder. The other
> review might take a few days. I'd also like to finish bug 1385573.
> 
> Let me fix the nits myself, I have the file open.
> 

i forgot to note in the other patch: many 'full stop' cases didn't account for a continuation of the sentence..
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/523f36e9ebc5
fix whitespace in nsMsgDBView.h. r=jorgk
Attachment #8912481 - Attachment description: markAllRead-whitespace.patch - nits fixed. → markAllRead-whitespace.patch - nits fixed [landed comment #20]
Attached patch markAllRead2.patch (obsolete) — Splinter Review
fix typo.
Attachment #8912464 - Attachment is obsolete: true
Attachment #8912464 - Flags: review?(jorgk)
Attachment #8913298 - Flags: review?(jorgk)
context menu UI.
Assignee: nobody → alta88
Attachment #8913309 - Flags: review?(richard.marti)
Hmm, on tip of m-c and c-c I get this when building with this patches:
 0:50.52 nsMsgDBView.cpp
 0:50.53 z:/Mozilla/comm-central/mailnews/base/src/nsMsgDBView.cpp(3399): error C2131: expression did not evaluate to a constant
 0:50.53 z:/Mozilla/comm-central/mailnews/base/src/nsMsgDBView.cpp(3399): note: failure was caused by non-constant arguments or reference to a non-constant symbol
 0:50.54 z:/Mozilla/comm-central/mailnews/base/src/nsMsgDBView.cpp(3399): note: see usage of 'length'
 0:50.57 z:/Mozilla/comm-central/mailnews/base/src/nsMsgDBView.cpp(3406): error C3863: array type 'nsMsgKey [length]' is not assignable
 0:50.61 mozmake.EXE[4]: *** [z:/Mozilla/comm-central/mozilla/config/rules.mk:1065: nsMsgDBView.obj] Error 2
 0:50.64 mozmake.EXE[4]: *** Waiting for unfinished jobs....
 0:50.64 nsMsgSpecialViews.cpp
 0:50.73 nsMsgGroupThread.cpp
 0:51.00 nsMsgGroupView.cpp
 0:51.08 nsMsgQuickSearchDBView.cpp
 0:51.46 nsMsgSearchDBView.cpp
 0:51.46 z:/Mozilla/comm-central/mailnews/base/src/nsMsgSearchDBView.cpp(888): warning C4477: 'fprintf' : format string '%i' requires an argument of type 'int', but variadic argument 1 has type 'nsTArray_base<Alloc,nsTArray_CopyChooser<E>::Type>::std::size_type'
 0:51.46         with
 0:51.46         [
 0:51.47             Alloc=nsTArrayInfallibleAllocator,
 0:51.47             E=uint32_t
 0:51.47         ]
 0:51.47 z:/Mozilla/comm-central/mailnews/base/src/nsMsgSearchDBView.cpp(888): note: consider using '%zi' in the format string
 0:51.51 nsMsgThreadedDBView.cpp
are you building on a mac?  does it fail the build?  i get merely a warning on linux, and it's just in a debug to be removed, so can be ingored..
oh, it looks like your compiler doesn't like 'length' assignment that way.
No, it's Windows.
Attached patch markAllRead2.patch (obsolete) — Splinter Review
paenglab, could you please give this a try - the printf is commented out so it should be a totally clean build.  apparently win wants the length cast as a const first.
Attachment #8913298 - Attachment is obsolete: true
Attachment #8913298 - Flags: review?(jorgk)
Attachment #8913352 - Flags: review?(jorgk)
Failed with this:
 0:49.22 z:/Mozilla/comm-central/mailnews/base/src/nsMsgDBView.cpp(3399): error C2131: expression did not evaluate to a constant
 0:49.22 z:/Mozilla/comm-central/mailnews/base/src/nsMsgDBView.cpp(3399): note: failure was caused by non-constant arguments or reference to a non-constant symbol
 0:49.24 z:/Mozilla/comm-central/mailnews/base/src/nsMsgDBView.cpp(3399): note: see usage of 'kLength'
 0:49.30 z:/Mozilla/comm-central/mailnews/base/src/nsMsgDBView.cpp(3400): error C2131: expression did not evaluate to a constant
 0:49.35 z:/Mozilla/comm-central/mailnews/base/src/nsMsgDBView.cpp(3399): note: failure was caused by non-constant arguments or reference to a non-constant symbol
 0:49.39 z:/Mozilla/comm-central/mailnews/base/src/nsMsgDBView.cpp(3399): note: see usage of 'kLength'
 0:49.42 z:/Mozilla/comm-central/mailnews/base/src/nsMsgDBView.cpp(3407): error C3863: array type 'nsMsgKey [kLength]' is not assignable
 0:49.45 mozmake.EXE[4]: *** [z:/Mozilla/comm-central/mozilla/config/rules.mk:1065: nsMsgDBView.obj] Error 2
I was hoping to avoid cargo culting the existing way, but.. And it still seems bogus the win compiler is so picky.

Paenglab, please try.
Attachment #8913352 - Attachment is obsolete: true
Attachment #8913352 - Flags: review?(jorgk)
Attachment #8913398 - Flags: review?(jorgk)
Yes, this works now.
Comment on attachment 8913398 [details] [diff] [review]
markAllRead2.patch


Unfortunately, the accommodation made for the case in comment 11 results in unacceptable perf for folders of any size, 10k messages range.  So matchAll folders need to be done the first way and not suffer perf, and non matchAll will by definition have to go through each message. I would think the former is the much larger use case (unified/smart folders).
Attachment #8913398 - Flags: review?(jorgk)
Comment on attachment 8913309 [details] [diff] [review]
markAllReadUI.patch

Thanks.
Attachment #8913309 - Flags: review?(richard.marti) → review+
Richard, please set ui-r+.

Note that I'm busy with bug 1404003 and bug 1403771. Since TB is really broken, I will not do non-trivial reviews until those issues are resolved. I hope you understand.
Sorry, I take that back, r+ is fine here. My apologies.
What's next?
Flags: needinfo?(alta88)
Duplicate of this bug: 405627
the patch in comment 11 needs to determine folder search matchAll context and be combined with the latest patch, for best perf.  unlikely to work on it currently.
Flags: needinfo?(alta88)
See Also: → 843396
Duplicate of this bug: 1517031
You need to log in before you can comment on or make changes to this bug.