1389062 - Implement "Edit Template" command and UI

Status: RESOLVED FIXED

(MailNews Core :: Composition, enhancement)

Description

[Thomas D. (:thomas8)]

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

Believe it or not, but there's currently no way to change a message template in Templates folder short of creating a new message from that template, save it as a new template, and then delete the old template. Which obviously violates ux-efficiency.

So this RFE seeks to implement "Edit Template" command and UI.
More specifically:
* XUL <command id="cmd_EditTemplate">
* XUL UI <menuitems> with strings (main menu, context menu, app menu)
* supportive JS code (I hope we won't need anything else in the compose pipeline)

Minimum UX requirements:
- Pre-select "Template" from composition's [Save|v] button flavors, otherwise the changed template will end up as a draft in drafts folder

UX desiderata:
- For "Edit Template", compose window title should be "Edit Template: My Template Subject"
- Yellow information bar in composition: You are editing a message template. [Save]
- to be defined: behaviour when user choses other flavors from [Save |v] button.

This is similar in appearance and code to bug 731688, Comment 28, where I just uncovered that we're lacking an explicit "New Message from Template" command.
It's also structurally similar to bug 1106412 where I implemented "Edit draft" command.

Comment 1

[Thomas D. (:thomas8)]

New 'Edit Template' command: UX details

1) Default composition mode: observe *message* format

Imo we can't default to editing an explicit HTML template in plaintext composition mode, even if the account pref is plaintext. That would not 'edit' the template, but 'downgrade/destroy, then edit'. But there's reason to assume that if a plaintext user has an HTML template in his Templates folder, it's an intentional deviation from his preferred format, and we also observe the message format when composing from that template.
Similarly, if an HTML user keeps a plaintext template (which isn't easy to create), that's intentional and it's unlikely that he intends to change the format of the template. In the unlikely event that user *does* want to change the template format: See 2) below.

2) Allow Shift modifier to edit template in *opposite* of message format
If 1) doesn't give you what you want, here you'll get it!
This allows to change the template format, i.e.
- edit and save an HTML template as plaintext, or
- edit and save a plaintext template as HTML.

Comment 2

[Thomas D. (:thomas8)]

We should understand the logic of bug 288702 before doing this.

Comment 3

[Thomas D. (:thomas8)]

Turns out bug 288702 was trying to do something which we'll also want to do as part of the behaviour here; but unfortunately, bug 288702 got it all wrong because it didn't observe the subtle difference between "Edit as New Message" and "Edit draft/template"... So while bug 288702 has been thoroughly undone over time, we might still get some ideas from there, wrt to this aspect:

> Minimum UX requirements:
> - Pre-select "Template" from composition's [Save|v] button flavors,
> otherwise the changed template will end up as a draft in drafts folder

In fact, we'll want exactly that behaviour which bug 288702 claimed to implement (but for the wrong command):
> saving an edited template should replace existing template, and keep same message-id
So we might even borrow from that code.

Comment 4

[Thomas D. (:thomas8)]

I'm preparing a patch for most of the XUL/JS frontend of this, hoping that Jörg will do the backend in bug 731688.
I might not get round to the save-button-flavor part, and in as far as that involves message ids, I'm sure others are in a better position to look into that than me.

Comment 5

[Thomas D. (:thomas8)]

Attachment: Part1 - Frontend - WIP Patch V.1: Implement cmd_editTemplateMsg

Hi Richard, pls have a look at this.
It's wip and it doesn't work yet, but we can agree on strings and menus.

a) I chose menu position of "Edit Template" very carefully and can give reasons on request.

b) "Open in new _Tab" and "Edit _Template" are sharing the same access key in context menu. There's no other free access key, so I thought a choice of related actions might be best to share an accesskey; arguably we're assisting user to decide what he really wants: open to look at or open to edit. I'd guess that both actions aren't very frequent. Imo no access key isn't good for actions which aren't available elsewhere via keyboard.

c) I deliberately used "Edit Template" (not "Edit Template Message") because this command is different from all others because we're NOT creating/editing a message for sending, but a template for future messages. So I'm hoping to improve ux-error-prevention by making this command caption look different from others. 

To-do:
1) 'save-as-template' flavor must be preselected in Save-button dropdown
2) 'save-as-Template' must overwrite the existing template, not create a new template (probably something about message-id and/or mime internals).
3) Ensure backend to do the right thing wrt 2) and Shift modifier.
Any help on those would be appreciated, it's unlikely that I'll do that anytime soon or at all.

