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

RESOLVED FIXED

Status

()

Core
Printing: Output
--
major
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Arne John Glenstrup, Assigned: Kenneth Herron)

Tracking

(Blocks: 1 bug, {fixed-seamonkey1.1a, fixed1.8.1, relnote})

Trunk
fixed-seamonkey1.1a, fixed1.8.1, relnote
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 209026 [details] [diff] [review]
Patch attempting to fix the bug

The most important part is the addition of "& ~gPrintSetInterface.kInitSavePrinterName" in setPrinterDefaultsForSelectedPrinter
Attachment #209026 - Flags: review?
(Reporter)

Updated

12 years ago
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)
(Reporter)

Updated

12 years ago
Attachment #209026 - Flags: review?(mconnor)
(Assignee)

Comment 3

12 years ago
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. 
(Reporter)

Comment 4

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

Comment 5

12 years ago
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.
Attachment #209383 - Flags: superreview?
Attachment #209383 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #209383 - Flags: superreview?(roc)
Attachment #209383 - Flags: superreview?
Attachment #209383 - Flags: review?(mconnor)
Attachment #209383 - Flags: review?

Comment 6

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

Comment 7

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

Updated

12 years ago
Attachment #209383 - Attachment is obsolete: true
Attachment #209383 - Flags: superreview?(roc)
Attachment #209383 - Flags: review?(mconnor)

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

12 years ago
Component: General → Printing
Flags: review?(mconnor)
Product: Firefox → Core
Version: unspecified → Trunk

Comment 8

12 years ago
-> All/All and is a Core rather than Firefox only issue.
OS: Linux → All
Hardware: PC → All

Updated

12 years ago
Blocks: 323781

Updated

12 years ago
Blocks: 312583
(Assignee)

Comment 9

12 years ago
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()|.
Assignee: nobody → kherron+mozilla
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Attachment #209987 - Flags: review?(mconnor)

Comment 10

12 years ago
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

12 years ago
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

12 years ago
Sorry for the repeat, bugzilla timed out on me

Comment 13

12 years ago
(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 ?
(Assignee)

Comment 14

12 years ago
> >+      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

12 years ago
(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
Keywords: relnote
Attachment #209987 - Flags: superreview+
Attachment #209987 - Flags: review?(mconnor)
Attachment #209987 - Flags: review+
(Assignee)

Comment 16

12 years ago
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
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 17

12 years ago
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?
(Assignee)

Comment 18

12 years ago
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)

Comment 19

12 years ago
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.

Updated

11 years ago
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.

Comment 21

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

Comment 23

11 years ago
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 → ---
(Assignee)

Comment 24

11 years ago
I've backed out the patch that was checked in feb. 4th. 

Comment 25

11 years ago
Just wondering how the new patch is progressing or shall I give it a go?

Comment 26

11 years ago
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
Attachment #218479 - Flags: review?(kherron+mozilla)
(Assignee)

Comment 27

11 years ago
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|.

Updated

11 years ago
Attachment #218479 - Flags: review?(kherron+mozilla)

Comment 28

11 years ago
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.
Attachment #218479 - Attachment is obsolete: true
Attachment #219955 - Flags: review?(kherron+mozilla)
(Assignee)

Comment 29

11 years ago
This looks good. Thanks for taking over on this.

Updated

11 years ago
Attachment #219955 - Flags: superreview?(roc)

Updated

11 years ago
Blocks: 326093
(Assignee)

Updated

11 years ago
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+

Comment 31

11 years ago
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
Attachment #219955 - Attachment is obsolete: true
Attachment #220571 - Flags: superreview+
Attachment #220571 - Flags: review+

Updated

11 years ago
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago11 years ago
Resolution: --- → FIXED

Comment 32

11 years ago
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.
(Reporter)

Comment 36

11 years ago
(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.
(Reporter)

Comment 38

11 years ago
(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+

Comment 39

11 years ago
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

11 years ago
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)

Updated

11 years ago
Keywords: fixed-seamonkey1.1a, fixed1.8.1
You need to log in before you can comment on or make changes to this bug.