366968 - Forward/Reply filter does not update message status for the forwarded/replied message

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070111 Thunderbird/3.0a1 ID:2007011103

If you create a filter to forward incoming messages to another address the filter doesn't update the X-Mozilla-Status and X-Mozilla-Status2. So forwarded messages don't have the small purple arrow in front of the subject. Same applies for current 1.8 branch builds.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Still happens with the latest nightly build on trunk.

Using this feature means you will not know if you have already forwarded or replied a message. Adding the dataloss keyword. Something which could be possible for Thunderbird 3?

Comment 2

[Damon Sicore (:damons)]

Not blocking 1.9.1.

Renom if you disagree.

Comment 3

[Magnus Melin [:mkmelin]]

Taking.

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Comment 5

[Magnus Melin [:mkmelin]]

(The "a the" comment type i already fixed locally.)

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 366194 [details] [diff] [review]
proposed fix

>+/**
>+ * Allow filters to automatically reply to a message. The reply message is
>+ * based on the given template.
>+ */
> NS_IMETHODIMP nsMsgComposeService::ReplyWithTemplate(nsIMsgDBHdr *aMsgHdr, const char *templateUri,
>                                              nsIMsgWindow *aMsgWindow, nsIMsgIncomingServer *aServer)

Hmm, could you add these comments to the idl instead (and for the other function)? Maybe add a few comments about the parameters as well.

>   pMsgComposeParams->SetType(/* forwardType ? nsIMsgCompType::ForwardInline : */nsIMsgCompType::ForwardAsAttachment);
...
>   pMsgComposeParams->SetOriginalMsgURI(msgUri.get());
...
>-  return pMsgCompose->SendMsg(nsIMsgSend::nsMsgDeliverNow, identity, nsnull, nsnull, nsnull) ;
>+  rv = pMsgCompose->SendMsg(nsIMsgSend::nsMsgDeliverNow, identity, nsnull, nsnull, nsnull);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  return folder->AddMessageDispositionState(aMsgHdr, nsIMsgFolder::nsMsgDispositionState_Forwarded);
> }

I don't understand why nsMsgCompose isn't doing this for us. We've set the type to forward, and the original message uri, so nsMsgCompose::ProcessReplyFlags should be setting the forward state on successful send.

In the reply case, we seem to be sending a new message rather than a reply, which seems a bit strange.

Comment 7

[Magnus Melin [:mkmelin]]

The immediate "cause" is that in nsMsgCompose we have

nsMsgComposeSendListener::OnStopSending

  nsCOMPtr<nsIMsgCompose> msgCompose = do_QueryReferent(mWeakComposeObj, &rv);
  if (msgCompose) {
  ...
    msgCompose->ProcessReplyFlags();

... but the if(msgCompose) is falsy. So ProcessReplyFlags will never get called.

mWeakComposeObj is set though. Don't know what to make if this... so suggestions welcome.

Comment 8

[David :Bienvenu]

sounds like the compose object has gone away already - perhaps because there's no compose window or js code holding a reference to it?

Comment 9

[Magnus Melin [:mkmelin]]

Ok, thx. 

So, either I could 
a) do like in the original patch
b) in the original patch, call pMsgCompose->ProcessReplyFlags(); instead of setting disposition
c) figure out how to keep the compose object around

I think I'd like a) the best, and have no idea if c) is even desirable

Comment 10

[Magnus Melin [:mkmelin]]

(In reply to comment #6)
> In the reply case, we seem to be sending a new message rather than a reply,
> which seems a bit strange.

Well, we are sending a new message - though conceptually it's a reply.

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Not *that* much changed from the previous version...

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 367855 [details] [diff] [review]
proposed fix, v2

Sorry for missing the comments on the bug. 

>+  return folder->AddMessageDispositionState(aMsgHdr, nsIMsgFolder::nsMsgDispositionState_Replied);

>+  return folder->AddMessageDispositionState(aMsgHdr, nsIMsgFolder::nsMsgDispositionState_Forwarded);

I think we'll go with this, but we should put a comment in each case explaining why we are doing it "manually" at that time and the compose code isn't.

r/sr=me with comments added.

Comment 13

[Magnus Melin [:mkmelin]]

changeset:   2245:c662c338ea41
http://hg.mozilla.org/comm-central/rev/c662c338ea41

... and of course i noticed my typos after committing:(
changeset:   2246:0485638c089a
http://hg.mozilla.org/comm-central/rev/0485638c089a

->FIXED