Comment 6

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #5)

> To-do:
> *** 1) 'save-as-template' flavor must be preselected in Save-button dropdown

We touched that code in bug 507103 (attachment 740225 [details] [diff] [review]), and there's some extensive related analysis from WADA in Bug 870313.

> *** 2) 'save-as-Template' must overwrite the existing template, not create a new
> template (probably something about message-id and/or mime internals).

Bug 288702 - saving an edited template should replace existing template, and keep same message-id
-> Attachment 179331 [details] [diff] - we might be able to borrow code from there. Note that bug 288702 has been mostly undone because somehow it did the wrong thing, probably by conflating "Edit as New Message" and "Edit Template/Draft".

> if (mDeliverMode == nsIMsgSend::nsMsgSaveAsDraft || mDeliverMode == nsIMsgSend::nsMsgSaveAsTemplate)

> fields->SetMessageId(id); // keep same message id for editing template.

> *** 3) Ensure backend to do the right thing wrt 2) and Shift modifier.

That's Jörg's domain, as in bug 731688, attachment 8899166 [details] [diff] [review].

> Any help on those would be appreciated, it's unlikely that I'll do that
> anytime soon or at all.

Comment 7

[Richard Marti (:Paenglab)]

Comment on attachment 8899248 [details] [diff] [review]
Part1 - Frontend - WIP Patch V.1: Implement cmd_editTemplateMsg

Your WIP looks good. What about using a accesskey from "Ignore Thread" or "Move To"?  Your actual choice makes it, that you have to press the key two times. Although when the "Open in Tab" is unlikely the one you want, it's the first which is shown. So the "Ignore Thread" is very unlikely someone wants in the template directory this command. Unfortunately the "I" and especially in lower case is the worst we can use. That's why I propose the "Move To". It could happen more, someone wants to move the message but the edit would be more used (I hope so ;) ) and the "Edit Template" would be then the first highlighted entry. That's why I think, "m" could be used.

Comment 8

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #7)
> Comment on attachment 8899248 [details] [diff] [review]
> Part1 - Frontend - WIP Patch V.1: Implement cmd_editTemplateMsg
> 
> Your WIP looks good. What about using a accesskey from "Ignore Thread" or
> "Move To"?  Your actual choice makes it, that you have to press the key two
> times. Although when the "Open in Tab" is unlikely the one you want, it's
> the first which is shown. So the "Ignore Thread" is very unlikely someone
> wants in the template directory this command. Unfortunately the "I" and
> especially in lower case is the worst we can use. That's why I propose the
> "Move To". It could happen more, someone wants to move the message but the
> edit would be more used (I hope so ;) ) and the "Edit Template" would be
> then the first highlighted entry. That's why I think, "m" could be used.

I understand where you are coming from. It's really a catch-22 (Zwickmühle :)
Mozilla accesskey policies" (1) has a good overview of factors to consider.

"m"
+ Yes, "m" has the advantage of being first in the row of two duplicates.
- But it's hard to remember, almost silent in the word.
- Another disadvantage: It's duplicate also in main menu (see below).

"T"
+ "T" is much more easy to remember because it's the first letter of a word (as recommended in policies).
+ Also, "T" is actually unique accesskey in the main menu (working "correctly"), while "m" is still duplicate.
- As you said, disadvantage of two key presses in context menu.

Another one which combines your idea with mine might be
"p":
+ first duplicate in context menu
+ print should never be used from context menu (universal Ctrl+P shortcut)
+ unique in main menu
- However, "p" is also hard to remember (but less silent than "m"), and it's
- not recommended because descender (the "leg" of p) with underline looks odd.

I don't know, but looking at all the factors, I think I still like "T" best because of it's high mnemonic value, easy to see, and unique in main menu. Uniqueness in at least one menu seems an important advantage for keyboard-centered/dependent users.

What do you think?

(1) https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/XUL_Accesskey_FAQ_and_Policies

Comment 9

[Thomas D. (:thomas8)]

I'd also think that duplicate "T" only in context menu is an acceptable disadvantage compared to the advantages, assuming that both "Open in new Tab" and "Edit Template" are somewhat rare actions. Even if "Edit Template" might be more frequent depending on use case, I'd still think that having at least one unique accesskey on the main menu helps, and T,T,Enter is still easier to remember than unrelated "m" or "p" with zero mnemonic value...

