1389083 - Implement "New Message from Template" command and UI

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[Thomas D. (:thomas8)]

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

This bug implements "New Message from Template" command and UI.
More specifically:
* XUL <command id="cmd_NewMessageFromTemplate">
* 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:
- "New Message from Template" command must be shown only for messages in Templates folder (or otherwise marked as template, if we have that)
- As default action for templates, I think this should become the topmost entry of context menu, followed by separator.

UX desiderata:
- Consider hiding "Edit as New" from context menu of templates (should be OK/useful to keep it on main and app menu)

This is structurally similar to bug 1106412 where I implemented "Edit draft" command, so we can borrow from there.

Comment 1

[Thomas D. (:thomas8)]

Historical background / Bug relations/motivations

Bug 731688 (current patch of attachment 8895783 [details] [diff] [review]) and this bug are two sides of the same coin, fixing the following ux-implementation-level problem:

Before Bug 731688, "New Message from Template" and "Edit as New Message" were wrongly conflated into current "Edit as New Message", which however behaved like "New Message from Template" should (observing template format), and didn't have the Shift trick for the opposite format. The net result was the massive problem of bug 78794: For "Edit as New Message", users got stuck in plaintext composition with their auto-downgraded sent messages originally composed in HTML.

Bug 731688 disentangles "Edit as New Message" from the template implementation, adds the Shift trick, and fixes bug 78794 in the process (by observing *account* format, so for HTML accounts, "Edit as new" will always compose HTML, regardless of original message format).

However, after Bug 731688, there'll be no menu access for the action of "New Message from Template" (observing template format), only double-click or Enter on template, which is also lacking the Shift-for-opposite-format trick.

Comment 2

[Thomas D. (:thomas8)]

Hints for implementation of this bug 1389083 (from my bug 731688, comment 29):

At https://dxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1984,
add the following function:
> function MsgEditMessageAsNew(aEvent)

function MsgNewMessageFromTemplate(aEvent)
{
  composeMsgByType(Components.interfaces.nsIMsgCompType.Template, aEvent);
}

Then implement the new command and UI in XUL:

https://dxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#253
>   <command id="cmd_editAsNew" oncommand="goDoCommand('cmd_editAsNew')"/>
   <command id="cmd_newMessageFromTemplate" oncommand="MsgNewMessageFromTemplate(event)"/>

Plus menu items with respective calls of the command (or the function), plus strings, plus hide logic if wanted.

Comment 3

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #1)
> However, after Bug 731688, there'll be no menu access for the action of "New
> Message from Template" (observing template format), only double-click or
> Enter on template, which is also lacking the Shift-for-opposite-format trick.
Right. I had code for the "opposite format" in bug 731688, but there was no way to trigger it, since there was no menu item for "New Message from Template". Here's the snippet just for reference:
+      if (body &&
+          mdd->format_out == nsMimeOutput::nsMimeMessageEditorTemplate)
+      {
+        if (msgComposeType == nsIMsgCompType::EditAsNew)
...
+        else if (mdd->overrideComposeFormat)
+        {
+          // When using a template and overriding, the user gets the
+          // "other" format.
+          if (composeFormat == nsIMsgCompFormat::PlainText)
+          {
+            printf("=== Template switching to HTML\n");
+            convert_plaintext_body_to_html(&body, bodyLen);
+            composeFormat = nsIMsgCompFormat::HTML;
+          }
+          else
+          {
+            // Conversion happens below.
+            printf("=== Template switching to plain text\n");
+            convertToPlainText = true;
+            composeFormat = nsIMsgCompFormat::PlainText;
+          }
+        }
+      }

That can just be added when bug 731688 is done.

Comment 4

[Thomas D. (:thomas8)]

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

Here's a WIP patch for the entire front end.
This is an exact clone of attachment bug 1106412, attachment 8729836 [details] [diff] [review]: Copy, paste, massage. It should work as-is (without Shift), but I don't have time to test if it does. So far, this should show a "New Message from Template" menu item (context menu, Message menu, app Menu), but only for messages in Templates folder, and should do exactly what it says, with 'Template message format' == 'composition mode/format'.

The front end should support Shift, but the backend doesn't.

I hope that  per his own comment 3 (code snippet), Jörg can upgrade the backend logic in another patch (Part 2), i.e. the internal implementation of Shift modifier to trigger opposite-of-default format.

Comment 5

[Thomas D. (:thomas8)]

Attachment: Part1 - Frontend - WIP Patch V.1.1: (hide 'Edit as New Message') Implement cmd_newMsgFromTemplate

...and this should hide 'Edit as New Message' for template messages where we show 'New Message from Template' default action instead.

They are not always exactly the same in behaviour depending on scenario, because 'Edit as New Message' observes *account* format, whereas 'New Message from Template' observes *message* format of the template.

But I think it makes sense not to show both:

a) to avoid duplication for the default scenarios of 'HTML template' on 'HTML account' (or 'plaintext template' on 'plaintext account'), where both actions would have same behaviour

