446041 - Firefox 3 only reads various settings from default printer, even when printing on other printers

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Daniel Holbert [:dholbert]]

Regardless of which printer you use, Firefox 3 seems to read the print_margin_XXX from the *default* printer.  (same with print_edge_XXX, and probably more properties)

** Note: I refer below to two pritners, "PrinterA" and "PrinterB".  PrinterA is assumed to be the default printer (in your OS's printer settings).

Steps to reproduce:
1. Start with a fresh profile

2. Print something on both Printer A and Printer B, to initialize their about:config settings.

3. In about:config, search for "print_margin" and make this change:
   print.printerB.print_margin_top = 4.0

4. Print the given URL (or any simple document) on Printer B.

5. Go back to about:config.  Set print.printerB.print_margin_top back to its default value of 0.5 (if it hasn't already been reset, which it may have been -- see "Actual Results") and now make this change for the other printer:
   print.printerA.print_margin_top = 3.0

6. Print the given URL (or any simple document) on Printer B.

ACTUAL RESULTS:
 - In step 4, the printed output looks normal - it ignores the large value that I set in step 3.
 - If I go to about:config after step 4, I see that the value of printerB.print_margin_top has in fact been *overwritten*, and has changed to match printerA.print_margin_top.  i.e., it's already been reset to 0.5
 - In step 6, the printerB output has a huge top margin, even though I changed the margin for the printerA.
 - If I go to about:config after step 6, I see that once again, printerB.print_margin_top has changed to match printerA.print_margin_top. (it now shows "3.00000007264316")

EXPECTED RESULTS:
 - In Step 4, the printed output should use my tweaked print_margin_top value for printerB, and print output with a 4-inch upper margin
 - In Step 6, the printed output should use the reset print_margin_top value for printerB, and print output with a normal 0.5 upper margin.
 - After steps 4 and 6, the printerB.print_margin_top value should still be at the value I set it to. (rather than changing to match printerA.print_margin_top)

Firefox 3.0.1 gives ACTUAL RESULTS.
Firefox 2.0.0.15 gives EXPECTED RESULTS.

Build IDs:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.1) Gecko/2008070206 Firefox/3.0.1
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.15) Gecko/20080702 Ubuntu/8.04 (hardy) Firefox/2.0.0.15

I'm pretty sure this is a cross-platform bug -- bug 445894 comment 31 suggests that it affects Windows XP.  But just in case, I'm leaving this marked Linux-only, until someone on Windows can reproduce the bug using my steps-to-reproduce listed above. (thanks in advance!)

Comment 1

[Daniel Holbert [:dholbert]]

ALTERNATE EXPECTED RESULTS:
  For all print properties that FF3 ignores on non-default printers, we could get rid of the duplicate properties and just use one global property.

Comment 2

[George]

Daniel, that's not what I am seeing (on Windows XP SP3). With print.printerB.print_margin_top = 4.0, I do see a large top margin. My value of 4.0 was not overwritten. At the end of the test, my default printerA still had print_margin_top set to 3.0.

I'll include attachments for my two printerB prints. (My printerB was Adobe PDF, while my printerA was my default Epson printer.)

Comment 3

[George]

Attachment: Test with print.printerB.print_margin_top = 4.0

Comment 4

[George]

Attachment: Test with print.printerA.print_margin_top = 3.0

Comment 5

[Daniel Holbert [:dholbert]]

George -- I'm confused.   From bug 445894 comment 20 and 445894 comment 31, it sounded like your Epson settings were controlling your PDF printer behavior.  But on comment 2 here, it sounds like the Epson settings *aren't* controlling your PDF printer behavior anymore.  Am I getting that right?

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> From bug 445894 comment 20 and 445894 comment 31
oops, forgot 'bug' -- make that "bug 445894 comment 31"

Comment 7

[George]

Daniel: yes, my Epson settings were definitely controlling my PDF printer behaviour. I don't know why this test didn't demonstrate the problem.

I'll try this test again now to see if it makes a difference.

Comment 8

[George]