Comment 10

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #8)
> ...
> What do you think?
> 
> (1)
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/
> XUL_Accesskey_FAQ_and_Policies

Accesskey is a trivial part of this compared to the other missing parts, but let's agree on something...

Comment 11

[Richard Marti (:Paenglab)]

There is no need to use the same accesskey in the context menu as in the other ones, see "Reply to All" or "Forward Ass..." as examples. In the normal menus you can still use "T" and in the context menu "m".

Comment 12

[Jorg K (CEST = GMT+2)]

I think we need to introduce a new compose type "edit template", like we have edit draft, to distinguish from "new message from template". That would look similar to the "edit as new" patch:
https://hg.mozilla.org/comm-central/rev/ace64c7317e8

That would then also fix bug 1421736 properly.

Or hold on and go back one step:

For drafts we have "edit draft" and "edit as new". The former pulls up a draft, edits it, when saving or sending the original is deleted. "Edit as new" branches off a new draft from an existing one and leaves the original alone.

What's the story for templates?
"Edit template" should overwrite the existing template. What should happen when sending? Or is there no send button? "New message from template" is equivalent to "edit as new" on a draft, no? It uses nsIMsgCompType::Template though. So what is "edit as new message" good for when used on a template?

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: Part1 - Frontend - WIP Patch V.1: Implement cmd_editTemplateMsg (rebased)

Rebased.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1389062_editTemplateCmd-CPP.patch (v1) - WIP

WIP. Untested but compiles.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1389062_editTemplateCmd-CPP.patch (v2)

This works and fixes bug 1421736 and bug 1417792 as well.

Thomas, are you going to ask for review for your part?

Also, should we remove "Edit as New Message" for templates? I'm confused as to what that's meant to do.

Comment 18

[Jorg K (CEST = GMT+2)]

Thomas, here's a try build for you:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1284a1f9d38574b93eab75bebc8337fceec49dde
You can download the built by clicking onto the B and and then onto the link next to Build (pre-guessing):
https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-1284a1f9d38574b93eab75bebc8337fceec49dde/try-comm-central-win32/

Comment 20

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+1) from comment #16)
> Also, should we remove "Edit as New Message" for templates? I'm confused as
> to what that's meant to do.
Maybe it's to branch of a new template from an old one. But then you could use "new message from template" and "save as template", no?

Comment 21

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #16)
> Created attachment 8933460 [details] [diff] [review]
> 1389062_editTemplateCmd-CPP.patch (v2)
> 
> This works and fixes bug 1421736 and bug 1417792 as well.
> 
> Thomas, are you going to ask for review for your part?

Sorry I haven't checked yet, does your patch implement the "Template flavor on save button" part?
Should I ask review before we have the full behaviour ready?
As hinted before, I can't do this atm.

> Also, should we remove "Edit as New Message" for templates? I'm confused as
> to what that's meant to do.

"Edit as New Message" and "New Message from Template" are slightly different actions which may do different things.
Jörg and I have sorted out the behaviour to be this:

"Edit as New Message" -> take the current message (from whereever, and whichever type of message it is), and start a new draft essentially using this message as a template, *except the message format*. We'll just use whichever format is the account's default format, for consistency with all other composing commands (forward, reply etc.). Shift will use the opposite format.

"New Message from Template" -> take the current message (an explicit template from a templates folder), and start a new draft exactly using this message as a template, *including* the message format. We respect the message format even if it's not the default format of your account (There's no reason to keep a plaintext template and expecting that to generate a HTML message). Shift will use the opposite format.

So I'm inclined to keeping both commands on templates for general consistency.
"Edit as New Message" is one of the swiss-knife basic actions which we probably want to allow on *any* message.
I think the potential for confusion is small because "New message from template" is the default action, clearly marked and first position in context menu, and triggered by double-click. So I don't think someone could miss "New message from template". If that's what you need, why look further.

I believe this was already ui-reviewed, considered, probably also discussed when we introduced "New message from template", so it's also a bit out of scope for this bug.

Comment 22

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. from comment #21)
> Sorry I haven't checked yet, does your patch implement the "Template flavor
> on save button" part?
Oh, I see, this from comment #0:
  Pre-select "Template" from composition's [Save|v] button flavors, otherwise the
  changed template will end up as a draft in drafts folder.
