Closed Bug 462684 Opened 11 years ago Closed 11 years ago

Combine messagePaneContext and threadPaneContext

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jminta, Assigned: jminta)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch patch v1 (obsolete) — Splinter Review
messagePaneContext and threadPaneContext are about 85% the same, and the xul duplication not only adds unnecessary nodes to the DOM, it creates extra code duplication. This patch combines to two menus into 1.
Attachment #345887 - Flags: review?(mkmelin+mozilla)
With this patch a lot of menus that are shown when they shouldn't. E.g. Report scam for the thread pane, Compose Mail To, Add to Address Book... and probably some more. 

Also, I think it will be quite confusing if the ids still say threadpaneContext when it's not only that...
Attached patch patch v2 (obsolete) — Splinter Review
This fixes a couple of bugs that caused things to remain showing when they shouldn't have. Note that I haven't changed the IDs here, because there are a variety of other places that depend on these IDs, so I think it'd be better to do that in a second patch.
Attachment #345887 - Attachment is obsolete: true
Attachment #345940 - Flags: review?(mkmelin+mozilla)
Attachment #345887 - Flags: review?(mkmelin+mozilla)
Comment on attachment 345940 [details] [diff] [review]
patch v2

> +  // don't show mail items for links/images, just show related items.

Capital D.

>+  // Most menu items are visible if there's 1 or 0 messages selected, and
>+  // enabled if there's exactly one selected. Handle those here.
>+ function setSingleSelection(aID, aOptional) {
>+    var optional = aOptional != undefined ? aOptional : true;
>+    ShowMenuItem(aID, single && !hideMailItems && optional);
>+    EnableMenuItem(aID, single);
>+  }

Doxygen/javadoc style for the function doc would be nice. Especially a @param comment about what the aOptional should do. I'm not too crazy about aParam names for javascript either.

