Last Comment Bug 324072 - Paper size and printer selection in printdialog are reflected incorrectly in preferences
: Paper size and printer selection in printdialog are reflected incorrectly in ...
Status: RESOLVED FIXED
: fixed-seamonkey1.1a, fixed1.8.1, relnote
Product: Core
Classification: Components
Component: Printing: Output (show other bugs)
: Trunk
: All All
: -- major (vote)
: ---
Assigned To: Kenneth Herron
:
Mentors:
Depends on: 326363
Blocks: 326093 312583 323781
  Show dependency treegraph
 
Reported: 2006-01-19 15:27 PST by Arne John Glenstrup
Modified: 2006-05-15 14:39 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch attempting to fix the bug (2.86 KB, patch)
2006-01-19 15:30 PST, Arne John Glenstrup
no flags Details | Diff | Review
Another way to fix the bug (20.51 KB, patch)
2006-01-23 12:32 PST, Kenneth Herron
no flags Details | Diff | Review
nsIPrintSettingsService change v2 (20.27 KB, patch)
2006-01-28 12:40 PST, Kenneth Herron
roc: review+
roc: superreview+
roc: approval‑branch‑1.8.1-
Details | Diff | Review
Widget non-API changing patch v3 (7.34 KB, patch)
2006-04-14 16:49 PDT, Ian Neal
no flags Details | Diff | Review
Tweaked widget non-API changing patch v3.1 (7.60 KB, patch)
2006-04-26 17:04 PDT, Ian Neal
kherron+mozilla: review+
roc: superreview+
Details | Diff | Review
Adjusted for loop non-API changing patch v3.2 (7.60 KB, patch)
2006-05-02 16:04 PDT, Ian Neal
iann_bugzilla: review+
iann_bugzilla: superreview+
roc: approval‑branch‑1.8.1+
Details | Diff | Review
Branch patch v3.2 (Checked in 1.8 branch) (9.13 KB, patch)
2006-05-11 14:53 PDT, Ian Neal
no flags Details | Diff | Review

Description Arne John Glenstrup 2006-01-19 15:27:04 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20060119 Firefox/1.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20060119 Firefox/1.5

There is a SEVERE bug in printdialog.js, causing printer
settings to be used incorrectly: In setPrinterDefaultsForSelectedPrinter,
the intention is to
1) First set gPrintSettings to printer default values,
2) Then augment them with preference settings for that printer.
However, in calling printService.initPrintSettingsFromPrefs, the
printer name currently in the preferences (print.print_printer)
is stored into gPrintSettings, and then used to determine from
which printer any augmenting settings should be selected.
This can lead to the following wrong scenario, assuming print.print_printer
is "Aprinter":
1) User opens printdialog, selects "Bprinter"
2) User clicks "Properties...", and is presented with "Aprinter"-specific options, which might not be applicable to "Bprinter"!
3) After printing, any changes are stored into "Aprinter"'s preferences.
This bug may also be the cause of many frustrated "I can't change the
default 'Letter' paper size"-complaints?

Reproducible: Always

Steps to Reproduce:
1. Open printdialog, select a different printer
2. Click "Properties...", select a different paper size
3. Click OK, OK to Print
4. Repeat from 1

Actual Results:  
The current printer is not changed, and the newly selected 
printer's options are unchanged as well.

Expected Results:  
Current printer change and its options should be stored correctly in preferences.
Comment 1 Arne John Glenstrup 2006-01-19 15:30:24 PST
Created attachment 209026 [details] [diff] [review]
Patch attempting to fix the bug

The most important part is the addition of "& ~gPrintSetInterface.kInitSavePrinterName" in setPrinterDefaultsForSelectedPrinter
Comment 2 Brian Ryner (not reading) 2006-01-19 17:16:01 PST
Comment on attachment 209026 [details] [diff] [review]
Patch attempting to fix the bug

can you get mconnor to review this? he's reviewed changes to this code before; i'm not really familiar with it.
Comment 3 Kenneth Herron 2006-01-20 14:09:00 PST
This is the same issue as bug 312583. I've spent a little time working that bug so I hope you don't mind if I comment.

