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

NEW
Assigned to

Status

Thunderbird
Folder and Message Lists
9 years ago
5 months ago

People

(Reporter: Joe Konecny, Assigned: alta88)

Tracking

({leave-open})

x86
Windows XP
leave-open

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
Version: unspecified → 3.0

Updated

9 years ago
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

Comment 3

9 years ago
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.

Comment 5

9 years ago
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

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

Comment 7

7 years ago
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

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

Updated

6 years ago
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

Updated

a year ago
Depends on: 539526
(Assignee)

Comment 9

10 months ago
Created attachment 8911585 [details] [diff] [review]
markAllRead-whitespace.patch [landed comment #13]
Attachment #8911585 - Flags: review?(jorgk)
(Assignee)

Comment 10

10 months ago
Created attachment 8911587 [details] [diff] [review]
markAllRead.patch

This is really a basic requirement for saved search with mulitple folders to be useful.
Attachment #8911587 - Flags: review?(jorgk)
(Assignee)

Updated

10 months ago
No longer depends on: 539526
(Assignee)

Comment 11

10 months ago
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-
(Assignee)

Updated

10 months ago
Attachment #8911587 - Flags: review-

Comment 12

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

Fine. Thank you.
Attachment #8911585 - Flags: review?(jorgk) → review+

Updated

10 months ago
Keywords: leave-open

Comment 13

10 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3f2554599137
fix whitespace issues in nsMsgSearchDBView missed before. r=jorgk

Updated

10 months ago
Attachment #8911585 - Attachment description: markAllRead-whitespace.patch → markAllRead-whitespace.patch [landed comment #13]
(Assignee)

Comment 14

10 months ago
Created attachment 8912457 [details] [diff] [review]
markAllRead-whitespace.patch

whitespace only.
Attachment #8912457 - Flags: review?(jorgk)
(Assignee)

Comment 15

10 months ago
Created attachment 8912464 [details] [diff] [review]
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.
Attachment #8911587 - Attachment is obsolete: true
Attachment #8912464 - Flags: review?(jorgk)
(Assignee)

Comment 16

10 months ago
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

10 months ago
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+

Comment 18

10 months ago
Created attachment 8912481 [details] [diff] [review]
markAllRead-whitespace.patch - nits fixed [landed comment #20]
Attachment #8912457 - Attachment is obsolete: true
Attachment #8912481 - Flags: review+
(Assignee)

Comment 19

10 months ago
(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

10 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/523f36e9ebc5
fix whitespace in nsMsgDBView.h. r=jorgk

Updated

10 months ago
Attachment #8912481 - Attachment description: markAllRead-whitespace.patch - nits fixed. → markAllRead-whitespace.patch - nits fixed [landed comment #20]
(Assignee)

Comment 21

10 months ago
Created attachment 8913298 [details] [diff] [review]
markAllRead2.patch

fix typo.
Attachment #8912464 - Attachment is obsolete: true
Attachment #8912464 - Flags: review?(jorgk)
Attachment #8913298 - Flags: review?(jorgk)
(Assignee)

Comment 22

10 months ago
Created attachment 8913309 [details] [diff] [review]
markAllReadUI.patch

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
(Assignee)

Comment 24

10 months ago
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..
(Assignee)

Comment 25

10 months ago
oh, it looks like your compiler doesn't like 'length' assignment that way.
No, it's Windows.
(Assignee)

Comment 27

10 months ago
Created attachment 8913352 [details] [diff] [review]
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.
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
(Assignee)

Comment 29

10 months ago
Created attachment 8913398 [details] [diff] [review]
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.
Attachment #8913352 - Attachment is obsolete: true
Attachment #8913352 - Flags: review?(jorgk)
Attachment #8913398 - Flags: review?(jorgk)
Yes, this works now.
(Assignee)

Comment 31

10 months ago
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+

Comment 33

10 months ago
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

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

Comment 35

5 months ago
What's next?
Flags: needinfo?(alta88)

Updated

5 months ago
Duplicate of this bug: 405627
(Assignee)

Comment 37

5 months ago
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)
You need to log in before you can comment on or make changes to this bug.