Roland Mainz Assignee
	Description • 22 years ago

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

	Roland Mainz Assignee
	Comment 1 • 22 years ago

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

... taking myself...

	Olav Vitters
	Comment 2 • 22 years ago

isn't this the same as bug 83750?

	Roland Mainz Assignee
	Comment 3 • 22 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.

	Roland Mainz Assignee
	Comment 5 • 22 years ago

Requesting r=/sr= ...

	Pete Zha
	Comment 6 • 22 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.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 22 years ago

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

sr=bzbarsky

	Pete Zha
	Comment 8 • 22 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.

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 22 years ago

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.

	Pete Zha
	Comment 10 • 22 years ago

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!

	Roland Mainz Assignee
	Comment 11 • 22 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).

	Pete Zha
	Comment 12 • 22 years ago

*** Bug 166321 has been marked as a duplicate of this bug. ***

	Roland Mainz Assignee
	Comment 13 • 22 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

	Roland Mainz Assignee
	Comment 14 • 22 years ago

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...

	Roland Mainz Assignee
	Comment 15 • 22 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...

	Roland Mainz Assignee
	Comment 16 • 22 years ago

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

	Pete Zha
	Comment 17 • 22 years ago

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

I have tested. This patch works well.

	Pete Zha
	Comment 18 • 22 years ago

r=pete

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 22 years ago

+  // NOTE: getPRinters sets up the PrintToFile radio buttons

"Printers", not "PRinters"

sr=bzbarsky with that change.

	Roland Mainz Assignee
	Comment 21 • 22 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) ...

	Roland Mainz Assignee
	Comment 22 • 22 years ago

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

	rods (gone)
	Comment 23 • 22 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.

	rods (gone)
	Comment 24 • 22 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.

	Roland Mainz Assignee
	Comment 25 • 22 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...

	rods (gone)
	Comment 26 • 22 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.

	rods (gone)
	Comment 27 • 22 years ago

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.

	Roland Mainz Assignee
	Comment 28 • 22 years ago

OK, I'll file an all-in-one patch later today...

	Roland Mainz Assignee
	Comment 29 • 22 years ago

*** Bug 152109 has been marked as a duplicate of this bug. ***

	Roland Mainz Assignee
	Comment 30 • 22 years ago

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 :) ...

	Roland Mainz Assignee
	Comment 31 • 22 years ago

Requesting r=/sr= ...

	Pete Zha
	Comment 32 • 22 years ago

When testing this bug, I found we don't save color after printing. bug 166826
has been filed to fix this.

	Roland Mainz Assignee
	Comment 33 • 22 years ago

*** Bug 166826 has been marked as a duplicate of this bug. ***

	Roland Mainz Assignee
	Comment 34 • 22 years ago

I've integrated the patch from bug 166826 ("We don't save print_in_color after
printing.") into this patch...

	Roland Mainz Assignee
	Comment 35 • 22 years ago

rods:
Can you r= this patch, please ?

	rods (gone)
	Comment 36 • 22 years ago

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

r=rods

	Brian Ryner (not reading)
	Comment 37 • 22 years ago

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?

	Pete Zha
	Comment 38 • 22 years ago

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".

	Scott Davis
	Comment 39 • 22 years ago

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

	rods (gone)
	Comment 40 • 22 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

	Brian Ryner (not reading)
	Comment 41 • 22 years ago

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

sr=bryner

	Pete Zha
	Comment 42 • 22 years ago

request for OEM branch

	Asa Dotzler [:asa]
	Comment 43 • 22 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.

	Pete Zha
	Comment 44 • 22 years ago

checked in trunk

	Pete Zha
	Comment 45 • 22 years ago

checked in NETSCAPE_7_0_OEM_BRANCH(a=jdunn@netscape.com)

	Pete Zha
	Comment 46 • 22 years ago

*** Bug 167804 has been marked as a duplicate of this bug. ***

	Roland Mainz Assignee
	Comment 47 • 22 years ago

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

	Roland Mainz Assignee
	Comment 48 • 22 years ago

Nominating for 1.0.2-branch...

	Roland Mainz Assignee
	Comment 49 • 22 years ago

*** Bug 169922 has been marked as a duplicate of this bug. ***

	Roland Mainz Assignee
	Comment 50 • 22 years ago

*** Bug 165791 has been marked as a duplicate of this bug. ***

	Roland Mainz Assignee
	Comment 51 • 22 years ago

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

	R.K.Aa.
	Comment 52 • 22 years ago

*** Bug 183117 has been marked as a duplicate of this bug. ***