Closed Bug 318955 Opened 19 years ago Closed 10 years ago

Add optional Print button for message composition (customize composition toolbar)

Categories

(Thunderbird :: Message Compose Window, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: BesTo, Assigned: dorsatum, Mentored)

References

Details

(Keywords: relnote, Whiteboard: [good first bug][tb31features])

Attachments

(2 files, 7 obsolete files)

Hey.

Im interessted in having an optinal printer-button via the customize-function of the toolbar in the message-compose-window.

Is this possible?
QA Contact: message-compose
Assignee: mscott → nobody
Summary: add optinal printer-button to message-compose-window → add optional printer-button to message-compose-window
Absolutely. We allow users to print a composition, so there's no reason why they shouldn't customize their composition toolbar and have a button for that. In fact, it's a use case that makes a lot of sense, and fwiw I often print sensitive messages before sending them.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: add optional printer-button to message-compose-window → Add optional Print button for message composition (customize composition toolbar)
See also:

bug 297702 to improve the formatting and completeness of "draft" printouts from composition window... This bug would make a nice complement of bug 297702, but bug 297702 is not required to add the button to toolbar customization.

bug 251953 to add "Print Preview" command & functionality to composition menu.
See Also: → 251953, 297702
Whiteboard: [good first bug]
Hi Thomas D., I'd like to work on this, could you help me out?
(In reply to Projjol [:dorsatum] from comment #5)
> Hi Thomas D., I'd like to work on this, could you help me out?

Sure. So let's assign this to you ;)
I'm a bit short of time, so I'll be a bit instructive.

We want to add *optional* print button, so it needs to end up on the <toolbarpalette> that you get when right-click on Composition toolbar > customize...

If you already know the file where most of the composition xul lives, you can try proceed with 2) below. Otherwise, 1) shows how to find the needle in haystack in unknown territory

1) Let's find the id of a neighbouring button to see how they do it

To find the toolbarpalette, let's look for any button on that toolbar and walk up from there.
To find the button in code, we need the id of the xul button.
To find the button id (short of guessing which might also sometimes work), let's use Dom Inspector Addon.
So to find button with DomI, we want to click on the button to identify it in DomI's dom tree view.
Put test123 into composition subject
In DomI window: File > Inspect Chrome Doc > Write: test123.
In DomI window: click first toolbar button "find a node to inspect by clicking on it"
use keyboard to navigate to composition test123.
first click onto Send button (or any other neighbouring button), should be highlighted with red outline by DomI
go back to DomI window, Send button is selected in tree
copy/memorize id of send button

2) Now let's find send button in code:

http://mxr.mozilla.org/comm-central > Search for: 
[id="###"] (brackets represent input field; replace ### with send button id which you found) (there can only be one button with that id in TB, and one for seamonkey, so it's more efficient to search for id="..." than just the id itself which might be reference elsewhere)

So after the xul for the send button, that's where we can insert our new xul for print button.
Look at the other buttons and take clues from there how to get your button right.

Then, use same method with DomI to find the id of print command which gets executed when you click on File > Print... in composition window. You want the same command on the command attribute of your xul print button. Let's try without using oncommand="goDoCommand(...)" and see if it works (imo, should work).

hth
Assignee: nobody → probaner23
Status: NEW → ASSIGNED
Whiteboard: [good first bug] → [good first bug][mentor=ThomasD]
Using DomI, if you want to inspect a menuitem by clicking on it, make sure you use keyboard-only interaction to open any menus before target, because *first* mouse click will be inspected (so you can't /click/ on File menu to inspect File > *Print*).
Additionally you can search for the main window's print button and use this code in composer and where needed adapt the ids and commands. Special here is, you need all between the #ifdef XP_MACOSX and #endif because OS X has no menu on the button.
Thank you, Thomas D., I shall get on it right away!
Attached file try_patch (obsolete) —
Not a patch, but since I was a bit unsure of what I was doing, I thought that i'll put the basic framework up here. The first toolbar tag, is existing code, the second one is what I'm proposing as the required change.
Attachment #8335160 - Flags: feedback?(bugzilla2007)
Comment on attachment 8335160 [details]
try_patch

Thank you dorsatum for working on this and accepting the challenge :)
Well done for finding some of the right elements in the vastness of code ;)
Of course, the devil is in the detail...

> <toolbarbutton class="toolbarbutton-1"
>               id="button-send" label="&sendButton.label;"
>               tooltiptext="&sendButton.tooltip;"
>               command="cmd_sendButton"
>               now_label="&sendButton.label;"
>               now_tooltiptext="&sendButton.tooltip;"
>               later_label="&sendLaterCmd.label;"
>               later_tooltiptext="&sendlaterButton.tooltip;">
>    </toolbarbutton>
>
>    <toolbarbutton class="toolbarbutton-1"
>                id="printMenuItem" label="Print&hellip;"
>                command="cmd_print;"
>                accesskey="P;"
>                key="key-print;">
>    </toolbarbutton>

Pls ensure understanding of each patch line you propose before reusing code (reusing is fine, but it should be informed reusing).
Just google for all the attributes that you're not sure about yet, e.g.
https://www.google.com/search?q=xul%20accesskey
Then read the information found to know more:
https://developer.mozilla.org/en-US/docs/XUL_Accesskey_FAQ_and_Policies
https://developer.mozilla.org/en-US/docs/XUL/Attribute/accesskey