b) to avoid confusion and user errors where explicit 'template format' != 'account format'; in that case, using 'Edit as New Message' would return the *opposite* template format, which imo doesn't make much sense for explicit templates. It might even be undesired for organizational setups where the message format is intentional and important.

c) to ease migration: before bug 731688, 'Edit as New Message' used to behave like 'New Message from Template' does now: observe *message* format. It's not unlikely that users may have used 'Edit as New Message' to create new messages from the template, with the same format. But now we've changed the logic, which could be very irritating for such users in terms of b) above. By hiding 'Edit as New Message' from template context menu, we'll gently push them into the new order.

Comment 6

[Richard Marti (:Paenglab)]

Comment on attachment 8896642 [details] [diff] [review]
Part1 - Frontend - WIP Patch V.1.1: (hide 'Edit as New Message') Implement cmd_newMsgFromTemplate

In mailContextMenus.js caller is wrong. It needs to be updateHiddenStateForNewMsgFromTemplateCmd().

The 'Edit as New Message' is hidden in context menu but not in main menu nor in AppMenu.

In main menu is no accesskey in AppMenu it's "VK_RETURN".

With this fixed, ui-r+

Comment 7

[Thomas D. (:thomas8)]

Attachment: Part1 - Frontend - WIP Patch V.1.2: (Nitfixes) Implement cmd_newMsgFromTemplate

