Closed Bug 366968 Opened 14 years ago Closed 12 years ago

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

Categories

(MailNews Core :: Filters, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: whimboo, Assigned: mkmelin)

References

(Depends on 1 open bug)

Details

(Keywords: dataloss)

Attachments

(1 file, 1 obsolete file)

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.
OS: Windows XP → All
Hardware: PC → All
Product: Core → MailNews Core
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?
Severity: normal → major
Flags: blocking1.9.1?
Keywords: dataloss
Summary: Forward filter does not update status for the forwarded message → Forward/Replay filter does not update message status for the forwarded/replied message
Not blocking 1.9.1.

Renom if you disagree.
Flags: blocking1.9.1? → blocking1.9.1-
Summary: Forward/Replay filter does not update message status for the forwarded/replied message → Forward/Reply filter does not update message status for the forwarded/replied message
Taking.
Assignee: nobody → mkmelin+mozilla
Attached patch proposed fix (obsolete) — Splinter Review
Attachment #366194 - Flags: superreview?(bugzilla)
Attachment #366194 - Flags: review?(bugzilla)
(The "a the" comment type i already fixed locally.)
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird 3.0b3
Attachment #366194 - Flags: superreview?(bugzilla)
Attachment #366194 - Flags: superreview-
Attachment #366194 - Flags: review?(bugzilla)
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.
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.
sounds like the compose object has gone away already - perhaps because there's no compose window or js code holding a reference to it?
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
(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.
Attached patch proposed fix, v2Splinter Review
Not *that* much changed from the previous version...
Attachment #366194 - Attachment is obsolete: true
Attachment #367855 - Flags: superreview?(bugzilla)
Attachment #367855 - Flags: review?(bugzilla)
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.
Attachment #367855 - Flags: superreview?(bugzilla)
Attachment #367855 - Flags: superreview+
Attachment #367855 - Flags: review?(bugzilla)
Attachment #367855 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.