254931 - Shift-click on Forward: toggles plain/HTML for As Attachment, but not for As Inline

Status: RESOLVED DUPLICATE of bug 228562

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Mike Cowperthwaite]

Using Shift+click on the toolbar buttons for Compose and Reply will open the 
message in the composition mode opposite from what is specified for the account: 
HTML for a normally-plain account, and vice versa.

Using Shift+Click on the Forward button also performs this action, IF the 
preference for forwarding is As Attachment.  If the preference is Inline, the 
shift makes no difference.

This is true in Thunderbird (0.7) as well as Seamonkey (current 1.8a builds, but 
I've seen this forever).

Fixing this bug would imply a WontFix for bug 111298.

Comment 1

[Mike Cowperthwaite]

Neil posted this in a related discussion in a newsgroup:
===
both Edit As New and Forward end up as a call to 
  nsMsgDraft::ProcessDraftOrTemplateOperation. 

once the mime code has extracted the body of the message, if the message was a 
draft or a template it is simply sent to the editor window of the same type. 
However things aren't quite as simple for a forwarded message. First, by this 
stage, the shift key state has been lost, so the code has to rely on your 
default compose type. Now if it's HTML but the original message is plain text it 
is wrapped in a <PRE>. Then the headers are added to the body. Finally if the 
original message was HTML but you default to plain text it gets converted.
===

Somewhat OT, it occurs to me that an alternate behavior might be desirable:

The default plain/HTML mode for a Forward Inline could follow the mode of the 
original message -- if an HTML is being forwarded inline, the new message is 
HTML, regardless of the mode specified for the account; and then Shift+Forward 
could switch the mode (forwarding an HTML as plain, or vice versa).

But this might require more far-reaching changes, since the menu variations of 
Forward Inline would need to reflect that basic difference as well.

Comment 2

[WADA:World Anti-bad-Duping Agency]

Bug 228562 is Thunderbird bug for this issue.

Comment 3

[Matthew Hodgson]

I've added a mildly hacky patch which fixes this to the Thunderbird bug #228562
(sorry, I now realise it should have been put here, as it only touches on
MailNews).  It works by propagates the shift-clicking through to the MIME code
which creates the composer window when doing an inline-forward.

Comment 4

[Mike Cowperthwaite]

(In reply to comment #3)
> I've added a mildly hacky patch which fixes this to the Thunderbird bug ...
> (sorry, I now realise it should have been put here...)

If your patch is suite-only, I suggest you mark the one at the other bug 
obsolete and reattach it here.  The easier it is for the reviewers, the more 
likely it is there will be some action on it.

Matthew, what do you think of the last part of my comment 1?  I don't know if 
that's even in conflict with the original part of this bug, it might be 
orthogonal.

Comment 5

[Matthew Hodgson]

Attachment: Propagates shift-click of Forward Inline button to mimedrft.cpp

As per the above comment, I'm reattaching my patch (originally attached to bug
228562) here.  This is a mildly hacky patch which propagates any shift-clicking
of the Forward button through to the MimeStreamConverter which MsgCreate uses
to
create a fake template-mail for inline forwarding.  It builds on the existing
hack to use templating for inline forwarding, rather than fixing it.  It could
probably do with the two separate calls to identity->GetComposeHtml() in
mailnews/mime/src/mimedrft.cpp being factored out - but as a proof of concept
it works in its current form.

Mike: I'm not convinced about the idea of the composer mode being determined by
default from the format of the original message being forwarded.  Most of the
HTML messages I forward have originated from Outlook, and I definitely want to
remove the HTML gibberish to conserve bandwidth, disk space, and sanity :)  In
the rare occasion that I want to forward a graphically intensive 'flyer' to
someone, then I'd be happy to make an exception and press shift whilst
forwarding (which is what this patch achieves).  Should you wish to have the
other behaviour, however, it would be very easy to expose as an option without
any major changes, as far as I understand the code.   For symmetry, I assume
the same behaviour would always apply for both Replying and Forwarding.

Comment 6

[[e-mail]]

Comment on attachment 192707 [details] [diff] [review]
Propagates shift-click of Forward Inline button to mimedrft.cpp

>+            rv = pMsgDraft->OpenDraftMsg(uriToOpen.get(), nsnull, identity, PR_FALSE, aMsgWindow, nsnull);
>           break;
nsnull? That's probably a misleading synonym for zero - perhaps you should use
nsIMsgCompFormat::Default here?

>-          if (bFormat)
>+          if (bFormat ||
>+              mdd->forwardFormat == nsIMsgCompFormat::OppositeOfDefault ||
>+              mdd->forwardFormat == nsIMsgCompFormat::HTML)
Is that accurate? It seems to me that if your format is opposite of default
then you want to flip bFormat. Maybe it would be possible to move the plain to
HTML converion into the CreateTheComposeWindow code?

Comment 7

[Matthew Hodgson]

Attachment: Propagates shift-click for Forward Inline - refactored & fixes nsnull bug

Yup, passing nsnull as a default arg to pMsgDraft->OpenDraftMsg() was indeed a
thinko - it's fixed to be nsIMsgCompFormat::Default in this updated patch. 

I've gone and refactored the code which determines the output message format
(outComposeFormat) code as described earlier, which also eliminates the
confusing conditional mentioned above and hopefully tidies things up a bit at
the expense of renaming the ambiguous composeFormat variable to inComposeFormat
(as it describes the source message's composition format, rather than the
forwarded/template message's format, which i've called outComposeFormat).

It's not convenient to group the ConvertBodyToPlainText and convert plaintext
body to HTML code together, however, as they need to operate at fairly
different points in packaging up the original message for
forwarding/templating.	But at least they are now symmetric in how they're
invoked.

Comment 8

[Mike Cowperthwaite]

(In reply to comment #5)
> For symmetry, I assume
> the same behaviour would always apply for both Replying and Forwarding.

No, I'd *only* want it for Forward Inline, if at all -- the idea being that a 
reply is a message I'm composing, while forward-inline is a message somebody 
else composed (usually), and that I'd like to send it on as it arrived.  But 
that's another bug.

Comment 9

[Matthew Hodgson]

I should mention that the above patch seems to introduce crazy IDL compiling
dependencies between mailnews/mime, mailnews/compose and mailnews/imap which
breaks the build when making from clean - I'll post an updated version once I've
figured out how to resolve it...

Comment 10

[Scott MacGregor]

Comment on attachment 192728 [details] [diff] [review]
Propagates shift-click for Forward Inline - refactored & fixes nsnull bug

per the last comment

Comment 11

[Magnus Melin [:mkmelin]]

Matthew: any progress on the patch?

Comment 13

[Erik Harris]

The "Product" for this should be modified.  The Mozilla Application Suite doesn't exist anymore, but this problem still affects Thunderbird (and presumably SeaMonkey).

Comment 14

[[e-mail]]

Bugzilla has plans to rename Mozilla Application Suite to SeaMonkey but not before Firefox 3 is released.

Comment 15

[Erik Harris]

Ignore last comment, I guess..  I just noticed that a separate bug # exists for Thunderbird, even though it's the same bug.

Comment 16

[Magnus Melin [:mkmelin]]

I don't see any difference between this and bug 228562 (which is now fixed).