(In reply to Richard Marti (:Paenglab) from comment #6)

Thanks!

> Comment on attachment 8896642 [details] [diff] [review]
> Part1 - Frontend - WIP Patch V.1.1: (hide 'Edit as New Message') Implement
> cmd_newMsgFromTemplate
> 
> In mailContextMenus.js caller is wrong. It needs to be
> updateHiddenStateForNewMsgFromTemplateCmd().

Fixed.

> The 'Edit as New Message' is hidden in context menu but not in main menu nor
> in AppMenu.

Hmmm. I'm deliberately only hiding 'Edit as New Message' in context menu, because
- space matters less in normal menus, but in context menu, it matters
- 'edit as new message' of template might still be useful (e.g. as a discoverable way of getting the opposite account format where applicable); we can end up with complaints, why are you hiding this command?
- Main message menu isn't shown by default
- App menu is certainly less frequently used than context menu

> In main menu is no accesskey in AppMenu it's "VK_RETURN".

Shortcut key. fixed.
No access keys because they are all taken; using duplicate access keys makes other access keys inefficient; for this command, no need for access key because it's always available from ENTER

> With this fixed, ui-r+

With this patch, I also added 'New Message from Template' under every 'New message' command, e.g.
> File > New > New Message
               New Message from Template,
> Appmenu > New Message | > Message
                            Message from Template,
because these two commands are very similar (also in word processors).

Comment 8

[Thomas D. (:thomas8)]

Some nits in last patch, wrong time to do patches...

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #7)
> Hmmm. I'm deliberately only hiding 'Edit as New Message' in context menu, ...
Is it really such a good idea to have the menus inconsistent? The "Edit Draft" "Edit As New Message" is also available on the context menu. With high-resolution screens everywhere, one item more shouldn't be a problem.

> With this patch, I also added 'New Message from Template' under every 'New
> message' command, e.g.
> > File > New > New Message
>                New Message from Template,
> > Appmenu > New Message | > Message
>                             Message from Template,
> because these two commands are very similar (also in word processors).
Hmm, I haven't tried the patch, but how do you select the template you want to use? In a word processor you typically get a list to chose from. I assume you need to be in the Templates folder and have a template selected for this to work, so I don't think this is a good idea, also given that there are three other ways (enter, message menu, context menu) to start your new message.

BTW, Aceman is very busy, so kindly let me review the next patch.

Comment 10

[Richard Marti (:Paenglab)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #7)
> Created attachment 8896669 [details] [diff] [review]
> Part1 - Frontend - WIP Patch V.1.2: (Nitfixes) Implement
> cmd_newMsgFromTemplate
> 
> > The 'Edit as New Message' is hidden in context menu but not in main menu nor
> > in AppMenu.
> 
> Hmmm. I'm deliberately only hiding 'Edit as New Message' in context menu,
> because
> - space matters less in normal menus, but in context menu, it matters
> - 'edit as new message' of template might still be useful (e.g. as a
> discoverable way of getting the opposite account format where applicable);
> we can end up with complaints, why are you hiding this command?

Who knows it's the opposite format? As normal user I see no difference in this two entries. Both opens the message and I can write my message. We should be consistent in the menus. We could get complaints, this entry is missing in the context menu.

> - Main message menu isn't shown by default

On Mac and Ubuntu shown by default.

> - App menu is certainly less frequently used than context menu

It still needs to be consistent for them which use it.

> > In main menu is no accesskey in AppMenu it's "VK_RETURN".
> 
> Shortcut key. fixed.
> No access keys because they are all taken; using duplicate access keys makes
> other access keys inefficient; for this command, no need for access key
> because it's always available from ENTER

Now both menus have "VK_RETURN". This is a internal name, the user should see the key he can use.
 
> With this patch, I also added 'New Message from Template' under every 'New
> message' command, e.g.
> > File > New > New Message
>                New Message from Template,
> > Appmenu > New Message | > Message
>                             Message from Template,
> because these two commands are very similar (also in word processors).

Clicking the entries does nothing when not template is selected. As Jörg wrote, how do you decide which template should be used? Then why we don't have the same for drafts? -> better don't do this. These menus are for generic functions or you need a dialog which let you choose the message -> too complicated when you have multiple accounts with templates.

If you still want it (I'm against it), file a new bug.

Comment 11

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #10)
> Who knows it's the opposite format? As normal user I see no difference in
> this two entries. Both opens the message and I can write my message. We
> should be consistent in the menus. We could get complaints, this entry is
> missing in the context menu.
Currently to create a new message from a template, you can double click the template, hit enter or use "Edit as new".

In bug 731688 we're planning to change the behaviour of "Edit as new" like this:
"New", "Reply" and "Forward" always use the account format, only "Edit as New" uses the message format. So we're planning on changing that, so "Edit as new" will also use the account format, but with the shift modifier it will use the opposite account format.

Additionally we're implementing "New message from template" here, which will use the format of the template and ignore the account format. When used with shift, "New message from template" will use the opposite template format. The latter is a little questionable since the auto-down- or -upgrade will always be suboptimal, so it might really be better to keep two different templates. After all, a template is a *template* to be used 1:1, and not some multi-purpose document.

That's why these two menu options are different, or more precisely, they will be different when bug 731688 is done.

Comment 12

[Jorg K (CEST = GMT+2)]

In fact, it would be useful to land this bug first and then bug 731688 could add the shift processing to "Edit as new", "New message from template" and also "Edit Draft" (bug 1389771) all in one go since it's all in the same C++ code.

Comment 13

[Richard Marti (:Paenglab)]

We should pay attention, all this opening differences don't confuse the user, also when they are logically correct.

Comment 14

[Thomas D. (:thomas8)]

Thanks, there's a lot of good input here and I'm in the process of preparing a new patch.

As a side note, context menu, as it stands, does NOT seem to be a full duplicate of all message menu commands. E.g., "Create filter from this message" and "Attachments > " are found in message menu, but not in context menu. I don't know if that's an omission or if it's by design, I suspect the latter. I can see an intention of making *all* message-related commands available from context menu, that's why we also have Print Preview and Print, even though maybe they are not so frequently used. Bottom line: Our context menu is VERY long already (dual buttons would really help, and there are bugs on record).

My argument here would be that for a template, "New Message from Template" will cover 95% of use cases, so also having "Edit as New Message" on the same context menu would appear to be confusing and duplicating functionality. But I wouldn't mind offering advanced users even more flexibility by making both "New Message from Template" and "Edit as New Message" available exclusively on *Message menu* of main menu and app menu. I don't think anyone will complain about "Edit as New Message" being missing on template context menu, because for most users, it's never needed on templates. I am a bit scared that someone who for whatever reason created a plaintext template on an HTML account might not find a way to compose with that template in HTML, because our Shift Trick is virtually undocumented and hence undiscoverable (Full Keyboard shortcuts documentation is too hard to find from inside the application). So by keeping both commands on Message menus, at least there'll always be a way out.

Otoh, Jörg has a point that templates can be assumed to come with an intentional format, and it's certainly an edge case wanting to override that format. For those who really need it, at least technically there'll be Shift (albeit undiscoverable). Maybe we can't or don't want to make everyone happy... Das Leben ist kein Ponyhof, und auch kein Wunschkonzert! :p

Ultimately, none of the 3 possible solutions for Template look totally wrong to me (in order of preference):

1) Only "New Message from Template" on context menu, but also keep "Edit as New Message" on Message menus (main/app)
2) Only "New Message from Template on both context menu and Message menus
3) Both "New Message from Template" and "Edit as New Message" on both context menu and Message menus

Comment 15

[Thomas D. (:thomas8)]

Attachment: Part1 - Frontend - WIP Patch V.2: (improved, variant 3) Implement cmd_newMsgFromTemplate

Surprise!

I like Jörg's proposal best, which also realizes Richard's call for cross-menu consistency (Variant 3 of comment 14).

(In reply to Jorg K (GMT+2) from comment #9)
> Is it really such a good idea to have the menus inconsistent? [For drafts,]
> "Edit Draft" [*and*] "Edit As New Message" [are] also available on the context menu.
> With high-resolution screens everywhere, one item more shouldn't be a problem.
Yeah.

Benefits/improvements:

a) Cross-menu consistency: All relevant menus now in sync: 
> Context menu / Message menu / App-Message menu

b) Cross-action consistency:
> Edit draft                + Edit as New Message
> New Message from Template + Edit as New Message

c) Default action on top of context menu (good UX practice, ux-consistent with TB and Windows)

