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

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Simon Paquet [:sipaq]]

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.

Comment 1

[Simon Paquet [:sipaq]]

Attachment: Patch for Thunderbird - v1

This should do the trick. Basic testing on my box shows no errors. 
I hope I did everything right for all the RDF stuff.

Comment 2

[Magnus Melin [:mkmelin]]

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!

Comment 3

[Magnus Melin [:mkmelin]]

Also traling space after key="key_getAllNewMessages" 

Comment 4

[Simon Paquet [:sipaq]]

Attachment: Patch for Thunderbird - v2

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?

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 296580 [details] [diff] [review]
Patch for Thunderbird - v2

Yup, r=mkmelin

Comment 6

[Simon Paquet [:sipaq]]

Attachment: Patch for Thunderbird - v3

Same patch as before. I just found more menuitem lacking an ID. Carrying forward mkmelin's review.

Comment 7

[Simon Paquet [:sipaq]]

Attachment: Patch for SeaMonkey - v1

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.

Comment 8

[David :Bienvenu]

Comment on attachment 296644 [details] [diff] [review]
Patch for Thunderbird - v3

thx very much, Simon!

Comment 9

[Reed Loden [:reed]]

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

Comment 10

[Simon Paquet [:sipaq]]

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.

Comment 11

[Simon Paquet [:sipaq]]

I filed bug 412252 (IDs for context-menus) and bug 412253 (IDs for mail toolbar) as followup bugs.

Comment 12

[Karsten Düsterloh]

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.

Comment 13

[Simon Paquet [:sipaq]]

Attachment: Patch for SeaMonkey - v2

Patch v2 addresses the review comments from Karsten.

Comment 14

[[e-mail]]

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.

Comment 15

[Simon Paquet [:sipaq]]

Attachment: Patch for SeaMonkey - v3

This patch fixes the small issues that Neil raised and removes all added IDs inside the templates. Carrying over Neil's sr.

Comment 16

[Karsten Düsterloh]

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!

Comment 17

[Caio Tiago Oliveira (asrail)]

This patch totally broken mailnews for me on SM.
The window itself opens with the panes, but all panes got no content.

Comment 18

[Phil Ringnalda (:philor)]

Attachment: Bustage fix (checked in)

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.

Comment 19

[Karsten Düsterloh]

Attachment: stay consistent with commands and backend by using "flagged" in ids [checked in]

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?

Comment 20

[David :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.

Comment 21

[Karsten Düsterloh]

Comment on attachment 298346 [details] [diff] [review]
stay consistent with commands and backend by using "flagged" in ids [checked in]

Landed on trunk.

Comment 23

[Simon Paquet [:sipaq]]

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.

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 296644 [details] [diff] [review]
Patch for Thunderbird - v3

approved for 1.8.1.15, a=dveditz for release-drivers

Comment 25

[Simon Paquet [:sipaq]]

Attachment: Branch version of attachment 298113 (for checkin)

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)

Comment 26

[Simon Paquet [:sipaq]]

Can someone with checkin privileges for mozilla/mail please drive this in?

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

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).

Comment 28

[Daniel Veditz [:dveditz]]

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

Comment 29

[Reed Loden [:reed]]

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