Print settings on Linux are saved at shutdown but not read at next start

RESOLVED FIXED in mozilla1.2alpha

Status

()

Core
Printing: Output
--
blocker
RESOLVED FIXED
16 years ago
14 years ago

People

(Reporter: Roland Mainz, Assigned: Roland Mainz)

Tracking

(Blocks: 1 bug, {fixedOEM, regression})

Trunk
mozilla1.2alpha
All
Linux
fixedOEM, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

16 years ago
Print settings on Linux/Unix platforms are saved at shutdown but not read at
next start
(Assignee)

Comment 1

16 years ago
My fault, introduced with bug 147605 ("Printing / Page settings reset themselves
after print (no landscape printing)") ... ;-(

... taking myself...
Assignee: rods → Roland.Mainz
Depends on: 147605
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.2alpha
(Assignee)

Updated

16 years ago
Keywords: regression

Comment 2

16 years ago
isn't this the same as bug 83750?
(Assignee)

Comment 3

16 years ago
bugzilla@bkor.dhs.org wrote:
> isn't this the same as bug 83750?

This bug is a spin-off for Linux since the upcoming fix is for the XUL print
dialog - which is only used on Linux/Unix and OS/2.
(Assignee)

Comment 4

16 years ago
Created attachment 97530 [details] [diff] [review]
Patch for 2002-08-30-08-trunk
(Assignee)

Comment 5

16 years ago
Requesting r=/sr= ...
Keywords: patch, review

Comment 6

16 years ago
Comment on attachment 97530 [details] [diff] [review]
Patch for 2002-08-30-08-trunk

>+
This extra new line is not needed.
> //---------------------------------------------------

>+var init_from_selection_once = true;
>+
This variable's name is a little strange. Maybe "firstTime" would be better.
But it's not a problem, just a thinking

r=pete, thanks Roland for this patch.
Attachment #97530 - Flags: review+
Comment on attachment 97530 [details] [diff] [review]
Patch for 2002-08-30-08-trunk

sr=bzbarsky
Attachment #97530 - Flags: superreview+

Comment 8

16 years ago
Roland,
I tested this patch, but I find this patch will cause the fix for bug 147605
doesn't work. The setting will be reset every time open the print dialog. I will
do more test later and let you know the result.
Comment on attachment 97530 [details] [diff] [review]
Patch for 2002-08-30-08-trunk

OK.  I'm rescinding sr till we have a patch that's actually been tested and
works.
Attachment #97530 - Flags: superreview+

Comment 10

16 years ago
Created attachment 97575 [details] [diff] [review]
patch

This patch will fix the problem and will not break the fix for bug 147605.
I just let the function load needed value from the prefs everytime and load
value from printer only when the user change the printer name from the list.
Roland, please take a look and give some suggestion. Thanks!
(Assignee)

Updated

16 years ago
Attachment #97575 - Flags: needs-work+
(Assignee)

Comment 11

16 years ago
Comment on attachment 97575 [details] [diff] [review]
patch

I am not sure whether it's wise to handle InitPrintSettingsFromPrinter() and
InitPrintSettingsFromPrefs() seperately... remember that I was complaining in
bug 147605 about the fact that we have too many init functions without any
protection from doing the wrong things (I still think that we should get rid of
the global |nsIPrintSettings| object and use per-printer objects).

Comment 12

16 years ago
*** Bug 166321 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 13

16 years ago
Maybe we should rename |InitPrintSettingsFromPrinter| to
|InitPrintPrefsFromPrinter| and only call it if there are no printer-specific
prefs for this printer yet - that would solve all problems:
- We get printer-specific defaults
- We would fetch the device defaults only _once_
- We would have a clean way to work around the limitations of the global
nsIPrintSettings object 
- We would populate the prefs (currently we're writing into the global
|nsIPrintSettings| object) for this printer once from the device defaults - and
let |InitPrintSettingsFromPrefs| init the print settings object

Updated

16 years ago
Blocks: 157675
(Assignee)

Comment 14

16 years ago
Created attachment 97588 [details] [diff] [review]
Patch for 2002-08-30-08-trunk

After some discussion and answering petez's question I had the feeling that
original code before bug 147605 was working correctly except that someone else
was calling a |InitPrintSettingsFrom*|-function some somewhere else.

After looking around I found that mozilla/gfx/src/nsPrintOptionsImpl.cpp is
calling |InitPrintSettingsFromPrinter()| each time we get the global
nsIPrintSettings object througth the print setting service. Removing that code 
(and backing out the "workaround" from bug 147605) seems to fix the problem...
Attachment #97530 - Attachment is obsolete: true
Attachment #97575 - Attachment is obsolete: true
(Assignee)

Comment 15

16 years ago
Comment on attachment 97588 [details] [diff] [review]
Patch for 2002-08-30-08-trunk

Nit: The "can't print landscape"-bug is back, but that's easy to fix...
Attachment #97588 - Flags: needs-work+
(Assignee)

Comment 16

16 years ago
Created attachment 97594 [details] [diff] [review]
New patch for 2002-08-30-08-trunk

Tested:
- Print dialog remembers values across prints and browser restarts
- Page setup now works correctly (I can print "portrait" and "landscape")
Attachment #97588 - Attachment is obsolete: true

Updated

16 years ago
Blocks: 125824

Comment 17

16 years ago
Comment on attachment 97594 [details] [diff] [review]
New patch for 2002-08-30-08-trunk

I have tested. This patch works well.
Attachment #97594 - Flags: review+

Comment 18

16 years ago
r=pete
+  // NOTE: getPRinters sets up the PrintToFile radio buttons

"Printers", not "PRinters"

sr=bzbarsky with that change.
(Assignee)

Comment 20

16 years ago
Created attachment 97741 [details] [diff] [review]
New patch for 2002-08-30-08-trunk to match sr=bz
Attachment #97594 - Attachment is obsolete: true
(Assignee)

Comment 21

16 years ago
Comment on attachment 97741 [details] [diff] [review]
New patch for 2002-08-30-08-trunk to match sr=bz

Carrying over r=petez and sr=bz (per bz's permission via IRC) ...
Attachment #97741 - Flags: superreview+
Attachment #97741 - Flags: review+
(Assignee)

Comment 22

16 years ago
rods:
Can we have a moa= from you, please ?

Comment 23

16 years ago
Comment on attachment 97741 [details] [diff] [review]
New patch for 2002-08-30-08-trunk to match sr=bz

This CANNOT be checked in! It will cause a lot of problems for printing on
Windows.
Attachment #97741 - Flags: needs-work+

Comment 24

16 years ago
Item #1 there are several duplicates in the list below:
+  var flags = gPrintSettingsInterface.kInitSaveMargins |
+              gPrintSettingsInterface.kInitSaveMargins |
+              gPrintSettingsInterface.kInitSaveOddEvenPages |
+              gPrintSettingsInterface.kInitSaveHeaderLeft |
+              gPrintSettingsInterface.kInitSaveHeaderCenter |
+              gPrintSettingsInterface.kInitSaveHeaderRight |
+              gPrintSettingsInterface.kInitSaveFooterLeft |
+              gPrintSettingsInterface.kInitSaveFooterCenter |
+              gPrintSettingsInterface.kInitSaveFooterRight |
+              gPrintSettingsInterface.kInitSaveBGColors |
+              gPrintSettingsInterface.kInitSaveBGImages |
+              gPrintSettingsInterface.kInitSaveInColor |
+              gPrintSettingsInterface.kInitSaveReversed |
+              gPrintSettingsInterface.kInitSaveOrientation |
+              gPrintSettingsInterface.kInitSaveOddEvenPages;

Item #2
I am not sure why the changes to the nsPrintOptionImpl are needed, but the
Windows platform completely relies on these for printing to work.

Note, that each platform has its own nsPrintOptions obj that is derived from
nsPrintOptionsImpl so you can override any of the methods. So you will want to
over these in those platform implementation.

Summary
The overall approach looks fine.
(Assignee)

Comment 25

16 years ago
rod wrote:
> Item #1 there are several duplicates in the list below:
> +  var flags = gPrintSettingsInterface.kInitSaveMargins |
> +              gPrintSettingsInterface.kInitSaveMargins |
> +              gPrintSettingsInterface.kInitSaveOddEvenPages |
[snip]
> +              gPrintSettingsInterface.kInitSaveOddEvenPages;

OK, I'll fix that...

----

> Item #2
> I am not sure why the changes to the nsPrintOptionImpl are needed, but the
> Windows platform completely relies on these for printing to work.

I thought that |nsPrintOptions::GetGlobalPrintSettings()| returns the object
"as-is", without doing any initalisation or modification and that the matching
platform modules are doing the initalisation for the matching printer they want
to use. Adding an "exception" for the default printer calls for trouble when
they want to use a different printer but don't run the matching initalisation
cycle for it.

Same reason for |nsPrintOptions::InitPrintSettingsFromPrinter()| - there should
be no extra rules for the default printer - all printers should be treated the
same way in the crossplatform code - that's why I removed this code...

Comment 26

16 years ago
I really don't remember what the intent was, your comments sound correct.

I'll try overriding the methods as an experiment. I think think the Mac relies
on this.

Comment 27

16 years ago
Created attachment 97774 [details] [diff] [review]
patch for nsPrintOptionImpl and nsPrintOptionWin

Made minor change to nsPrintOptionImpl from the previous patch and moved the
current impls of the methods in question to nsPrintOptionWin. Tested printing
on Windows, seems to work fine as one would expect for C++ to do its job with
overridden methods.
(Assignee)

Comment 28

16 years ago
OK, I'll file an all-in-one patch later today...
(Assignee)

Comment 29

16 years ago
*** Bug 152109 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 30

16 years ago
Created attachment 97884 [details] [diff] [review]
New all-in-one patch for 2002-08-30-08-trunk

New all-in-one patch for Win32 and Unix/Linux, incl. minor cleanup and adding
GUI support for the "printerfeatures" stuff (some widgets are now only enabled
if the printer really supports the matching feature :) ...
Attachment #97741 - Attachment is obsolete: true
Attachment #97774 - Attachment is obsolete: true
(Assignee)

Comment 31

16 years ago
Requesting r=/sr= ...

Comment 32

16 years ago
When testing this bug, I found we don't save color after printing. bug 166826
has been filed to fix this.
(Assignee)

Comment 33

16 years ago
*** Bug 166826 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 34

16 years ago
Created attachment 98032 [details] [diff] [review]
New patch for 2002-08-30-08-trunk

I've integrated the patch from bug 166826 ("We don't save print_in_color after
printing.") into this patch...
Attachment #97884 - Attachment is obsolete: true
(Assignee)

Comment 35

16 years ago
rods:
Can you r= this patch, please ?

Comment 36

16 years ago
Comment on attachment 98032 [details] [diff] [review]
New patch for 2002-08-30-08-trunk

r=rods
Attachment #98032 - Flags: review+
(Assignee)

Updated

16 years ago
Blocks: 147384
Comment on attachment 98032 [details] [diff] [review]
New patch for 2002-08-30-08-trunk

>diff -N -r -u original/mozilla/gfx/src/windows/nsPrintOptionsWin.cpp mozilla/gfx/src/windows/nsPrintOptionsWin.cpp
>--- original/mozilla/gfx/src/windows/nsPrintOptionsWin.cpp	Thu Jul 25 20:30:28 2002
>+++ mozilla/gfx/src/windows/nsPrintOptionsWin.cpp	Thu Sep  5 01:17:37 2002
>@@ -39,7 +39,9 @@
> #include "nsPrintOptionsWin.h"
> #include "nsPrintSettingsWin.h"
> 
>-
>+#include "nsGfxCIID.h"
>+#include "nsIServiceManager.h"
>+static NS_DEFINE_IID(kPrinterEnumeratorCID, NS_PRINTER_ENUMERATOR_CID);

Please use the contract id instead ("@mozilla.org/gfx/printerenumerator;1".

>+NS_IMETHODIMP 
>+nsPrintOptionsWin::GetGlobalPrintSettings(nsIPrintSettings * *aGlobalPrintSettings)
>+{
>+  if (!mGlobalPrintSettings) {
>+    CreatePrintSettings(getter_AddRefs(mGlobalPrintSettings));
>+    NS_ASSERTION(mGlobalPrintSettings, "Can't be NULL!");

Why not move the early return if we couldn't get mGlobalPrintSettings up here? 
That way the next block wouldn't need to be indented (as much).

>+    // The very first we should initialize from the default printer

Correct this comment (looks like it should be "The very first time ...."

>+
>+  *aGlobalPrintSettings = mGlobalPrintSettings.get();

No need for this .get().

>+  NS_ADDREF(*aGlobalPrintSettings);
>+
>diff -N -r -u original/mozilla/gfx/src/windows/nsPrintOptionsWin.h mozilla/gfx/src/windows/nsPrintOptionsWin.h
>--- original/mozilla/gfx/src/windows/nsPrintOptionsWin.h	Mon Mar 25 12:55:27 2002
>+++ mozilla/gfx/src/windows/nsPrintOptionsWin.h	Thu Sep  5 01:17:37 2002
>@@ -35,8 +35,13 @@
>   nsPrintOptionsWin();
>   virtual ~nsPrintOptionsWin();
> 
>+protected:
>   NS_IMETHOD CreatePrintSettings(nsIPrintSettings **_retval);
> 
>+  NS_IMETHOD GetGlobalPrintSettings(nsIPrintSettings * *aGlobalPrintSettings);
>+  NS_IMETHOD GetNewPrintSettings(nsIPrintSettings * *aNewPrintSettings);
>+  NS_IMETHOD InitPrintSettingsFromPrinter(const PRUnichar *aPrinterName, nsIPrintSettings *aPrintSettings);
>+
> };
> 
> 

Why are you making these methods, which are part of the nsIPrintSettingsService
interface, protected?

Comment 38

16 years ago
Created attachment 98312 [details] [diff] [review]
patch with bryner's comments

This patch include changes from bryner's comments. Only following problem I
can't reslove.

>Why are you making these methods, which are part of the
nsIPrintSettingsService
>interface, protected?
This question need rods to answer. I just moved out the "protected".

Comment 39

16 years ago
This problem is more than just Linux - I see it on Sun/Solaris as well.

Comment 40

16 years ago
Comment on attachment 98312 [details] [diff] [review]
patch with bryner's comments

I don't have a good reason for making them protected, so they are fine as
public. r=rods
Attachment #98312 - Flags: review+
Comment on attachment 98312 [details] [diff] [review]
patch with bryner's comments

sr=bryner
Attachment #98312 - Flags: superreview+

Comment 42

16 years ago
request for OEM branch
Whiteboard: branchOEM

Comment 43

16 years ago
Comment on attachment 98312 [details] [diff] [review]
patch with bryner's comments

a=asa (on behalf of drivers) for checkin to 1.2a (the trunk). Time is short. If
this is going to make 1.2a then it needs to land quickly.
Attachment #98312 - Flags: approval+

Comment 44

16 years ago
checked in trunk

Updated

16 years ago
Whiteboard: branchOEM → branchOEM+

Comment 45

16 years ago
checked in NETSCAPE_7_0_OEM_BRANCH(a=jdunn@netscape.com)
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Whiteboard: branchOEM+ → branchOEM+ fixedOEM

Comment 46

16 years ago
*** Bug 167804 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

16 years ago
Blocks: 164618
(Assignee)

Updated

16 years ago
Attachment #98032 - Attachment is obsolete: true
(Assignee)

Comment 47

16 years ago
This patch caused bug 167894 ("InitPrintSettingsFromPrinter() not called for
Print Preview") which rendered print preview unuseable in some cases... ;-(
(Assignee)

Updated

16 years ago
Blocks: 168057
(Assignee)

Comment 48

16 years ago
Nominating for 1.0.2-branch...
Keywords: mozilla1.0.2

Updated

16 years ago
Keywords: review → fixedOEM
Whiteboard: branchOEM+ fixedOEM
(Assignee)

Comment 49

16 years ago
*** Bug 169922 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 50

16 years ago
*** Bug 165791 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 51

16 years ago
*** Bug 171410 has been marked as a duplicate of this bug. ***

Comment 52

15 years ago
*** Bug 183117 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.