d) Default action now marked as default
- ux-consistency with host OS, Windows; see Windows Explorer context menu
- this is good UX practice and it goes a long way wrt to our intention here, to emphasize the best choice, and de-emphasize less relevant actions.
- platform-aware: will only be used on platforms that support this, see: 
  https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/PopupGuide/ContextMenus#Indicating_the_Default_Item

e) improved/streamlined XUL, exploiting existing menu magic
   https://dxr.mozilla.org/comm-central/search?q=hideIfAppropriate&redirect=false

f) Improved: position of "New Message from Template" in Message menus, less prominent so that the more generic actions always remain en bloc (New, Reply, etc), with special actions as an 'edge case'. Also ux-consistent with "Edit Draft". Might have to be revisited when we get "Edit Template". Also note that using "New Message from Template" from main menu makes no sense at all when you can just double-click, press Enter, or use context menu, so we're just listing them here for completeness/accessibility, not to encourage users into using the main menu.

g) Nits fixed.

h) Removed clutter which I had added to "New >" menus, per Richard's comment 10.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8896785 [details] [diff] [review]
Part1 - Frontend - WIP Patch V.2: (improved, variant 3) Implement cmd_newMsgFromTemplate

Sorry about the change of mind, I'm struggling with some bustage right now, so perhaps Aceman has more cycles than I do.

Comment 17

[Richard Marti (:Paenglab)]

Comment on attachment 8896785 [details] [diff] [review]
Part1 - Frontend - WIP Patch V.2: (improved, variant 3) Implement cmd_newMsgFromTemplate

I think, we can go with this.

Comment 18

[Jorg K (CEST = GMT+2)]

Hmm, something wrong here. When I'm in a Drafts folder, I get both "Edit Draft Message" and "New Message from Template". That's not right, is it?

Comment 19

[Richard Marti (:Paenglab)]

Not here. In all menus? I tried to go first to templates then drafts and see only the correct entry.

Comment 20

[Jorg K (CEST = GMT+2)]

Hmm, I have a Drafts folder in my testing environment which produces the problem that I see "Edit Draft" and "New Message from Template". I deleted the folder and recreated it and the problem persists. Maybe it has the template flag stored against it. How would I check? All other Drafts folders I had in other accounts and profiles don't show the problem.

Thomas' code looks right, so it must be a data problem. But the two bold items at the beginning of the menu are clearly visible.

Comment 21

[Jorg K (CEST = GMT+2)]

I debugged it a bit, somehow the Drafts folder that the "template bit". No idea why.

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 8896785 [details] [diff] [review]
Part1 - Frontend - WIP Patch V.2: (improved, variant 3) Implement cmd_newMsgFromTemplate

Looks good. Maybe Aceman can look at it, too.

Comment 23

[:aceman]

(In reply to Jorg K (GMT+2) from comment #21)
> I debugged it a bit, somehow the Drafts folder that the "template bit". No
> idea why.

Check your accounts settings (all accounts), in Copies&Folders if Templates and Drafts of some accounts point to the same folder.

Comment 24

[Jorg K (CEST = GMT+2)]

I did. Didn't find anything. Then I checked prefs.js and indeed the Drafts folder was set as stationery. So I fixed that. But the problem persisted. In the end I did a folder.clearFlag(nsMsgFolderFlags.Templates) :-(

Comment 25

[:aceman]

Comment on attachment 8896785 [details] [diff] [review]
Part1 - Frontend - WIP Patch V.2: (improved, variant 3) Implement cmd_newMsgFromTemplate

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

This seems to work in practice, I just have some questions to the code.

::: mail/base/content/mail3PaneWindowCommands.js
@@ +653,5 @@
>        case "cmd_editDraftMsg":
>          MsgEditDraftMessage(null);
>          break;
> +      case "cmd_newMsgFromTemplate":
> +        MsgNewMessageFromTemplate(null);

How is this called? I see no goDoCommand('cmd_newMsgFromTemplate') while draft has it.

::: mail/base/content/mailContextMenus.js
@@ +82,5 @@
>  
>    // Hide "Edit Draft Message" menus if we're not in a draft folder.
>    updateHiddenStateForEditDraftMsgCmd();
> +  // Hide "New Message from Template" menus if we're not in a templates folder.
> +  updateHiddenStateForNewMsgFromTemplateCmd();

You are lucky this isn't the longest function name in the neighbourhood :)

::: mail/base/content/mailWindowOverlay.js
@@ +593,5 @@
>                        (folder && folder.getFlag(nsMsgFolderFlags.Drafts));
>    document.getElementById("cmd_editDraftMsg").setAttribute("hidden", !inDraftFolder);
>  }
>  
> +function updateHiddenStateForNewMsgFromTemplateCmd()

How I feel the urge to merge updateHiddenStateForEditDraftMsgCmd() and updateHiddenStateForNewMsgFromTemplateCmd() :) They are the same just check different folder flag.