Let me see what I can do. When will you be back in action?

As for "New message from template" and "edit as new message" on a template: There won't be a whole lot of behaviour difference, however, of course, "edit as new" uses the account format potentially converting the original message, whereas "new message from template" uses the template format. I'm OK with leaving both.

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8933374 [details] [diff] [review]
Part1 - Frontend - WIP Patch V.1: Implement cmd_editTemplateMsg (rebased)

Looks like this is ready to go.

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: 1389062_editTemplateCmd-CPP.patch (v3)

OK, this is now setting the default save operation to "template" causing "Edit Template" to save as template by default.

So that should implement everything we need.

Comment 25

[Thomas D. (:thomas8)]

I won't download the outdated trybuild from comment 18 because I understand Jörg's new patch has different behaviour (plus limited bandwidth, and no electricity again...). If you want me to test this in action, pls provide a new trybuild.

Comment 26

[Thomas D. (:thomas8)]

Comment on attachment 8933747 [details] [diff] [review]
1389062_editTemplateCmd-CPP.patch (v3)

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

Wow, thanks a lot, this is quite deep into the internals, and I wouldn't want to search for these spots in code or debug that.
I can't claim to know much about this code, so I was just trying to understand what you're trying to do (which also means that I haven't checked for omissions, and probably couldn't).

So from that limited perspective, this looks good and functional in general (from reading the code only, no testing).
I think there's a flaw when it comes to handling and commenting message ID setting for "Edit Template", and I saw a // debug flag and printf's there, so it seems as if you were planning to look into that again.

::: mailnews/compose/public/nsIMsgComposeParams.idl
@@ +23,5 @@
>      const long ReplyToSender            = 6;
>      const long ReplyToGroup             = 7;
>      const long ReplyToSenderAndGroup    = 8;
>      const long Draft                    = 9;
> +    const long Template                 = 10;  // New message from template.

Thank you for this clarifying comment, that's very helpful.

I think we should file a new bug to rename this constant to "NewMsgFromTemplate", that's exactly where all the mess started which we're now cleaning up, namely that there was now distinction between "Edit as New message" and "New message from Template", and there was no "Edit Template" at all.
I think renaming is safe because we've changed the meaning anyway when we split out "Edit as New message" from this comp type and changed the behaviour of that.