From documentation, you can find out why accesskey is not applicable here on the toolbar.
Pls report results or ask questions.

Also, pls analyse more details by direct comparison:

The assumption being that both sendButton and your printButton are buttons on same toolbar so that should look structurally similar in their attributes, attribute values etc.
However, your toolbarbutton (in the code) looks significantly different from the structure of send button.
So pls try to read and understand structure of send button first, and from there, conclude what's missing or wrong on your print button. E.g., compare:

Send button  vs  your Print button

id=button-send  vs. id=printMenuItem - is that right?
tooltiptext=... vs. no tooltiptext - why?
now_label="&sendButton.label;" vs. label="Print&hellip;"  (note that all labels on send button have this structure: "&foovar;" (ampersand + varname + semicolon) - but yours has "String&var;" - pls use mxr search to find sendButton.label and see how it works there in existing code for sendButton, and your printButton should be similar...)
no key          vs. key="key-print;" - why?
no accesskey    vs. accesskey="P;" - why?

As for key, which is the shortcut key, it's probably set either in its own <key...> or otherwise key attribute on the cmd_print, so we don't need to set that again on our button because it's already set for the command.

command, accesskey and key (irregardless if needed or not) have the wrong syntax in your try_patch:
Correct possible syntax:

e.g. accesskey="P" (that would be a hardcoded string, so it's not localizable) or
     accesskey="&print.accesskey;" (that's a variable which will be defined in the language files, so it's localizable, so in the language file, print.accesskey variable will be resolved into "P" string.)

It's fine to post the simple version of the print button first so that we start with getting that correct first. From there, we want to make our new button a dualbutton with a dropdown part, as seen on the Print button on the mailtoolbar in main 3pane view:

[Print | v]
Print...
Print Preview

So that's where you can use your new DomI search skills ("find a node to inspect by clicking on it") to follow Richard's hint and find that special button in the existing code of the main mail window:

(In reply to Richard Marti [:Paenglab] from comment #8)
> Additionally you can search for the main window's print button and use this
> code in composer and where needed adapt the ids and commands. Special here
> is, you need all between the #ifdef XP_MACOSX and #endif because OS X has no
> menu on the button.
Attachment #8335160 - Flags: feedback?(bugzilla2007) → feedback-
(In reply to Thomas D. from comment #11)
Note that comment 11 analyises the simple single button tentatively suggested by attachment 8335160 [details], and suggests ways of correcting that.
When we do the dualmenu button suggested by comment 8, it's a whole new story again because there we have a *button* that comes with a dropdown *menu*, so it's a bit of both and some of the attributes which are not needed for the simple button will be needed for the menus again...
(In reply to Thomas D. from comment #11)

> Pls ensure understanding of each patch line you propose before reusing code
> (reusing is fine, but it should be informed reusing).
Definitely, I was re-using a good deal of code and making approximations, but now with the feedback subsequent versions would have better, cleaner code.


> id=button-send  vs. id=printMenuItem - is that right?
> tooltiptext=... vs. no tooltiptext - why?
> now_label="&sendButton.label;" vs. label="Print&hellip;"  (note that all
> labels on send button have this structure: "&foovar;" (ampersand + varname +
> semicolon) - but yours has "String&var;" - pls use mxr search to find
> sendButton.label and see how it works there in existing code for sendButton,
> and your printButton should be similar...)
> no key          vs. key="key-print;" - why?
> no accesskey    vs. accesskey="P;" - why?
These were the attributes that I could find using DomI, but of course, the format in which the send button operates and what I've proposed is totally different and seeing how they should be similar structurally, I'll look further into it. Could you tell me how I can look for these attributes though? Should I search for the print button in MXR?
(In reply to Projjol [:dorsatum] from comment #13)
> (In reply to Thomas D. from comment #11)
> 
> > Pls ensure understanding of each patch line you propose before reusing code
> > (reusing is fine, but it should be informed reusing).
> Definitely, I was re-using a good deal of code and making approximations,
> but now with the feedback subsequent versions would have better, cleaner
> code.
> 
> 
> > id=button-send  vs. id=printMenuItem - is that right?
> > tooltiptext=... vs. no tooltiptext - why?
> > now_label="&sendButton.label;" vs. label="Print&hellip;"  (note that all
> > labels on send button have this structure: "&foovar;" (ampersand + varname +
> > semicolon) - but yours has "String&var;" - pls use mxr search to find
> > sendButton.label and see how it works there in existing code for sendButton,
> > and your printButton should be similar...)
> > no key          vs. key="key-print;" - why?
> > no accesskey    vs. accesskey="P;" - why?
> These were the attributes that I could find using DomI, but of course, the
> format in which the send button operates and what I've proposed is totally
> different and seeing how they should be similar structurally, I'll look
> further into it. Could you tell me how I can look for these attributes
> though? Should I search for the print button in MXR?

For explanation of attributes, use Mozilla's documentation, e.g. for accesskey attribute, use this:
https://developer.mozilla.org/en-US/docs/XUL/Attribute/accesskey

For ideas/template for the print dualbutton (with dropdown) that you ultimately want to create, do this:
- main 3pane, close all tabs except main tab
- on empty space next to top of 1st tab, right-click to get this context menu:
Mail Toobar
Menu bar
Customize...
- put checkmark on mail toolbar and menu bar to have them both shown
- right-click on mail toolbar, Customize...
- drag Print button into mailtoolbar, where it looks like this: [(icon) Print | v]
- use your DomI skills to inspect this very print button (procedure as described in comments above) and find out its ID or any other unique property.
- then search on mxr for that ID or unique property to find that button in the actual source code

The code of that print button (from main 3pane) is a good starting point for your print button in composition.

Next thing is to compare with sendButton or other buttons on the composition toolbar and ensure e.g. that the structure of ID attribute of your print button is similar to the id pattern used in those other buttons (if there's a pattern at all), iow compare with neighbouring buttons to see if you can make them a bit similar structurally (if all buttons have a tooltip, yours should have one, too, and the tooltip should be structurally similar to other tooltips etc.).

To get the right command attribute for actual actual print and print preview menuitems, you have to use DomI and inspect this from composition's (!) main menu:
File | Print...
File | Print Preview...
After activating DomI's "Inspect node by clicking", use keyboard to navigate into the file menu, so the first mouse click is on the Print... menuitem, then inspect that in DomI.
You can either find the command attributes in DomI viewer, or you use their ID to find the menuitems in mxr.
Note that all strings that appear in UI, like labels, tooltips, access keys, even shortcut keys (key attribute) must be localizable and hence are defined in separate language resource files (never hardcoded as actual strings in code). Most strings for composition are defined here:

http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd

So e.g. in line 9 of messengercompose.dtd (English version), there's this:
<!ENTITY fileMenu.label "File">

So in the xul code for composition's main menu, there will be something like this:
<menuitem label="&fileMenu.label;" ...>
Which means the code uses attribute="&variablename;" to use the localized strings from the language resource files. So in EN version of TB, users see "File", and in German, they will see "Datei".
Hi, Thomas D., I'm very sorry for not replying in such a long time. I have read up as you suggested, and implemented the print button from the main 3pane window and I have a good idea as to how my next file should look like. However, I'd really appreciate it, if I could be given time till the 14th of December'13. My college's end semester exams end then and I'll definitely have a working file out by then. Thank you.
(In reply to Projjol [:dorsatum] from comment #16)
No problem at all, pls take yr time.
Attachment #8335160 - Attachment is obsolete: true
Attachment #8348050 - Flags: review?(bugzilla2007)
Attached file ComposeIcons.zip
Projjol,

in ZIP file are the images for Windows and OS X. Linux uses the GTK icons.
For examples how the CSS should look, see in platforms primaryToolbar.css for #button-print.
Attached patch print.patch (obsolete) — Splinter Review
Made changes to other requisite files.
Attachment #8348050 - Attachment is obsolete: true
Attachment #8348050 - Flags: review?(bugzilla2007)
Attachment #8349997 - Flags: review?(richard.marti)
Attachment #8348050 - Flags: review?(richard.marti)
Attachment #8348050 - Flags: review?(bugzilla2007)
Attachment #8348050 - Flags: review?(richard.marti)
Attachment #8348050 - Flags: review?(bugzilla2007)
Comment on attachment 8349997 [details] [diff] [review]
print.patch

Review of attachment 8349997 [details] [diff] [review]:
-----------------------------------------------------------------

This looks already good but needs some changes I addressed in the comments below.

Please test your patch before you are asking for review. Here you need only to add the button to the toolbar, if on Linux or XP you can also check big and small icon mode. Then also test if pressing the button and selecting the menuentries is doing what they should do (here printing and showing the print preview).

::: mail/components/compose/content/messengercompose.xul
@@ +768,5 @@
>                       command="cmd_saveAsTemplate" type="radio" name="radiogroup_SaveAs"/>
>           </menupopup>
>      </toolbarbutton>
>  
> +    <toolbarbutton id="button-print"

OS X doesn't need the menu in this button. Please look at http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#3188 to line 3215 how this is done with the #ifdef XP_MACOSX, #else, #endif

@@ +772,5 @@
> +    <toolbarbutton id="button-print"
> +                    class="toolbarbutton-1"
> +                    label="&printButton.label;"
> +                    observes="button_print"
> +                    oncommand="cmd_print"

oncommand doesn't work here, use command="cmd_print".

@@ +779,5 @@
> +        <menupopup id="printMenu" onpopupshowing="goUpdateCommand('cmd_printpreview');">
> +         <menuitem id="button-printMenu"
> +                   label="&printCmd.label;"
> +                   accesskey="&printCmd.accesskey;"
> +                   default="true"/>

A command="cmd_print" is needed here.

@@ +784,5 @@
> +         <menuitem id="button-printPreviewMenu"
> +                   label="&printPreviewCmd.label;"
> +                   accesskey="&printPreviewCmd.accesskey;"
> +                   observes="cmd_printpreview"
> +                   command="cmd_printpreview"/>

The case of this cmd is wrong on observes and command, use cmd_printPreview.

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ +214,5 @@
>  <!ENTITY saveButton.tooltip "Save this message">
>  <!ENTITY cutButton.tooltip              "Cut">
>  <!ENTITY copyButton.tooltip             "Copy">
>  <!ENTITY pasteButton.tooltip            "Paste">
> +<!ENTITY printButton.tooltip " Print this message">

Please remove the space between " and P.

::: mail/themes/linux/mail/compose/messengercompose.css
@@ +118,5 @@
>  
>  #paste-button[disabled="true"] {
>    list-style-image: url("moz-icon://stock/gtk-paste?size=toolbar&state=disabled");
>  }
> +#button-print {

Please add a empty line before #button-print.

@@ +125,1 @@
>  

Disabled state is missing.

@@ +198,5 @@
>  
>  toolbar[iconsize="small"] #paste-button[disabled="true"] {
>    list-style-image: url("moz-icon://stock/gtk-paste?size=menu&state=disabled");
>  }
> +toolbar[iconsize="small"] #button-print[disabled="true"] {

Please add a empty line before #button-print.
Normal (not disabled) state is missing

@@ +199,5 @@
>  toolbar[iconsize="small"] #paste-button[disabled="true"] {
>    list-style-image: url("moz-icon://stock/gtk-paste?size=menu&state=disabled");
>  }
> +toolbar[iconsize="small"] #button-print[disabled="true"] {
> +  list-style-image: url("moz-icon://stock/gtk-print?size=toolbar");

For small icon mode size is menu not toolbar. For disabled state add state=disabled. Look how it's done at #paste-button[disabled="true"] above.

@@ +200,5 @@
>    list-style-image: url("moz-icon://stock/gtk-paste?size=menu&state=disabled");
>  }
> +toolbar[iconsize="small"] #button-print[disabled="true"] {
> +  list-style-image: url("moz-icon://stock/gtk-print?size=toolbar");
> +}

Please add a empty line after.

::: mail/themes/osx/mail/compose/messengercompose.css
@@ +316,5 @@
>  
>  #paste-button:not([disabled="true"]):active {
>    -moz-image-region: rect(18px 180px 36px 162px);
>  }
> +#button-print {

Please add a empty line before #button-print.

@@ +324,1 @@
>  

Please add the :active state. Use the #paste-button above as an example.

@@ +415,5 @@
>  
>    #paste-button:not([disabled="true"]):active {
>      -moz-image-region: rect(36px 360px 72px 324px);
>    }
> +  #button-print:not([disabled="true"]):active {

Please add a empty line before #button-print.
The normal mode is missing. The list-style-image and -moz-image-region are for thr normal mode.

@@ +418,5 @@
>    }
> +  #button-print:not([disabled="true"]):active {
> +  list-style-image: url("chrome://messenger/skin/messengercompose/compose-toolbar.png");
> +  -moz-image-region: rect(0px 396px 36px 360px);
> +}