::: mail/base/content/mailWindowOverlay.xul
@@ +427,5 @@
>    <key id="key_replyall" key="&replyToAllMsgCmd.key;"                oncommand="goDoCommand('cmd_replyall')" modifiers="accel, shift"/>
>    <key id="key_replylist" key="&replyToListMsgCmd.key;"              oncommand="goDoCommand('cmd_replylist')" modifiers="accel, shift"/>
>    <key id="key_forward" key="&forwardMsgCmd.key;"                    oncommand="goDoCommand('cmd_forward')" modifiers="accel"/>
>    <key id="key_editAsNew" key="&editAsNewMsgCmd.key;"                oncommand="goDoCommand('cmd_editAsNew')" modifiers="accel"/>
> +  <key id="key_newMsgFromTemplate" keycode="&newMsgFromTemplateCmd.keycode;"/><!-- for display on menus only -->

Why isn't this linked to the command?

@@ +679,5 @@
>      </menu>
>  
> +    <menuitem id="mailContext-editDraftMsg"
> +              label="&contextEditDraftMsg.label;"
> +              default="true"

This makes the items 'default', currently shown as bold. We don't use this in any other case (only e.g. default account or default identity).  But it could be interesting to make use of this more.

@@ +680,5 @@
>  
> +    <menuitem id="mailContext-editDraftMsg"
> +              label="&contextEditDraftMsg.label;"
> +              default="true"
> +              oncommand="MsgEditDraftMessage(event);"/>

So why is this not command="cmd_editDraftMsg" ? Wouldn't the event work? I think Thomas wrote it still works. The command implementation also references 'event'.

::: mail/base/content/messageWindow.js
@@ +1152,5 @@
>        case "cmd_editDraftMsg":
>          MsgEditDraftMessage(null);
>          break;
> +      case "cmd_newMsgFromTemplate":
> +        MsgNewMessageFromTemplate(null);

How is this called? I see no goDoCommand('cmd_newMsgFromTemplate') while draft has it.

Comment 26

[:aceman]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #14)
> As a side note, context menu, as it stands, does NOT seem to be a full
> duplicate of all message menu commands. E.g., "Create filter from this
> message" and "Attachments > " are found in message menu, but not in context
> menu. I don't know if that's an omission or if it's by design, I suspect the
> latter. 

"Create filter from message" makes no sense on the message itself. It needs to see which header you mean. This item is in the context menu when right-clicking a From or To address in the message header pane.

Comment 27

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #25)
> Comment on attachment 8896785 [details] [diff] [review]
> Part1 - Frontend - WIP Patch V.2: (improved, variant 3) Implement
> cmd_newMsgFromTemplate
> 
> Review of attachment 8896785 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems to work in practice, I just have some questions to the code.
> 
> ::: mail/base/content/mail3PaneWindowCommands.js
> @@ +653,5 @@
> >        case "cmd_editDraftMsg":
> >          MsgEditDraftMessage(null);
> >          break;
> > +      case "cmd_newMsgFromTemplate":
> > +        MsgNewMessageFromTemplate(null);
> 
> How is this called? I see no goDoCommand('cmd_newMsgFromTemplate') while
> draft has it.

I think there's no goDoCommand('cmd_editDraftMsg') either and yet we put it here.
I think in this case it's not relevant if called or not. As long as goDoCommand() exists, we must add all commands so that potential callers don't get stranded (including possibly addons: it's much safer for them to call a command than a specific function which we might change at any time).

> ::: mail/base/content/mailContextMenus.js
> @@ +82,5 @@
> >  
> >    // Hide "Edit Draft Message" menus if we're not in a draft folder.
> >    updateHiddenStateForEditDraftMsgCmd();
> > +  // Hide "New Message from Template" menus if we're not in a templates folder.
> > +  updateHiddenStateForNewMsgFromTemplateCmd();
> 
> You are lucky this isn't the longest function name in the neighbourhood :)

Lucky I am, and consistent, too ;)

> ::: mail/base/content/mailWindowOverlay.js
> @@ +593,5 @@
> >                        (folder && folder.getFlag(nsMsgFolderFlags.Drafts));
> >    document.getElementById("cmd_editDraftMsg").setAttribute("hidden", !inDraftFolder);
> >  }
> >  
> > +function updateHiddenStateForNewMsgFromTemplateCmd()
> 
> How I feel the urge to merge updateHiddenStateForEditDraftMsgCmd() and
> updateHiddenStateForNewMsgFromTemplateCmd() :) They are the same just check
> different folder flag.

I saw this, too, but strangely I never felt the urge... :| Rather I thought I could get away with(out) it...
I can try, but I don't think it matters much.

> ::: mail/base/content/mailWindowOverlay.xul
> @@ +427,5 @@
> >    <key id="key_replyall" key="&replyToAllMsgCmd.key;"                oncommand="goDoCommand('cmd_replyall')" modifiers="accel, shift"/>
> >    <key id="key_replylist" key="&replyToListMsgCmd.key;"              oncommand="goDoCommand('cmd_replylist')" modifiers="accel, shift"/>
> >    <key id="key_forward" key="&forwardMsgCmd.key;"                    oncommand="goDoCommand('cmd_forward')" modifiers="accel"/>
> >    <key id="key_editAsNew" key="&editAsNewMsgCmd.key;"                oncommand="goDoCommand('cmd_editAsNew')" modifiers="accel"/>
> > +  <key id="key_newMsgFromTemplate" keycode="&newMsgFromTemplateCmd.keycode;"/><!-- for display on menus only -->
> 
> Why isn't this linked to the command?