>+  // handle our separators
>+  function hideIfAppropriate(aID) {
>+    var separator = document.getElementById(aID);
>+    var sibling = separator.previousSibling;
>+    while (sibling) {
>+      if (sibling.getAttribute("hidden") != "true") {
>+        if (sibling.localName != "menuseparator" &&
>+            hasAVisibleNextSibling(separator)) {
>+          ShowMenuItem(aID, true);
>+        } else {
>+          ShowMenuItem(aID, false);
>+        }

Wrong braces style (here, and a few other places). But then again you don't need any braces at all here. 
>-  if(item && item.hidden != "true") 
>+  if (item.hidden != "true") 
>     item.hidden = !showItem;

Trailing space, and i'd think you don't have to explicitly compare to "true" here

---

Then a few functional comments:

E.g. for a HTML message, right click image, or link

 Select ALl
 Copy [disabled]
 ----
 ----
 Copy Image Location

I also get a disabled "Copy" item shown a lot.

In the standalone msg window I seem to get no context menu at all. 

As much as it sucks to change ids, I'd prefer making the id names sane now (a few months after they landed) rather than later. Maybe threadPane/messagePaneContext-XXX -> mailContext-XXX or something.
Attachment #345940 - Flags: review?(mkmelin+mozilla) → review-
Attached patch patch v3 (obsolete) — Splinter Review
Addresses previous review comments. (For the record, I'm much in favor of "} else {" style, but you're right, it wasn't necessary here.)
Attachment #345940 - Attachment is obsolete: true
Attachment #347086 - Flags: review?(mkmelin+mozilla)
Comment on attachment 347086 [details] [diff] [review]
patch v3

For multiple selection, fwd as attachments is missing. (Also Get Selected Messages, but i guess that's intentional, as it was grayed out earlier - didn't investigate furhter.)

Html msg img/link context menu, still get the (double) extra separator.

> +  var enabled = (item.getAttribute('disabled') !='true');

Missing space.

> + function setSingleSelection(aID, aOptional) {
> +    var optional = aOptional != undefined ? aOptional : true;

Still don't quite understand what this param implies. I suggest you rename it to something that says what it really does. 

> +  var enabled = (item.getAttribute('disabled') !='true');
> +  if (enableItem != enabled) {
> +    item.setAttribute('disabled', enableItem ? '' : 'true');

item.disabled = !enableItem; ?

I also got an exception "GetThreadTree is not defined" while I was clicking around, in news? (I assume it's due to this patch.)
Attachment #347086 - Flags: review?(mkmelin+mozilla) → review-
Attached patch patch v4Splinter Review
Addresses comments from previous review.
Attachment #347086 - Attachment is obsolete: true
Attachment #348204 - Flags: review?(mkmelin+mozilla)
Comment on attachment 348204 [details] [diff] [review]
patch v4

Looks good! r=mkmelin

One minor nit, add a period:
> +  // Copy is available as long as something is selected
Attachment #348204 - Flags: review?(mkmelin+mozilla) → review+
Blocks: TB2SM
Comment on attachment 348204 [details] [diff] [review]
patch v4

>diff -r 9413bd795016 mail/base/content/ABSearchDialog.xul
>--- a/mail/base/content/ABSearchDialog.xul	Fri Nov 14 15:05:50 2008 +0100
>+++ b/mail/base/content/ABSearchDialog.xul	Fri Nov 14 11:51:47 2008 -0500
>@@ -105,17 +105,17 @@
>       </hbox>
>     </vbox>
> 
>     <splitter id="gray_horizontal_splitter" collapse="after" persist="state"/>
> 
>     <vbox id="searchResults" flex="4" persist="height">
>       <vbox id="searchResultListBox" flex="1" >
>         <tree id="abResultsTree" flex="1" enableColumnDrag="true" class="plain"
>-              context="threadPaneContext"
>+              context="mailContext"
As this has a tree id="abResultsTree" instead of "threadTree" is this going to cause a problem with your new code?
>               onclick="AbResultsPaneOnClick(event);"
>               onkeypress="AbResultsPaneKeyPress(event);"
>               onselect="this.view.selectionChanged();"
>               sortCol="GeneratedName"
>               persist="sortCol">
> 
>diff -r 9413bd795016 mail/base/content/mailContextMenus.js

>+  // Select-all and copy are only available in the message-pane
>+  if (inThreadPane) {
>+    document.getElementById("mailContext-selectall").hidden = true;
>+    document.getElementById("mailContext-copy").hidden = true;
>+  }
>+  ShowMenuItem("threadPaneContet-openNewTab", inThreadPane);
Typo? Contet? Correct here and in the xul whilst you're touching the files?

>diff -r 9413bd795016 mail/base/content/mailWindowOverlay.xul

>     <menuitem id="threadPaneContet-openNewTab"
>               label="&contextOpenNewTab.label;"
>               accesskey="&contextOpenNewTab.accesskey;"
>               oncommand="MsgOpenNewTabForMessage();"/>
And as mentioned on IRC, need to make sure any Lightning bustage (as they rely on threadPaneContext to overlay) is dealt with.
(In reply to comment #9)
> And as mentioned on IRC, need to make sure any Lightning bustage (as they rely
> on threadPaneContext to overlay) is dealt with.

and messagePaneContext
Fallen asked me to cc the lightning bugmail, since this will cause issues with Lightning. (I'm not sure why Lightning redefines these function rather than using addEventListener("popupshowing") to handle its overlaid elements...)
Comment on attachment 348204 [details] [diff] [review]
patch v4

>+  /**
>+   * Most menu items are visible if there's 1 or 0 messages selected, and
>+   * enabled if there's exactly one selected. Handle those here.
>+   * @param aID   the id of the element to display/enable
>+   * @param aHide (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
>+   */
>+ function setSingleSelection(aID, aHide) {
>+    var hide = aHide != undefined ? aHide : true;
>+    ShowMenuItem(aID, single && !hideMailItems && hide);
>+    EnableMenuItem(aID, single);
>+  }

To me the second (optional) argument of this function is wrong.
aHide should either be renamed or the logic changed - "If false, we'll hide the item no matter what messages are selected", the name suggests it should hide the item not show it!
So I would change it to:
>+   * Most menu items are visible if there's 1 or 0 messages selected, and
>+   * enabled if there's exactly one selected. Handle those here.
>+   * @param aID   the id of the element to display/enable
>+   * @param aHide (optional)  an additional criteria to evaluate when we
>+   *              decide whether to display the element. If true, we'll hide
>+   *              the item no matter what messages are selected
>+   */
>+ function setSingleSelection(aID, aHide) {
>+    var hide = aHide != undefined ? aHide : false;
>+    ShowMenuItem(aID, single && !hideMailItems && !hide);
>+    EnableMenuItem(aID, single);
>+  }
then change the various callers where need be.
or change aHide/hide to aShow/show
SM equivalent bug 469498
Depends on: 470509
threadPaneContet-openNewTab type fixed in 470509. (Fix caller before checking in.)
No longer depends on: 470509
Blocks: 470522
Landed as 1544:71688230bf66
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
No longer blocks: TB2SM
Blocks: 472098
Depends on: 473168
seems this change might have caused bug 473168
Depends on: 486430
You need to log in before you can comment on or make changes to this bug.