Indent this three lines by 4 spaces.

::: mail/themes/windows/mail/compose/messengercompose.css
@@ +163,5 @@
>  
>  #paste-button[disabled="true"] {
>    -moz-image-region: rect(48px 240px 72px 216px) !important;
>  }
> +#button-print {

Please add a empty line before #button-print.

@@ +171,1 @@
>  

:hover and [disabled="true"] are missing

@@ +285,5 @@
>  
>  toolbar[iconsize="small"] #paste-button[disabled="true"] {
>    -moz-image-region: rect(32px 160px 48px 144px) !important;
>  }
> +toolbar[iconsize="small"] #button-print[disabled="true"] {

Please add a empty line before #button-print.
normal and :hover are missing.

@@ +287,5 @@
>    -moz-image-region: rect(32px 160px 48px 144px) !important;
>  }
> +toolbar[iconsize="small"] #button-print[disabled="true"] {
> +  list-style-image: url("chrome://messenger/skin/messengercompose/compose-toolbar.png");
> +  -moz-image-region: rect(0px 264px 24px 248px);

Both lines are for the big icons in normal mode. Use for normal mode:
  list-style-image: url("chrome://messenger/skin/messengercompose/compose-toolbar-small.png");
  -moz-image-region: rect(0px 176px 16px 160px);