Daniel, I repeated the steps, but I got exactly the same results as my first try.

Is there something else I can try?

Comment 9

[George]

Daniel, I tried your test a third time, but now I just used my old default profile. I am still getting exactly the same results that I saw before.
 

Comment 10

[Daniel Holbert [:dholbert]]

Ok, I just talked to George on IRC -- after some experimentation, it seems that Firefox3/WinXP always considers the last printer you used to be the "default" one.  (as compared to Firefox3/Linux, which always considers the system-configured default printer to be the "default" one.)

So, my "Steps to Reproduce" from Comment 0 will only work on Windows if you print some document to Printer A just before step 3, and then again just before step 5.  That way, Firefox will consider Printer A to be default during steps 3-4 and steps 5-6.

Comment 11

[Daniel Holbert [:dholbert]]

So, to sum up, here's the situation as I see it:
 - FF3 only ever reads the "default" printer's print-settings (however that's determined)
 - If you choose to print to a non-default printer, the "default" printer's settings overwrite that printer's settings. (And on Windows, that will become the new "default" printer)
 - We could achieve pretty much the exact same behavior by storing *one set* of printer settings, and using those settings no matter what printer is selected.

Is there any reason we store per-printer settings?  

(I can see reasons why one *might* want to store per-printer settings, but given our current broken behavior, I don't think any of those reason really apply.)

Comment 12

[George]

When I followed Daniel's updated "Steps to Reproduce" I am able to reproduce
the problem. I see the Actual Results instead of the Expected Results.

(I am using Windows XP Service Pack 3.)

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So this is a regression against Firefox 2. George, do you have time for finding the regression range? Would be great to have the bug # and the patch which introduced this issue in Firefox 3.

Comment 14

[George]

Henrik, I could spend some time here and there trying to find out when it was broken in Firefox 3.

I would go to /pub/mozilla.org/firefox/nightly (possibly going back to /pub/mozilla.org/firefox/nightly/2007) and look at the 2008-MM-DD-??-mozilla1.9.0 directories, right?

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

No, if you are going back you should have a look at the trunk sub folder e.g. http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2008/01/2008-01-02-05-trunk/

Thanks for taking the time. It would be great to have a daily time span.

Comment 17

[Jan Horak [:jhorak]]

Question is: should be printer settings shared among printers or individual for each printer?

Current implementation is trying to make individual but AFAIK GtkPrintUnixDialog - https://developer.gnome.org/gtk3/3.0/GtkPrintUnixDialog.html does not support it (GtkPrintSettings stores settings only for selected printer).

As example GtkPrintUnixDialog used in Gedit use shared settings. Writing patch where shared setting will be used seems to be quite easy.

Comment 18

[Jan Horak [:jhorak]]

Attachment: Added shared printer settings preference v1

This patch adds 'print.use_individual_settings_for_printers' preference (default: true). 

When set to false, all printers share same settings (like page format, scaling, orientation, header and footer etc.).

Setting this preference to false at least on Linux makes sense, because individual settings are broken currently and it also corresponds to other GTK applications (for example gedit or gimp).

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 727623 [details] [diff] [review]
Added shared printer settings preference v1

Thanks for looking into this!  Hmm... I haven't looked at how this works for quite a while. At the time that I filed this bug, I think we successfully stored & used *some* settings on a per-printer basis, so I wonder if this would break that.  We have to be somewhat careful, because printing is woefully under-tested. :-/  (it's hard to have automated tests for print dialog interaction)

But: assuming for the moment that this patch is taking us in the right direction, a few nits:

>+// When this is set to true each printer store its own PrintSettings,
>+// when false all printers share same settings.
>+pref("print.use_individual_settings_for_printers", true);

Add an "s" at the end of "each printer store" (s/store/stores/)

Also: this pref name is a bit wordy -- maybe "print.use_per_printer_settings"?

