1265920 - Move unrelated paragraph-handling code from cloudAttachmentLinkManager.js to a more intuitive place

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[rsx11m]

+++ This bug was initially created as a clone of Bug #1265246 +++

(Quoting to Jorg K (GMT+2) from bug 1265246 comment #17)
> (In reply to rsx11m from bug 1265246 comment #15)
> > Indeed, that's what I did before, but got distracted by the name
> > gCloudAttachmentLinkManager and thus didn't bother trying to further
> > understand what it is doing assuming it's only related to the filelink stuff.
> > Maybe rename cloudAttachmentLinkManager.js to something that's more general,
> > given that it does things beyond cloud attachments?
> Yes, this is a little unfortunate. It was even more unfortunate before bug 736584
> ripped out a lot of processing there on inline forwarded messages.

(Quoting Magnus Melin from bug 1265246 comment #19)
> The paragraph handling should probably move to be handled in
> MsgComposeCommands.js stateListener instead.

(Quoting rsx11m from bug 1265246 comment #20)
> That's approaching 5000 lines as well but appears to be intuitively the
> better spot.
> 
> With bug 736584 having removed the <div> stuff from
> NotifyComposeBodyReadyForwardInline, it appears that the entire set of
> NotifyComposeBodyReadyForward* functions can just be moved to the
> stateListener.

(Quoting Jorg K (GMT+2) from bug 1265246 comment #21)
> Personally, I'd put it into a new source file.

Yeah, I'd agree that this would be preferable vs. pushing MsgComposeCommands.js over the 5000-line mark. These are about 150 lines of code to be moved, where possibly some more code doing manipulation of the message body when initializing the composition window can be accommodated there.

Comment 1

[rsx11m]

Attachment: Proposed patch

This patch moves the paragraph-handling code from cloudAttachmentLinkManager.js to a new file I've named composeBodyManager.js, keeping it generic so that it can catch various cases to manipulate the message content before the composition window is presented to the user. The listener is still needed for cloud-attachment events.

I didn't see any new failures running the cloudfile and composition tests with this patch on Linux.

Comment 2

[rsx11m]

Attachment: Informational

Attachment 8743048 [details] [diff] was generated using "hg cp" to preserve line history in composeBodyManager.js. This is a more eye-friendly version presenting it simply as a new file.

Comment 3

[Magnus Melin [:mkmelin]]

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

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

Thx for looking in to this! 
I'm not convinced it should be a separate file though, especially as there is also the same statelistener in MsgComposeCommands.js. Yes what this does is you could call manage the compose body, but it's a fine line as what that would be. Why not merge the code with the one of the existing statelistener? Otherwise someone's bound to duplicate a lot of this code when they need, say a NotifyComposeBodyReadyForwardInline.

Comment 4

[rsx11m]

Unless Jorg had some other motivation in mind, the rationale was to extract the functions for this kind of post-processing once the message body has been established into that separate file for clarity. If someone wants to add another function to NotifyComposeBodyReadyForwardInline, he or she is hopefully invited do so in that file as its name is generic enough. On the other hand, I see your point that someone could just make another copy of that file for a specific purpose and thus we end up with half a dozen listeners.

I'm open to either solution, just using MsgComposeCommands.js obviously avoids some complexity (and another listener) but adds to the complexity of that already rather crowded file (where stateListener itself is still on the easy side).

Comment 5

[rsx11m]

Maybe just adding a comment at the top of composeBodyManager.js clearly stating what its purpose is?

// This file contains all code to manipulate the message body after the editor
// has been initialized but before the window is presented to the user.
// Different modes are handled in each NotifyComposeBodyReady*() function.

Comment 6

[Magnus Melin [:mkmelin]]

I think there's still not so much justification putting it in separate file. Having it in one place just seems easier.

Comment 7

[rsx11m]

Attachment: Just move to stateListener

Ok, let's keep it simple then. I've also corrected a typo in the comment above the switch().

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 8743524 [details] [diff] [review]
Just move to stateListener

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

Looks good otherwise, so r=mkmelin with the below fixed

::: mail/components/compose/content/MsgComposeCommands.js
@@ +312,3 @@
>      if (gMsgCompose.composeHTML)
>        loadHTMLMsgPrefs();
>      AdjustFocus();

^^  this never get run now as you return from all the swith cases. you should probably move it first in the function

Comment 9

[Jorg K (CEST = GMT+2)]

Or check which one of the listeners runs first and then adjust it accordingly so the current order is maintained.

Comment 10

[rsx11m]

Attachment: Proposed patch (v3)

Good catch, thanks. I figured that the AdjustFocus() was intended to run at the very end when the message is set up completely, thus placed everything else before it. The simple fix is to replace the returns with breaks, and I've verified that the end of this function is now reached.

As another drive-by fix, I've moved Components.interfaces.nsIMsgCompType.ForwardAsAttachment: into the .New: case as it's essentially a new message with just an attachment (i.e., forwarding as attachment did show up as empty "Body Text" regardless of the setting). It's a different story with .MailToUrl: where it depends whether or not a "?...&body=" parameter is given. If so, an empty <p> is generated with the intended body text placed underneath, so that isn't going to work if, e.g., a formatted reply is expected. Thus, I've left that one for a follow-up bug.

Comment 11

[rsx11m]

Thanks for the review, pushed as http://hg.mozilla.org/comm-central/rev/0e4ea4d22bc1

Do we want this on 45.x to fix the ForwardAsAttachment case?

Comment 12

[Joe Sabash [:JoeS1]]

"(i.e., forwarding as attachment did show up as empty "Body Text" regardless of the setting)"

For the sake of consistency, probably should file a follow up bug for this, lets see how the acceptance of paragraph mode by default goes, before we file that

Comment 13

[rsx11m]

Joe, I've fixed ForwardAsAttachment with the patch here but not MailToUrl, which is a more complicated case as described in comment #10. I'll file a follow-up bug on that in a moment.

Or, do you mean filing a follow-up bug to fix ForwardAsAttachment on comm-esr45, i.e., same patch without moving the paragraph-handling code as such into another file?

> lets see how the acceptance of paragraph mode by default goes, before we file that

Even if Thunderbird decides to flip the pref's default back to "false" that code would still be relevant if the default is changed. Thus, it's worth fixing either way.

Comment 14

[rsx11m]

(In reply to rsx11m from comment #13)
> Or, do you mean filing a follow-up bug to fix ForwardAsAttachment on comm-esr45, i.e., same patch
> without moving the paragraph-handling code as such into another file?

Hmm, the patch in attachment 8744030 [details] [diff] [review] doesn't apply to comm-esr45 anyway due to bug 777732.

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to rsx11m from comment #11)
> Do we want this on 45.x to fix the ForwardAsAttachment case?
Personally, I'd fix it by adding the missing case as a one-liner, if at all.

Comment 16

[rsx11m]

Attachment: Branch patch fixing ForwardAsAttachment

Yeah, forward as attachment isn't the default for Thunderbird (in contrast to SeaMonkey, which was the reason that I've noticed it start with), thus the impact should be limited.

It's a simple 1½-liner, thus no harm done to fix this if Magnus agrees.
I can move this patch into a separate bug if desired.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8744594 [details] [diff] [review]
Branch patch fixing ForwardAsAttachment

Thanks for the patch. I will land this in bug 1267025.

Comment 18

[Jorg K (CEST = GMT+2)]

Landed on Aurora branch (TB 47):
https://hg.mozilla.org/releases/comm-aurora/rev/f2494750025a

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 8744594 [details] [diff] [review]
Branch patch fixing ForwardAsAttachment

Moved to attachment 8744669 [details] [diff] [review]