Attachment #8349997 - Flags: review?(richard.marti) → review-
(In reply to Richard Marti (:Paenglab) from comment #21)

Thanks Richard for the review, especially the icon stuff :)

> > +    <toolbarbutton id="button-print"
> > +                    class="toolbarbutton-1"
> > +                    label="&printButton.label;"
> > +                    observes="button_print"
> > +                    oncommand="cmd_print"

I'm also wondering about the observes="button_print" attribute. As it is, it won't work, because there's no such command to observe in the context of composition.
That construction is found in the main 3pane, but I don't think it's needed in composition.
Not seen on the neighbouring buttons like button-send or button-attach, either:
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#693
(In reply to Thomas D. from comment #22)
> (In reply to Richard Marti (:Paenglab) from comment #21)
> I'm also wondering about the observes="button_print" attribute. As it is, it
> won't work, because there's no such command to observe in the context of
> composition.

Good catch, this line isn't needed here and can be removed.
Thank you both for your review, I'll make the changes and submit a patch ASAP.
(In reply to Richard Marti (:Paenglab) from comment #21)
> ::: mail/components/compose/content/messengercompose.xul
> @@ +768,5 @@
> >                       command="cmd_saveAsTemplate" type="radio" name="radiogroup_SaveAs"/>
> >           </menupopup>
> >      </toolbarbutton>
> >  
> > +    <toolbarbutton id="button-print"
> 
> OS X doesn't need the menu in this button. Please look at
> http://mxr.mozilla.org/comm-central/source/mail/base/content/
> mailWindowOverlay.xul#3188 to line 3215 how this is done with the #ifdef
> XP_MACOSX, #else, #endif
Is #endif needed? because in mailWindowOverlay.xul, the file uses endif for a new button.


> @@ +324,1 @@
> >  
> 
> Please add the :active state. Use the #paste-button above as an example.
How do i get the pixel co-ordinates for this one?
 

 
> @@ +171,1 @@
> >  
> 
> :hover and [disabled="true"] are missing
For : hover and the disabled state how do I calculate the pixel co-ordinates?
 
> @@ +285,5 @@
> >  
> >  toolbar[iconsize="small"] #paste-button[disabled="true"] {
> >    -moz-image-region: rect(32px 160px 48px 144px) !important;
> >  }
> > +toolbar[iconsize="small"] #button-print[disabled="true"] {
> 
> Please add a empty line before #button-print.
> normal and :hover are missing.
 
Again for normal and :hover the pixel co-ordinates?

(In reply to Thomas D. (away till 31st January) from comment #22)
> I'm also wondering about the observes="button_print" attribute. As it is, it
> won't work, because there's no such command to observe in the context of
> composition.

Removed. Thank you both for your review.
(In reply to Projjol [:dorsatum] from comment #25)
> (In reply to Richard Marti (:Paenglab) from comment #21)
> > ::: mail/components/compose/content/messengercompose.xul
> > OS X doesn't need the menu in this button. Please look at
> > http://mxr.mozilla.org/comm-central/source/mail/base/content/
> > mailWindowOverlay.xul#3188 to line 3215 how this is done with the #ifdef
> > XP_MACOSX, #else, #endif
> Is #endif needed? because in mailWindowOverlay.xul, the file uses endif for
> a new button.

Yes, a #ifdef needs always a #endif. This will work like this:

#ifdef XP_MACOSX
  The XUL-button definition for OS X without the menu.
#else
  The XUL-button definition for Linux and Windows with the menu.
#endif

> > Please add the :active state. Use the #paste-button above as an example.
> How do i get the pixel co-ordinates for this one?

As I wrote check the #paste-button as an example:

In -moz-image-region the second and fourth value stay always the same. Only the first and third values change to move the vertical position.
Here it would be:

#button-print:not([disabled="true"]):active {
  -moz-image-region: rect(18px 198px 36px 180px);
}

> > :hover and [disabled="true"] are missing
> For : hover and the disabled state how do I calculate the pixel co-ordinates?

The same as above. Check the other buttons in this file and adapt it to the #button-print.

> > >  toolbar[iconsize="small"] #paste-button[disabled="true"] {
> > >    -moz-image-region: rect(32px 160px 48px 144px) !important;
> > >  }
> > > +toolbar[iconsize="small"] #button-print[disabled="true"] {
> > 
> > Please add a empty line before #button-print.
> > normal and :hover are missing.
>  
> Again for normal and :hover the pixel co-ordinates?

See above.
Attached patch print.patch (obsolete) — Splinter Review
I have Linux on my machine and it seems to be working fine. If I have missed out on any of the changes that you'd mentioned, I'm really sorry. If you could kindly point them out, I'll update the patch with it. Overall seems ok, though.
Attachment #8349997 - Attachment is obsolete: true
Attachment #8350959 - Flags: review?(richard.marti)
Attachment #8350959 - Flags: review?(bugzilla2007)
Comment on attachment 8350959 [details] [diff] [review]
print.patch

Review of attachment 8350959 [details] [diff] [review]:
-----------------------------------------------------------------

Getting better every time :)
Assuming that you've tested the functionality, I've only proofread your new code in messengercompose.xul and messengercompose.dtd, some nits found. For icons, we need Richard's review :)

::: mail/components/compose/content/messengercompose.xul
@@ +769,5 @@
>           </menupopup>
>      </toolbarbutton>
>  
> +#ifdef XP_MACOSX
> +        <toolbarbutton id="button-print"

Nit: To align correctly with preceding toolbarbuttons, only 4 spaces in front of this line, attributes in following lines should be left-aligned to the id attribute here.

@@ +776,5 @@
> +                    command="cmd_print"
> +                    tooltiptext="&printButton.tooltip;"/>
> +#else
> +    <toolbarbutton id="button-print"
> +                    class="toolbarbutton-1"

Nit: one space too much before class & following attributes

@@ +781,5 @@
> +                    label="&printButton.label;"
> +                    command="cmd_print"
> +                    tooltiptext="&printButton.tooltip;"
> +                    type="menu-button">
> +        <menupopup id="printMenu" onpopupshowing="goUpdateCommand('cmd_printpreview');">

this should have id="button-printPopup", printMenu is too ambiguous and not consistent with similar buttons here like the attach button (and yes, we don't have an ideal and consistent naming system).

nit: Although I appreciate the idea of aligning all the attributes of this block (looks nice), I think we should stay with the conventional indenting of 2 spaces per nested layer. So menupopup needs 2 spaces more indent than preceding toolbarbutton, and attributes left-aligned below menupopup's id attribute.

@@ +782,5 @@
> +                    command="cmd_print"
> +                    tooltiptext="&printButton.tooltip;"
> +                    type="menu-button">
> +        <menupopup id="printMenu" onpopupshowing="goUpdateCommand('cmd_printpreview');">
> +         <menuitem id="button-printMenu"

Nit: Indent 2 spaces more than correctly indented menupopup, and re-align attributes under id attribute.

@@ +787,5 @@
> +                   label="&printCmd.label;"
> +                   accesskey="&printCmd.accesskey;"
> +                   command="cmd_print"
> +                   default="true"/>
> +         <menuitem id="button-printPreviewMenu"

Nit: adjust indenting as above

@@ +790,5 @@
> +                   default="true"/>
> +         <menuitem id="button-printPreviewMenu"
> +                   label="&printPreviewCmd.label;"
> +                   accesskey="&printPreviewCmd.accesskey;"
> +                   observes="cmd_printPreview"

I still think we don't need observes attribute.
Attachment #8350959 - Flags: review?(bugzilla2007) → feedback+
Comment on attachment 8350959 [details] [diff] [review]
print.patch

Review of attachment 8350959 [details] [diff] [review]:
-----------------------------------------------------------------

Additionally to ThomasD's comments, my comments.

I'm sure, the next version, with all this comments fixed, will be the last :).

r- until then.

::: mail/themes/linux/mail/compose/messengercompose.css
@@ +123,5 @@
> +#button-print {
> +  list-style-image: url("moz-icon://stock/gtk-print?size=toolbar");
> +}
> +
> +#button-print {

A [disabled="true"] is missing.

@@ +197,5 @@
>    list-style-image: url("moz-icon://stock/gtk-copy?size=menu&state=disabled");
>  }
>  toolbar[iconsize="small"] #paste-button {
>    list-style-image: url("moz-icon://stock/gtk-paste?size=menu");
>  }

