1072611 - Ctrl+P not working from Composition's Print Preview window

Status: RESOLVED FIXED

(MailNews Core :: Printing, defect)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #581470 +++

Ctrl+P is default shortcut to launch Print dialog. Pressing Ctrl+P in composition's "Print Preview" window to actually print seems to have no effect (ux-inconsistent with print preview in message reader, bug 581470)...

Reproducible: Always

1) From TB composition window, File > Print Preview
2) Press Ctrl+P, expecting to invoke the print dialogue

Actual Result

- nothing

Expected Result

- invoke the Print dialogue

Reasons:
1) Ctrl+P to invoke the Print dialogue is expected default behaviour, anywhere
2) Ctrl+P works as expected in message reader's print preview, and Firefox 4 Print Preview
3) No reason to force the user into clumsy Alt+P only

This should be straightforward and "easy" to implement. Any volunteers?

Comment 1

[sabbiralam.cse]

I want to volunteer. Will you please provide me the instruction I should follow ? 

Thank you.

Comment 2

[Thomas D. (:thomas8)]

Oh, mentor field was carried over from the cloned bug...
I'm a bit swamped with stuff currently, so at best I can try drive-by mentoring...
Anyone with mentoring knowledge, please be most welcome to join and offer advice, I won't be able to mentor this alone and I appreciate cooperation!

(In reply to sabbiralam.cse from comment #1)
> I want to volunteer. Will you please provide me the instruction I should
> follow ? 

Hi sabbiralam,

thanks for volunteering and welcome!

For most bugs and in the long run, you want this for developing Thunderbird:
https://developer.mozilla.org/en-US/docs/Simple_Thunderbird_build
Unfortunately I can't say more about that because I haven't done it so far.

If you're the hard-thinking type, for some bugs (perhaps including this one), you can sometimes get away with fixing things from just looking at the code and fixing it.

The final thing you want to deliver is a patch file (diff) like the one on seen on twin Bug 581470, attachment 714891 [details] [diff] [review], which added Ctrl+P keyboard shortcut to message reader's print preview. You can definitely get some inspiration how to fix this from that patch, but unfortunately, composition's print preview doesn't work exactly the same way as message reader's print preview.

For finding code, one way is to use Dom Inspector addon in Thunderbird:
https://addons.mozilla.org/en-US/thunderbird/addon/dom-inspector-6622/

For more information how to find the right spot in source code, you can use advice by me and :rkent in Bug 507103 Comment 8, Bug 507103 Comment 10, and Bug 507103 Comment 11.
Source code is mirrored in http://mxr.mozilla.org/comm-central/, and you can download raw files from there, fix them, and do the comparison (diff) on your own machine with appropriate tools.

Comment 3

[sabbiralam.cse]

(In reply to Thomas D. from comment #2)
> Oh, mentor field was carried over from the cloned bug...
> I'm a bit swamped with stuff currently, so at best I can try drive-by
> mentoring...
> Anyone with mentoring knowledge, please be most welcome to join and offer
> advice, I won't be able to mentor this alone and I appreciate cooperation!
> 
> (In reply to sabbiralam.cse from comment #1)
> > I want to volunteer. Will you please provide me the instruction I should
> > follow ? 
> 
> Hi sabbiralam,
> 
> thanks for volunteering and welcome!
> 
> For most bugs and in the long run, you want this for developing Thunderbird:
> https://developer.mozilla.org/en-US/docs/Simple_Thunderbird_build
> Unfortunately I can't say more about that because I haven't done it so far.
> 
> If you're the hard-thinking type, for some bugs (perhaps including this
> one), you can sometimes get away with fixing things from just looking at the
> code and fixing it.
> 
> The final thing you want to deliver is a patch file (diff) like the one on
> seen on twin Bug 581470, attachment 714891 [details] [diff] [review], which
> added Ctrl+P keyboard shortcut to message reader's print preview. You can
> definitely get some inspiration how to fix this from that patch, but
> unfortunately, composition's print preview doesn't work exactly the same way
> as message reader's print preview.
> 
> For finding code, one way is to use Dom Inspector addon in Thunderbird:
> https://addons.mozilla.org/en-US/thunderbird/addon/dom-inspector-6622/
> 
> For more information how to find the right spot in source code, you can use
> advice by me and :rkent in Bug 507103 Comment 8, Bug 507103 Comment 10, and
> Bug 507103 Comment 11.
> Source code is mirrored in http://mxr.mozilla.org/comm-central/, and you can
> download raw files from there, fix them, and do the comparison (diff) on
> your own machine with appropriate tools.

Hi Thomas D.
Thank you for your information. I am on it from right now. I have a thunderbird local build. Don't know how use DOM inspector. I am going to use it from now on. 

Thank you.

Comment 4

[Thomas D. (:thomas8)]

(In reply to sabbiralam.cse from comment #3)

> Hi Thomas D.
> Thank you for your information. I am on it from right now. I have a
> thunderbird local build. Don't know how use DOM inspector. I am going to use
> it from now on. 

Hi Sabbir Alam, I've described in Bug 507103 Comment 8 how you can use DOM inspector to find code starting out from the real UI:

(In reply to Thomas D. from comment #8)
> 1) Install Dom Inspector *for Thunderbird* plugin (different from DomI for
> FF)
> 2) Compose, subject: Test123
> 3) From TB main window, start DomI (Ctrl+Shift+I)
> 4) From DomI window's menu:
> File > Inspect Chrome Document > Write: Test123
> -> Title bar of DomI is now "Write: Test123 - Dom Inspector
> View > Document Viewer > Dom Nodes (or same from button on the left)
> -> Caption of left panel of DomI is now "Document > Dom Nodes"
> -> "Find a Node" button is now enabled (icon has mouse pointer)
> 5) In DomI window, click "Find a node to inspect by clicking on it" button
> 6) Go back to composition window, click on security button *dropdown*

