Closed Bug 324072 Opened 18 years ago Closed 18 years ago

Paper size and printer selection in printdialog are reflected incorrectly in preferences

Categories

(Core :: Printing: Output, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: panic, Assigned: kherron+mozilla)

References

Details

(Keywords: fixed-seamonkey1.1a, fixed1.8.1, relnote)

Attachments

(4 files, 3 obsolete files)

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.
The most important part is the addition of "& ~gPrintSetInterface.kInitSavePrinterName" in setPrinterDefaultsForSelectedPrinter
Attachment #209026 - Flags: review?
Attachment #209026 - Flags: review? → review?(bryner)
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.
Attachment #209026 - Flags: review?(bryner)
Attachment #209026 - Flags: review?(mconnor)
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. 
(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.
Attached patch Another way to fix the bug (obsolete) — Splinter Review
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.
Attachment #209383 - Flags: superreview?
Attachment #209383 - Flags: review?
Attachment #209383 - Flags: superreview?(roc)
Attachment #209383 - Flags: superreview?
Attachment #209383 - Flags: review?(mconnor)
Attachment #209383 - Flags: review?
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.
(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.
Attachment #209383 - Attachment is obsolete: true
Attachment #209383 - Flags: superreview?(roc)
Attachment #209383 - Flags: review?(mconnor)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: General → Printing
Flags: review?(mconnor)
Product: Firefox → Core
Version: unspecified → Trunk
-> All/All and is a Core rather than Firefox only issue.
OS: Linux → All
Hardware: PC → All
Blocks: 323781
Blocks: 312583
Second try. This retrieves a printer list directly from the nsIPrinterEnumerator object, instead of calling |AvailablePrinters()|.
Assignee: nobody → kherron+mozilla
Status: NEW → ASSIGNED
Attachment #209987 - Flags: review?(mconnor)
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 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?
Sorry for the repeat, bugzilla timed out on me
(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 ?
> >+      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.
(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
Attachment #209987 - Flags: superreview+
Attachment #209987 - Flags: review?(mconnor)
Attachment #209987 - Flags: review+
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.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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 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.
Attachment #209987 - Flags: branch-1.8.1?(roc)
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.
Depends on: 326363
(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.
(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 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.
Attachment #209987 - Flags: approval-branch-1.8.1?(roc) → approval-branch-1.8.1-
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I've backed out the patch that was checked in feb. 4th. 
Just wondering how the new patch is progressing or shall I give it a go?
Attached patch Widget non-API changing patch v3 (obsolete) — Splinter Review
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
Attachment #218479 - Flags: review?(kherron+mozilla)
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|.
Attachment #218479 - Flags: review?(kherron+mozilla)
Changes since v3.0:
* Moved setting of printer name from InitPrintSettingsFromPrefs into _CreatePrintSettings as suggested - will prevent bustage on OSX too.
Attachment #218479 - Attachment is obsolete: true
Attachment #219955 - Flags: review?(kherron+mozilla)
This looks good. Thanks for taking over on this.
Attachment #219955 - Flags: superreview?(roc)
Blocks: 326093
Attachment #219955 - Flags: review?(kherron+mozilla) → review+
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)
?
Attachment #219955 - Flags: superreview?(roc) → superreview+
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
Attachment #219955 - Attachment is obsolete: true
Attachment #220571 - Flags: superreview+
Attachment #220571 - Flags: review+
Status: REOPENED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
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.
Attachment #220571 - Flags: approval-branch-1.8.1?(roc)
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.
I have to add that I see the same behaviour w/o the patch. So this might be another issue.
Maybe I should just shut off because the patch wasn't applied correctly in the build.
(In reply to comment #33)

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

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.
(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.
Attachment #220571 - Flags: approval-branch-1.8.1?(roc) → approval-branch-1.8.1+
This is the branch version of patch v3.2 also includes fixes from bug 304646. Will checkin once branch is open again.
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
Attachment #221749 - Attachment description: Branch patch v3.2 → Branch patch v3.2 (Checked in 1.8 branch)
You need to log in before you can comment on or make changes to this bug.