@@ +28,5 @@
>      const long MailToUrl                = 11;
>      const long ReplyWithTemplate        = 12;
>      const long ReplyToList              = 13;
>  
>      /** 

Maybe remove the existing trailing space while we're here?

::: mailnews/mime/src/mimedrft.cpp
@@ +1402,5 @@
>  
>      draftInfo = MimeHeaders_get(mdd->headers, HEADER_X_MOZILLA_DRAFT_INFO, false, false);
>  
> +    // Keep the same message id when editing a draft or creating a message from
> +    // a template but not when we're editing a message "as new message" or

This comment sounds wrong to me, and I'm also not sure if the code does the right thing. Maybe we overlooked this already when implementing "New message from Template"?

When "creating a (new) message from template" (the new command which we implemented recently), we do NOT want to keep the same message id, right?
But when "editing a draft" OR "editing a template" (this bug), that is when we want to keep same message ID.

So maybe this comment should read:
Keep the same message id when editing a draft or editing a template,
but NOT when we're edting a message "as new message" or forwarding inline.

@@ +1404,5 @@
>  
> +    // Keep the same message id when editing a draft or creating a message from
> +    // a template but not when we're editing a message "as new message" or
> +    // forwarding inline.
> +    if (!(mdd->format_out == nsMimeOutput::nsMimeMessageEditorTemplate &&

What is the meaning of "nsMimeOutput::nsMimeMessageEditorTemplate"?
Iirc, it's not what it sounds to be, but something else used solely for internal purposes. If so, we might want to add a comment here. Jörg, it looks like you double-checked this corner (see printf), can you share in code comments what you found?

@@ +1410,2 @@
>          fields && !forward_inline) {
> +      printf("=== setting id %s\n", id);

Please correct me if I'm wrong (I don't claim to know much about this): From my reading, this code translates to:

If (NOT (outputformat == nsMimeMessageEditorTemplate AND
         URL has &editasnew flag) AND
If we have fields AND
If NOT forwardInline)
THEN Keep original message ID.

So I'm failing to see how this covers our new case of "&edittempl=true" flag where we also want to keep the original message ID, isn't it?

Furthermore, how does this cover the recently introduced case of "New message from template" which I would also expect to be in the IF NOT conditions? "New message from template" must NOT keep the original message ID.

@@ +1412,1 @@
>        fields->SetMessageId(id);

This KEEPS the original message ID, isn't it? If so, maybe that should reflect in the printf, that's if we're keeping the printf:
printf("=== preserving original msg id %s\n", id);

@@ +1643,5 @@
>          }
>        }
>        else if (body && mdd->overrideComposeFormat &&
>                 (msgComposeType == nsIMsgCompType::Template ||
> +                msgComposeType == nsIMsgCompType::EditTemplate ||

nsIMsgCompType::Template is "New message from template" case, isn't it?
For that case, overriding format (potentially losing all HTML) isn't harmful because the original HTML template will not be touched, only the new message is mutilated, and if that's unwanted, user can just start another message without override.

But now, we're editing the template itself, so the "override" of losing all HTML will be permanent dataloss at the next autosave.

Allowing the override is fine (thanks), but imo, this definitely needs an explicit warning message to the user for the (EditTemplate && "downconvert HTML to plaintext") case. Without such a warning message, this would need to be vetoed by ui-review.

@@ +1675,5 @@
>        // the message store.
>        //
>        if (mdd->format_out == nsMimeOutput::nsMimeMessageEditorTemplate)
>        {
> +        // Here we have to competing interests:

"two" I guess

@@ +1679,5 @@
> +        // Here we have to competing interests:
> +        // When editing a draft "as new message", we do NOT want the original
> +        // to be overwritten, hence we don't want to set the draft ID.
> +        // When creating a message from a template, we do want the original
> +        // to be overwritten when saving the template again.

This sounds confusing and wrong, and I'm not sure if it's doing the right thing (see similar problem above where I discussed setting ID).

"Edit as new message" and "New message from template" are two different commands which both require a NEW message with a NEW message ID (so "When creating a message from a template, we do *N O T* want the original to be overwritten").

This sounds more correct to me:

        // When editing an existing message "as new message", or creating a "new message from template", we do NOT want the original
        // to be overwritten, hence we don't want to set the draft ID.
        // When editing a template message, we do want the original
        // to be overwritten when saving the template again.

@@ +1681,5 @@
> +        // to be overwritten, hence we don't want to set the draft ID.
> +        // When creating a message from a template, we do want the original
> +        // to be overwritten when saving the template again.
> +        if (msgComposeType == nsIMsgCompType::EditTemplate)
> +          fields->SetDraftId(mdd->url_name);

What does this do?

@@ +1682,5 @@
> +        // When creating a message from a template, we do want the original
> +        // to be overwritten when saving the template again.
> +        if (msgComposeType == nsIMsgCompType::EditTemplate)
> +          fields->SetDraftId(mdd->url_name);
> +        if (msgComposeType == nsIMsgCompType::EditTemplate) // debug

As the comment says, this needs debugging.
Is it not trying to add the missing case which I mentioned above?
If so, maybe it should be added at the above spot, or the above spot altogether should be moved here?
Doing message IDs in two different places should be avoided if possible.

@@ +1683,5 @@
> +        // to be overwritten when saving the template again.
> +        if (msgComposeType == nsIMsgCompType::EditTemplate)
> +          fields->SetDraftId(mdd->url_name);
> +        if (msgComposeType == nsIMsgCompType::EditTemplate) // debug
> +          printf("=== setting draft ID so previous template will be overwritten\n");

printf to be removed after debugging?

printf("=== preserving original message ID so original template message will be overwritten\n");

Comment 27

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #26)
> Comment on attachment 8933747 [details] [diff] [review]
> I think there's a flaw when it comes to handling and commenting message ID
> setting for "Edit Template", and I saw a // debug flag and printf's there,
> so it seems as if you were planning to look into that again.

For completeness of review summary comment:
Plus we need a warning message when overriding the format for the case of "Downconvert HTML to plaintext" && EditTemplate, because that's irreversible dataloss as it will overwrite the original HTML template at the next autosave without any way of recovering the original HTML template.

Comment 28

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #26)
> Comment on attachment 8933747 [details] [diff] [review]
> ::: mailnews/compose/public/nsIMsgComposeParams.idl
> @@ +23,5 @@
> >      const long ReplyToSender            = 6;
> >      const long ReplyToGroup             = 7;
> >      const long ReplyToSenderAndGroup    = 8;
> >      const long Draft                    = 9;
> > +    const long Template                 = 10;  // New message from template.
> 
> Thank you for this clarifying comment, that's very helpful.
> 
> I think we should file a new bug to rename this constant to
> "NewMsgFromTemplate", that's exactly where all the mess started which we're
> now cleaning up, namely that there was now distinction between "Edit as New

typo: now -> no

> message" and "New message from Template", and there was no "Edit Template"
> at all.

In another bug, maybe we could also rename "Draft" constant to "EditDraft", because there are all sorts of flavors which produce a draft but this constant is only about the plain vanilla "EditDraft" case if I understand this correctly.
Other constants also have names which resemble the command caption or command action.

Comment 29

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #11)
> There is no need to use the same accesskey in the context menu as in the
> other ones,

Note to self: Nitfix accesskeys before landing this.

> see "Reply to All" or "Forward Ass..." as examples.

"Forward *ASS*" !???  8-O
I wasn't aware that TB includes that functionality... interesting... :p ;)) :)))
But I'd be less surprised to see this in Firefox Private Mode... 8)

>In the normal menus you can still use "T" and in the context menu "m".

Not ideal, but let's try that!

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: 1389062_editTemplateCmd-CPP.patch (v3b)

This addresses Thomas comments. Note the following.

No further debugging is necessary here. The printf statements are left in for the reviewer so they can follow what the code does. They will be removed before check-in, equally:
if (xxx) // debug for review
  printf(...);

I modified the code that preserves the message ID to be more legible. I also added further explanations as requested.

Please do not confuse message ID with draft ID. I draft ID is an internal thing, a URL addressing this draft or template. If set, the previous version will be superseded upon save of the new version.

(In reply to Thomas D. from comment #26)
> Allowing the override is fine (thanks), but imo, this definitely needs an
> explicit warning message to the user for the (EditTemplate && "downconvert
> HTML to plaintext") case. Without such a warning message, this would need to
> be vetoed by ui-review.
I suggest to add this to the JS part then, so you don't have to veto your own patch ;-) It's too late deep down in the bowels of C++ processing.

New try build:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=11c3e0b3a24bc1ed2280f373d893b905896ee92a
I've done a debug build so you can see the prints on the console and follow the logic, OK?

Result should arrive here in a while:
https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-11c3e0b3a24bc1ed2280f373d893b905896ee92a/try-comm-central-win32/

P.S.: I do bulk removal of trailing spaces in bug 1399756, sadly I missed the IDL files. I'll keep it in mind.

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: 1389062_editTemplateCmd-CPP.patch (v3c)

Damn, left in an old block of comment I rewrote. Try build is still valid.
Use interdiff with v3 to see the changes.

Comment 32

[Jorg K (CEST = GMT+2)]

Regarding the hunk with the draft ID, please note:
https://hg.mozilla.org/comm-central/rev/1a06d4cf1ad8#l3.12
Setting of the draft ID in that spot was removed to fix bug 321355, but it needs to be set when editing a template. Hence the "two competing interests". However, I rephrased that comment.

Comment 33

[Jorg K (CEST = GMT+2)]

Actually, it's a debug build hence:
https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-11c3e0b3a24bc1ed2280f373d893b905896ee92a/try-comm-central-win32-debug/

Comment 34

[Thomas D. (:thomas8)]

Thanks a lot for your help/contribution here, Jörg! :)

Comment 35

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #33)
> Actually, it's a debug build hence:
> https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-
> 11c3e0b3a24bc1ed2280f373d893b905896ee92a/try-comm-central-win32-debug/

I installed this as my daily.workbench flat installation (and accidentally killed my daily.workbench repository with local patches in the process, even file recovery helped nothing, grrrr).

1 Bug found (probably pre-existing):

Auto-Save moves the template msg to drafts folder, but should just overwrite existing template.

I think Auto-Save must always use the same flavor as Save, so we should check this for "save as file" flavor, too (and ensure that it still works for plain drafts).
Also, changing auto-save interval (e.g. down to 1 minute) does not seem to take effect immediately, if at all (unrelated bug).

Comment 36

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. from comment #34)
> Thanks a lot for your help/contribution here, Jörg! :)
I'm interested to get this bug fixed since it will fix two real issues in bug 1417792 and bug 1421736.

As for the autosave saving to the wrong folder, I guess that's pre-existing.

Comment 37

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #36)
> (In reply to Thomas D. from comment #26)
> > Allowing the override is fine (thanks), but imo, this definitely needs an
> > explicit warning message to the user for the (EditTemplate && "downconvert
> > HTML to plaintext") case. Without such a warning message, this would need to
> > be vetoed by ui-review.
> I suggest to add this to the JS part then, so you don't have to veto your
> own patch ;-) It's too late deep down in the bowels of C++ processing.

Sadly, I have no idea how to find out from within JS if a given message would be considered plaintext or HTML and opened with HTML editor or PlainText editor accordingly.

- Maybe involving Content-Type, but the main content type can have various flavours and I'm not sure if all of them are prejudicial to the format (maybe multipart can have only text parts? etc.).
- Components.interfaces.nsIMsgCompFormat comes to mind also, but that looks more like setting, not getting to me; iow, it looks like the desired format for composition, not the current format.
- forceplaintext is something else to consider (although that would be absurd for an HTML template, but maybe it's possible)

I have seen something like what I believe we would need in cpp:

https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#1479-1494
if (mdd->messageBody)
    {
      MSG_ComposeFormat composeFormat = nsIMsgCompFormat::Default;
      if (!mdd->messageBody->m_type.IsEmpty())
      {
        if(mdd->messageBody->m_type.Find("text/html", /* ignoreCase = */ true) != -1)
          composeFormat = nsIMsgCompFormat::HTML;
        else if (mdd->messageBody->m_type.Find("text/plain", /* ignoreCase = */ true) != -1 ||
                 mdd->messageBody->m_type.LowerCaseEqualsLiteral("text"))
          composeFormat = nsIMsgCompFormat::PlainText;
        else
          //We cannot use this kind of data for the message body! Therefore, move it as attachment
          bodyAsAttachment = true;
      }
      else
        composeFormat = nsIMsgCompFormat::PlainText;

