Closed Bug 411481 Opened 12 years ago Closed 12 years ago

Make it easier for extensions to overlay the TB menubar by adding IDs

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3

People

(Reporter: sipaq, Assigned: sipaq)

References

()

Details

(Keywords: fixed1.8.1.15)

Attachments

(5 files, 4 obsolete files)

The Thunderbird menubar is currently very hard to extend for extensions in some places, because its items lack IDs in some places. In consequence it is very hard to add items to the menubar or hide items from TB and one has to hack around this issue. 

See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/calendar/base/content/today-pane.js&rev=1.11&mark=93-112#92 for an example of such a hack.

As of today (revision 1.230 of mailWindowOverlay.xul on trunk) the following items miss an ID:

<menuseparator>
Lines 1163, 1174, 1186, 1196, 1214, 1228, 1231, 1236, 1251, 1256, 1261, 1268, 1273, 1315, 1328, 1365, 1368, 1381, 1400, 1402, 1441, 1449, 1455, 1533, 1566, 1587, 1646, 1654, 1675, 1715, 1716, 1730, 1737, 1751, 1789, 1798, 1844

<spacer>
Line 1855

<menu>
Lines 1461, 1828, 1830

<menupopup>
Lines 1176, 1194, 1225, 1248, 1258, 1270, 1321, 1335, 1350, 1395, 1411, 1444, 1462, 1464, 1476, 1520, 1537, 1564, 1584, 1625, 1652, 1672, 1722

<menuitem>
Lines 1181, 1195, 1229, 1230, 1232, 1233, 1259, 1450, 1521, 1522, 1565, 1585, 1653, 1673, 1746

There are also <rule> and <template> items without an ID and <menu>, <menupopup> and <menuitem> items within these <rule> and <template> items, which do not contain an idea. But since I really do not understand this RDF stuff, I left these out of the numbers above.
Summary: Make it easier for extension to overlay the TB menubar by adding IDs → Make it easier for extensions to overlay the TB menubar by adding IDs
Attached patch Patch for Thunderbird - v1 (obsolete) — Splinter Review
This should do the trick. Basic testing on my box shows no errors. 
I hope I did everything right for all the RDF stuff.
Attachment #296432 - Flags: superreview?
Attachment #296432 - Flags: review?(mkmelin+mozilla)
Attachment #296432 - Flags: superreview? → superreview?(bienvenu)
Yeah, I can't really judge the rdf stuff my either...

Is there a reason you don't use consistent case for the id naming? I think you should. Everything else  in the file seems lowerCamelCase, so why not these too? Except for one place you do use lowerCC, and one place the <menuseparator id="menu_HelpAboutSeparator"/>

Also, remove the trailing space after <menuitem id="menu_getAllNewMsg" label="&getAllNewMsgCmd.label;"

Other than that, looks good!
Also traling space after key="key_getAllNewMessages" 
Attached patch Patch for Thunderbird - v2 (obsolete) — Splinter Review
This patch addresses the review comments from Magnus. I removed the two trailing spaces and used lowerCamelCase for all the new IDs.

Magnus, am I correct in assuming that your "Other than that, looks good!" means r=mkmelin?
Attachment #296432 - Attachment is obsolete: true
Attachment #296580 - Flags: superreview?(bienvenu)
Attachment #296580 - Flags: review?(mkmelin+mozilla)
Attachment #296432 - Flags: superreview?(bienvenu)
Attachment #296432 - Flags: review?(mkmelin+mozilla)
Comment on attachment 296580 [details] [diff] [review]
Patch for Thunderbird - v2

Yup, r=mkmelin
Attachment #296580 - Flags: review?(mkmelin+mozilla) → review+
Same patch as before. I just found more menuitem lacking an ID. Carrying forward mkmelin's review.
Attachment #296580 - Attachment is obsolete: true
Attachment #296644 - Flags: superreview?(bienvenu)
Attachment #296644 - Flags: review+
Attachment #296580 - Flags: superreview?(bienvenu)
Attached patch Patch for SeaMonkey - v1 (obsolete) — Splinter Review
Since Karsten asked so nicely in bug 405303, here's the same patch for SeaMonkey. It's mostly copy&paste from the other patch.

I'm not familiar with SM's review requirements. Karsten, if a super-review is needed, please request it from the appropriate super-reviewer for SM code.
Attachment #296645 - Flags: review?(mnyromyr)
Comment on attachment 296644 [details] [diff] [review]
Patch for Thunderbird - v3

