188051 - IMAP mark as read sends DeleteOrMoveMsgCompleted event

Status: VERIFIED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[[e-mail]]

When using an IMAP folder, and I click on the Unread toggle in the thread pane,
then immediately open the context menu for a message, then that message is
loaded into the message pane. This doesn't happen with a local (POP) or news folder.

Comment 1

[David :Bienvenu]

wfm - this is not likely to be an imap problem - it's the front end that tries
to load the message.

Comment 2

[[e-mail]]

BTW, I forgot to mention, the problem doesn't occur working offline either.

Comment 3

[grylchan]

works for me in 1/6 trunk on xp.

What build are you using Neil?

and in your steps:
1.I click on the Unread toggle in the thread pane,
2.then immediately open the context menu for a message, 
3.then that message is loaded into the message pane.

Before step 1, is there a mesg already selected or no mesg selected
at all in thread pane?

after you do step 1, is that mesg selected?

I assume in step 2, you are clicking another mesg and not the one
you chose in step 1.

What type of imap mail server?
thanks.

Comment 4

[[e-mail]]

IMAP server is from www.gordano.com * OK NTMail IMAP4 server v6.04.0010 ready

Also, your analysis of my steps is correct - the message which I am
right-clicking is not previously selected.

Also, I have just discovered by tracing though with Venkman, that the problem is
caused by a HandleDeleteOrMoveMsgCompleted item event. This event thinks that I
just deleted a message in another window and checks the selection, sees that
there is one item, and loads it. So why is this event being sent when I mark as
read? It is sent for every mark as read, but this is normally not noticable.

Comment 5

[David :Bienvenu]

Neil, is this a release build or your own build? If the latter, have you tried
doing a clobber? Have you been able to reproduce this with a released build?

Comment 6

[[e-mail]]

This was originally in a Win nightly, and I double checked with a Linux build.

Comment 7

[[e-mail]]

FYI: The event is generated by line 4485 in nsImapMailFolder.cpp

Comment 8

[[e-mail]]

The URL being processed is imap://bad@mail:143/addmsgflags>UID>.INBOX>759>1

Comment 9

[[e-mail]]

Attachment: Extract of nsImapMailFolder.cpp

Surely the NotifyFolderEvent belongs inside this if?

Comment 10

[[e-mail]]

Attachment: Proposed patch

This fixes the problem for me.

Comment 11

[David :Bienvenu]

No, I think the correct fix is to break out second clause of the if (the
ShowDeletedMessages() part) and then put the Notify event inside the first
clause. Otherwise, you're going to break stuff. So it should look something like
this:

            if (flags & kImapMsgDeletedFlag)
            {
               if (!ShowDeletedMessages())
               { 
                nsCOMPtr<nsIMsgDatabase> db;
                rv = GetMsgDatabase(nsnull, getter_AddRefs(db));
                if (NS_SUCCEEDED(rv) && db)
                {
                    nsMsgKeyArray keyArray;
                    char *keyString = nsnull;
                    imapUrl->CreateListOfMessageIdsString(&keyString);
                    if (keyString)
                    {
                      ParseUidString(keyString, keyArray);
                      db->DeleteMessages(&keyArray, nsnull);
                      db->SetSummaryValid(PR_TRUE);
                      db->Commit(nsMsgDBCommitType::kLargeCommit);
                      nsCRT::free(keyString);
                    }
                }
            }
            NotifyFolderEvent(mDeleteOrMoveMsgCompletedAtom);
           }

Comment 12

[[e-mail]]

Bah, DeleteOrMoveMessageCompleted is broken for IMAP mark as deleted anyway :-P

Comment 13

[(not reading, please use [e-mail] instead)]

hoping to get to this for 1.3 final

Comment 14

[Samir Gehani]

Mail triage team: nsbeta1+/adt3

Comment 15

[(not reading, please use [e-mail] instead)]

accepting, since it must be driving imap delete model users (like neil) nuts.

I'll investigate the fix bienvenu suggested.

Comment 16

[(not reading, please use [e-mail] instead)]

I'm using mark as deleted as my imap delete model, but I've not figure out how 
to reproduce this.

Comment 17

[David :Bienvenu]

do you need to have a stand alone msg window open as well?

Comment 18

[(not reading, please use [e-mail] instead)]

I am seeing something that neil reported back in comment #4.

I see HandleDeleteOrMoveMsgCompleted() getting called when a message is marked 
as unread.  

I'll start there.

Comment 19

[(not reading, please use [e-mail] instead)]

ok, this is what bienvenu was talking about in comment #11.

I'm trying out his suggested fix now.

what the current code does is when we add imap flags, we always send the 
deleted or copied folder event, but we should only be doing that when we add 
the "imap delete" flag.

Comment 20

[(not reading, please use [e-mail] instead)]

Comment on attachment 113677 [details] [diff] [review]
Proposed patch

bienvenu suggested something else, I'll attach it.

Comment 21

[(not reading, please use [e-mail] instead)]

Attachment: bienvenu's suggested fix, testing it now...

Comment 22

[(not reading, please use [e-mail] instead)]

Attachment: same fix, more comments, still need to test.

Comment 23

[David :Bienvenu]

Comment on attachment 113849 [details] [diff] [review]
same fix, more comments, still need to test.

r/sr=bienvenu

Comment 24

[(not reading, please use [e-mail] instead)]

Comment on attachment 113849 [details] [diff] [review]
same fix, more comments, still need to test.

r/sr=sspitzer on bienvenu patch.

I tested mark as delete and move to trash delete models, and we do the right
thing.

I bet there are other existing bugs that are dups of this.

worth taking for 1.3 beta, so seeking approval.

Comment 25

[(not reading, please use [e-mail] instead)]

> Bah, DeleteOrMoveMessageCompleted is broken for IMAP mark as deleted anyway

I think I know what neil is referring to.

in this delete model, we don't select the next message, instead of the next un-
deleted message.  

see bug #65823.  if you mean something else, let me know.

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 113849 [details] [diff] [review]
same fix, more comments, still need to test.

I think we're wrapped on 1.3beta so this will have to wait 'till final.

Comment 27

[[e-mail]]

As well as those issues, I seem to remember that no event is generated when you
are offline or after a compact.

Comment 28

[(not reading, please use [e-mail] instead)]

Comment on attachment 113849 [details] [diff] [review]
same fix, more comments, still need to test.

seeking approval for 1.3 final.

Comment 29

[(not reading, please use [e-mail] instead)]

Comment on attachment 113849 [details] [diff] [review]
same fix, more comments, still need to test.

asa says (over irc) a=asa for 1.3 final, but wait until the tree is ready for
1.3 final checkins.

Comment 30

[(not reading, please use [e-mail] instead)]

fixed.

Comment 31

[(not reading, please use [e-mail] instead)]

> As well as those issues, I seem to remember that no event is generated when 
> you are offline or after a compact.

neil, can you log a new bug on that issue?

Comment 32

[[e-mail]]

Bliss!