Please, don't remove this line.

@@ +206,5 @@
> +toolbar[iconsize="small"] #button-print[disabled="true"] {
> +  list-style-image: url("moz-icon://stock/gtk-print?size=menu&state=disabled");
> +}
> +toolbar[iconsize="small"] #button-print {
> +  list-style-image: url("moz-icon://stock/gtk-print?size=toolbar");

Should be size=menu

Please switch this two rules so the [disabled="true"] rule is after the normal rule. I works like it is now but the other way it is consistent with the other buttons.
And please insert a LF between the rules.

@@ +207,5 @@
> +  list-style-image: url("moz-icon://stock/gtk-print?size=menu&state=disabled");
> +}
> +toolbar[iconsize="small"] #button-print {
> +  list-style-image: url("moz-icon://stock/gtk-print?size=toolbar");
> +}

LF after this line.

::: mail/themes/osx/mail/compose/messengercompose.css
@@ +323,5 @@
> +  -moz-image-region: rect(0px 198px 18px 180px);
> +}
> +
> +#button-print:not([disabled="true"]):active {
> +  list-style-image: url("chrome://messenger/skin/messengercompose/compose-toolbar.png");

The list-style-image isn't needed to declare here. The one from normal rule is enough.

@@ +422,5 @@
>      -moz-image-region: rect(36px 360px 72px 324px);
>    }
> +
> +  #button-print {
> +  list-style-image: url("chrome://messenger/skin/messengercompose/compose-toolbar.png");