Would it work? Are you aware that &newMsgFromTemplateCmd.keycode; == VK_Return? Enter key is implemented elsewhere, so this is really just for showing the key on menu, not for assigning a real shortcut key. If we link to command, then assign Enter key (VK_Return) to actually trigger that command, I'd expect that results are somewhat unpredictable because any Enter key press anywhere could trigger this command, which isn't what we want.

> 
> @@ +679,5 @@
> >      </menu>
> >  
> > +    <menuitem id="mailContext-editDraftMsg"
> > +              label="&contextEditDraftMsg.label;"
> > +              default="true"
> 
> This makes the items 'default', currently shown as bold. We don't use this
> in any other case (only e.g. default account or default identity).  But it
> could be interesting to make use of this more.

Irgendwann ist immer das erste Mal... There's a first time for everything... 
Yes, I think this is really nice, professional and helpful, and we should use this whereever default actions are applicable. I suspect that there might be only one other case, opening messages; maybe something in AB. Default action for "open" needs a bit more work because we need to establish the default opening method first (tab/window).
Since drafts and templates are "special" folders, I don't think anyone will/can blame us if we only mark default actions there, for now.

> @@ +680,5 @@
> >  
> > +    <menuitem id="mailContext-editDraftMsg"
> > +              label="&contextEditDraftMsg.label;"
> > +              default="true"
> > +              oncommand="MsgEditDraftMessage(event);"/>
> 
> So why is this not command="cmd_editDraftMsg" ? Wouldn't the event work? I
> think Thomas wrote it still works. The command implementation also
> references 'event'.

Yes, event would still work when hooked to cmd_x. But unfortunately, enabling the menu items fails for certain scenarios, as we found when implementing 'Edit draft' command in bug 1106412. You yourself added a comment in code to that end.
But that's exactly where my surprise is, I'm quite failing to understand WHY it fails, given that before showing the context menu, we call two routines which should correctly enable the command. But maybe the isEnabled routines of the command trigger again *after* our two context-menu routines (updateHidden... and setSingleSelection(...)). I'm also not sure why isEnabled of command would disable for right-click on non-selected item, given that setSingleSelection has something like a numSelectedItems check which is *also* tested for 1, so it looks selected. Unless that context menu implementation of numSelectedItems has a special case where non-selections count as 1-selection. The fact that you can right-click an item which is not yet selected is a bug imo, and nobody will use that behaviour because it's inconsistent with basic OS selection patterns. Similar to what we fixed for contacts side bar selection patterns.
Ideally, I'd prefer it if we can pack all isEnabled and hiding logic into the isEnabled of the cmd_x, then remove all the special treatments. Even special updating calls before showing any of the three menu types should not be necessary given there's an isEnabled handler of the command; maybe goUpdateCommand at the maximum. But I guess it's a lot of work for little gain, because at least we know that the current conglomerate works! :)

> ::: mail/base/content/messageWindow.js
> @@ +1152,5 @@
> >        case "cmd_editDraftMsg":
> >          MsgEditDraftMessage(null);
> >          break;
> > +      case "cmd_newMsgFromTemplate":
> > +        MsgNewMessageFromTemplate(null);
> 
> How is this called? I see no goDoCommand('cmd_newMsgFromTemplate') while
> draft has it.

See above, let's just keep things complete and consistent just in case there'll be a caller using this method.
Btw the event is not used here because these calls are also used for keyboard shortcuts, which might have Shift in the shortcut, not as a modifier for toggling behaviour. Another reason to add this here, if some addon wants to add their own keyboard shortcut.

Comment 28

[:aceman]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #27)
> > ::: mail/base/content/mail3PaneWindowCommands.js
> > @@ +653,5 @@
> > >        case "cmd_editDraftMsg":
> > >          MsgEditDraftMessage(null);
> > >          break;
> > > +      case "cmd_newMsgFromTemplate":
> > > +        MsgNewMessageFromTemplate(null);
> > 
> > How is this called? I see no goDoCommand('cmd_newMsgFromTemplate') while
> > draft has it.
> 
> I think there's no goDoCommand('cmd_editDraftMsg') either and yet we put it
> here.

It is in the patch:
253  <command id="cmd_editDraftMsg" oncommand="goDoCommand('cmd_editDraftMsg')"/> 
254  + <command id="cmd_newMsgFromTemplate" oncommand="MsgNewMessageFromTemplate(event);"/> 
So why not goDoCommand('cmd_newMsgFromTemplate') too? But please check if event is passed where relevant and in some window/menuitem the null is passed.