thx very much, Simon!
Attachment #296644 - Flags: superreview?(bienvenu) → superreview+
Keywords: checkin-needed
Checking in mail/base/content/mailWindowOverlay.xul;
/cvsroot/mozilla/mail/base/content/mailWindowOverlay.xul,v  <--  mailWindowOverlay.xul
new revision: 1.232; previous revision: 1.231
done
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 3
This bug is now FIXED thanks to Reed's checkin.

Whether the SM crowd takes my patch or not does not concern the status of this bug.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I filed bug 412252 (IDs for context-menus) and bug 412253 (IDs for mail toolbar) as followup bugs.
Comment on attachment 296645 [details] [diff] [review]
Patch for SeaMonkey - v1

One bug, plus a few nits.

>Index: mailnews/base/resources/content/mailWindowOverlay.xul
>===================================================================
>+           <menuitem id="menu_settingsOffline" label="&settingsOfflineCmd.label;"
>+                     accesskey="&settingsOfflineCmd.accesskey;"
>+                     command="cmd_settingsOffline"/>
>+           <menuseparator id="offlineMenuAfterSettingsSeparator"/>
>+           <menuitem id="menu_downloadStarred" label="&downloadStarredCmd.label;"
>+                      accesskey="&downloadStarredCmd.accesskey;"
>+                      command="cmd_downloadFlagged"/>
>+           <menuitem id="menu_downloadSelected" label="&downloadSelectedCmd.label;"
>+                     accesskey="&downloadSelectedCmd.accesskey;"
>+                     command="cmd_downloadSelected"/>

Put the labels on its own line, too, and make sure the attributes are aligned vertically (as in the first and third above).

Ditto on some lines below: if you're touching the menuitem anyway, please adhere to the one-line-per-attribute meme.

>-              <menupopup>
>-                <menuitem label="&fileHereMenu.label;" accesskey="&fileHereMenu.accesskey;"
>-                  oncommand="MsgMoveMessage(event.target.parentNode.parentNode)"/>
>-                <menuseparator/>
>+              <menupopup id="menu_fileHerePopup2">
>+                <menuitem id="menu_fileHere2" label="&fileHereMenu.label;"
>+                          accesskey="&fileHereMenu.accesskey;"
>+                          oncommand="MsgMoveMessage(event.target.parentNode.parentNode.id)"/>

This is wrong, SM still uses the node as the parameter.

>-              <menupopup>
>-                <menuitem label="&copyHereMenu.label;" accesskey="&copyHereMenu.accesskey;"
>-                  oncommand="MsgCopyMessage(event.target.parentNode.parentNode)"/>
>-                <menuseparator/>
>+              <menupopup id="menu_copyHerePopup2">
>+                <menuitem id="menu_copyHere2" label="&copyHereMenu.label;"
>+                          accesskey="&copyHereMenu.accesskey;"
>+                          oncommand="MsgCopyMessage(event.target.parentNode.parentNode.id)"/>

And here again.
Attachment #296645 - Flags: review?(mnyromyr) → review-
Attached patch Patch for SeaMonkey - v2 (obsolete) — Splinter Review
Patch v2 addresses the review comments from Karsten.
Attachment #296645 - Attachment is obsolete: true
Attachment #298031 - Flags: superreview?(neil)
Attachment #298031 - Flags: review?(mnyromyr)
Comment on attachment 298031 [details] [diff] [review]
Patch for SeaMonkey - v2

Bah, too many people with bugzilla@ email addresses :-P

>         <menuitem id="context-openlinkintab"
>-	          label="&openLinkCmdInTab.label;"
>+            label="&openLinkCmdInTab.label;"
Oops ;-)