Comment 38

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #36)
> (In reply to Thomas D. from comment #34)
> > Thanks a lot for your help/contribution here, Jörg! :)
> I'm interested to get this bug fixed since it will fix two real issues in
> bug 1417792 and bug 1421736.

Those two bugs look like two sides of the same coin (almost duplicates of each other), and then again they are just variants/symptoms of the only *one* real issue here, the design gap that "Edit Template" UI and functionality has been missing in TB, as reported by me in this bug. So afasics, you are de facto interested to get this bug fixed for the sake of itself (also supported by the fact that you've bundled the respective works here)... :)

> As for the autosave saving to the wrong folder, I guess that's pre-existing.

Yes, as I also assumed in my comment. Pre-existing or not, we need to fix it here and now to make "Edit Template" work.

Comment 39

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #0)

> UX desiderata:
> - For "Edit Template", compose window title should be "Edit Template: My
> Template Subject"

That's bug 1417788, and it would be really nice to fix that here.
Almost required because otherwise it's all but impossible to tell the difference between editing a template and editing a regular draft message, which matters much. I should look into that, as I've recently done this when I added "Write: " and "Print Preview: " to compose window titles in Bug 759039.

> - Yellow information bar in composition: You are editing a message template.
> [Save]

I guess that's for a followup bug, but definitely very nice to have.

> - to be defined: behaviour when user choses other flavors from [Save |v]
> button.

Generally good in terms of ux-consistency, "save-as" is known from other apps like word to save a new document and then continue working on that. Details are Pandora's box, not to be opened here. The current design especially around the "save-as-file" flavor isn't ideal, especially as it lacks the most obvious and preservative format, .eml.

Comment 40

[Jorg K (CEST = GMT+2)]

Attachment: 1389062_editTemplateCmd-CPP.patch (v3c) - rebased

Rebased after white-space removal.

While I'm here:
I've looked at the warning issue again, it can't be done where the information is available deep down in mime_parse_stream_complete() since that function doesn't even have a return code.

I tried looking through the code and noticed the following:
In MsgEditTemplateMessage() we inspect aEvent.shiftKey. If set, we get the message that's being edited:
  let msgFolder = gFolderDisplay ? GetFirstSelectedMsgFolder() : null;
  let msgUris = gFolderDisplay ? gFolderDisplay.selectedMessageUris : null;
  (from function composeMsgByType()).

Next we get the message header:
  hdr = messageArray.length > 1 ? null : messenger.msgHdrFromURI(messageArray[0]);
  (from function ComposeMessage()).

And then I got stuck. I haven't found a way to dig out whether a message is HTML or plaintext. To work that out, it has to be run through MIME. To be continued.

Comment 41

[Jorg K (CEST = GMT+2)]

Found it. With the header you can call MsgHdrToMimeMessage().

Comment 42

[Thomas D. (:thomas8)]

For the avoidance of doubt, I couldn't push this any further than I did. The rest is for others to complete. Thank you.
As of today, my frontend patch still applies without rejects, with some minor offsets.

Comment 43

[Jorg K (CEST = GMT+2)]

Comment on attachment 8937562 [details] [diff] [review]
1389062_editTemplateCmd-CPP.patch (v3c) - rebased

I think this is good to go, I don't insist on a warning when <shift>editing a HTML template and having it downgraded to plaintext.

If necessary, that can be done in a follow-up but should block fixing this bug and the duplicates, bug 1417792 and bug 1421736.

Comment 44

[:aceman]

Comment on attachment 8937562 [details] [diff] [review]
1389062_editTemplateCmd-CPP.patch (v3c) - rebased

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

Thanks, this works for me and I couldn't find any problem. Do tests pass with this latest version?

::: mailnews/mime/src/mimedrft.cpp
@@ +1401,5 @@
>      }
>  
>      draftInfo = MimeHeaders_get(mdd->headers, HEADER_X_MOZILLA_DRAFT_INFO, false, false);
>  
> +    // We always preserve an exisiting message ID, if present, apart from some exceptions.