> > > +  <key id="key_newMsgFromTemplate" keycode="&newMsgFromTemplateCmd.keycode;"/><!-- for display on menus only -->
> > 
> > Why isn't this linked to the command?
> 
> Would it work? Are you aware that &newMsgFromTemplateCmd.keycode; ==
> VK_Return? Enter key is implemented elsewhere, so this is really just for
> showing the key on menu, not for assigning a real shortcut key. If we link
> to command, then assign Enter key (VK_Return) to actually trigger that
> command, I'd expect that results are somewhat unpredictable because any
> Enter key press anywhere could trigger this command, which isn't what we
> want.

Yeah, I can understand this worry.

> > @@ +680,5 @@
> > >  
> > > +    <menuitem id="mailContext-editDraftMsg"
> > > +              label="&contextEditDraftMsg.label;"
> > > +              default="true"
> > > +              oncommand="MsgEditDraftMessage(event);"/>
> > 
> > So why is this not command="cmd_editDraftMsg" ? Wouldn't the event work? I
> > think Thomas wrote it still works. The command implementation also
> > references 'event'.
> 
> Yes, event would still work when hooked to cmd_x. But unfortunately,
> enabling the menu items fails for certain scenarios, as we found when
> implementing 'Edit draft' command in bug 1106412. You yourself added a
> comment in code to that end.

So if we do not link to the command and do not inherit the disabling from it, is there other special code that disables this mailContext-editDraftMsg element (and now template).

> > ::: mail/base/content/messageWindow.js
> > @@ +1152,5 @@
> > >        case "cmd_editDraftMsg":
> > >          MsgEditDraftMessage(null);
> > >          break;
> > > +      case "cmd_newMsgFromTemplate":
> > > +        MsgNewMessageFromTemplate(null);
> > 
> > How is this called? I see no goDoCommand('cmd_newMsgFromTemplate') while
> > draft has it.
> 
> See above, let's just keep things complete and consistent just in case
> there'll be a caller using this method.

It seems to me the <command> is defined inconsistently to what draft does, see above.

Comment 29

[Thomas D. (:thomas8)]

Attachment: Part1 - Frontend - WIP Patch V.3: (new generic function) Implement cmd_newMsgFromTemplate

(In reply to :aceman from comment #25)
> How I feel the urge to merge updateHiddenStateForEditDraftMsgCmd() and
> updateHiddenStateForNewMsgFromTemplateCmd() :) They are the same just check
> different folder flag.

Let's make everyone happy where we can...
Voilà, a nice and neat generic function with positive logic!

> showCommandInSpecialFolder("cmd_newMessageFromTemplate", "Templates")
>
> function showCommandInSpecialFolder(aCommandId, aFolderFlagName)
> {...}

