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

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Joe Konecny]

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.

Comment 2

[[:Aureliano Buendía]]

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.

Comment 3

[webolutionary]

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.

Comment 4

[Ludovic Hirlimann [:Usul]]

(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.

Comment 5

[Kent James (:rkent)]

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.

Comment 6

[Juliano F. Ravasi]

I confirm that 'mark all read' is also not working in "saved searches" folders.

Comment 7

[Lary Singer]

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.

Comment 8

[Stefan G.]

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.

Comment 9

[alta88]

Attachment: markAllRead-whitespace.patch [landed comment #13]

Comment 10

[alta88]

Attachment: markAllRead.patch

This is really a basic requirement for saved search with mulitple folders to be useful.

Comment 11

[alta88]

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..

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8911585 [details] [diff] [review]
markAllRead-whitespace.patch [landed comment #13]

Fine. Thank you.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3f2554599137
fix whitespace issues in nsMsgSearchDBView missed before. r=jorgk

Comment 14

[alta88]

Attachment: markAllRead-whitespace.patch

whitespace only.

Comment 15

[alta88]

Attachment: markAllRead2.patch

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.

Comment 16

[alta88]

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 17

[Jorg K (CEST = GMT+2)]

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.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: markAllRead-whitespace.patch - nits fixed [landed comment #20]

Comment 19

[alta88]

(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..

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/523f36e9ebc5
fix whitespace in nsMsgDBView.h. r=jorgk

Comment 21

[alta88]

Attachment: markAllRead2.patch

fix typo.

Comment 22

[alta88]

Attachment: markAllReadUI.patch

context menu UI.

Comment 23

[Richard Marti (:Paenglab)]

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

Comment 24

[alta88]

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..

Comment 25

[alta88]

oh, it looks like your compiler doesn't like 'length' assignment that way.

Comment 26

[Richard Marti (:Paenglab)]

No, it's Windows.

Comment 27

[alta88]

Attachment: markAllRead2.patch

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.

Comment 28

[Richard Marti (:Paenglab)]

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

Comment 29

[alta88]

Attachment: markAllRead2.patch

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.

Comment 30

[Richard Marti (:Paenglab)]

Yes, this works now.

Comment 31

[alta88]

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).

Comment 32

[Richard Marti (:Paenglab)]

Comment on attachment 8913309 [details] [diff] [review]
markAllReadUI.patch

Thanks.

Comment 33

[Jorg K (CEST = GMT+2)]

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.

Comment 34

[Jorg K (CEST = GMT+2)]

Sorry, I take that back, r+ is fine here. My apologies.

Comment 35

[Wayne Mery (:wsmwk)]

What's next?

Comment 37

[alta88]

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.

Comment 39

[Geoff Lankow (:darktrojan)]

Attachment: Bug 519687 - Fix cmd_markAllRead for virtual folders. r=#thunderbird-reviewers

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/24dba6fd0179
Fix cmd_markAllRead for virtual folders. r=mkmelin

Comment 41

[Magnus Melin [:mkmelin]]

comm/mail/test/browser/folder-display/browser_messageCommands.js is failing due to this
https://treeherder.mozilla.org/jobs?repo=comm-central&selectedTaskRun=I3UtCvWYRwWQpf4CJx-BvQ.0

Comment 42

[Geoff Lankow (:darktrojan)]

Attachment: Bug 519687 follow-up - Fix a broken test. r=#thunderbird-reviewers

There was no need for this function to take an argument, that was a leftover piece from an earlier attempt.

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/949870ea2023
follow-up - Fix a broken test. r=mkmelin

Comment 44

[Wayne Mery (:wsmwk)]

good for 115?

(I think it's great that we are slowly bringing Unified Folders up to parity with other folders)

Comment 45

[Geoff Lankow (:darktrojan)]

Comment on attachment 9361133 [details]
Bug 519687 - Fix cmd_markAllRead for virtual folders. r=#thunderbird-reviewers

Ship it. Two patches here.

Comment 46

[Wayne Mery (:wsmwk)]

Comment on attachment 9361133 [details]
Bug 519687 - Fix cmd_markAllRead for virtual folders. r=#thunderbird-reviewers

[Triage Comment]
Approved for esr115

Comment 47

[Wayne Mery (:wsmwk)]

Comment on attachment 9361545 [details]
Bug 519687 follow-up - Fix a broken test. r=#thunderbird-reviewers

[Triage Comment]
Approved for esr115

Comment 48

[Rob Lemley [:rjl]]

Thunderbird 115.5.2:
https://hg.mozilla.org/releases/comm-esr115/rev/c150358d9763
https://hg.mozilla.org/releases/comm-esr115/rev/a3a63c9d0d24