This should be compose-toolbar@2x.png.

@@ +427,5 @@
> +  -moz-image-region: rect(0px 396px 36px 360px);
> +  }
> +
> +  #button-print:not([disabled="true"]):active {
> +  list-style-image: url("chrome://messenger/skin/messengercompose/compose-toolbar.png");

The list-style-image isn't needed to declare here. The one from normal rule is enough.

::: mail/themes/windows/mail/compose/messengercompose.css
@@ +170,5 @@
> +  -moz-image-region: rect(0px 264px 24px 240px);
> +}
> +
> +#button-print:hover {
> +  list-style-image: url("chrome://messenger/skin/messengercompose/compose-toolbar.png");

The list-style-image isn't needed to declare here. The one from normal rule is enough.

The same for the disabled rule.

@@ +297,5 @@
>  toolbar[iconsize="small"] #paste-button[disabled="true"] {
>    -moz-image-region: rect(32px 160px 48px 144px) !important;
>  }
> +toolbar[iconsize="small"] #button-print {
> +  list-style-image: url("chrome://messenger/skin/messengercompose/compose-toolbar.png");

This should be compose-toolbar-small.png.

@@ +302,5 @@
> +  -moz-image-region: rect(0px 176px 16px 160px);
> +}
> +
> +toolbar[iconsize="small"] #button-print:hover {
> +  list-style-image: url("chrome://messenger/skin/messengercompose/compose-toolbar.png");

The list-style-image isn't needed to declare here. The one from normal rule is enough.

The same for the disabled rule.
Attachment #8350959 - Flags: review?(richard.marti) → review-
(In reply to Thomas D. (away till 31st January) from comment #28) 
> @@ +790,5 @@
> > +                   default="true"/>
> > +         <menuitem id="button-printPreviewMenu"
> > +                   label="&printPreviewCmd.label;"
> > +                   accesskey="&printPreviewCmd.accesskey;"
> > +                   observes="cmd_printPreview"
> 
> I still think we don't need observes attribute.

I'm sorry for that, I know you'd mentioned it earlier, but I overlooked it accidentally while writing that block. It's been rectified now.

(In reply to Richard Marti (:Paenglab) from comment #29)

> @@ +197,5 @@
> >    list-style-image: url("moz-icon://stock/gtk-copy?size=menu&state=disabled");
> >  }
> >  toolbar[iconsize="small"] #paste-button {
> >    list-style-image: url("moz-icon://stock/gtk-paste?size=menu");
> >  }
> 
> Please, don't remove this line.

