Use file checkbox instead of radio buttons in print dialog

VERIFIED FIXED

Status

()

Toolkit
Printing
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: Peter Weilbacher, Assigned: Peter Weilbacher)

Tracking

unspecified
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
Created attachment 170461 [details] [diff] [review]
patch

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

Comment 2

13 years ago
Comment on attachment 170461 [details] [diff] [review]
patch

Mike, can you please review and check this in, if you like it.
Attachment #170461 - Flags: first-review?(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.
(Assignee)

Comment 4

13 years ago
Created attachment 172656 [details]
Screenshot (old left, new right)

The requested screenshot...
(Assignee)

Comment 5

13 years ago
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?
Attachment #170461 - Flags: first-review?(mconnor) → first-review?(bsmedberg)

Comment 6

13 years ago
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.
Attachment #170461 - Flags: first-review?(bsmedberg) → first-review-
(Assignee)

Comment 7

13 years ago
Created attachment 172682 [details] [diff] [review]
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.
(Assignee)

Updated

13 years ago
Attachment #170461 - Attachment is obsolete: true
Attachment #172682 - Flags: first-review?(bsmedberg)

Updated

13 years ago
Attachment #172682 - Flags: first-review?(benjamin) → first-review+
(Assignee)

Comment 8

13 years ago
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 9

13 years ago
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?
Attachment #172682 - Flags: second-review?(mconnor)

Updated

13 years ago
Attachment #172682 - Flags: second-review?(mconnor) → second-review+
(Assignee)

Comment 10

13 years ago
Created attachment 176253 [details] [diff] [review]
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).
Attachment #172682 - Attachment is obsolete: true
Attachment #176253 - Flags: second-review+
Attachment #176253 - Flags: first-review+
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

13 years ago
Assignee: nobody → mozilla
Status: ASSIGNED → NEW

Comment 11

13 years ago
Fix checked in - watch for regressions Peter! :)
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

13 years ago
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+.
Status: RESOLVED → VERIFIED

Updated

11 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.