277265 - Use file checkbox instead of radio buttons in print dialog

Status: VERIFIED FIXED

(Toolkit :: Printing, defect)

Description

[Peter Weilbacher]

The current print dialog uses radio buttons to let the user decide between
printing to printer and printing to file. This is confusing as printing to file
still uses the settings from the selected printer. A checkbox positioned below
the printer selection (like the windows native print dialog) instead more
successfully conveys the message "use the above printer driver/settings to print
to file" to the user.

This change was already implemented in XPFE (bug 263312).

Additionally, for Seamonkey it was decided to drop the filename selection field
and button and instead let the user select a filename from a file picker that
comes up after pressing Print. This should be in line with the Aviary philosophy
to remove features that only a very small number of users need like batch
printing using the same filename.

I will shortly attach a patch ported to Toolkit.

Comment 1

[Peter Weilbacher]

Attachment: patch

I have tested this patch and it works fine on a current Firefox trunk build.

Comment 2

[Simon Paquet [:sipaq]]

Comment on attachment 170461 [details] [diff] [review]
patch

Mike, can you please review and check this in, if you like it.

Comment 3

[Mike Connor [:mconnor]]

Comment on attachment 170461 [details] [diff] [review]
patch

I'm not likely to get to this in a code review sense anytime soon.  If there is
a screenshot, that'd be good for the UI review part, but please find another
toolkit peer (there's a fair number) unless you want to wait a few weeks more.

Comment 4

[Peter Weilbacher]

Attachment: Screenshot (old left, new right)

The requested screenshot...

Comment 5

[Peter Weilbacher]

Comment on attachment 170461 [details] [diff] [review]
patch

Benjamin, you seem to be a toolkit peer with a shorter review queue. Maybe you
can take a look?

Comment 6

[Benjamin Smedberg]

Comment on attachment 170461 [details] [diff] [review]
patch

>Index: toolkit/components/printing/content/printdialog.js

>+    if (gPrintSettings.printToFile)
>+      if (!chooseFile())
>         return false;

if (gPrintSettings.printToFile && !chooseFile())

>   try {
>-    var fp = Components.classes["@mozilla.org/filepicker;1"].createInstance(nsIFilePicker);
>+    var fp = Components.classes["@mozilla.org/filepicker;1"]
>+             .createInstance(nsIFilePicker);

nit, align the dots of .classes and .createInstance

This is pretty good, but please go ahead and post a new patch with those two
nits. I'm also going to talk to mconnor before marking r+, because I don't do
UI reviews.

Comment 7

[Peter Weilbacher]

Attachment: Second patch attending the nits

OK, your points were of course easily rectified.

Additionally, I noticed that I had accidentally removed the "Print" from the
button on the dialog as can be seen on the screenshot (XPFE didn't need those
two extra lines). This new patch again has "Print" on the button as it should
be.

Comment 8

[Peter Weilbacher]

That I got a r+ probably means that I can confirm this bug. :-)

Do I need another review for toolkit patches as well, or was it enough that
Benjamin talked to mconnor before plusing it?

Comment 9

[Peter Weilbacher]

Comment on attachment 172682 [details] [diff] [review]
Second patch attending the nits

Perhaps you can sr this patch, as Benjamin supposedly asked you about it
anyway?

Comment 10

[Peter Weilbacher]

Attachment: Clean patch ready for checkin

Thanks for reviews. I carry them over to this new attachment which is necessary
as icon properties of the buttons in printdialog.xul conflicted with the older
patch.

I would be grateful if someone could check this in, I don't have a CVS account
(yet).

Comment 11

[Mike Kaply [:mkaply]]

Fix checked in - watch for regressions Peter! :)

Comment 12

[Peter Weilbacher]

Thanks Mike. I didn't see any regressions from this nor did I see anybody else
talk about regressions caused by this. Verified from Andy's static firefox build
Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8b2) Gecko/20050311 Firefox/1.0+.