(In reply to :aceman from comment #28)
> (In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment
> It is in the patch:
> 253  <command id="cmd_editDraftMsg"
> oncommand="goDoCommand('cmd_editDraftMsg')"/> 
> 254  + <command id="cmd_newMsgFromTemplate"
> oncommand="MsgNewMessageFromTemplate(event);"/> 
> So why not goDoCommand('cmd_newMsgFromTemplate') too? But please check if
> event is passed where relevant and in some window/menuitem the null is
> passed.

Because goDoCommand doesn't support event/shift.
We'll actually remove goDoCommand for cmd_editDraftMsg and cmd_editAsNew because we also want to allow Shift for 'Edit Draft' (bug 1389771) and 'Edit as New Message' (bug 731688).

> > > > +    <menuitem id="mailContext-editDraftMsg"
> > > > +              label="&contextEditDraftMsg.label;"
> > > > +              default="true"
> > > > +              oncommand="MsgEditDraftMessage(event);"/>
> > > 
> > > So why is this not command="cmd_editDraftMsg" ? Wouldn't the event work? I
> > > think Thomas wrote it still works. The command implementation also
> > > references 'event'.
> > 
> > Yes, event would still work when hooked to cmd_x. But unfortunately,
> > enabling the menu items fails for certain scenarios, as we found when
> > implementing 'Edit draft' command in bug 1106412. You yourself added a
> > comment in code to that end.
> 
> So if we do not link to the command and do not inherit the disabling from
> it, is there other special code that disables this mailContext-editDraftMsg
> element (and now template).

There must be. Maybe our own function (updateHiddenState...) or maybe something in setSingleSelection(...). Sorry I don't fully understand the interactions of this code. But wrt enabling/disabling, here I'm just doing exactly the same as we've done for Edit Draft command, so I think we're good, because it has already been reviewed and works. We can try to optimize more in another bug.

> > > ::: mail/base/content/messageWindow.js
> > > @@ +1152,5 @@
> > > >        case "cmd_editDraftMsg":
> > > >          MsgEditDraftMessage(null);
> > > >          break;
> > > > +      case "cmd_newMsgFromTemplate":
> > > > +        MsgNewMessageFromTemplate(null);
> > > 
> > > How is this called? I see no goDoCommand('cmd_newMsgFromTemplate') while
> > > draft has it.
> > 
> > See above, let's just keep things complete and consistent just in case
> > there'll be a caller using this method.
> 
> It seems to me the <command> is defined inconsistently to what draft does,
> see above.

See above, we want to be consistent with the new order (like reply etc.), and wrt Shift, 'Edit draft' is old order, which doesn't know Shift.

Comment 30

[Thomas D. (:thomas8)]

Attachment: Part1 - Frontend - WIP Patch V.3.1: (incl. tweak for cmd_editDraftMsg) Implement cmd_newMsgFromTemplate

Same as V.3, but I've added one line to tweak the frontend required for bug 1389771 (backend currently planned tbd by Jörg in bug 731688):

> <command id="cmd_editDraftMsg" oncommand="goDoCommand('cmd_editDraftMsg')"/>
<command id="cmd_editDraftMsg" oncommand="MsgEditDraftMessage(event);"/>

Otherwise I had already made the frontend ready for Shift in bug 1106412.

Comment 31

[Thomas D. (:thomas8)]

Comment on attachment 8898633 [details] [diff] [review]
Part1 - Frontend - WIP Patch V.3.1: (incl. tweak for cmd_editDraftMsg) Implement cmd_newMsgFromTemplate

When checking this in, pls add ui-r=paenglab from comment 17.

Comment 32

[:aceman]

Comment on attachment 8898633 [details] [diff] [review]
Part1 - Frontend - WIP Patch V.3.1: (incl. tweak for cmd_editDraftMsg) Implement cmd_newMsgFromTemplate

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

Thanks, this works for me now.

::: mail/base/content/mailWindowOverlay.js
@@ +590,2 @@
>   */
> +function showCommandInSpecialFolder(aCommandId, aFolderFlagName)

I'd rather like the function to take the whole folder flag. I will update the patch.

Comment 33

[:aceman]

Attachment: patch v3.2

Comment 34

[Thomas D. (:thomas8)]

Comment on attachment 8899125 [details] [diff] [review]
patch v3.2

Nit: When checking in, please adjust this function comment:
> * aCommandId   the ID of a command to be shown in folders having aFolderFlagName
 * aCommandId   the ID of a command to be shown for selected messages having aFolderFlag

Aceman, can you please enlighten me why you prefer using the actual flag instead of the flag name in the function, which makes calls much longer and in that sense harder to read?

> showCommandInSpecialFolder("cmd_newMessageFromTemplate", Components.interfaces.nsMsgFolderFlags.Templates)

vs.
> showCommandInSpecialFolder("cmd_newMessageFromTemplate", "Templates")

There's this in mailContextMenus.js:
> Components.utils.import("resource://gre/modules/Services.jsm");
> Components.utils.import("resource:///modules/mailServices.js");

From those, ci object shortcut for components.interfaces is not available?
ci.nsMsgFolderFlags.Templates would look much nicer...

Comment 35

[:aceman]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #34)
> Aceman, can you please enlighten me why you prefer using the actual flag
> instead of the flag name in the function, which makes calls much longer and
> in that sense harder to read?

It is easier to search in the code, you see immediately that is a folder flag, not some string "Draft".

> There's this in mailContextMenus.js:
> > Components.utils.import("resource://gre/modules/Services.jsm");
> > Components.utils.import("resource:///modules/mailServices.js");
> 
> From those, ci object shortcut for components.interfaces is not available?
> ci.nsMsgFolderFlags.Templates would look much nicer...

I don't think you get Ci just by including those modules, many files define Ci again and again.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1b41114997ea
Implement 'New Message from Template' command (cmd_newMsgFromTemplate) and UI. r=aceman,jorgk, ui-r=Paenglab DONTBUILD

Comment 37

[Jorg K (CEST = GMT+2)]

I fixed the comment and made it:
* aCommandId   the ID of a command to be shown in folders having aFolderFlag
The suggestion wasn't accurate:
* aCommandId   the ID of a command to be shown for selected messages having aFolderFlag

The messages don't have the flag but their folder has or the folder that's currently displayed.

Thanks for your work, Thomas, I can now move ahead with bug 731688 and bug 1389771, and don't we want the shift trick for "New Message from Template" as well?

Comment 38

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #37)
> I fixed the comment and made it:
> * aCommandId   the ID of a command to be shown in folders having aFolderFlag
> The suggestion wasn't accurate:
> * aCommandId   the ID of a command to be shown for selected messages having
> aFolderFlag
> 
> The messages don't have the flag but their folder has or the folder that's
> currently displayed.
> 
> Thanks for your work, Thomas, I can now move ahead with bug 731688 and bug
> 1389771, and don't we want the shift trick for "New Message from Template"
> as well?

Yes, we want the Shift trick for the bugs you mentioned, including this bug 1389083 (as explained in comment 4, hence my patch was called "Part 1"), and for bug 1389062 (Edit Template), too, please...
Basically, we want the Shift trick for *all* ways of going into composition... :)

Comment 39

[Thomas D. (:thomas8)]

Thank you Jörg, Aceman, and Richard for fast reviews and landing, this is fun! So good to know that when I get the ball rolling, there's others to push on! Nice teamwork with smart division of labor according to personal skills, availability and preferences! :)