I'm sorry, but which line are you referring to, I couldn't quite catch that. All other changes have been made.
(In reply to Projjol [:dorsatum] from comment #30)
> > Please, don't remove this line.
> 
> I'm sorry, but which line are you referring to, I couldn't quite catch that.
> All other changes have been made.

You accidentally removed an existing linefeed (blank/empty line):
http://mxr.mozilla.org/comm-central/source/mail/themes/linux/mail/compose/messengercompose.css#195
Attached patch print.patch (obsolete) — Splinter Review
Hope this rectifies everything :)
Attachment #8350959 - Attachment is obsolete: true
Attachment #8351026 - Flags: review?(richard.marti)
Attachment #8351026 - Flags: review?(bugzilla2007)
Comment on attachment 8351026 [details] [diff] [review]
print.patch

Review of attachment 8351026 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Projjol, thanks for rapid response and new patch with some corrections successfully made.
Unfortunately the indenting of nested code layers still needs correction. I tried to explain with more detail, but please do not hesitate to contact me via private mail if there's any question.
I'll leave the icon parts of this patch for :paenglab's review.

::: mail/components/compose/content/messengercompose.xul
@@ +765,5 @@
>                       command="cmd_saveAsDraft" type="radio" name="radiogroup_SaveAs"/>
>             <menuitem id="savePopup_saveAsTemplate"
>                       label="&saveAsTemplateCmd.label;" accesskey="&saveAsTemplateCmd.accesskey;"
>                       command="cmd_saveAsTemplate" type="radio" name="radiogroup_SaveAs"/>
> +    </menupopup>

Pls don't touch this line.
I can see the good intention of fixing the indent of that which indeed is wrong in the current code base; however, especially when you're still new to patches, pls don't touch things which are not part of what you're fixing. It's better to fix wrong indents of existing code in its own bug, because here it's just confusing and timewasting for reviewers. Also, you only moved the closing tag; if anything, you'd have to re-indent both opening and closing tag and the tags nested inside, too.

@@ +770,5 @@
>      </toolbarbutton>
>  
> +#ifdef XP_MACOSX
> +    <toolbarbutton id="button-print"
> +                    class="toolbarbutton-1"

You still have a 1 character offset here, why? Again, can we please vertically align all attributes so that id, class etc. start underneath each other at exactly the same character position, like that:
    <toolbarbutton id="button-print"
		   class="toolbarbutton-1"
                   etc.

(and you have to view this in a monospaced/fixed width text editor)

@@ +776,5 @@
> +                    command="cmd_print"
> +                    tooltiptext="&printButton.tooltip;"/>
> +#else
> +    <toolbarbutton id="button-print"
> +                    class="toolbarbutton-1"

class attribute etc. still not left-aligned underneath id attribute

@@ +781,5 @@
> +                    label="&printButton.label;"
> +                    command="cmd_print"
> +                    tooltiptext="&printButton.tooltip;"
> +                    type="menu-button">
> +                 <menupopup id="button-printPopup" onpopupshowing="goUpdateCommand('cmd_printpreview');">

Indenting still needs correction, <menupopup> needs an absolute total indent of 4+2 = 6 characters from outer left margin (4 chars indent inherited from toolbarbutton (the parent element), +2 chars relative indent for menupopup, the child element), not the 17 chars total indent used in this patch.

OK, let's try this again:
Per Mozilla's coding style [1], we use 2 space characters for indentation. Indenting 2 spaces means to move a given block of code 2 spaces away (to the right!) from the previous indent level or margin of the containing element or structure. The resulting nested structure looks like this (for demonstration purposes, I've replaced indenting spaces by # character for better visibility):

<tag1 attribute1="foo"
      attr2="bar">
##<nested1_tag2 attribute1="foo"><!-- child of tag1, single-indented by 2 characters -->
                attr2="bar">
####<nested2_tag3a\><!-- child of tag2, double-indented by 2+2 chars-->
####<nested2_tag3b\><!-- another child of tag2, double-indented by 2+2 chars-->
##</nested1_tag2>
</tag1>

[1] https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

@@ +782,5 @@
> +                    command="cmd_print"
> +                    tooltiptext="&printButton.tooltip;"
> +                    type="menu-button">
> +                 <menupopup id="button-printPopup" onpopupshowing="goUpdateCommand('cmd_printpreview');">
> +                 <menuitem id="button-printMenu"

dito, wrong indent. pls verfiy correct alignment of attributes underneath id attribute, too.

@@ +787,5 @@
> +                   label="&printCmd.label;"
> +                   accesskey="&printCmd.accesskey;"
> +                   command="cmd_print"
> +                   default="true"/>
> +                 <menuitem id="button-printPreviewMenu"

dito, wrong indent.

::: mail/themes/linux/mail/compose/messengercompose.css
@@ +195,5 @@
>  
>  toolbar[iconsize="small"] #copy-button[disabled="true"] {
>    list-style-image: url("moz-icon://stock/gtk-copy?size=menu&state=disabled");
>  }
> +

