Closed Bug 1425188 Opened 2 years ago Closed 2 years ago

Remove the Postscript option from the GTK print dialog window

Categories

(Core :: Widget: Gtk, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

As a step towards removing cairo from the tree, we should remove the Postscript option from the GTK print dialog window. (This option appears when the "Print to File" "Printer" is selected.

(Chromium does not provide the ability to output Postscript FWIW.)
Attached patch patch (obsolete) — Splinter Review
Attachment #8936751 - Flags: review?(karlt)
Attached patch patch (obsolete) — Splinter Review
Attachment #8936751 - Attachment is obsolete: true
Attachment #8936751 - Flags: review?(karlt)
Attachment #8936755 - Flags: review?(karlt)
Comment on attachment 8936755 [details] [diff] [review]
patch

>-    const gchar* fmtGTK =
>-      gtk_print_settings_get(mPrintSettings,
>-                             GTK_PRINT_SETTINGS_OUTPUT_FILE_FORMAT);
>-    if (fmtGTK) {
>-      if (nsDependentCString(fmtGTK).EqualsIgnoreCase("pdf")) {
>-        format = nsIPrintSettings::kOutputFormatPDF;
>-      } else {
>-        format = nsIPrintSettings::kOutputFormatPS;
>-      }

nsPrintSettingsGTK::SetToFileName() can still set "ps" in the
GtkPrintSettings.  Wouldn't this cause a problem if GTK thinks the output is
PS but Gecko thinks it is PDF?
Flags: needinfo?(jwatt)
Attachment #8936755 - Flags: review?(karlt)
(In reply to Karl Tomlinson (back Jan 3 :karlt) from comment #3)
> Wouldn't this cause a problem if GTK thinks the output is
> PS but Gecko thinks it is PDF?

It would do, *if* that were to happen.

nsPrintSettingsGTK::SetToFileName seems to have three callers:

- nsPrinterEnumeratorGTK::InitPrintSettingsFromPrinter passes a
  hardcoded ".pdf" file name.

- nsPrintOptions::DeserializeToPrintSettings deserializes and
  passes whatever value was saved to cache previously (as opposed
  to creating something that might be .ps).

- nsPrintOptions::ReadPrefs passes the value saved to the
  pref print.print_to_filename (or a
  print.<printer-name>.print_to_filename pref).

Only the latter seems capable of passing a ".ps" file name to SetToFileName, and presumably those prefs can have .ps values if the user last saved to file a Postscript file.

Since this patch would remove the save as Postscript option from the print dialog and only allowing PDF, it would seem to make sense to me to:

- convert any old .ps value from the prefs in nsPrintOptions::ReadPrefs

- replace the .ps part of the if-else check in nsPrintSettingsGTK::SetToFileName
  with an assertion
Flags: needinfo?(jwatt)
Good catch BTW.
Attached patch patchSplinter Review
Attachment #8936755 - Attachment is obsolete: true
Attachment #8940008 - Flags: review?(karlt)
Comment on attachment 8940008 [details] [diff] [review]
patch

>+  MOZ_ASSERT(StringEndsWith(aToFileName, NS_LITERAL_STRING(".ps")),
>+             "We should no longer be asked to save Postscript files");

nsIPrintSettings toFilename is scriptable, and such a restriction in the
filename is not documented, and so I'm not happy with this assertion.
r+ on the rest of the patch if you drop this assertion.

>+      if (StringEndsWith(str, NS_LITERAL_STRING(".ps"))) {
>+        // We only support PDF since bug 1425188 landed.  Users may still have
>+        // prefs with .ps filenames if they last saved a file as Postscript
>+        // though, so we fix that up here.  (The pref values will be
>+        // overwritten the next time they save to file as a PDF.)
>+        str.Truncate(str.Length() - 2);
>+        str.AppendLiteral("pdf");
>+      }

In a build without this patch, if I print to file and set the filename to
mozilla.svg, then I have print.print_to_filename;/home/karl/mozilla.svg, but
printing again, the dialog shows mozilla.pdf ‎so I suspected that manually
changing the pref would be unnecessary.  However, I assume it was
file_printer_output_file_format_changed() changing the suffix, and that
function would not be called when only PDF is supported, and so I expect
different behavior with this patch.

Changing the pref suffix from ps to pdf is therefore probably helpful to
indicate what's changed to any users in this situation, and so r+ on this
change.  If this causes any problems, however, I'm also happy if you drop this
part.  UNIX systems are usually more clever than assuming that a filename
indicates its format.
Attachment #8940008 - Flags: review?(karlt) → review+
(In reply to Karl Tomlinson (:karlt) from comment #7)
> However, I assume it was
> file_printer_output_file_format_changed() changing the suffix, and that
> function would not be called when only PDF is supported, and so I expect
> different behavior with this patch.

Yes, that's right. So this part of the patch is indeed needed to make sure we don't save PDF contents into a .ps file for users that last saved to a Postscript file before getting this change.

> UNIX systems are usually more clever than assuming that a filename
> indicates its format.

True, but it would still be poor form to create PDF files with a .ps extension so I'll keep this part of the patch.
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c85b83645bb3
Remove the ability to save as Postscript from the print dialog on Linux. r=karlt
https://hg.mozilla.org/mozilla-central/rev/c85b83645bb3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Blocks: 1322756
You need to log in before you can comment on or make changes to this bug.