Closed
Bug 166217
Opened 22 years ago
Closed 22 years ago
Print settings on Linux are saved at shutdown but not read at next start
Categories
(Core :: Printing: Output, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.2alpha
People
(Reporter: roland.mainz, Assigned: roland.mainz)
References
Details
(Keywords: fixedOEM, regression)
Attachments
(1 file, 8 obsolete files)
17.96 KB,
patch
|
rods
:
review+
bryner
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
Print settings on Linux/Unix platforms are saved at shutdown but not read at next start
Assignee | ||
Comment 1•22 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•22 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.2alpha
Assignee | ||
Updated•22 years ago
|
Keywords: regression
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.
Assignee | ||
Comment 4•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.
Attachment #97530 -
Flags: review+
Comment 7•22 years ago
|
||
Comment on attachment 97530 [details] [diff] [review] Patch for 2002-08-30-08-trunk sr=bzbarsky
Attachment #97530 -
Flags: superreview+
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 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.
Attachment #97530 -
Flags: superreview+
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!
Assignee | ||
Updated•22 years ago
|
Attachment #97575 -
Flags: needs-work+
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).
Comment 12•22 years ago
|
||
*** Bug 166321 has been marked as a duplicate of this bug. ***
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
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...
Attachment #97530 -
Attachment is obsolete: true
Attachment #97575 -
Attachment is obsolete: true
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...
Attachment #97588 -
Flags: needs-work+
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")
Attachment #97588 -
Attachment is obsolete: true
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.
Attachment #97594 -
Flags: review+
Comment 18•22 years ago
|
||
r=pete
Comment 19•22 years ago
|
||
+ // NOTE: getPRinters sets up the PrintToFile radio buttons "Printers", not "PRinters" sr=bzbarsky with that change.
Assignee | ||
Comment 20•22 years ago
|
||
Attachment #97594 -
Attachment is obsolete: true
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) ...
Attachment #97741 -
Flags: superreview+
Attachment #97741 -
Flags: review+
Assignee | ||
Comment 22•22 years ago
|
||
rods: Can we have a moa= from you, please ?
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.
Attachment #97741 -
Flags: needs-work+
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.
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...
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.
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.
Assignee | ||
Comment 28•22 years ago
|
||
OK, I'll file an all-in-one patch later today...
Assignee | ||
Comment 29•22 years ago
|
||
*** Bug 152109 has been marked as a duplicate of this bug. ***
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 :) ...
Attachment #97741 -
Attachment is obsolete: true
Attachment #97774 -
Attachment is obsolete: true
Assignee | ||
Comment 31•22 years ago
|
||
Requesting r=/sr= ...
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.
Assignee | ||
Comment 33•22 years ago
|
||
*** Bug 166826 has been marked as a duplicate of this bug. ***
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...
Attachment #97884 -
Attachment is obsolete: true
Assignee | ||
Comment 35•22 years ago
|
||
rods: Can you r= this patch, please ?
Comment 36•22 years ago
|
||
Comment on attachment 98032 [details] [diff] [review] New patch for 2002-08-30-08-trunk r=rods
Attachment #98032 -
Flags: review+
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?
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".
Comment 39•22 years ago
|
||
This problem is more than just Linux - I see it on Sun/Solaris as well.
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
Attachment #98312 -
Flags: review+
Comment 41•22 years ago
|
||
Comment on attachment 98312 [details] [diff] [review] patch with bryner's comments sr=bryner
Attachment #98312 -
Flags: superreview+
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.
Attachment #98312 -
Flags: approval+
Comment 44•22 years ago
|
||
checked in trunk
Comment 45•22 years ago
|
||
checked in NETSCAPE_7_0_OEM_BRANCH(a=jdunn@netscape.com)
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Whiteboard: branchOEM+ → branchOEM+ fixedOEM
Comment 46•22 years ago
|
||
*** Bug 167804 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•22 years ago
|
Attachment #98032 -
Attachment is obsolete: true
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... ;-(
Assignee | ||
Comment 49•22 years ago
|
||
*** Bug 169922 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 50•22 years ago
|
||
*** Bug 165791 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 51•22 years ago
|
||
*** Bug 171410 has been marked as a duplicate of this bug. ***
Comment 52•22 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.
Description
•