>+        <template>
>+        <rule nc:NoSelect="true" iscontainer="true" isempty="false">
>+          <menupopup id="menu_folderMenuPopup1">
I don't think it's worth putting ids inside templates unless you can come up with a specific use case (I can't think of one). Also your indentation is wrong.

>-<spacer flex="100%"/>
>+<spacer id="menubar_spacer" flex="100%"/>
I don't think we need this at all, particulary w.r.t. toolbar customisation.
Attachment #298031 - Flags: superreview?(neil) → superreview+
This patch fixes the small issues that Neil raised and removes all added IDs inside the templates. Carrying over Neil's sr.
Attachment #298031 - Attachment is obsolete: true
Attachment #298113 - Flags: superreview+
Attachment #298113 - Flags: review?(mnyromyr)
Attachment #298031 - Flags: review?(mnyromyr)
Comment on attachment 298113 [details] [diff] [review]
Patch for SeaMonkey - v3

>Index: mailnews/base/resources/content/mailWindowOverlay.xul
>===================================================================
>@@ -1602,13 +1683,12 @@
>             <menu uri="..."  class="folderMenuItem menu-iconic" label="rdf:http://home.netscape.com/NC-rdf#Name"
>              SpecialFolder="rdf:http://home.netscape.com/NC-rdf#SpecialFolder"
>              BiffState="rdf:http://home.netscape.com/NC-rdf#BiffState"
>              IsServer="rdf:http://home.netscape.com/NC-rdf#IsServer"
>              IsSecure="rdf:http://home.netscape.com/NC-rdf#IsSecure"
>              ServerType="rdf:http://home.netscape.com/NC-rdf#ServerType">
>-              <menupopup/>

Landed on trunk without this removal.

Many thanks for patching us, Simon!
Attachment #298113 - Flags: review?(mnyromyr) → review+
This patch totally broken mailnews for me on SM.
The window itself opens with the panes, but all panes got no content.
Checked this in so you wouldn't wind up with a busted nightly. I'm guessing that the id="menu_downloadStarred" even though the entities and the command are downloadFlagged was intentional, for the benefit of cross-app extensions.
Philor, thanks for fixing this!
Actually, for consistency's sake, I think we should keep the ids in sync with the commands and the backend, thus the *starred* ids should use *flagged* instead. David, what do you think?
Attachment #298346 - Flags: superreview?(bienvenu)
Attachment #298346 - Flags: review?(bienvenu)
Comment on attachment 298346 [details] [diff] [review]
stay consistent with commands and backend by using "flagged" in ids [checked in]

my first inclination was to keep the UI consistent with the xul (imagine I'm an extension writer, trying to find the xul for the starred function in TB...) but if SM is keeping flagged, I guess there's a higher degree of consistency if we can keep SM and TB's mailWindowOverlay.xul slightly more in sync with each other, and both in sync with the backend.
Attachment #298346 - Flags: superreview?(bienvenu)
Attachment #298346 - Flags: superreview+
Attachment #298346 - Flags: review?(bienvenu)
Attachment #298346 - Flags: review+
Comment on attachment 298346 [details] [diff] [review]
stay consistent with commands and backend by using "flagged" in ids [checked in]

Landed on trunk.
Attachment #298346 - Attachment description: stay consistent with commands and backend by using "flagged" in ids → stay consistent with commands and backend by using "flagged" in ids [checked in]
Duplicate of this bug: 380047
Comment on attachment 296644 [details] [diff] [review]
Patch for Thunderbird - v3

Asking for approval after discussion with David Ascher and Dan Mosedale.

The risk should be minimal, but the reward would be high, especially for extension developers.
Attachment #296644 - Flags: approval1.8.1.15?
Comment on attachment 296644 [details] [diff] [review]
Patch for Thunderbird - v3

approved for 1.8.1.15, a=dveditz for release-drivers
Attachment #296644 - Flags: approval1.8.1.15? → approval1.8.1.15+
Here's the branch version of attachment 298113 [details] [diff] [review] for checkin purposes. I also included the changes made in attachment 298346 [details] [diff] [review] (starred -> flagged in IDs)
Can someone with checkin privileges for mozilla/mail please drive this in?
Keywords: checkin-needed
Whiteboard: checkin-needed: 1.8 branch
Comment on attachment 317994 [details] [diff] [review]
Branch version of attachment 298113 [details] [diff] [review] (for checkin)

Requesting branch approval (this is a merger of two previous attachments, only one of which got approved).
Attachment #317994 - Flags: approval1.8.1.15?
Keywords: checkin-needed
Whiteboard: checkin-needed: 1.8 branch → [approval needed]
Comment on attachment 317994 [details] [diff] [review]
Branch version of attachment 298113 [details] [diff] [review] (for checkin)

approved for 1.8.1.15, a=dveditz for release-drivers
Attachment #317994 - Flags: approval1.8.1.15? → approval1.8.1.15+
Keywords: checkin-needed
Whiteboard: [approval needed] → ["branch version" patch needs to land on 1.8 branch]
MOZILLA_1_8_BRANCH:

Checking in mail/base/content/mailWindowOverlay.xul;
/cvsroot/mozilla/mail/base/content/mailWindowOverlay.xul,v  <--  mailWindowOverlay.xul
new revision: 1.116.2.83; previous revision: 1.116.2.82
done
Keywords: checkin-needed
Whiteboard: ["branch version" patch needs to land on 1.8 branch]
You need to log in before you can comment on or make changes to this bug.