Typo 'existing'.

Comment 45

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #44)
> Thanks, this works for me and I couldn't find any problem. Do tests pass
> with this latest version?
No idea, but I'll test it before landing. I'm not aware of any template tests, but I could be wrong.

Comment 46

[:aceman]

Comment on attachment 8933374 [details] [diff] [review]
Part1 - Frontend - WIP Patch V.1: Implement cmd_editTemplateMsg (rebased)

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

Thanks, this works nicely for me.
Sorry for the delay.

::: mail/base/content/mailContextMenus.js
@@ +88,3 @@
>    showCommandInSpecialFolder("cmd_newMsgFromTemplate",
>                               Components.interfaces.nsMsgFolderFlags.Templates);
> +  showCommandInSpecialFolder("cmd_editTemplateMsg", 

trailing space

@@ +88,4 @@
>    showCommandInSpecialFolder("cmd_newMsgFromTemplate",
>                               Components.interfaces.nsMsgFolderFlags.Templates);
> +  showCommandInSpecialFolder("cmd_editTemplateMsg", 
> +                             Components.interfaces.nsMsgFolderFlags.Templates);

The determination if folder is a Templates folder inside  showCommandInSpecialFolder is quite heavy and we already know if the folder is a Templates folder (from the call above). So this looks inefficient, but the construct is the norm here.
ShowMenuItem("cmd_newMsgFromTemplate", IsMenuItemShowing("cmd_editTemplateMsg")) would be a faster hack, but we are setting commands, not menuitems so it is semantically not correct.

Any interest in a followup to make showCommandInSpecialFolder() take an array of commandIDs as first argument? :)

Comment 47

[:aceman]

(In reply to Jorg K (GMT+1) from comment #45)
> I'm not aware of any template tests, but I could be wrong.

There could be draft tests, which this could have affected.

Comment 48

[Jorg K (CEST = GMT+2)]

Let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c34057dc6b2d7c84c11e2f52069f07be5f3b5ac6

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/921778a9bb1a
Implement 'Edit Template' command (cmd_editTemplateMsg) and UI. r=aceman, ui-r=paenglab.
https://hg.mozilla.org/comm-central/rev/5dd349d1efbc
Implement 'Edit Template' command (cmd_editTemplateMsg). C++ part. r=aceman

Comment 50

[Jorg K (CEST = GMT+2)]

I've landed this (without the print statements) to finally close this bug.

Thomas, please file a follow-up for the warning you require (comment #26, comment #40 and comment #43). If you want, you can attach a patch here to make showCommandInSpecialFolder() accept an array of IDs (comment #46) or also file a new bug for it.

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9f30d72759f5
Follow-up: typo fixes. rs=typo-fix DONTBUILD