1134024 - "Mark Thread As Read" does not update X-Mozilla-Status flags in message store for local folders.

Status: ASSIGNED

(MailNews Core :: Backend, defect)

Description

[:aceman]

+++ This bug was initially created as a clone of Bug #840418 +++

While fixing bug 840418 I found out that the context menu Mark Thread Read also does not set the X-mozilla-status lines in mbox properly. But it has a different problem that the other actions so I split it here.

WADA, you changed the summary in bug 840418 to also contain detach/delete (probably attachment) without any comment mentioning it. I could not reproduce the problem with this operation. Could you test it with tomorrow's nightly (that has bug 840418 fixed) and provide exact steps if you still see it?

Comment 1

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #0)
> WADA, you changed the summary in bug 840418 to also contain detach/delete  (probably attachment) without any comment mentioning it. 

It came from bug 913778 which was closed as dup of bug 840418.
   Because bug opener of bug 913778 said "dup of bug 840418", I closed bug 913778 as dup of bug 840418.
   So,  I added case of bug 913778 to bug summar of bug 840418 in order to avoid loss of case of bug 913778.

Comment 2

[:aceman]

OK, I understand now. The detach/delete is an alternative operation to repair or delete .msf. Not alternative to reply/forward/mark all read. In that case I tested it and it is really fixed by 840418. Thanks.

If we do not see any other cases of lost flags I can upload my fix for the identified "mark thread read" problem.

Comment 3

[:aceman]

Attachment: patch

I do not know why nsMsgDBView does not use the MarkThreadRead method from nsIMsgFolder, IMAPFolder, LocalFolder. Maybe there are different ways to mark a thread read and with different side-effects. So I at least make it not only work on the DB, but call the real nsIMsgFolder methods (that get overriden and save to the store too).
The test changes must be applied on top of the second patch in bug 840418.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 8566032 [details] [diff] [review]
patch

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

Looks ok to me

Comment 5

[Kent James (:rkent)]

Comment on attachment 8566032 [details] [diff] [review]
patch

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

The changes that you did won't work for a variety of reasons.

1) The dbView currently calls imap to set the message flags after MarkThreadRead is one, as does the folder, so you are now calling that twice for each message.

2) For cross-server search folders, m_folder is null and your method fails completely to mark the thread as read.

I don't understand what you are trying to accomplish. What method are you trying to call that is going to result in X-Mozilla-Status being updated properly?

::: mailnews/base/src/nsMsgDBView.cpp
@@ +6287,5 @@
>  nsresult nsMsgDBView::MarkThreadRead(nsIMsgThread *threadHdr, nsMsgViewIndex threadIndex, nsTArray<nsMsgKey> &idsMarkedRead, bool bRead)
>  {
> +    NS_ENSURE_STATE(m_folder);
> +
> +    // TODO: What threadElided used for?

Looking at the history, this was in the first landing of this code and never used. You can remove all of the threadElided calculation if you want.

Comment 6

[:aceman]

(In reply to Kent James (:rkent) from comment #5)
> 1) The dbView currently calls imap to set the message flags after
> MarkThreadRead is one, as does the folder, so you are now calling that twice
> for each message.
Yeah, ApplyCommandToIndices does marking on IMAP server again.

> 2) For cross-server search folders, m_folder is null and your method fails
> completely to mark the thread as read.
I wondered what was another m_viewFolder for.

I now see also "Mark All Read" does not work properly in a Search folder (it bails out if !m_folder).

> I don't understand what you are trying to accomplish. What method are you
> trying to call that is going to result in X-Mozilla-Status being updated
> properly?
I try to call the proper methods in nsIMsgFolder, that get overriden in nsLocalMailFolder, nsImapMailFolder so that the backend is written properly. Those overriden methods *::Mark* call nsIMsgFolder::Mark* AND do the needed stuff for the particular backend. That was the idea behind bug 840418.

The current nsMsgDBView only calls the DB AND does the needed stuff for IMAP (hardcoded), but not for other server types.

Comment 7

[Kent James (:rkent)]

Here's what you'll need to do. You can still use the folder methods, but you need to get them from the msgHdr for each email. Note it would be bad to call MarkRead separately for each email, so you should combine as many calls as you can.

I think that upstream code already does that for you, so that you can just get the folder from the first message, but I would have to check to be sure. You could be safe if you want, check the folder for each subsequent message, and if it has changed the call MarkRead for the existing messages and start adding new messages to the list.

Problem 1. can be solved I believe by simply not adding the messages to idsMarkedRead as all this does is call the imap-specific method to mark at the end, or so I believe (you should check that to be sure).

Comment 8

[:aceman]

Attachment: WIP patch v2

Yes, you are right. I was looking at it today and this is what I came up with (before your comment). So it seems we got to the same conclusion.
I found out there are different code paths (via nsMsgSearchDBView.cpp or nsMsgQuickSearchDBView.cpp) depending on how may folders does the saved search folder cover. Yes, the IMAP case will be very slow if we mark each message individually.

On the other hand I find it strange the nsMsgDBView does these direct IMAP manipulation. That should be done internally in the folder type implementation. Why should it know what type the folder is? Doesn't it violate the levels of abstraction (like you pointed out in the compact patch)? It was probably done for speed.
I'd think if IMAP (or local folder or whatever) needs any batching, it should do it internally, e.g. in nsImapMailFolder::StoreImapFlags(). At most nsMsgDBView could send it hints like StartBatching, StopBatching (flush).

I am still working on some tests using virtual folders so this is tested thoroughly.

Comment 9

[:aceman]

(In reply to Kent James (:rkent) from comment #7)
> Here's what you'll need to do. You can still use the folder methods, but you
> need to get them from the msgHdr for each email. Note it would be bad to
> call MarkRead separately for each email, so you should combine as many calls
> as you can.
> 
> I think that upstream code already does that for you, so that you can just
> get the folder from the first message, but I would have to check to be sure.
> You could be safe if you want, check the folder for each subsequent message,
> and if it has changed the call MarkRead for the existing messages and start
> adding new messages to the list.

Inside nsMsgDBView::ApplyCommandToIndices we have only a single folder to process.
However, then nsMsgDBView::ApplyCommandToIndices does:
if (command == nsMsgViewCommandType::markThreadRead)
{ for (int32_t index = 0; index < numIndices; index++) {
   SetThreadOfMsgReadByIndex(indices[index], imapUids, true); }}

So it processes each selected message to search its thread. As I commented in the nsMsgDBView::MarkThreadRead(), I see no guarantee that there we will process only messages from a single folder. In a virtual folder, threads seem to be built even from messages from multiple folders.

Comment 10

[Kent James (:rkent)]

Have you checked out this code:

http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgSearchDBView.cpp#774

Comment 11

[:aceman]

Yes, I even change it in my last patch. But I don't think it solves comment 9.