>diff --git a/widget/xpwidgets/nsPrintOptionsImpl.cpp b/widget/xpwidgets/nsPrintOptionsImpl.cpp
[...]
>+  // Check if user want to use individual settings for each printer
>+  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID,&rv));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  bool individual_settings;
>+  prefs->GetBoolPref("print.use_individual_settings_for_printers", &individual_settings);
>+  if (individual_settings) 
>+  {

Bump the curly-brace up to end of previous line.

The entire above chunk can also be replaced with:
 if (Preferences::GetBool("print.use_individual_settings_for_printers")) {
if you #include "mozilla/Preferences.h".

>+    // Get the printer name from the PrinterSettings for an optional prefix.
>+    rv = GetAdjustedPrinterName(aPS, aUsePNP, prtName);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    if (prtName.IsEmpty()) {
>+      NS_WARNING("Caller should supply a printer name.");
>+      return NS_OK;
>+    }
>+    // Now read any printer specific prefs
>+    rv = ReadPrefs(aPS, prtName, aFlags);
>+    if (NS_SUCCEEDED(rv))
>+      aPS->SetIsInitializedFromPrefs(true);
>   }

Add curly-braces around the aPS->SetIsInitializedFromPrefs clause. (I know the code didn't have them before, but it sorta should've, and it's more important now that there's a new differently-indented curly-brace right after the clause).

> nsresult
> nsPrintOptions::SavePrintSettingsToPrefs(nsIPrintSettings *aPS,
>                                          bool aUsePrinterNamePrefix,
>                                          uint32_t aFlags)
> {
>   NS_ENSURE_ARG_POINTER(aPS);
>   nsAutoString prtName;
> 
>-  // Get the printer name from the PrinterSettings for an optional prefix.
>-  nsresult rv = GetAdjustedPrinterName(aPS, aUsePrinterNamePrefix, prtName);
>+  nsresult rv;
>+  
>+  // Check if user want to use individual settings for each printer
>+  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
>   NS_ENSURE_SUCCESS(rv, rv);
>+  nsCOMPtr<nsIPrefBranch> prefBranch;
>+  bool individual_settings;
>+  prefs->GetBoolPref("print.use_individual_settings_for_printers", &individual_settings);
>+ 
>+ if (individual_settings) 
>+  {

As above, this can be replaced w/ a one-liner Preferences::GetBoolPref call.

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #19)
> Comment on attachment 727623 [details] [diff] [review]
> Added shared printer settings preference v1
> 
> Hmm... I haven't looked at how this works for
> quite a while. At the time that I filed this bug, I think we successfully
> stored & used *some* settings on a per-printer basis, so I wonder if this
> would break that.

(oh: I guess in comment 11 I was suggesting something along the lines of the attached patch, though not necessarily pref-controlled)

Comment 21

[Daniel Holbert [:dholbert]]

It'd also still be nice to track down what broke this. (Per end of comment 0, it was working in Firefox 2, broken in Firefox 3).   If this was e.g. caused by a small typo in some patch during that range, I'd rather fix this by just reverting that typo.

Comment 22

[Jan Horak [:jhorak]]

Attachment: Added shared printer settings preference v2

Thanks for feedback, I've fixed the patch, attaching and asking for review again.

About regression, at least for Linux I don't think it is a regression in Mozilla code. There's no way to set each printer settings by GtkPrintSettings. I've tried to save settings to file by gtk_print_settings_to_file (which I tried) and it saves print settings of last selected printer only and does not keep settings of other printers.

Gtk application on Linux share settings among printers (the best example is evince, which tries to preserve special printer settings like duplex or dpi - what I'm trying to fix in bug 539427). Enabling shared settings at least for Linux should be convenient.

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 729563 [details] [diff] [review]
Added shared printer settings preference v2

># HG changeset patch
># Parent e23e43a2c14e051ec59b4b186cf2946ce2c1ee58
># User Jan Horak <[e-mail]>
># Bug #446041 - This patch adds 'print.use_individual_settings_for_printers' preference
># to allow sharing printer settings among all printers.
>
>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js
>--- a/modules/libpref/src/init/all.js
>+++ b/modules/libpref/src/init/all.js
>@@ -459,16 +459,20 @@ pref("print.print_footerright", "&D");
> pref("print.show_print_progress", true);
> 
> // xxxbsmedberg: more toolkit prefs
> 
> // When this is set to false each window has its own PrintSettings
> // and a change in one window does not affect the others
> pref("print.use_global_printsettings", true);

observation: This pref ^^^ is now a bit confusingly-named, in light of your new pref.  It'd be better named as "print.use_per_window_settings" (with the opposite true/false value) -- I was going to suggest filing a followup on renaming it, but I actually think we should just drop support for it, so I filed bug 855889 on doing that.

>+// When this is set to true each printer stores its own PrintSettings,
>+// when false all printers share same settings.
>+pref("print.use_per_printer_settings", true);

This comment needs tweaking -- the printer itself doesn't actually store anything for us, of course.

Maybe replace with:
// When this is set to true, we store separate settings for each printer.
// When false, all printers share the same settings.

>diff --git a/widget/xpwidgets/nsPrintOptionsImpl.cpp b/widget/xpwidgets/nsPrintOptionsImpl.cpp
>+  // Check if user want to use individual settings for each printer
>+  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID,&rv));
>+  NS_ENSURE_SUCCESS(rv, rv);

I don't think you need "prefs" here anymore, do you? (now that you're querying the pref with Preferences::GetBool)

>+  if (Preferences::GetBool("print.use_per_printer_settings")) {
>+    // Get the printer name from the PrinterSettings for an optional prefix.
>+    rv = GetAdjustedPrinterName(aPS, aUsePNP, prtName);
>+    NS_ENSURE_SUCCESS(rv, rv);

Your patch leaves us with two copies of this prtName-lookup code -- one outside this clause, and one inside. I imagine you want to delete the outside-this-clause one, right?

> nsresult
> nsPrintOptions::SavePrintSettingsToPrefs(nsIPrintSettings *aPS,
>                                          bool aUsePrinterNamePrefix,
>                                          uint32_t aFlags)
> {
>   NS_ENSURE_ARG_POINTER(aPS);
>   nsAutoString prtName;
> 
>+  nsresult rv;
>+  
>+  // Check if user want to use individual settings for each printer
>+  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
>   NS_ENSURE_SUCCESS(rv, rv);

As above, I don't think you need to look up |prefs| here -- you don't use it, at least.

>+  nsCOMPtr<nsIPrefBranch> prefBranch;
>+  if (Preferences::GetBool("print.use_per_printer_settings")) {
>+    rv = GetAdjustedPrinterName(aPS, aUsePrinterNamePrefix, prtName);

...so this can be "nsresult rv = ..." (since you don't need rv up above)

I think this is good otherwise! As in the other bug, I think you should probably get review from roc on this. This is a non-trivial change to how we handle print settings.

Comment 24

[Daniel Holbert [:dholbert]]

Also, nit on commit message:
># Bug #446041 - This patch adds 'print.use_individual_settings_for_printers' preference
># to allow sharing printer settings among all printers.

Just say "Bug 446041" (no "#"), replace "This patch adds" with "Add", and (less important) just make it one long line rather than two lines.

Comment 25

[Jan Horak [:jhorak]]

Attachment: Added shared printer settings preference v3

Fixed mentioned issues. Kindly asking roc for review.

Comment 26

[Daniel Holbert [:dholbert]]

Actually: I just noticed the "aUsePrinterNamePrefix" argument here.  That should be useful to us.

The existing patch could be reduced to just adding...
 if ((Preferences::GetBool("print.use_per_printer_settings")) {
   aUsePrinterNamePrefix = false;
 }

...towards the beginning of both modified functions.  (in the first one, it looks like the arg is named "aUsePNP")

We also could fix this by fixing the callers to just pass the "correct" value for this arg, according to whatever conditions we intend to use to set your proposed pref (e.g. if platform is linux, callers will presumably want to pass "false").  Then we wouldn't need a pref at all.  I'm not sure whether it'd be kludgier to have those checks all over the place (in the various callers) versus having them centralized and pref-controlled, overriding the behavior that the callers think they're going to get.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 732314 [details] [diff] [review]
Added shared printer settings preference v3

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

Per dholbert's latest comment

Comment 28

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #26)
> We also could fix this by fixing the callers to just pass the "correct"
> value for this arg, according to whatever conditions we intend to use to set
> your proposed pref 

In particular, the relevant callers of "InitPrintSettingsFromPrefs" and "SavePrintSettingsToPrefs" that you'd want to tweak would be the ones in and the ones in printUtils.js (which is used when we actually print), and the ones in nsPrintDialogServiceGTK::ShowPageSetup, at https://mxr.mozilla.org/mozilla-central/source/widget/gtk2/nsPrintDialogGTK.cpp#567  (for File | Page Setup) 

I'm becoming less sure that sharing printer-settings globally is really what we want, though.

Also:
(In reply to jhorak from comment #22)
> Gtk application on Linux share settings among printers

How does this jive with the fact that gtk_print_run_page_setup_dialog() still exists (and is inocable via File | Page Setup), and returns per-printer paper-size information? (and appears to let you customize the page size / margins)? Does it not work anymore?  If GTK can still deal with per-printer page-sizes and margins, I'd expect it should also be able to deal with other per-printer settings, too...

Comment 29

[Daniel Holbert [:dholbert]]

(s/inocable/invokable/)

Comment 30

[Daniel Holbert [:dholbert]]

(Actually, it looks like the Page Setup functionality is now built-in to the print dialog, in its own tab, and other GTK apps (gedit, evince) don't have a standalone "Page Setup" menu item anymore. Not sure when/why that changed -- perhaps we should follow suit and ditch our Page Setup dialog? Not necessarily as part of this bug, but perhaps in a related bug, because it exposes a bunch of printer-specific settings, which is confusing if we're trying to unify settings here.)

Comment 31

[Jan Horak [:jhorak]]

I did some tests with gtk_page_setup_to_file and it looks like Page setup dialog does not work for individual printers (gtk_page_setup_to_file saves it as global settings even when setting specific printer). It might store the settings in memory somewhere, but it doesn't survive application restart. API doesn't support per printer settings (https://developer.gnome.org/gtk3/3.0/GtkPageSetup.html).

Yes, evince and gedit doesn't have Page setup anymore, since page settings moved to print dialog they found it as obsolete so removing Page setup from Firefox/Thunderbird makes sense for GTK frontend. 

I'm for setting aUsePNP to false in callers for Linux systems as you mentioned because it doesn't work anyway.

Comment 32

[Jan Horak [:jhorak]]

Attachment: Disable per printer settings for Linux v1

I've made patch which disables per printer settings for Linux because GTK backend doesn't support it. Please have a look. I'm not sure about JS Linux platform test. Thanks.

Comment 33

[Jan Horak [:jhorak]]

Attachment: Disable per printer settings for Linux v2

Fixed typo.

Comment 34

[Daniel Holbert [:dholbert]]

Comment on attachment 736217 [details] [diff] [review]
Disable per printer settings for Linux v2

Looks good to me.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 736217 [details] [diff] [review]
Disable per printer settings for Linux v2

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

::: toolkit/components/printing/content/printUtils.js
@@ +45,3 @@
>                                         printSettings.kInitSaveAll);
> +        if (savePerPrinterSettings) {
> +          PSSVC.savePrintSettingsToPrefs(printSettings, false,

Putting platform-specific code in here is a bit gross. Can we just have savePrintSettingsToPrefs with kInitSavePrinterName be a noop on Linux?

@@ +137,2 @@
>      // now augment them with any values from last time
> +    aPSSVC.initPrintSettingsFromPrefs(aPrintSettings, initPerPrinterSettings, aPrintSettings.kInitSaveAll);

As above.

Comment 36

[Jan Horak [:jhorak]]

Attachment: Disable per printer settings for Linux v3

I hope you mean something like attached patch. Please have a look.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c626d0a4b3ac

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c626d0a4b3ac