This is the SM equivalent of TB bug 462684 At the moment there is a lot of duplication between the messagePaneContext and threadPaneContext menus. The patch will combine the two context menus.

Based on the idea by Joey Minta over on bug 462684

Comment on attachment 352865 [details] [diff] [review] Patch to combine context menus v0.1 >-function threadPaneOnPopupHiding() >+function mailContextOnPopupHiding() > { >+ gContextMenu = null; > RestoreSelectionWithoutContentLoad(GetThreadTree()); What happens if we're actually right-clicking the message pane? >+ var showMailItems = inThreadPane || >+ (!gContextMenu.onImage && !gContextMenu.onLink); Can gContextMenu.onImage/onLink be set if inThreadPane is true? >+ function setSelection(aID, aEnable, aShow) { I don't like this name, SetupMenuItem would have been more descriptive. >+ var show = aShow != undefined ? aShow : aEnable; >+ ShowMenuItem(aID, showMailItems && show); >+ EnableMenuItem(aID, aEnable); Hmm, so how many disabled menuitems do we actually show? >+ ShowMenuItem("context-addemail", >+ gContextMenu.onMailtoLink && !inThreadPane); >+ ShowMenuItem("context-composeemailto", >+ gContextMenu.onMailtoLink && !inThreadPane); >+ ShowMenuItem("context-createfilterfrom", >+ gContextMenu.onMailtoLink && !inThreadPane); && !inThreadPane probably unnecessary again. >+ if (!inThreadPane && gContextMenu.onLink) And again, only this time it's !inThreadPane && >diff --git a/mailnews/base/resources/content/mailWindowOverlay.xul b/mailnews/base/resources/content/mailWindowOverlay.xul A -w extdiff might be handy here. >+ <tree id="abResultsTree" flex="1" context="mailContext"/> I was surprised to see this, I don't think this tree has a context menu. (Neither does the mail search dialog, although that's less clear why.) >diff --git a/suite/common/nsContextMenu.js b/suite/common/nsContextMenu.js Is this change so that the content menuitems get hidden for the thread pane?

Note to self gContextMenu.viewImage needs changing to gContextMenu.viewMedia due to fix for Bug 461136

(In reply to comment #2) > (From update of attachment 352865 [details] [diff] [review]) > >-function threadPaneOnPopupHiding() > >+function mailContextOnPopupHiding() > > { > >+ gContextMenu = null; > > RestoreSelectionWithoutContentLoad(GetThreadTree()); > What happens if we're actually right-clicking the message pane? As far as I can see from testing and stepping through the code, nothing detrimental. > > >+ var showMailItems = inThreadPane || > >+ (!gContextMenu.onImage && !gContextMenu.onLink); > Can gContextMenu.onImage/onLink be set if inThreadPane is true? Well they have a value, but looking for showMailItems to be true if either in ThreadPane or in MessagePane and neither on an image or a link. > > >+ function setSelection(aID, aEnable, aShow) { > I don't like this name, SetupMenuItem would have been more descriptive. Noted. > > >+ var show = aShow != undefined ? aShow : aEnable; > >+ ShowMenuItem(aID, showMailItems && show); > >+ EnableMenuItem(aID, aEnable); > Hmm, so how many disabled menuitems do we actually show? None, that I can think of, historical coding being carried forward? > > >+ ShowMenuItem("context-addemail", > >+ gContextMenu.onMailtoLink && !inThreadPane); > >+ ShowMenuItem("context-composeemailto", > >+ gContextMenu.onMailtoLink && !inThreadPane); > >+ ShowMenuItem("context-createfilterfrom", > >+ gContextMenu.onMailtoLink && !inThreadPane); > && !inThreadPane probably unnecessary again. True. > > >+ if (!inThreadPane && gContextMenu.onLink) > And again, only this time it's !inThreadPane && True. > > >diff --git a/mailnews/base/resources/content/mailWindowOverlay.xul b/mailnews/base/resources/content/mailWindowOverlay.xul > A -w extdiff might be handy here. How do you get that through hg qdiff? > > >+ <tree id="abResultsTree" flex="1" context="mailContext"/> > I was surprised to see this, I don't think this tree has a context menu. > (Neither does the mail search dialog, although that's less clear why.) I presumed it was there for a reason. > > >diff --git a/suite/common/nsContextMenu.js b/suite/common/nsContextMenu.js > Is this change so that the content menuitems get hidden for the thread pane? That is correct.

(In reply to comment #4) > (In reply to comment #2) > > (From update of attachment 352865 [details] [diff] [review] [details]) > > >+ var show = aShow != undefined ? aShow : aEnable; > > >+ ShowMenuItem(aID, showMailItems && show); > > >+ EnableMenuItem(aID, aEnable); > > Hmm, so how many disabled menuitems do we actually show? > None, that I can think of, historical coding being carried forward? For Copy To under certain circumstances For Delete/Print when right click on mailnews welcome page.

Changes since v0.1: * Rewrote setSelection to become SetupMenuItem and took out the EnableMenuItem part as most of the time we do not show disabled menu items. * For those instances where we do show a disabled menu item specifically use EnableMenuItem. * Changed viewImage to viewMedia in mailWindowOverlay.xul to fix bustage from fix to bug 461136. * Put back in checks of event.target against this. * Removed unneeded !inThreadPane checks. * Removed changes to ABSearchDialog and SearchDialog as they are not needed. * Corrected issue with CopyMessageUrl showing up for non-newsgroup accounts. * Show "Edit As New" for any number of selected messages instead of just one - it works so why hide it?

Shows the non-whitespace only changes to mailWindowOverlay.xul to help with review process.

Comment on attachment 352901 [details] [diff] [review] Exclude whitespace only changes for mailWindowOverlay.xul >+ <menuitem id="context-viewimage" >+ label="&viewImageCmd.label;" >+ accesskey="&viewImageCmd.accesskey;" >+ oncommand="gContextMenu.viewMedia();"/> Nice catch :-)

Comment on attachment 352900 [details] [diff] [review] Patch to combine context menus v0.1a Please use -p as well for diffing (or maybe add "showfunc=true" to the [diff]-section in your .hgrc). >diff --git a/mailnews/base/resources/content/mailContextMenus.js b/mailnews/base/resources/content/mailContextMenus.js >+function fillMailContextMenu(aTarget) > { >+ var inThreadPane = false; >+ var node = document.popupNode; >+ while (node && !inThreadPane) { Prevalent bracing style is "own line", please stick to that. >diff --git a/mailnews/base/resources/content/mailWindow.js b/mailnews/base/resources/content/mailWindow.js >@@ -343,19 +343,19 @@ >- //To move and copy menus in message pane context >- SetupMoveCopyMenus("messagePaneContext-copyMenu", accountManagerDataSource, folderDataSource); >- SetupMoveCopyMenus("messagePaneContext-moveMenu", accountManagerDataSource, folderDataSource); >+ //To move and copy menus in mail context >+ SetupMoveCopyMenus("mailContext-copyMenu", accountManagerDataSource, folderDataSource); >+ SetupMoveCopyMenus("mailContext-moveMenu", accountManagerDataSource, folderDataSource); Use '' for consistency here. Missing space after // throughout the function - but actually the "to..." comments in this function are pretty meaningless anyway and could be removed along with most of the empty lines. >+++ b/mailnews/base/resources/content/msgMail3PaneWindow.js > function InitializeDataSources() > { > //Setup common mailwindow stuff. > AddDataSources(); > > SetupMoveCopyMenus('goMenu', accountManagerDataSource, folderDataSource); >- >- //To threadpane move context menu >- SetupMoveCopyMenus('threadPaneContext-moveMenu', accountManagerDataSource, folderDataSource); >- >- //To threadpane copy content menu >- SetupMoveCopyMenus('threadPaneContext-copyMenu', accountManagerDataSource, folderDataSource); > } These lines can go because of mailWindow.js::AddDataSources() now handling both cases? >+++ b/suite/common/nsContextMenu.js > initMenu : function ( popup ) { > // Save menu. > this.menu = popup; > >- const xulNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; >- if ( document.popupNode.namespaceURI == xulNS ) { >- this.shouldDisplay = false; >- return; >- } [...] > setTarget : function ( node, rangeParent, rangeOffset ) { >+ const xulNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; >+ if ( node.namespaceURI == xulNS ) { >+ this.shouldDisplay = false; >+ return; >+ } Why is this needed?

(In reply to comment #9) > (From update of attachment 352900 [details] [diff] [review]) > Please use -p as well for diffing (or maybe add "showfunc=true" to the > [diff]-section in your .hgrc). > > > >diff --git a/mailnews/base/resources/content/mailContextMenus.js b/mailnews/base/resources/content/mailContextMenus.js > > Prevalent bracing style is "own line", please stick to that. Done. > > >diff --git a/mailnews/base/resources/content/mailWindow.js b/mailnews/base/resources/content/mailWindow.js > Use '' for consistency here. Missing space after // throughout the function - > but actually the "to..." comments in this function are pretty meaningless > anyway and could be removed along with most of the empty lines. Done > > >+++ b/mailnews/base/resources/content/msgMail3PaneWindow.js > >- > >- SetupMoveCopyMenus('threadPaneContext-moveMenu', accountManagerDataSource, folderDataSource); > >- SetupMoveCopyMenus('threadPaneContext-copyMenu', accountManagerDataSource, folderDataSource); > > } > > These lines can go because of mailWindow.js::AddDataSources() now handling both > cases? Yes. > > >+++ b/suite/common/nsContextMenu.js > >- const xulNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; > >- if ( document.popupNode.namespaceURI == xulNS ) { > >- this.shouldDisplay = false; > >- return; > >- } > [...] > > setTarget : function ( node, rangeParent, rangeOffset ) { > >+ const xulNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; > >+ if ( node.namespaceURI == xulNS ) { > >+ this.shouldDisplay = false; > >+ return; > >+ } > > Why is this needed? This change is so that the content menuitems get hidden for the thread pane and the context menu works properly on the xul parts of the panes (see what happens without this part of the patch).

Changes since v0.1b: * Bracing style made more consistent in mailContextMenus.js * " changed to ' and unneeded comments/empty lines removed from mailWindow.js * Diff now done with showfunc = true

> + <menu id="mailContext-moveMenu" [....] > <menupopup> > <menu label="&contextMoveCopyMsgRecentMenu.label;" > + <menu id="mailContext-copyMenu" [....] > <menupopup> > <menu label="&contextMoveCopyMsgRecentMenu.label;" Could you take this opportunity to give these <menupopup> the same IDs as Thunderbird? Thanks!

Comment on attachment 354743 [details] [diff] [review] Patch to combine context menus v0.1b >diff --git a/mailnews/base/resources/content/mailContextMenus.js b/mailnews/base/resources/content/mailContextMenus.js >-function threadPaneOnPopupHiding() >+function mailContextOnPopupHiding() Functions in this file are supposed to start with an uppercase letter. >+function fillMailContextMenu(aTarget) I understand that Lightning replaces this function(!) with it's own variant, calling back onto this one, introduced by a patch of a certain individual ;-) in bug 472098, following the pattern Lightning used before TB had a combined context menu - but that's an extremely whacky and fragile (and lazy!) way of overlaying. :( And should be fixed! Maybe this is the right time to make custom additions to the context menu easier: - assume several addons/overlays use the JS-function-overlay pattern Lightning uses now - this means they don't have control over the order of execution of all these JS-function-overlays anyway - so we should just make FillMailContextMenu(!) call eg. nsIObserverService.notifyObservers(..., "mailnews:fillmailcontextmenu", ...) - interested parties should register a nsIObserver for that That's what nsIObserverService was made for! >+ * Handle displaying and enable status of menu items that are to do >+ * with number of messages selected here. Er ... what? :) >+ * @param aID the id of the element to display/enable >+ * @param aShow (optional) an additional criteria to evaluate when we >+ * decide whether to display the element. If false, we'll hide >+ * the item no matter what messages are selected Rogue whitespace. And sentences should start with uppercase and end dotted. >+ function SetupMenuItem(aID, aShow) >+ { >+ var show = aShow != undefined ? aShow : single; >+ ShowMenuItem(aID, showMailItems && show); >+ } This subfunction, depending upon the context (single, showMailItems), needs to be created anew each time FillMailContextMenu is run - I doubt it's worth the effort just to spare the parameters. Especially, since you use single in various places to create the aShow value. Furthermore, using 'undefined' with !== or === doesn't make much sense in the first place and could be replaced by "aShow || single" anyway. Basically, you're doing ShowMenuItem(aID, showMailItems && (aShow || single)) Why don't you define variables for "numSelected > 0" as well? All summed up, I don't think that SetupMenuItem is useful, just pass the parameters to ShowMenuItem directly. > function ShowSeparator(aSeparatorID) ... >+function SiblingHidden(aSibling, aNext) SiblingHidden should be defined before ShowSeparator (yes, I know JS doesn't need to care). >+ while (aSibling && >+ (aSibling.localName != "menuseparator" || >+ (aSibling.localName == "menuseparator" && >+ aSibling.getAttribute("hidden") == "true"))) >+ { >+ siblingID = aSibling.getAttribute("id"); >+ // For some reason, context-blockimage and context-unblockimage are not >+ // hidden on the very first time the context menu is invoked. They are only >+ // hidden on subsequent triggers of the context menu. Since we're not >+ // using these two menuitems in mailnews, we can ignore them if encountered. >+ if ((aSibling.getAttribute("hidden") != "true") && Do you need all these getAttribute calls at all? hidden and id should be XUL element properties. > function CopyMessageUrl() > { >- try { >+ try >+ { > var hdr = gDBView.hdrForFirstSelectedMessage; > var server = hdr.folder.server; > > var url = (server.socketType == Components.interfaces.nsIMsgIncomingServer.useSSL) ? > "snews://" : "news://"; > url += server.hostName; > url += ":"; > url += server.port; > url += "/"; > url += hdr.messageId; While you're at, kill all these useless += as well. >diff --git a/mailnews/base/resources/content/mailWindowOverlay.xul b/mailnews/base/resources/content/mailWindowOverlay.xul >+ <popup id="mailContext" >+ onpopupshowing="if (event.target != this) return true; >+ return f" return event.target != this || FillMailContextMenu(this); >+ <menuitem id="context-openlink" Basically, *all* these menuitems should have an id. >+ <menuitem id="mailContext-saveAs" >+ label="&contextSaveAs.label;" >+ accesskey="&contextSaveAs.accesskey;" >+ oncommand="MsgSaveAsFile();"/> >+ <menuitem id="mailContext-printpreview" >+ label="&contextPrintPreview.label;" >+ accesskey="&contextPrintPreview.accesskey;" >+ oncommand="PrintEnginePrintPreview();"/> >+ <menuitem id="mailContext-print" >+ label="&contextPrint.label;" >+ accesskey="&contextPrint.accesskey;" >+ oncommand="PrintEnginePrint();"/> >+ <menuitem id="mailContext-delete" >+ command="cmd_delete"/> This order is odd - delete should follow saveAs. >diff --git a/mailnews/base/resources/content/messageWindow.xul b/mailnews/base/resources/content/messageWindow.xul >+ <browser id="messagepane" context="mailContext" > tooltip="aHTMLTooltip" > style="height: 0px; min-height: 1px" flex="1" name="messagepane" > disablesecurity="true" disablehistory="true" type="content-primary" > onresize="return messagePaneOnResize(event);" autofind="false" > src="about:blank" onclick="return messagePaneOnClick(event);"/> Feel free to rearrange the attributes to "one line per attribute" style. >diff --git a/mailnews/base/resources/content/messenger.xul b/mailnews/base/resources/content/messenger.xul > <browser id="messagepane" name="messagepane" height="0" flex="1" > tooltip="aHTMLTooltip" >- context="messagePaneContext" minwidth="1" minheight="1" >+ context="mailContext" minwidth="1" minheight="1" > disablesecurity="true" disablehistory="true" autofind="false" > onresize="return messagePaneOnResize(event);" > type="content-primary" onclick="return messagePaneOnClick(event);"/> Feel free to rearrange the attributes to "one line per attribute" style.

Changes since v0.1b: * Removed SetupMenuItem function * Added ids to all menuitems * Upper cased first letter of functions * Revised order of Move, Copy, Save As and Delete menu items * Now do a notifyObserver for FillMailContextMenu * Various other changes suggested by reviewer

Changes since v0.1c: * Remove notifyObservers part as now using event listener for popupshowing.

Comment on attachment 357430 [details] [diff] [review] Patch to combine context menus patch v0.1d >+++ b/mailnews/base/resources/content/messageWindow.xul >+ <browser id="messagepane" ... >+ onresize="return messagePaneOnResize(event);" >+ autofind="false" >+ src="about:blank" >+ onclick="return messagePaneOnClick(event);"/> All event handler attributes should go last. >+++ b/mailnews/base/resources/content/messenger.xul >+ <browser id="messagepane" ... > onresize="return messagePaneOnResize(event);" >- type="content-primary" onclick="return messagePaneOnClick(event);"/> >+ type="content-primary" >+ onclick="return messagePaneOnClick(event);"/> Ditto. r=me with that.

Changes since v0.1d: * Moved event handler attributes as suggested. Carrying forward r= and requesting sr.

Comment on attachment 352901 [details] [diff] [review] Exclude whitespace only changes for mailWindowOverlay.xul Any chance of a whitespace diff but with mailContext moved down to where messagePaneContext used to be? (Also if this reduces the size of the main patch then move it there too.)

Comment on attachment 358604 [details] [diff] [review] Patch to combine context menus patch v0.1e >-function threadPaneOnPopupHiding() >+function MailContextOnPopupHiding() > { >+ gContextMenu = null; > RestoreSelectionWithoutContentLoad(GetThreadTree()); > } So, STR for the regression as suggested earlier: 1. Open a folder for a server that you haven't seen yet this session, so that your mail start page loads. (You do have a start page, don't you?) 2. Tab to or scroll the thread pane, so that the stupid toolkit tree code sets the current index to a random thread. 3. Right-click the message pane and select all. Result: "current" message loads. This already happened for the thread pane with the switch to toolkit, but now it happens with the message pane context menu, which is nastier. >+function SiblingHidden(aSibling, aNext) >+{ >+ var siblingID; >+ while (aSibling && >+ (aSibling.localName != "menuseparator" || >+ (aSibling.localName == "menuseparator" && >+ aSibling.hidden))) So what's the deal with the hidden test? As far as I can see it should make no difference whether the separator is hidden. >+ tooltip="aHTMLTooltip" >+ style="height: 0px; min-height: 1px" >+ flex="1" >+ name="messagepane" >+ disablesecurity="true" >+ disablehistory="true" >+ type="content-primary" Trailing spaces crept in on three of these lines. Whose idea was it to move the tag and mark menus?

Comment on attachment 358604 [details] [diff] [review] Patch to combine context menus patch v0.1e Note: function modified slightly to ignore the image blocker menuitems, since they're not directly relevant to this comment. >+function SiblingHidden(aSibling, aNext) >+{ >+ while (aSibling && >+ (aSibling.localName != "menuseparator" || >+ (aSibling.localName == "menuseparator" && >+ aSibling.hidden))) >+ { >+ if (!aSibling.hidden) >+ return false; >+ aSibling = aNext ? aSibling.nextSibling : aSibling.previousSibling; > } >+ return true; > } OK, so I found this was really confusing, so let's turn it into an infinite loop to make it a bit clearer: >+function SiblingHidden(aSibling, aNext) >+{ >+ for (;;) { >+ if (!aSibling) >+ break; >+ if (aSibling.localName == "menuseparator" && !aSibling.hidden) >+ break; >+ if (!aSibling.hidden) >+ return false; >+ aSibling = aNext ? aSibling.nextSibling : aSibling.previousSibling; > } >+ return true; > } First, let's turn the second break; into return true; since that's where it breaks to. Better still, turn it into return aSibling.localName == "menuseparator"; - we'll see why in a sec. Now, let's turn the return false; into return aSibling.localName == "menuseparator"; - we can do this because if it was a visible separator we would have returned true by now. This now gives us: >+function SiblingHidden(aSibling, aNext) >+{ >+ for (;;) { >+ if (!aSibling) >+ break; >+ if (aSibling.localName == "menuseparator" && !aSibling.hidden) >+ return aSibling.localName == "menuseparator"; >+ if (!aSibling.hidden) >+ return aSibling.localName == "menuseparator"; >+ aSibling = aNext ? aSibling.nextSibling : aSibling.previousSibling; > } >+ return true; > } Now, since both returns are returning the same computation, we can combine the if statements. In fact, the first is a special case of the second, so it's completely unnecessary. We then finish by converting the infinite loop back into a while loop, et voilà! (with imageblocking restored): function ShouldHideSeparator(aSibling, aNext) { while (aSibling) { siblingID = aSibling.id; if (!aSibling.hidden && (siblingID != "context-blockimage") && (siblingID != "context-unblockimage")) return aSibling.localName == "menuseparator"; aSibling = aNext ? aSibling.nextSibling : aSibling.previousSibling; } return true; } Much better don't you think? :-)

Except for the internal bracing. ;-) *hide*

(In reply to comment #21) > Except for the internal bracing. ;-) *hide* I overlooked that, because the code used too many lines for the condition ;-) while (aSibling) {

This excludes whitespace changes when mailContext code is put after folderPaneContext.

Changes since v0.1e: * Made suggested improvements to function SiblingHidden. * Removed trailing spaces and tidied up <browser> parts of messenger.xul and messageWindow.xul. * function MailContextOnPopupHiding only calls RestoreSelectionWithoutContentLoad when in the threadPane. * mailContext moved down to where messagePaneContext used to be in mailWindowOverlay.xul. * correctly name copy and selectall items and only have selectall appear in message pane when a single message is selected.

-ShowMenuItem("mailContext-openNewWindow", showMailItems && single); +ShowMenuItem("mailContext-openNewWindow", inThreadPane && single); Above change made locally, question is do we want to hide if more than one message is selected, we do pre-patch.

(In reply to comment #25) > -ShowMenuItem("mailContext-openNewWindow", showMailItems && single); > +ShowMenuItem("mailContext-openNewWindow", inThreadPane && single); > > Above change made locally, question is do we want to hide if more than one > message is selected, we do pre-patch. Yes, because MsgOpenNewWindowForMessage can only open one message at a time.

Comment on attachment 359167 [details] [diff] [review] Patch to combine context menus patch v0.1f (Checked in: Comment 27) http://hg.mozilla.org/comm-central/rev/f61570d094c5