Print settings are written to two sets of prefs, a general set based on the last print job and a printer-specific set. The name of the printer is stored in "print.print_printer" and "print.printer_<name>.print_printer". The first of these is effectively the name of the last printer used, and the second is useless (name of the printer when using that printer).

|initPrintSettingsFromPrefs()| receives an nsIPrintSettings object which may be initialized with a printer name. It should load the general prefs followed by the prefs for the named printer. However, when the caller specifies to load all prefs (the kInitSaveAll flag), the printer name is overwritten with the name of the last printer used while loading the general prefs.

I think the core issue is that the Print Settings interface is a little misdesigned. |initPrintSettingsFromPrefs()| shouldn't be loading the printer name pref at all; the print settings service should provide a separate function to load the last-used printer. The name of the last-used printer is only needed when initializing the dialog, but |initPrintSettingsFromPrefs()| is called every time the user manipulates the printer widget to select a different printer, and it's usually called with a printer name specified. I don't think the caller should have to specify flags like |kInitSaveAll & ~kInitSavePrinterName| to make this function do the right thing. Also when saving prefs, the last printer should only be written to the general pref, not the printer-specific one. 
Comment 4 Arne John Glenstrup 2006-01-21 16:13:27 PST
(In reply to comment #3)
> I don't think
> the caller should have to specify flags like |kInitSaveAll &
> ~kInitSavePrinterName| to make this function do the right thing.

So, are you saying that "& ~nsIPrintSettings::kInitSavePrinterName"
should be added to both calls to ReadPrefs in 
mozilla/gfx/src/nsPrintOptionsInpl.cpp:1089:
nsPrintOptions::InitPrintSettingsFromPrefs
or are you into some deeper changes?

> Also when
> saving prefs, the last printer should only be written to the general pref, not
> the printer-specific one. 

Well, as you say, the printer-specific printer name preference is useless,
so it should do no real harm to write it.
Comment 5 Kenneth Herron 2006-01-23 12:32:57 PST
Created attachment 209383 [details] [diff] [review]
Another way to fix the bug

I've tested this with seamonkey & firefox on linux. It corrects the problem from bug 312583 of the printer properties dialog showing properties for the wrong printer. I think bug 323781 is also being caused by this issue, but I don't build on windows so I can't test that.

This patch changes the nsIPrintSettingsService API a bit. |initPrintSettingsFromPrefs| is too complicated and poorly described. The description in the IDL (see <http://lxr.mozilla.org/seamonkey/source/gfx/idl/nsIPrintSettingsService.idl#89>) claims that it reads prefs for the printer named in its aPrintSettings argument. But in fact it may only read the general preferences, and it may overwrite the printer name in aPrintSettings with the name of the last printer used. This makes it difficult to use the function correctly, leading to bugs bug 312583. So I've changed |initPrintSettingsFromPrefs| to never read the printer name preference, and to always read prefs for the printer named in aPrintSettings. This makes it just a way to read prefs for a printer specified by the caller.

nsIPrintSettingsService also contains an attribute |defaultPrinterName|. Right now it's just the system default printer. This value isn't really all that useful; in most cases where we want default to a printer, we really want the last printer used, not necessarily the system default. Since |initPrintSettingsFromPrefs| no longer reads the last-printer pref, I've augmented |GetDefaultPrinterName| to return  the user's last printer if it's still valid, and otherwise return the system default as before.

This is an alternative to Arne's patch, and I think it's also an alternative to Ian Neal's patch in bug 323781.
Comment 6 Ian Neal 2006-01-24 06:17:08 PST
Comment on attachment 209383 [details] [diff] [review]
Another way to fix the bug

>Index: gfx/src/nsPrintOptionsImpl.cpp
>===================================================================
>@@ -979,16 +980,49 @@ nsPrintOptions::GetNewPrintSettings(nsIP
> NS_IMETHODIMP
> nsPrintOptions::GetDefaultPrinterName(PRUnichar * *aDefaultPrinterName)
> {
>   nsresult rv;
>   nsCOMPtr<nsIPrinterEnumerator> prtEnum = do_GetService(kPrinterEnumeratorCID,
>                                                          &rv);
>   NS_ENSURE_SUCCESS(rv, rv);
Can this bit go after the end of the added bit?
> 
>+  // Look up the printer from the last print job
>+  nsAutoString lastPrinterName;

>@@ -1100,22 +1134,19 @@ nsPrintOptions::InitPrintSettingsFromPre
>   nsAutoString prtName;
>   // read any non printer specific prefs
>   // with empty printer name
>   nsresult rv = ReadPrefs(aPS, prtName, aFlags);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // Get the Printer Name from the PrintSettings
>   // to use as a prefix for Pref Names
>-  rv = GetAdjustedPrinterName(aPS, aUsePNP, prtName);
>+  rv = GetAdjustedPrinterName(aPS, PR_TRUE, prtName);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  if (prtName.IsEmpty())
>-    return NS_OK;
>-
Why remove this? Is prtName always non-empty?

>   // Now read any printer specific prefs
>   rv = ReadPrefs(aPS, prtName, aFlags);
>   if (NS_SUCCEEDED(rv))
>     aPS->SetIsInitializedFromPrefs(PR_TRUE);
> 
>   return NS_OK;
> }

I have tested I can build and it seems to give the right output from the JS side of things and testing by printing to printer A, deleting printer A and then printing again, does go to printer B.
Probably needs to be tested on OSX too though.
Comment 7 Kenneth Herron 2006-01-24 09:41:29 PST
(In reply to comment #6)
> (From update of attachment 209383 [details] [diff] [review] [edit])
> >Index: gfx/src/nsPrintOptionsImpl.cpp
> >===================================================================
> >@@ -979,16 +980,49 @@ nsPrintOptions::GetNewPrintSettings(nsIP
> > NS_IMETHODIMP
> > nsPrintOptions::GetDefaultPrinterName(PRUnichar * *aDefaultPrinterName)
> > {
> >   nsresult rv;
> >   nsCOMPtr<nsIPrinterEnumerator> prtEnum = do_GetService(kPrinterEnumeratorCID,
> >                                                          &rv);
> >   NS_ENSURE_SUCCESS(rv, rv);
> Can this bit go after the end of the added bit?

Hmm. Why does gecko have two separate printer enumerator classes? And how did I manage to use both of them in the same function without noticing? Yeah, there's probably a better way to do this.

> >-  if (prtName.IsEmpty())
> >-    return NS_OK;
> >-
> Why remove this? Is prtName always non-empty?

Under the new model, the caller is supposed to always specify a printer name, so it shouldn't be empty. If it is empty, it's harmless; the code will read the general prefs a second time.

> I have tested I can build and it seems to give the right output from the JS
> side of things and testing by printing to printer A, deleting printer A and
> then printing again, does go to printer B.
> Probably needs to be tested on OSX too though.

Thanks for testing it. I'll have an updated patch out in a couple of days.
Comment 8 Ian Neal 2006-01-26 08:19:03 PST
-> All/All and is a Core rather than Firefox only issue.
Comment 9 Kenneth Herron 2006-01-28 12:40:29 PST
Created attachment 209987 [details] [diff] [review]
nsIPrintSettingsService change v2

Second try. This retrieves a printer list directly from the nsIPrinterEnumerator object, instead of calling |AvailablePrinters()|.
Comment 10 Ian Neal 2006-01-28 14:33:44 PST
Comment on attachment 209987 [details] [diff] [review]
nsIPrintSettingsService change v2

>Index: gfx/src/nsPrintOptionsImpl.cpp
>===================================================================
>@@ -979,16 +980,42 @@ nsPrintOptions::GetNewPrintSettings(nsIP
> NS_IMETHODIMP
> nsPrintOptions::GetDefaultPrinterName(PRUnichar * *aDefaultPrinterName)
> {
>   nsresult rv;
>   nsCOMPtr<nsIPrinterEnumerator> prtEnum = do_GetService(kPrinterEnumeratorCID,
>                                                          &rv);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  // Look up the printer from the last print job
>+  nsAutoString lastPrinterName;
>+  ReadPrefString(kPrinterName, lastPrinterName);
>+  if (!lastPrinterName.IsEmpty()) {
>+    // Verify it's still a valid printer
>+    PRUnichar **printers;
>+    PRUint32 ctPrinters;
>+    rv = prtEnum->EnumeratePrinters(&ctPrinters, &printers);
>+    if (NS_SUCCEEDED(rv)) {
>+      PRBool isValid = PR_FALSE;
>+      for (int ii = ctPrinters; ii--; ) {
This doesn't look right. Should it be:
        for (int ii = ctPrinters - 1; ii >= 0; --ii) {
or something like that?
Comment 11 Ian Neal 2006-01-28 14:35:41 PST
Comment on attachment 209987 [details] [diff] [review]
nsIPrintSettingsService change v2

>Index: gfx/src/nsPrintOptionsImpl.cpp
>===================================================================
>@@ -979,16 +980,42 @@ nsPrintOptions::GetNewPrintSettings(nsIP
> NS_IMETHODIMP
> nsPrintOptions::GetDefaultPrinterName(PRUnichar * *aDefaultPrinterName)
> {
>   nsresult rv;
>   nsCOMPtr<nsIPrinterEnumerator> prtEnum = do_GetService(kPrinterEnumeratorCID,
>                                                          &rv);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  // Look up the printer from the last print job
>+  nsAutoString lastPrinterName;
>+  ReadPrefString(kPrinterName, lastPrinterName);
>+  if (!lastPrinterName.IsEmpty()) {
>+    // Verify it's still a valid printer
>+    PRUnichar **printers;
>+    PRUint32 ctPrinters;
>+    rv = prtEnum->EnumeratePrinters(&ctPrinters, &printers);
>+    if (NS_SUCCEEDED(rv)) {
>+      PRBool isValid = PR_FALSE;
>+      for (int ii = ctPrinters; ii--; ) {
This doesn't look right. Should it be:
        for (int ii = ctPrinters - 1; ii >= 0; --ii) {
or something like that?
Comment 12 Ian Neal 2006-01-28 14:36:36 PST
Sorry for the repeat, bugzilla timed out on me
Comment 13 Ian Neal 2006-01-28 14:42:06 PST
(In reply to comment #11)
> >+      for (int ii = ctPrinters; ii--; ) {
> This doesn't look right. Should it be:
>         for (int ii = ctPrinters - 1; ii >= 0; --ii) {
> or something like that?
> 
Oh and shouldn't ii be a PRUint32 ?
Comment 14 Kenneth Herron 2006-01-29 00:57:19 PST
> >+      for (int ii = ctPrinters; ii--; ) {
> This doesn't look right. Should it be:
>         for (int ii = ctPrinters - 1; ii >= 0; --ii) {
> or something like that?

It's a common idiom for a countdown loop. I used a countdown loop because it doesn't matter in this case which way it iterates through the list, and because compilers can generate more efficient code for a comparison to zero than for a comparison to a number stored in a variable.

However, you're right that it should be a PRUint32. I could fix that when it's checked in, if that's okay with the reviewer.
Comment 15 Ian Neal 2006-01-30 09:36:08 PST
(In reply to comment #9)
> Created an attachment (id=209987) [edit]
> nsIPrintSettingsService change v2
> 
> Second try. This retrieves a printer list directly from the
> nsIPrinterEnumerator object, instead of calling |AvailablePrinters()|.
> 

Okay, tested this patch too and it fixes the issues I was having over in bug 323781
Comment 16 Kenneth Herron 2006-02-04 11:48:15 PST
Checked in, along with a change to mozilla/camino/src/embedding/CHBrowserView.mm that I had missed because I don't build camino. Resolving fixed.
Comment 17 Jesus Cea 2006-02-05 05:14:01 PST
Will this patch be applied to 1.8.0 and 1.8.1 branches?. The issue seems severe enough to deploy in the 1.5.0.* releases.

Must someone requests a blocking flag?
Comment 18 Kenneth Herron 2006-02-05 06:57:05 PST
Comment on attachment 209987 [details] [diff] [review]
nsIPrintSettingsService change v2

Requesting to check in on the 1.8 branch. If this patch breaks the rule about changing XPCOM interfaces, I could work up an alternate patch that doesn't change nsIPrintSettingsService.
Comment 19 Jesus Cea 2006-02-05 07:59:39 PST
Kenneth, I guess it would be very nice if you could keep the interface. Then it could be committed the patch also in the 1.8.0 branch.
Comment 20 Daniel Veditz [:dveditz] 2006-02-09 15:10:06 PST
(In reply to comment #19)
> Kenneth, I guess it would be very nice if you could keep the interface. Then it
> could be committed the patch also in the 1.8.0 branch.

The API-conservation rules apply to the 1.8 branch and Firefox 2, not just the 1.8.0 security/stability branch.
Comment 21 Ian Neal 2006-02-13 05:56:37 PST
(In reply to comment #18)
> (From update of attachment 209987 [details] [diff] [review] [edit])
> Requesting to check in on the 1.8 branch. If this patch breaks the rule about
> changing XPCOM interfaces, I could work up an alternate patch that doesn't
> change nsIPrintSettingsService.
> 
Are you progressing this or shall I try and knock something up quickly?
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-02-13 13:10:06 PST
Comment on attachment 209987 [details] [diff] [review]
nsIPrintSettingsService change v2

right, as per the interface rules, this can't land on the 1.8.1 branch as-is.
Comment 23 Kenneth Herron 2006-02-13 15:00:09 PST
Reopening. I think this bug should be fixed on the branch, but I don't think it's worth forking the nsIPrintSettingsService API to do it. I'm going to back out the previous patch and develop something that's acceptable for the branch.
Comment 24 Kenneth Herron 2006-03-04 10:13:00 PST
I've backed out the patch that was checked in feb. 4th. 
Comment 25 Ian Neal 2006-04-12 07:11:56 PDT
Just wondering how the new patch is progressing or shall I give it a go?
Comment 26 Ian Neal 2006-04-14 16:49:39 PDT
Created attachment 218479 [details] [diff] [review]
Widget non-API changing patch v3

This patch:
* Removes the need to change the API by using the fact that when aUsePNP is false we also want to be using the printer obtained from GetDefaultPrinterName.
* Keeps the ReadPrefs, WritePrefs and GetDefaultPrinterName.

I have tested the patch on Windows XP and it does resolve my issue from Bug 323781
Comment 27 Kenneth Herron 2006-04-20 15:02:55 PDT
Comment on attachment 218479 [details] [diff] [review]
Widget non-API changing patch v3

>@@ -1092,29 +1113,40 @@ nsPrintOptions::InitPrintSettingsFromPre
>   NS_ENSURE_ARG_POINTER(aPS);
> 
>   PRBool isInitialized;
>   aPS->GetIsInitializedFromPrefs(&isInitialized);
> 
>   if (isInitialized)
>     return NS_OK;
> 
>+  nsresult rv;
>+
>+  if (!aUsePNP) {
>+    nsXPIDLString printerName;
>+    rv = GetDefaultPrinterName(getter_Copies(printerName));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    aPS->SetPrinterName(printerName.get());
>+  }

Per our conversation, overall this patch looks great. But if it's feasible I'd like to avoid setting a printer name at all inside |InitPrintSettingsFromPrefs|. Instead the caller should set a printer name first if it wants per-printer prefs. Scanning through the code, it looks like there is only one caller (aside from mac, which doesn't use per-printer prefs) that doesn't set a printer name first, |nsPrintOptions::_CreatePrintSettings|.
Comment 28 Ian Neal 2006-04-26 17:04:02 PDT
Created attachment 219955 [details] [diff] [review]
Tweaked widget non-API changing patch v3.1

Changes since v3.0:
* Moved setting of printer name from InitPrintSettingsFromPrefs into _CreatePrintSettings as suggested - will prevent bustage on OSX too.
Comment 29 Kenneth Herron 2006-04-27 06:59:27 PDT
This looks good. Thanks for taking over on this.
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-05-02 15:09:16 PDT
Comment on attachment 219955 [details] [diff] [review]
Tweaked widget non-API changing patch v3.1

+      for (PRUint32 ii = ctPrinters; ii--; ) {

Can you rewrite this as the more usual
    for (PRInt32 ii = ctPrinters - 1; ii >= 0; --ii)
?
Comment 31 Ian Neal 2006-05-02 16:04:03 PDT
Created attachment 220571 [details] [diff] [review]
Adjusted for loop non-API changing patch v3.2

Changes since v3.1:
* For loop tweaked as requested by roc

Carrying forward r/sr

Checking in (trunk)
public/nsIPrintSettingsService.idl;
new revision: 1.5; previous revision: 1.4
src/xpwidgets/nsPrintOptionsImpl.cpp;
new revision: 1.83; previous revision: 1.82
done
Comment 32 Ian Neal 2006-05-08 15:37:37 PDT
Comment on attachment 220571 [details] [diff] [review]
Adjusted for loop non-API changing patch v3.2

Would be good to get this onto the 1.8.1 branch, no issues seem to have appeared on the trunk from this checkin.
Comment 33 Wolfgang Rosenauer [:wolfiR] 2006-05-10 03:22:36 PDT
I've tested the latest patch on 1.5.0.3 but something seems to fail for me.
I changed the paper size and the print command for Postscript/default and the settings seems to be correct in the prefs.
print.printer_Postscript/default.print_command shows the new value.

But when I revisit the UI for the Postscript/default printer I see again the original value "lpr ..." in the print command.
Another issue I see is that the setting doesn't get saved after closing the properties dialog with OK but only if I really print afterwards.
Comment 34 Wolfgang Rosenauer [:wolfiR] 2006-05-10 03:43:57 PDT
I have to add that I see the same behaviour w/o the patch. So this might be another issue.
Comment 35 Wolfgang Rosenauer [:wolfiR] 2006-05-10 03:48:10 PDT
Maybe I should just shut off because the patch wasn't applied correctly in the build.
Comment 36 Arne John Glenstrup 2006-05-10 03:52:06 PDT
(In reply to comment #33)

Not having time to check details, is this problem related to
bug 326093?

Comment 37 Wolfgang Rosenauer [:wolfiR] 2006-05-10 11:02:42 PDT
Tested again with the patch applied correctly.
It works as expected.

NB: What I still don't like is that I have to print to save these settings.
Comment 38 Arne John Glenstrup 2006-05-10 11:38:31 PDT
(In reply to comment #37)

Yes, but it would be worse if the settings were saved when you
click "Cancel"; users expect their actions (setting print settings)
to be ignored when clicking cancel.

Several alternatives to solving this problem exist in various
applications.  One possibility is to add an "Apply" or "Save
settings" button, another is to add a "Printer settings..." item
to the File menu, which would bring up an identical dialog, but
with the "Print" button replaced with a "Save" button.

Either way, it seems to be a different bug than this one.
Comment 39 Ian Neal 2006-05-11 14:53:17 PDT
Created attachment 221749 [details] [diff] [review]
Branch patch v3.2 (Checked in 1.8 branch)

This is the branch version of patch v3.2 also includes fixes from bug 304646. Will checkin once branch is open again.
Comment 40 Ian Neal 2006-05-15 14:38:29 PDT
Comment on attachment 221749 [details] [diff] [review]
Branch patch v3.2 (Checked in 1.8 branch)

Checking in (1.8 branch)
idl/nsIPrintSettingsService.idl;
new revision: 1.2.28.1; previous revision: 1.2
src/nsPrintOptionsImpl.cpp;
new revision: 1.76.2.1; previous revision: 1.76
done

Note You need to log in before you can comment on or make changes to this bug.