Thank you for adding this newline character which was missing in the original code.
Attachment #8351026 - Flags: review?(bugzilla2007) → review-
(In reply to Thomas D. (away till 31st January) from comment #33)
> Comment on attachment 8351026 [details] [diff] [review]
> print.patch
> Unfortunately the indenting of nested code layers still needs correction. 

And fwiw, please do not take some of the existing surrounding xul code as a model, because indenting is also incorrect there (e.g. on button-save and quoteButton), but we're not here to fix that (needs another bug).
Comment on attachment 8351026 [details] [diff] [review]
print.patch

Review of attachment 8351026 [details] [diff] [review]:
-----------------------------------------------------------------

r- for ThomasD's comments and mine.

All the functionality is okay now. The indentation and linefeeds are important for the readabilty. I know some parts of the files aren't looking like we want it now, but have in mind some files exist since about 10 years (or maybe more).

I'm okay if you plan to file a bug to fix this indentations. ;)

::: mail/themes/linux/mail/compose/messengercompose.css
@@ +125,5 @@
> +}
> +
> +#button-print[disabled="true"] {
> +  list-style-image: url("moz-icon://stock/gtk-print?size=toolbar&state=disabled");
> +}

Please add a LF here.

::: mail/themes/osx/mail/compose/messengercompose.css
@@ +324,5 @@
> +}
> +
> +#button-print:not([disabled="true"]):active {
> +  -moz-image-region: rect(18px 198px 36px 180px);
> +}

Please add a LF here.

@@ +422,5 @@
>    }
> +
> +  #button-print {
> +  list-style-image: url("chrome://messenger/skin/messengercompose/compose-toolbar@2x.png");
> +  -moz-image-region: rect(0px 396px 36px 360px);

Please indent both property line by 4 spaces.

@@ +426,5 @@
> +  -moz-image-region: rect(0px 396px 36px 360px);
> +  }
> +
> +  #button-print:not([disabled="true"]):active {
> +  -moz-image-region: rect(36px 396px 72px 360px);

Again 4 spaces.

@@ +427,5 @@
> +  }
> +
> +  #button-print:not([disabled="true"]):active {
> +  -moz-image-region: rect(36px 396px 72px 360px);
> +}

Indent this brace by 2 spaces.
Attachment #8351026 - Flags: review?(richard.marti) → review-
Attached patch print.patch (obsolete) — Splinter Review
Attachment #8351026 - Attachment is obsolete: true
Attachment #8351682 - Flags: review?(richard.marti)
Attachment #8351682 - Flags: review?(bugzilla2007)
Comment on attachment 8351682 [details] [diff] [review]
print.patch

Review of attachment 8351682 [details] [diff] [review]:
-----------------------------------------------------------------

Well done, Projjol, now flawless afasics :)

r+ for this patch, but I'm not an official reviewer yet, so it's f=me :)
Attachment #8351682 - Flags: review?(bugzilla2007) → feedback+
Comment on attachment 8351682 [details] [diff] [review]
print.patch

Review of attachment 8351682 [details] [diff] [review]:
-----------------------------------------------------------------

r+ :)
but only with fixing the two comments.

Thank you for taking this bug and your endurance.

::: mail/components/compose/content/messengercompose.xul
@@ +781,5 @@
> +                   label="&printButton.label;"
> +                   command="cmd_print"
> +                   tooltiptext="&printButton.tooltip;"
> +                   type="menu-button">
> +      <menupopup id="button-printPopup" onpopupshowing="goUpdateCommand('cmd_printpreview');">

Please move the onpopupshowing to a new line aligned with id=.

::: mail/themes/osx/mail/compose/messengercompose.css
@@ +428,5 @@
> +  }
> +
> +  #button-print:not([disabled="true"]):active {
> +    -moz-image-region: rect(36px 396px 72px 360px);
> +}

Please indent this by two spaces.
Attachment #8351682 - Flags: review?(richard.marti) → review+
Attached patch print.patch (obsolete) — Splinter Review
Changes made :)
Comment on attachment 8351702 [details] [diff] [review]
print.patch

You changed the menupopup id="button-savePopup" instead of the menupopup id="button-printPopup". And with indent two spaces I meant the brace and not the property.

Marking this patch as obsolete.
Attachment #8351702 - Attachment is obsolete: true
Attached patch print.patchSplinter Review
I'm sorry for that. Did it mechanically, should've paid attention, it's been sorted now.
Attachment #8351682 - Attachment is obsolete: true
Comment on attachment 8351711 [details] [diff] [review]
print.patch

I'll give r+ and ui-r+ to be clear for the nice check-in guy.
Attachment #8351711 - Flags: ui-review+
Attachment #8351711 - Flags: review+
I set also the checkin-needed keyword. Then when the tree is open again, someone with the rights checks this patch in.
Keywords: checkin-needed
Thanks! :D
https://hg.mozilla.org/comm-central/rev/f27e938cc2af
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
Mentor: bugzilla2007
Whiteboard: [good first bug][mentor=ThomasD] → [good first bug]
I've been somewhat hesitating to add this as [tb31features] for release notes, as it's somewhat trivial that print button should be available even for compositions, but on the other hand, we've now fixed the issue so why not let users know (we also have no other way of telling them because the button is still optional, probably good given the poor formatting/incomplete information of the actual printout). Depending on scenario and environment, this might be a must-have feature e.g. for business users who want to proofread their messages in printed form before sending.
Keywords: relnote
Whiteboard: [good first bug] → [good first bug][tb31features]
You need to log in before you can comment on or make changes to this bug.