RememberQueuedDisposition shouldn't hand-roll URIs to retrieve message headers

RESOLVED FIXED in Thunderbird 64.0

Status

enhancement
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: neil, Assigned: neil)

Tracking

unspecified
Thunderbird 64.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

RememberQueuedDisposition wants to set properties on message headers so that when you edit and send the draft message it can set the original message as replied (or whatever).
To do this, it performs some wacky string manipulation and passes the result off to the message service, which has to undo the string manipulation to retrieve the original folder URI and message key, and then actually retrieve the header.
It would make sense not to to the string manipulation because a) it hard-codes the supported URI schemes b) we end up undoing it anyway.
Again, don't know who should review this, sorry.
Attachment #9010693 - Flags: review?(jorgk)
Comment on attachment 9010693 [details] [diff] [review]
Proposed patch

Mailnews patches can be reviewed by myself, Aceman or Magnus since Kent and Joshua are inactive.

I would review this, and it looks good of course, but we're trying to eliminate use of RDF. So let's see what Aceman has to say. Maybe we take the RDF stuff now and then replace it later.
Attachment #9010693 - Flags: review?(jorgk) → review?(acelists)
Comment on attachment 9010693 [details] [diff] [review]
Proposed patch

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

While I do not understand the change, the added RDF seems OK and necessary as it is the current way to fetch the folder.
The pattern will be converted when we do bug 453908, see current patch at https://bug453908.bmoattachments.org/attachment.cgi?id=8854310 .
Well, it just replaces the horrible string processing. It now gets to the header via the folder and the key instead of constructing a *-message URL and getting to the header that way.

I haven't tested it but if more RDF use is allowable, this should be fine.
To test it, I'd just get some "good" attributes from the header to make sure the new code returns the new header. Should I take the review again?
Comment on attachment 9010693 [details] [diff] [review]
Proposed patch

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

OK, if you can see the logic, please take the review.
I think the RDF is fine as it does not add a new pattern, only one that is already in the tree 100 times and we will replace all at once.
Attachment #9010693 - Flags: review?(acelists) → review?(jorgk)
Comment on attachment 9010693 [details] [diff] [review]
Proposed patch

This is fine. Tested by inserting
    nsString subj;
    msgHdr->GetMime2DecodedSubject(subj);
    printf("=== subj %s\n", NS_ConvertUTF16toUTF8(subj).get());
into the function with and without the patch. Seems to retrieve the correct header.
Attachment #9010693 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/97064b1acf53
nsMsgCompose::RememberQueuedDisposition() shouldn't hand-roll URIs to retrieve message header. r=jorgk
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
Actually to test it all you needed to do is to reply to a test message, save the draft, then edit the draft and finish the test reply, and observe that the original test message is still marked as having been replied to.
You need to log in before you can comment on or make changes to this bug.