412253 - Make it easier for extensions to overlay the TB mail toolbar by adding IDs

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Simon Paquet [:sipaq]]

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

Comment 1

[Simon Paquet [:sipaq]]

Attachment: This patch should do the trick

Note to reviewers:
The patch touches a lot of code, where he doesn't change anything. I just change the style of these code lines (e.g. just one rule per line per XUL item, 4-space-indentation, etc.). I think those changes make the code much more approachable and will be beneficial in the longer term.

The actual amount of real changes is pretty small. I'll also supply a slightly smaller uw-patch that doesn't cover pure whitespace changes.

Comment 2

[Simon Paquet [:sipaq]]

Attachment: Patch (with diff -uw) for easier review

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 318000 [details] [diff] [review]
This patch should do the trick

>Index: mail/base/content/mailWindowOverlay.xul
>+          <menupopup id="button-getMsgPopup" 

Stray space at the end

>+          <menupopup id="button-filePopup
>+                     "type="folder"

Whops.

>+      <toolbarbutton class="toolbarbutton-1"
>+                     type="menu-button"
>+                     id="button-goback"

While you're at it, move this id first too, and for all the other buttons you're touching.

>+          <menupopup id="button-goBackPopup" onpopupshowing="backToolbarMenu_init(this)">
>+              <menuitem id="button-goBack" 
>+                        label="&goBackCmd.label;"
>+                        command="cmd_goBack"/>
>+              <menuseparator id="button-goBackSeparator"/>
>+          </menupopup>
>       </toolbarbutton>

Two space indentation. Here and for goForwardsSeparator and junk-deck

>+      <toolbarbutton id="button-print" 

Stray space at the end.

>+      <toolbarbutton id="button-print" 

There's a stray space here too.
Also use 2 space indentation.

Don't feel like repeating my self anymore, but for the rest of the patch too: two space indentation (which we try to use for new and cleaned up code) and let the id come first.

Furthermore, Thunderbird doesn't work with the patch applied, some menupopup is not closed. Or maybe that's the whops.

I thought about whether it makes sense to sacrifice cvs blame to tidy up the long rows, but since most of them are really way too long, I think that's ok and will be better in the long run.

Comment 4

[Simon Paquet [:sipaq]]

Attachment: Patch v2

I updated the patch with the issue found during review. I also removed one more stray space at the end. I'll also supply a diff -uw patch for easier review again.

Comment 5

[Simon Paquet [:sipaq]]

Attachment: Patch v2 (with diff -uw for easier review)

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 318793 [details] [diff] [review]
Patch v2

removing sr?, as mail/ doesn't require superreview.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 318793 [details] [diff] [review]
Patch v2

Looks good! r=mkmelin

Comment 8

[Simon Paquet [:sipaq]]

Comment on attachment 318793 [details] [diff] [review]
Patch v2

Requesting branch approval. 

Same reason as in bug 411481: Minimal risk (only ID's are added to the XUL code, but high reward since the added IDs will greatly benefit extension developers

Comment 9

[Simon Paquet [:sipaq]]

Can someone please check this in for me on trunk.

Comment 10

[Mark Banner (:standard8)]

(In reply to comment #9)
> Can someone please check this in for me on trunk.
> 
This can't currently be checked in due to the tree closure for TB 3.0a1 (http://tinderbox.mozilla.org/Thunderbird/). I (or someone else I'm sure) will be happy to do this after the tree reopens.

Comment 11

[Mark Banner (:standard8)]

Patch checked into trunk:

Checking in mail/base/content/mailWindowOverlay.xul;
/cvsroot/mozilla/mail/base/content/mailWindowOverlay.xul,v  <--  mailWindowOverlay.xul
new revision: 1.239; previous revision: 1.238
done

Comment 12

[Simon Paquet [:sipaq]]

Thanks Mark. Marking FIXED.

Comment 13

[Magnus Melin [:mkmelin]]

Setting milestone so it's easier to see when things got fixed (and search by it).

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 318793 [details] [diff] [review]
Patch v2

dmose: this OK with you for the branch?

Comment 15

[Simon Paquet [:sipaq]]

Comment on attachment 318793 [details] [diff] [review]
Patch v2

Removing approval request for now. Branch and trunk are totally different in this code area.

So a branch patch requires a lot more thought and effort.