+++ This bug was initially created as a clone of Bug #321355 +++ For messages in draft folders, bug 321355 fixes "Edit as New Message" menus to do just that: create a new copy of the draft and edit that. Unfortunately, thereafter there'll be no way of accessing "Edit (this) draft" functionality via menus (only "Edit" button on draft infobar). So this bugs adds "Edit (this) draft" to main menu's message menu and to the context menu, but only shown when we are in a draft folder. We'll want to use proper command layout to benefit from command attribute forwarding to UI elements which observe the central command.

Carry over relevant discussion from bug 321355; sorry I don't have time so it's just unfiltered copy & paste (In reply to WADA from comment bug 321355 #27) > (In reply to Thomas D. from comment bug 321355 #25) > > "Edit draft" (the default action) is currently missing on context menu, (snip) > > "Edit" button is shown at Message Header Box when mail is mail in Drafts > folder. > This is done by ; > > http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#3029 > > setDraftEditMessage: function() > > if (msgHdr.folder.isSpecialFolder(nsMsgFolderFlags.Drafts, true)) > > { > > let buttons = [{ ..., ..., ..., > > callback: function(aNotification, aButton) { > > MsgComposeDraftMessage(); > > return true; // keep notification open > > } > > }] ; > MsgComposeDraftMessage() is sibling of MsgEditMessageAsNew(), > MsgForwardAsAttachment(event), MsgForwardAsInline(event) etc. which is > defined in mailWindowOverlay.js. > > http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1862 > (In reply to Thomas D. from comment #28) > (In reply to WADA from comment #27) > > Thank you WADA for this helpful comment, with which I agree! :) > > > To do same thing at menu, following is needed. > > 1. Add "Edit Draft" to "Message" of main menu and Context menu of mail, as > > currently done on "Edit As New". > > Detailed definition at one of 2 is sufficient, if other watches primary > > menuitem item or toolbarbutton. > > "common command use in menuitem/toolbarbutton definition" is also > > possible. > > Yes, central command controller would be best, than let the UI observe that. > > > 2. If "command" is utilized, in Command Controller, enable/disable the > > menuitem according to context, status etc., > > as currently done on Forward, Forward As, Edit As New etc. > > 3. When "Edit Draft" menu, it's maybe better hidden at main menu/context > > menu if folder is not Drafts, > > instead of simply disabling(gray out at menu). > > +1, hiding this command for non-draft folders lgtm. > > > In this case, control in onmenupopup is additionlly needed. > > Yeah, most likely. I wonder if that could be streamlined using something > like a "hidden" attribute on the command controller which is then > auto-forwarded to UI elements observing that command? > > > > EditAsNew relevant definitions for menu > > > http://mxr.mozilla.org/comm-central/search?string=editasnew > > > Edit As New in main menu > > > http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#2687 > > > Edit As New in context menu > > > http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#725 > > > > I think following are different jobs and (b) is better done in separated > > bug. (b) is never fault of patch for this bug :-) > > (a) "Edit As New" should be "Edit As New" at anywhere. "Edit As New" > > shouldn't be "Edit draft". > > Change "Edit As New of draft mail" from same one as "Edit draft" to > > same one as "Edit New of Template or ordinal mail". > > Rephrase for clarity: > (a) "Edit As New" should mean/do "Edit as New" anywhere in the UI, because > "Edit As New" cannot and should never mean "Edit this draft" > Consequently, in this bug we are changing "Edit As New" for message in > draft folder from current wrong behaviour ("Edit draft") to correct > behaviour, which is same as "Edit As New Msg" of msg in Template folder or > any other ordinary (non-draft) msg. > > > (b) If "Edit As New" is "Edit As New" at anywhere as expected, we loose > > "Edit draft" via. menu. > > (b) After this bug, when "Edit As New Msg" means/does "Edit as New Msg" > anywhere in the UI as expected, we'll have lost "Edit draft" functionality > via menu for draft msgs, which might be considered a regression of this bug. > So I think before landing this bug, we should fix that: > > > Add equivalence to "Edit button at Message Header Box of draft mail" > > in main menu and/or context menu. > > Iow (in a new bug) add "Edit draft" command to main menu's message menu and > context menu in drafts folder (we currently *do* offer default actions in > context menus, which imo makes sense, because it assists discovery and > understanding, and also helps when you are tired, more control-oriented, or > just insecure, and thus prefer explicit context menu actions over trusting > that double click or Enter will do the right thing for you).

FYI. Definition/code for double click at threadPane. > http://mxr.mozilla.org/comm-central/source/mail/base/content/threadPane.js#39 > In ThreadPaneOnClick(event), if double clicked at non-twisty, call ThreadPaneDoubleClick() > http://mxr.mozilla.org/comm-central/source/mail/base/content/threadPane.js#146 > In ThreadPaneKeyDown(event), if "Enter", call ThreadPaneDoubleClick() > http://mxr.mozilla.org/comm-central/source/mail/base/content/threadPane.js#129 > ThreadPaneDoubleClick() > If Drafts, MsgComposeDraftMessage(), > Else If Templates, ComposeMessage(), > Else MsgOpenSelectedMessages(). MsgComposeDraftMessage() is commonly/consistently used for "Edit draft". Needed action is; > 1. <command id="cmd_editDraft" oncommand="goDoCommand('cmd_editDraft')"/> > 2. <menupopup id="appmenu_messageMenuPopup" onpopupshowing="InitAppMessageMenu();"> > <menuitem id="appmenu_editDraft" label="&teditDraftMsgCmd.label;" key="key_editAsNew" command="cmd_editDraft"/> > 3. <menupopup id="messageMenuPopup" onpopupshowing="InitMessageMenu();"> > <menuitem id="menu_editDraft" label="&teditDraftMsgCmd.label;" key="key_editAsNew" command="cmd_editDraft"/> > 4. In Command Controller, same action as cmd_editAsNew for cmd_editDraft. > 5. In InitAppMessageMenu() and InitMessageMenu(). > disable(and hide) editDraft menuitem as done on appmenu_forwardAsMenu/menu_forwardAsMenu(disable if no selected msg). > (a) If not special folder of Drafts, disable and hide editDraft menu. > (b) When imap folder and specil folder of "Inbox" is also set in addition to "Drafts", > if message doesn't have \Draft flag, disable and hide editDraft menu, in case of "user uses Inbox as Draft too". > Same action may be needed in ThreadPaneDoubleClick(). Mail composition related menu/menuitem is now busy, so "hiding menu/menuitem if not usable(disabled)" may be better. Note: When multiple messages are selected at threadPane, appMenu(main menu) is not altered, but Context menu is drastically changed : No mail composition related menu except "Forward as Attachments".

Patch for Bug 321355 is one liner patch. It simply stops "setting previous/editing DraftId upon composition start by Edit As New". > if (mdd->format_out == nsMimeOutput::nsMimeMessageEditorTemplate) > { > - fields->SetDraftId(mdd->url_name); > After it, Open Composer Window. This is done in mime-parser(mimedrft.cpp). i.e. it's set upon parsing existent mail data for edit, forward etc. Trick in original "Edit draft"(double click at Drafts) / "Edit As New" was "if folder is not Drafts folder, original DraftId is ignored". i.e. Before bug 321355 : Edit As New at non-Drafts = Edit draft + Don't delete original upon first draft save/send Edit As New at Drafts = Edit draft(==deletes original upon first draft save/send) Edit draft at Drafts = Edit draftl(==deletes original upon first draft save/send) After bug 321355 : Edit As New at any folder = Edit template(==doesn't delete original upon first draft save/send) Edit draft at Drafts = Edit template + Deletes original upon first draft save/send Following is currently used interface for "Edit As New" and "Edit(draft)". ComposeMessage() is finally used used in both case. composeMsgByType used by EditAsNew was for "Shift modifier" handling. > http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1867 > function MsgEditMessageAsNew() > { > composeMsgByType(Components.interfaces.nsIMsgCompType.Template); > } > function composeMsgByType(aCompType, aEvent) { > if (aEvent && aEvent.shiftKey) { > ComposeMessage(aCompType, > Components.interfaces.nsIMsgCompFormat.OppositeOfDefault, > msgFolder, msgUris); > } > else { > ComposeMessage(aCompType, Components.interfaces.nsIMsgCompFormat.Default, > msgFolder, msgUris); > } > } > function MsgComposeDraftMessage() > { > ComposeMessage(Components.interfaces.nsIMsgCompType.Draft, > Components.interfaces.nsIMsgCompFormat.Default, > gFolderDisplay.displayedFolder, > gFolderDisplay.selectedMessageUris); > } > http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCommands.js#154 > function ComposeMessage(type, format, folder, messageArray) > What is done in this module when nsIMsgCompType.Draft was ; > Check whether Edit Draft window is already opened for this mail, or not. > If lredy opened, merely switch to it(avoid duplicate Edit of a draft mail). > Finally, MailServices.compose.OpenComposeWindow() is called to open Composer window. > MailServices.compose.OpenComposeWindow() is defined at here. > http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#497 > After patch for bug 321355, I think "delete of original draft mail in Edit draft" won't be executed, because fields->SetDraftId(mdd->url_name) is removed by the patch. "What is needed for editDraft after bug 321355" is "set previous/editing DraftId upon composition start" at somewhere.

(In reply to WADA from comment #3) > After patch for bug 321355, I think "delete of original draft mail in Edit > draft" won't be executed, because fields->SetDraftId(mdd->url_name) is > removed by the patch. draftId is set at https://hg.mozilla.org/comm-central/file/3f82c17bf9af/mailnews/mime/src/mimedrft.cpp#l1487 > "What is needed for editDraft after bug 321355" is "set previous/editing > DraftId upon composition start" at somewhere. You don't need to worry about draftId in case of "Edit" in drafts folder.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4) > (In reply to WADA from comment #3) > > After patch for bug 321355, I think "delete of original draft mail in Edit > > draft" won't be executed, because fields->SetDraftId(mdd->url_name) is removed by the patch. > > draftId is set at > https://hg.mozilla.org/comm-central/file/3f82c17bf9af/mailnews/mime/src/mimedrft.cpp#1487 But your patch removes the line... > You don't need to worry about draftId in case of "Edit" in drafts folder. I guess why "Edit As New at Drafts folder currently deletes edited mail" is as follows. I couldn't find code which sets draftID of edited mail when composition-type=Draft is requested via Edit button/double click at Drafts folder, except mimedrft.cpp#1487. I could find logic of "if composition type=Draft, deleteDraft=true" only. How "edited mail in Drafts folder" is deleted if Edit button/double click at Drafts folder, after mimedrft.cpp#1487 is removed? Why "edited mail is not deleted upon first draft save/send if Edit As New of non-Drafts folder even if draftID is set" is; draftID is set by mimedrft.cpp#1487, but if compositin-type=Template is requested via. "Edit As New" menu at non-Drafts folder, deleteDraft=false is intially set, so "delete editing mail" is not executed upon first draft save/send. And, when draft save happens, URL of newly saved draft is set in draftID, and deleteDraft=true is set. Note: Following is actually ofserved phenomenon. When "Edit As New" at Template, initial state is : draftID=edited template, deleteDraft=false After Save" : draftID=newly saved draft(==currently edited mail), deleteDraft=true Why "edited mail is deleted upon first draft save/send if Edit As New of Drafts folder" is; (a) draftID is set by mimedrft.cpp#1487, (b) but if edit at Drafts folder, even if "edit of mail" is requested with compositin-type=Template by "Edit As New" menu, deleteDraft=true is set(it's perhaps because of "Edit *AT* Drafts folder), (c) because draftID is set and deleteDraft=true, "editing draft" is deleted upon first draft save or send, and URL of newly saved draft is set in draftID. Your patch removes mimedrft.cpp#1487.

(In reply to WADA from comment #5) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #4) > > (In reply to WADA from comment #3) > > > After patch for bug 321355, I think "delete of original draft mail in Edit > > > draft" won't be executed, because fields->SetDraftId(mdd->url_name) is removed by the patch. > > > > draftId is set at > > https://hg.mozilla.org/comm-central/file/3f82c17bf9af/mailnews/mime/src/mimedrft.cpp#1487 > > But your patch removes the line... The patch removes another SetDraftId(). https://hg.mozilla.org/comm-central/file/3f82c17bf9af/mailnews/mime/src/mimedrft.cpp#l1455

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6) > The patch removes another SetDraftId(). > https://hg.mozilla.org/comm-central/file/3f82c17bf9af/mailnews/mime/src/mimedrft.cpp#l1455 Gotcha! Sorry for my confusions. If no draftID when Template request, "delete mail in draftID" can't occur at any folder when "Edit As New" :-). Sorry, I msundestood test results. When "Edit As New of non-Drafts folder", draftID=original mail, deleteDraft=false was set, and original mail(in draftID) is not deleted upon "Save as draft", as I previously wrote. However, when "Edit As New of a draft mail in Drafts folder", draftID=edited draft, deleteDraft=false was set, but edited draft=original=draftID was deleted upon save(problem of .bug 321355). If draftID is set, and if Drafts folder, Tb always deletes draftID regardless of deleteDraft upon draft save/send?

FYI. Why "edited mail by Edit As New is deleted if Drafts" but "edited mail by Edit As New is not deleted if non-Drafts" is perhaps: > http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3674 > After end of SaveAsDrft/SaveAsTemplate/QueueForLater/DeliverBackground, RemoveCurrentDraftMessage() is called. > http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3728 > if draftID is not set, RemoveCurrentDraftMessage does do nothing. > if draftID is set && folderFlags has nsMsgFolderFlags::Drafts, RemoveCurrentDraftMessage deletes draftID. > even if draftID is set, if folderFlags doesn't have nsMsgFolderFlags::Drafts, RemoveCurrentDraftMessage does do nothing. Pretty small mismatch on "delete original or not" is seen in composer component modules. Someone deletes originl if draftID is set && deleteDraft=true regardless f folder type, but other someone deletes original if draftID is set && folder is Drafts folder regardless of deleteDraft. This is perhaps difference between "process during mail composition" and "post save/send process".

FYI. By patch for bug 321355, "Edit As New" won't delete "edited mail" even at Drafts folder, and if editDraft requests with composition mode=Draft as done currently, "edited mail" is still deleted as expected as far as it's requested at Drafts folder.. As seen in Bug 731688, "Shift modifier" is better used in "mail composition related XUL elements" commonly/consistently. Such thing is already sorted out in Forward/Reply relevant elemetnts. So, "ForwardInline" is better used as template of both "editAsNew" and "editDraft". > Common composeMsgByType() use for common/consistent Shift modifier support. > function composeMsgByType(aCompType, aEvent) { > if (aEvent && aEvent.shiftKey) { ComposeMessage(aCompType, > Components.interfaces.nsIMsgCompFormat.OppositeOfDefault, msgFolder, msgUris); > } > else { ComposeMessage(aCompType, > Components.interfaces.nsIMsgCompFormat.Default, msgFolder, msgUris); > } > } > Function definition for each XUL element. > function MsgForwardAsInline(event) { > composeMsgByType(Components.interfaces.nsIMsgCompType.ForwardInline, event); } > In command Controler, pass null, in case of goDoCommand use in command. > This is not actully used, because function is directly called by command with passing event object. > case "cmd_forwardInline": > MsgForwardAsInline(null); > break; > command, menuitem definition > <command id="cmd_forwardInline" oncommand="MsgForwardAsInline(event)"/> > <menuitem id="menu_forwardAsInline" label="&forwardAsInline.label;" > command="cmd_forwardInline" accesskey="&forwardAsInline.accesskey;" /> > <menuitem id="menu_forwardAsInline" label="&forwardAsInline.label;" > command="cmd_forwardInline" accesskey="&forwardAsInline.accesskey;" />

WADA, can you please post a patch to fix this issue? I think you have enough knowledge to fix this issue.

If I try to create patch, I want to do change like next, because I dislike coding like current one : What is actually executed is unknown until deeper level function is read. I like "What is actually done is coded in main module, subfunctions is helper of mail module" style. > function Msg_functionnameforeachmenuitem(event) { > ComposeMessage(Components.interfaces.nsIMsgCompType.ForwardInline, > MsgDerermineCompFormat(event), msgFolder, msgUris); } > function MsgDerermineCompFormat(aEvent) { > // easy to read than xxx?ValueForTru:ValueForFalse style, concise than if-then-else, safer than eval() > let CompFormat = {} ; > CompFormat[true] ="OppositeOfDefault" ; > CompFormat[false]="Default" ; > let ShiftModifier = (aEvent && aEvent.shiftKey) ; > return Components.interfaces.nsIMsgCompFormat [ CompFormat[ShiftModifier] ] ; i.e. If I create patch, I want to do excess changes which may produce regression in other components :-) And, I don't want to decide about function name, label string, etc. Further, I don't know "how to create patch for Mozilla family" :-) Therefore, I can't create patch for this bug. Thomas D., UI part of Bug 731688(add Shift support in Edit As New) and UI part of this bug(Edit draft) are same, exept used name/string/label and except "hiding menu if non-Drafts in this bug". Can you create UI patch for both?

Not yet control the menu state depends on the current folder.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12) > WIP v1 Ikezoe-san, thanks for quick WIP v1. Do following change, please. (1) Inherit from "Forward in Inline" which supports Shift, in both "Edit As New" & "Edit Draft". (2) Function Name = MsgEditDraftMessage instead of MsgComposeDraftMessage, for consistency with cmd_editDraftMsg, (3) Add "Hide editDraft menu if not Drafts" code to onpopupshowing event handler.

(In reply to WADA from comment #13) > Ikezoe-san, thanks for quick WIP v1. > Do following change, please. > (1) Inherit from "Forward in Inline" which supports Shift, in both "Edit As > New" & "Edit Draft". > (2) Function Name = MsgEditDraftMessage instead of MsgComposeDraftMessage, > for consistency with cmd_editDraftMsg, > (3) Add "Hide editDraft menu if not Drafts" code to onpopupshowing event > handler. Feel free to modify the patch as you want.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14 > Feel free to modify the patch as you want. If replacing some patch lines, I can do it, but for no control statement part, I can't modify... Can you start with at least "inheriting from Forward in Inline" with consistent function name and commad id? And, please include control statements for added/replaced functions and two onpopupshowing modules.

FYI. Difference between "Edit As New" and "Edit draft" is composition-type = Template or Draft only at UI level. So, single MsgEditMessage(EditType,event) can be used in both command, if <command id="cmd_???" oncommand="MsgEditMessage(this.id,event)"/> is used. Further, it's applicable to any function of Msg???(event) for mail composition related menu. command_id and function name/structure was cleanly, clearly sorted out by great effort for improvements/sorting out of Reply/Forward related feature. So, if <command id="cmd_???" oncommand="MsgInvokeComposerForExistentMail(this.id,event)"/> is used, single MsgInvokeComposerForExistentMail(RequestType,event) is currently sufficient, except for "Compose New Message". Because id of command is chosen pretty well and it's abstract name of "requested feature", it's usable at any place for knowing "which feature is requested by user". This technique can be called "modified goDoCommand to support event object passing for Shift modifier". Needless to say, enhancement/improvement of goDoCommand interface is better : if goDoCommand() is goDoCommand(ParmObject) and the ParmObject is passed to invoked functions, we can freely use <command id="cmd_???" command="goDoCommand( {event:event, cmdID:this.id, p1:"xxx", ,,, } )"/>.

FYI. Because "Edit *AT* Drafts/Templates folder" and "Edit *AS* Draft/Template" is clearly isolated by bug 321355, following is possible. Edit As New at any folder = Compose new mail from this template : Don't delete edited mail, Send is possible <= bug 321355 If Templates or Drafts folder, label of menu better changed from "Edit As New"? Edit draft at Drafts = Continue mail compositiont : Originlal is deleted by save/send, Send is possible <= this bug Edit template at Templates = Modify this template : Mail is replaced by new version upon save, Send is prohibited <= Needed? Note: absolutely same as "Edit draft", except "Send is prohibited". Shift works too.

Question about ThreadPaneDoubleClick(). 1. Why ComposeMesssge() is directly called for Templates folder but Msg???() is called for other folders? 2. "When single statement in if-then-else, don't use {} in if-then-else" was coding style in Mozilla family, wasn't it? Or it was "When if/else is single line with single statement"? > http://mxr.mozilla.org/comm-central/source/mail/base/content/threadPane.js#129 > function ThreadPaneDoubleClick() > { > const nsMsgFolderFlags = Components.interfaces.nsMsgFolderFlags; > if (IsSpecialFolderSelected(nsMsgFolderFlags.Drafts, true)) { MsgComposeDraftMessage(); } <= merged to one line by me > else if(IsSpecialFolderSelected(nsMsgFolderFlags.Templates, true)) { > ComposeMessage(Components.interfaces.nsIMsgCompType.Template, ... (snip) ) > } > else { MsgOpenSelectedMessages(); } <= merged to one line by me > }

Question about SeaMonkey. No need of same change in SeaMonkey?

want to finish the patch?

(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #20) > want to finish the patch? Alright..., thanks for the pointer :) So here's the new patch, based on the groundwork of attachment 8531807 [details] [diff] [review] and attachment 8531870 [details], only better! E.g. using global command attribute forwarding instead of clumsy references to each and every menu. I tested this in my local installation and it seems to work as expected for all 3-pane use cases. Hiding "Edit Draft Message" is not yet implemented for context menu of global search results in tree-style - I'd need help on that one or we can do it in a followup bug. Caveat: Due to bug 321355, "Edit as New" command does NOT work for drafts yet, both before and after this patch.

This patch is cleaner and now also works on context menu of global search tree-style results. It also correctly shows a disabled "Edit Draft Message" menu (from the main menu) when there's no message selected (use Ctrl+Click to unselect current msg). Known issues: 1) Once "Edit Draft Message" is disabled (for menus without selected messages), it does never re-enable again, so the functionality is henceforth not available until restart of TB. I have not been able to track down the cause of this, and I'm surprised that it happens, because I think that I've tracked every corner where we have "Edit as new" and the "Edit Draft Message" command is just an exact duplicate of that. So why does it behave differently then? Fyi, "Edit as new" correctly re-enables. Any ideas? 2) The Shift modifier to compose in non-default format does not seem to have any effect in my setup (yet to test without addons). This may or may not be an error of my patch (I think not). Caveat: Due to bug 321355, "Edit as New" command does NOT work for drafts yet, both before and after this patch.

Sorry, last patch did not have one of the local changes concerning the global search context menu. Jorg, if you're available and interested, you could help us to debug this per comment 22. (In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #22) > Created attachment 8725343 [details] [diff] [review] > WIP Patch v.3 > > This patch is cleaner and now also works on context menu of global search > tree-style results. It also correctly shows a disabled "Edit Draft Message" > menu (from the main menu) when there's no message selected (use Ctrl+Click > to unselect current msg). > > Known issues: > 1) Once "Edit Draft Message" is disabled (for menus without selected > messages), it does never re-enable again, so the functionality is henceforth > not available until restart of TB. > I have not been able to track down the cause of this, and I'm surprised that > it happens, because I think that I've tracked every corner where we have > "Edit as new" and the "Edit Draft Message" command is just an exact > duplicate of that. So why does it behave differently then? Fyi, "Edit as > new" correctly re-enables. > Any ideas? > > 2) The Shift modifier to compose in non-default format does not seem to have > any effect in my setup (yet to test without addons). This may or may not be > an error of my patch (I think not). > > Caveat: Due to bug 321355, "Edit as New" command does NOT work for drafts > yet, both before and after this patch.

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #22) > Created attachment 8725343 [details] [diff] [review] > WIP Patch v.3 > > 2) The Shift modifier to compose in non-default format does not seem to have > any effect in my setup (yet to test without addons). This may or may not be > an error of my patch (I think not). Perhaps because I haven't set oncommand with direct function call on the menuitems, but was using command instead (which forwards to oncommand on the command itself), which is more correct, but perhaps it doesn't always work as expected.

Comment on attachment 8725348 [details] [diff] [review] WIP patch v.3.1 Review of attachment 8725348 [details] [diff] [review]: ----------------------------------------------------------------- Very nice, a few nits. It works and is straight forward. This should land with bug 321355 to fix up this area. Please get another review since I'm only peer in mail/components/compose/ (but I feel capable of reviewing this ;-)). Why the WIP? What else is planned? ::: mail/base/content/mailWindowOverlay.js @@ +568,5 @@ > +{ > + let hideEditDraftMsgCmd = !(gFolderDisplay.selectedMessage && gFolderDisplay.selectedMessage.folder.isSpecialFolder(nsMsgFolderFlags.Drafts, true) || > + gFolderDisplay.displayedFolder && (gFolderDisplay.displayedFolder.flags & nsMsgFolderFlags["Drafts"])); > + document.getElementById("cmd_editDraftMsg").setAttribute("hidden", hideEditDraftMsgCmd); > +} Please reformat this to more or less 80 chars per line. @@ +1955,2 @@ > { > + composeMsgByType(Components.interfaces.nsIMsgCompType.Template,event); Nit: Space after comma. @@ +1957,5 @@ > +} > + > +function MsgEditDraftMessage(event) > +{ > + composeMsgByType(Components.interfaces.nsIMsgCompType.Draft,event); Same here.

(In reply to Jorg K (GMT+1) from comment #25) > Comment on attachment 8725348 [details] [diff] [review] > WIP patch v.3.1 > > Review of attachment 8725348 [details] [diff] [review]: > ----------------------------------------------------------------- > > Very nice, a few nits. It works and is straight forward. This should land > with bug 321355 to fix up this area. Please get another review since I'm > only peer in mail/components/compose/ (but I feel capable of reviewing this > ;-)). > > Why the WIP? What else is planned? Thanks for rapid review. However, as stated, I'd appreciate your help to *debug* this patch because it is NOT working correctly. See comment 22. While we can easily postpone the shift-modifier problem to another bug, I'm not happy with this: 1) Ensure that no messages are selected in message pane of a draft folder (Ctrl+click to unselect a selected msg). 2) Main menu > Message > (look at) Edit Draft Message (correctly disabled) 3) Select a draft msg and *right-click* for context menu > (look at) Edit Draft Message (wrongly disabled). Interestingly, this only happens for exactly that sequence of steps. If you select a msg and then go to the main message menu first, the command will be correcty re-enabled for all menus. So there's a different pattern between the main message menu and the mailcontext menu, and the mailcontext menu is lacking something or doing something odd in command updating which the message menu has or does correctly. Maybe something around this setSingleSelection function which is only on the context menu.

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #26) > Thanks for rapid review. However, as stated, I'd appreciate your help to > *debug* this patch because it is NOT working correctly. See comment 22. In this case ... only f+ ;-) I'll look into the problems mentioned.

You made a good number of copy/paste mistakes ;-) (I'm still embarrassed about the r+ I gave.) Note, you don't hide commands, you hide elements of the document. Please try this. I gave it a brief test and it looks good. The interdiff will show you the changes I made.

Comment on attachment 8725788 [details] [diff] [review] WIP patch v.3.1 after JK fixed problems Review of attachment 8725788 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Jorg K (GMT+1) from comment #28) > Created attachment 8725788 [details] [diff] [review] > WIP patch v.3.1 after JK fixed problems So which problems does this actually fix? None of the problems mentioned in comment 22 anyway. > You made a good number of copy/paste mistakes ;-) No, I didn't. Every deviation was well-considered and working; only wrt to comment 22 we still need to find the reason why it fails on those edge cases. Unfortunately it's you who is introducing copy/paste issues which are not seen on my patch: JK added: > <key id="key_editDraftMsg" key="&editDraftMsgCmd.key;" > oncommand="goDoCommand('cmd_editDraftMsg')" modifiers="accel"/> > <!ENTITY editDraftMsgCmd.key "d"> This conflicts with Lightning's keyboard shortcut for "New Task" (Ctrl+D). Most other possible keyboard shortcuts are taken. Plus we don't really need nor want to add a keyboard shortcut for "Edit Draft Message" because we already have two keyboard ways for that: * Enter (!) * Alt+E (which wfm even though it seems to conflict with main menu's Alt+E for Edit menu). JK added: > <menuitem id="mailContext-editDraftMsg" > label="&contextEditDraftMsg.label;" > accesskey="&editDraftMsgCmd.accesskey;" > oncommand="MsgEditDraftMessage(event);"/> I had deliberately omitted the access key on the context menu because it conflicts with "D" access key of "Delete Message" command on context menu. All other access keys from the "Edit Draft Message" string are also taken. Conflicting access key are undesirable (although they don't break the thing). Given the easy and straightforward keyboard accessibility options above which will directly edit the selected draft message (Enter / Alt+E), plus a big [Edit...] button on an even bigger notification bar, using the context menu is a very unlikely scenario for this particular command, and should not be encouraged. Delete also has "DEL", but I'd still think it's slightly more likely to be accessed from context menu then Edit Draft, so I wouldn't want to take away or tamper with its access key. So I guess it's reasonable and acceptable in this exceptional case to omit the access key for "Edit Draft Message". Furthermore, unless changing command="cmdname" to oncommand="function..." attribute actually helps (which I don't see yet), we should keep the more correct style of using command attribute. > (I'm still embarrassed about the r+ I gave.) If there's a need to use strong words like "embarassed" at all (which also seems to suggest that my patch is so obviously pretty bad, which is not true; it's 98% working as designed), make sure you're embarrassed about the right thing. Nothing wrong with your review. More likely your patch and its presentation. > Note, you don't hide commands, you hide elements of the document. It's not about "hiding commands" as such. As documented in comment 0 and comment 21, I've correctly used an intelligent inbuilt XUL feature called "Command Attribute Forwarding" (1). All elements having the command id in their command attribute will automatically observe attribute changes broadcasted by the main <command> element. (We could even define the command label on the command only and it would be forwarded to all the observing menu elements, but I refrained from that for certain reasons). Which works like a charme, and makes for streamlined, robust and easily maintainable code. More specifically, instead of clumsily setting attributes on each and every element using hard-coded IDs... > document.getElementById("menu_editDraftMsg").setAttribute("hidden", hideEditDraftMsgCmd); > document.getElementById("mailContext-editDraftMsg").setAttribute("hidden", hideEditDraftMsgCmd); > document.getElementById("appmenu_editDraftMsg").setAttribute("hidden", hideEditDraftMsgCmd); ...we can just use a single line to change the attribute on the central command and all of the above elements which have command="cmd_editDraftMsg" attribute will observe that change automatically: > document.getElementById("cmd_editDraftMsg").setAttribute("hidden", hideEditDraftMsgCmd); Then, if we or addons later decide to add e.g. another UI element for the same command, you'll just add command="cmd_editDraftMsg" attribute on the element and it's all hooked up and behaving correctly. Same if we decide to shuffle our UI or change element IDs, just keep the command hook and you're safe. > Please try this. I gave it a brief test and it looks good. Unfortunately it just adds unnecessary and problematic code but does not fix any of the issues of comment 22 per my testing... (1) https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Broadcasters_and_Observers

Comment on attachment 8725788 [details] [diff] [review] WIP patch v.3.1 after JK fixed problems obsolete per comment 29

Comment on attachment 8725348 [details] [diff] [review] WIP patch v.3.1 restored as latest WIP patch per comment 29

Comment on attachment 8725348 [details] [diff] [review] WIP patch v.3.1 (In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #26) > (In reply to Jorg K (GMT+1) from comment #25) > I'm not happy with this: > 1) Ensure that no messages are selected in message pane of a draft folder > (Ctrl+click to unselect a selected msg). > 2) Main menu > Message > (look at) Edit Draft Message (correctly disabled) > 3) Select a draft msg and *right-click* for context menu > (look at) Edit > Draft Message (wrongly disabled). Confirmed NOT working with attachment 8725348 [details] [diff] [review]. Confirmed WORKING with MY patch attachment 8725788 [details] [diff] [review]. I don't have more to say. If you don't want my help, please DO NOT ask for it. You're wasting my time.

I should look what is going on here.

I may have messed up an access key, but you can easily fix that.

The problem you describe is this: You're using the "elegant" document.getElementById("cmd_editDraftMsg").setAttribute("hidden", hideEditDraftMsgCmd); and therefore on the context menu you need to put: <menuitem id="mailContext-editDraftMsg" label="&contextEditDraftMsg.label;" accesskey="&editDraftMsgCmd.accesskey;" command="cmd_editDraftMsg"/> <======= My working solution uses: document.getElementById("mailContext-editDraftMsg").setAttribute("hidden", hideEditDraftMsgCmd); and <menuitem id="mailContext-editDraftMsg" label="&contextEditDraftMsg.label;" accesskey="&editDraftMsgCmd.accesskey;" oncommand="MsgEditDraftMessage(event);"/> <====== Like this, your problem doesn't occur. Also, you get the event passed, since this is what you want. If you look at the other context menu items, some/most use "oncommand". So your intelligent solution doesn't work, but my dumb copying does(*). Sorry, you didn't make copy/paste mistakes, you deliberately chose a different (non-working) solution. Maybe it can be made to work. There, I helped you debug it, so now you figure out what you want to do. Please note that in my patch I fixed a few nits, like I made the "let hideEditDraftMsgCmd" readable. (*) https://en.wikiquote.org/wiki/Confucius: Man has three ways of acting wisely. First, on meditation; that is the noblest. Secondly, on imitation; that is the easiest. Thirdly, on experience; that is the bitterest. Also quoted as: Man has three ways of acting correctly. First, by thinking; that is the best. Secondly, by imitation; that is the easiest. Thirdly, by experience; that is the hardest. I went for easy ;-)

Using command is preferred if it works as you can share the code (function call) to be run. On the other hand, if you need the 'event' you must test whether the event fires on the <menuitem> or the <command> element. We have some bugs about converting to <command> globally, but I am not sure if it make sense now, when whole XUL is on the way out.

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #26) > While we can easily postpone the shift-modifier problem to another bug, I'm > not happy with this: > 1) Ensure that no messages are selected in message pane of a draft folder > (Ctrl+click to unselect a selected msg). > 2) Main menu > Message > (look at) Edit Draft Message (correctly disabled) > 3) Select a draft msg and *right-click* for context menu > (look at) Edit > Draft Message (wrongly disabled). > Interestingly, this only happens for exactly that sequence of steps. If you > select a msg and then go to the main message menu first, the command will be > correcty re-enabled for all menus. > So there's a different pattern between the main message menu and the > mailcontext menu, and the mailcontext menu is lacking something or doing > something odd in command updating which the message menu has or does > correctly. > Maybe something around this setSingleSelection function which is only on the > context menu. Precisely. That function sets the enabled/disabled only on the mailContext-editAsNew menuitems (in context menu) itself, while the menuitem is also observes the command cmd_editAsNew and their values are now conflicting. I am working on a solution. We need to update the command and but also hide the menuitem if needed (but not hide the whole command). The problem is, the command may be disabled and no msg selected, but if you right click a message (without clicking it to preview it) the menu item should be enabled (as all others are). So maybe that is why the mailContext-editAsNew was NOT observing a command. So we could do that for editDraftMsg too.

This should fix it. I added the comments to say why we can't use the command for the context menu.

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #22) > 2) The Shift modifier to compose in non-default format does not seem to have > any effect in my setup (yet to test without addons). This may or may not be > an error of my patch (I think not). This also doesn't work for the original Edit as new message. It's probably because the goDoCommand() (in the declaration of the command) does not propagate event. Anyway this should be pursued in another bug, it is a separate feature.

Comment on attachment 8728144 [details] [diff] [review] patch v3.2 Review of attachment 8728144 [details] [diff] [review]: ----------------------------------------------------------------- Item not hidden from context menu. (Or am I missing something.) ::: mail/base/content/mailContextMenus.js @@ +77,5 @@ > goUpdateCommand('cmd_print'); > > updateCheckedStateForIgnoreAndWatchThreadCmds(); > > + // Hide "Edit Draft Message" menus if we're not in a draft folder Nit. Full stop missing. ::: mail/base/content/mailWindowOverlay.js @@ +478,5 @@ > // Disable the Tag menu item if no message is selected or when we're > // not in a folder. > document.getElementById("tagMenu").disabled = !messageStoredInternally; > > + // Hide "Edit Draft Message" menus if we're not in a draft folder Ditto. @@ +539,5 @@ > // Disable the Tag menu item if no message is selected or when we're > // not in a folder. > document.getElementById("appmenu_tagMenu").disabled = !messageStoredInternally; > > + // Hide "Edit Draft Message" menus if we're not in a draft folder Ditto. @@ +573,5 @@ > + let folder = gFolderDisplay.displayedFolder; > + let inDraftFolder = (msg && > + msg.folder.isSpecialFolder(nsMsgFolderFlags.Drafts, true)) || > + (folder && folder.getFlag(nsMsgFolderFlags.Drafts)); > + document.getElementById("cmd_editDraftMsg").setAttribute("hidden", !inDraftFolder); This is Thomas' original approach of hiding the command. Sadly, since the context menu doesn't use the command, it's not hidden from the context menu in a non-drafts folder. ::: mail/base/content/mailWindowOverlay.xul @@ +732,5 @@ > <menuitem id="mailContext-multiForwardAsAttachment" > label="&contextMultiForwardAsAttachment.label;" > accesskey="&contextMultiForwardAsAttachment.accesskey;" > oncommand="MsgForwardAsAttachment(event);"/> > + <!-- These 2 edit items intentionally do not reference the cmd_edit* commands. --> Nit. Please s/These 2/The following two/. Maybe you can explain the intention a little. @@ +739,5 @@ > accesskey="&contextEditMsgAsNew.accesskey;" > + oncommand="MsgEditMessageAsNew(event);"/> > + <menuitem id="mailContext-editDraftMsg" > + label="&contextEditDraftMsg.label;" > + oncommand="MsgEditDraftMessage(event);"/> As the comment says, you don't use cmd_editDraftMsg here. I think we already established in comment #35 that consequently the item is not hidden from the context menu in a non-draft folder. ::: mail/base/content/nsContextMenu.js @@ +295,5 @@ > > this.setSingleSelection("mailContext-replySender"); > + // These 2 items are not hooked to their commands because we need to enable > + // them even in some cases there is no real selected message yet, > + // only a rightclick on a message. Nit. Please s/These 2/The following two/

Good catch, thanks.

Comment on attachment 8728533 [details] [diff] [review] patch v3.3 Review of attachment 8728533 [details] [diff] [review]: ----------------------------------------------------------------- Nice. I think cmd_editAsNew is never hidden. Also, as a nit, I prefer to move the comment to were it makes more sense. Sorry, German pedant. ::: mail/base/content/mailWindowOverlay.xul @@ +732,5 @@ > <menuitem id="mailContext-multiForwardAsAttachment" > label="&contextMultiForwardAsAttachment.label;" > accesskey="&contextMultiForwardAsAttachment.accesskey;" > oncommand="MsgForwardAsAttachment(event);"/> > + <!-- The following two edit items intentionally do not reference the cmd_edit* commands. --> I still think the intent would be better explained by moving the comment below up here. ::: mail/base/content/nsContextMenu.js @@ +295,5 @@ > > this.setSingleSelection("mailContext-replySender"); > + // The following two items are not hooked to their commands because we need > + // to enable them even in some cases there is no real selected message yet, > + // only a rightclick on a message. I think this comment should go up above. @@ +298,5 @@ > + // to enable them even in some cases there is no real selected message yet, > + // only a rightclick on a message. > + this.setSingleSelection("mailContext-editAsNew", > + document.getElementById("cmd_editAsNew") > + .getAttribute("hidden") != "true"); I trust that you know best what do to here. !document.getElementById("cmd_editAsNew") ... won't work? Oh, actually, why do you have this? "Edit as new" is never hidden. Please clarify or remove. If you remove it, the comment above needs to change. @@ +301,5 @@ > + document.getElementById("cmd_editAsNew") > + .getAttribute("hidden") != "true"); > + this.setSingleSelection("mailContext-editDraftMsg", > + document.getElementById("cmd_editDraftMsg") > + .getAttribute("hidden") != "true"); This one is the one you need. @@ +739,5 @@ > * enabled if there's exactly one selected. Handle those here. > * Exception: playable media is selected, in which case, don't show them. > * > * @param aID the id of the element to display/enable > + * @param aShow (optional) an additional criteria to evaluate when we Nice clean-up here.

(In reply to Jorg K (GMT+1) from comment #42) > ::: mail/base/content/nsContextMenu.js > @@ +295,5 @@ > > > > this.setSingleSelection("mailContext-replySender"); > > + // The following two items are not hooked to their commands because we need > > + // to enable them even in some cases there is no real selected message yet, > > + // only a rightclick on a message. > > I think this comment should go up above. Ok. > > @@ +298,5 @@ > > + // to enable them even in some cases there is no real selected message yet, > > + // only a rightclick on a message. > > + this.setSingleSelection("mailContext-editAsNew", > > + document.getElementById("cmd_editAsNew") > > + .getAttribute("hidden") != "true"); > > I trust that you know best what do to here. > !document.getElementById("cmd_editAsNew") ... won't work? Just !document.getElementById("cmd_editAsNew") can't work. If you meant document.getElementById("cmd_editAsNew").hidden, then that could maybe work, but we set the state via attribute so I check it back via an attribute (not property). > Oh, actually, why do you have this? "Edit as new" is never hidden. > > Please clarify or remove. If you remove it, the comment above needs to > change. My pedantism :) What if we need to hide it in the future? Let's keep the infrastructure on both commands the same. > @@ +739,5 @@ > > * enabled if there's exactly one selected. Handle those here. > > * Exception: playable media is selected, in which case, don't show them. > > * > > * @param aID the id of the element to display/enable > > + * @param aShow (optional) an additional criteria to evaluate when we > > Nice clean-up here. Thanks.

(In reply to :aceman from comment #43) > > I trust that you know best what do to here. > > !document.getElementById("cmd_editAsNew") ... won't work? You didn't see the ... I didn't want to copy the whole thing. I meant: ! document.getElementById("cmd_editAsNew").getAttribute("hidden") But the attribute comes back as a string, OK, then that's fine. > My pedantism :) What if we need to hide it in the future? Let's keep the > infrastructure on both commands the same. Well, I'm not convinced. If ever we want to hide it in the future, we will do so. And the "edit draft" can serve as model. Please, let's not half-implement something with the view to do something else later. "Edit as new" was never hidden, and I don't see it coming. Why keep the infrastructure for two very different things the same. "Edit draft" only applies to draft folders, so it needs special treatment. People will get confused and look for the hiding of the command and not see it.

(In reply to Jorg K (GMT+1) from comment #45) > > My pedantism :) What if we need to hide it in the future? Let's keep the > > infrastructure on both commands the same. > Well, I'm not convinced. If ever we want to hide it in the future, we will > do so. And the "edit draft" can serve as model. Please, let's not > half-implement something with the view to do something else later. "Edit as > new" was never hidden, and I don't see it coming. Maybe there could be accounts where the items displayed are no messages so "edit as new" makes no sense? Like the IM accounts. Fortunately, those do not use the main 3pane view but their own tab and commands. > People will get confused and look for the hiding of the command and not see > it. Yes, I can imagine this :)

Comment on attachment 8729836 [details] [diff] [review] patch v3.5 Review of attachment 8729836 [details] [diff] [review]: ----------------------------------------------------------------- Perfect. Thanks to all involved, especially to Thomas for getting the ball rolling to clean up this area. Note that bug 321355 is also ready to land when current bustage is resolved (in fact, it was landed on Aurora already).

Thanks :) This could use porting to Seamonkey later.

Is there any reason this is not landed on comm-aurora? It sounds like a regression that we need for TB45.

To edit a draft, you double click it or use the big "Edit" button in the notification area. Having this on the menus is nice but not necessary. Certainly not a regression. The bug has some string changes, so no uplift here.

https://hg.mozilla.org/comm-central/rev/e2845f0b82eb5e02a580d3d29e57dd84e36c21c2 Bug 1106412 - Implement Edit Draft Message Command. r=jorgk