"Security button" is specific to Bug 507103, just replace all occurences of that with "File > Print..."
So in step 6), you'll need to open composition's "File" menu using *keyboard* (to avoid DOMi triggering prematurely on the wrong node), then *click* on "Print..."

> 7) Go back to DomI window, toolbarbutton of security button is now
> highlighted
> 8) Click on all nested [+] twisties until you find the menuitem you're
> looking for
> 9) copy the ID of the target menuitem, and search for that ID on MXR code
> mirror to find the source file

Comment 5

[Thomas D. (:thomas8)]

(In reply to sabbiralam.cse from comment #3)
> (In reply to Thomas D. from comment #2)
> > Oh, mentor field was carried over from the cloned bug...
[Snip]
> > download raw files from there, fix them, and do the comparison (diff) on
> > your own machine with appropriate tools.
> 
> Hi Thomas D.

As a side note, pls avoid quoting full comments but only quote what's necessary for the context of your reply.

Comment 6

[Thomas D. (:thomas8)]

QA contact field practically isn't used in TB project, and if it were, it usually wouldn't be the assignee, but another independent person which has that role in the project.

Comment 7

[sabbiralam.cse]

(In reply to Thomas D. from comment #6)

Oh! I am sorry. I am changing it to the default null value. 

Thank you.

Comment 8

[sabbiralam.cse]

Its already changed. Sorry for the mistakes. Thank you again for guiding me.

Comment 9

[Thomas D. (:thomas8)]

Sabbiralam, let us know if you need help...

You can also ask for help via IRC [irc://irc.mozilla.org#maildev].

Comment 10

[Abhinandan]

Hi Sabbiralam,

Please let me know if you are still working on this one. If not I would like to work on it.

Thanks,
Abhinandan

Comment 11

[Thomas D. (:thomas8)]

(In reply to Abhinandan from comment #10)
> Hi Sabbiralam,
> 
> Please let me know if you are still working on this one. If not I would like
> to work on it.

Hi Abhinandan, welcome and thanks for showing up here :)
I'd think you can just start working on this anyway, Sabbiralam has been silent for 6 months so perhaps he doesn't have time to continue. Let's wait a bit for his reply and then pls ping me to assign the bug to you.

Comment 12

[Abhinandan]

Hey Thomas :)

I am woking on Mac OSX and I do not see the "print preview" option in the file menu anymore and so I was unable to reproduce this. Am I missing something here?

Comment 13

[Magnus Melin [:mkmelin]]

Macs don't use a print preview menu, but instead allow you to preview from within the print dialog IIRC.

Comment 14

[Thomas D. (:thomas8)]

Which means that this RFE can only be implemented for Windows, and probably Linux, too.
On Windows, there's a File > Print Preview Command in Composition.

Comment 15

[Abhinandan]

Thanks for all the help. I managed to build it on Ubuntu and after searching a little I found that this patch fixes the problem. But looks a little too simple to me. It would be great if someone could test it out :)

PS: I am a newbie to mercurial. I'm sorry if I've made any mistakes. I used to git quite a bit but this looks a lot different.

Comment 16

[Abhinandan]

Attachment: patch_file_v1.patch

Comment 17

[Magnus Melin [:mkmelin]]

Thx, that looks like it might be it, haven't tested it yet though. 
Two things: you should move it down to under key_print, and also, add a "commit message" like "Bug 1072611 - Ctrl+P not working from Composition's Print Preview window, r=?"

(You get that doing "hg qrefresh -e")

Also, to get the patch reviewed, please ask for review by setting the review? flag to my email adress
See https://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree#Getting_the_patch_reviewed

Comment 18

[Abhinandan]

Attachment: Restore Ctrl+P functionality in Print Preview

Thanks.

I have changed it and uploaded a new patch :)

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 8600723 [details] [diff] [review]
Restore Ctrl+P functionality in Print Preview

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

Sorry for the delay. Looks good! r=mkmelin

::: mail/components/compose/content/messengercompose.xul
@@ +199,5 @@
>    <key id="key_send" keycode="&sendCmd.keycode;" observes="cmd_sendWithCheck" modifiers="accel"/>
>    <key id="key_sendLater" keycode="&sendLaterCmd.keycode;" observes="cmd_sendLater" modifiers="accel, shift"/>
>    <key id="key_print" key="&printCmd.key;" command="cmd_print" modifiers="accel"/>
> +  <key id="printKb" key="&printCmd.key;" command="cmd_print" modifiers="accel"/>
> +  

nit: trailing whitespace, but I'll fix that for you

Comment 20

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/4459cf4154eb -> FIXED

More patches welcome, let me know if you want help finding something suitable to work on ;)

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 8600723 [details] [diff] [review]
Restore Ctrl+P functionality in Print Preview

[Approval Request Comment]
Risk to taking this patch (and alternatives if risky): Small polish patch with low risk so might as well uplift.

Comment 22

[Kent James (:rkent)]

Comment on attachment 8600723 [details] [diff] [review]
Restore Ctrl+P functionality in Print Preview

http://hg.mozilla.org/releases/comm-aurora/rev/23286f787689
TB-39: http://hg.mozilla.org/releases/comm-beta/rev/6082953ddad7
TB-38: http://hg.mozilla.org/releases/comm-beta/rev/a06908db7521

Comment 23

[Abhinandan]

Hey Magnus,

Thanks for merging the patch :) I would love to work on something a little more harder. It would be great if you could help me find a bug :)

Abhi