Print orientation and other Mac page setup things aren't remembered

RESOLVED FIXED

Status

()

RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: mark, Assigned: mark)

Tracking

({fixed1.8, regression})

Trunk
PowerPC
macOS
fixed1.8, regression
Points:
---
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [needs review mano, SR sfraser])

Attachments

(1 attachment)

(Assignee)

Description

14 years ago
On the Mac, Mac-specific print settings aren't saved.  Changes made in the Page
Setup dialog aren't saved and don't even apply to subsequent print attempts.

To reproduce: open a web page, do File:Page Setup…, and change the orientation.
 Click OK.  Reopen File:Page Setup….  Note that the dialog's initial state has
been restored, and your changes were made in vain.  Try changing the orientation
again, and click OK.  Now, do File:Print…, and print the document (or get a
preview).  Note that your orientation changes were not applied.

This is a regression since Aviary 1.0, which seems to remember page setup during
a session, but forgets it if the browser is relaunched.  Page setup data is
designed to be saved and we have infrastructure to do so, we should put it to
work for us.

I have a working patch (needs some cleanup.)
(Assignee)

Updated

14 years ago
Flags: blocking1.8b4?

Comment 1

14 years ago
Mark, can you post your patch? Do you know what broke here and when?
Flags: blocking1.8b4? → blocking1.8b4+
(Assignee)

Comment 2

14 years ago
Created attachment 194258 [details] [diff] [review]
v1.0, Update prototypes and initialize print settings from prefs

Bug 297277 changed the prototypes of a couple of virtual methods in
nsPrintOptionsImpl that were overridden in nsPrintOptionsX.  They now use
nsAString insead of nsString.  The Mac-specific methods were never updated. 
They need to be updated in order for the Mac WritePrefs to write page setup
data to the preferences.

But that's only half the battle.

The base class' CreatePrintSettings method loads saved printing settings with a
call to InitPrintSettingsFromPrefs, but the Mac version overrides that
entirely, instead initializing from the defaults.  I'm modifying the Mac
version so that it loads from the prefs after doing the Mac-specific setup.  In
effect, it's the same as the overridden base method but with the Mac-specific
Init method called in the middle.

This makes Page Setup actually work, even across app launches.
Attachment #194258 - Flags: superreview?(sfraser_bugs)
Attachment #194258 - Flags: review?(bugs.mano)
(Assignee)

Comment 3

14 years ago
The regression for the second part of the problem (and the first part of the
patch) was something else that bug 297277 did.  Formerly,
nsPrintOptions::GetGlobalPrintSettings used mGlobalPrintSettings if it existed,
and called CreatePrintSettings otherwise.  Since the patch for that bug landed,
CreatePrintSettings is called every time, so it will run whenever the user
attempts to do Page Setup or Print.  I'm not convinced that the new behavior is
correct, but I assume it was done for a good reason.  This explains why 1.0.x
would allow settings to persist during a session but would revert after
restarting the app.  The page setup data saved in the profile were never being
read under 1.0.x, but the defaults would only trample the settings at launch.

Regardless, I believe that any call, extraneous or otherwise, to the
CreatePrintSettings should in turn call InitPrintSettingsFromPrefs to read the
page setup data from the profile.
Status: NEW → ASSIGNED
Whiteboard: [needs review mano, SR sfraser]

Comment 4

14 years ago
(In reply to comment #3)
> The regression for the second part of the problem (and the first part of the
> patch) was something else that bug 297277 did.  Formerly,
> nsPrintOptions::GetGlobalPrintSettings used mGlobalPrintSettings if it existed,
> and called CreatePrintSettings otherwise.  Since the patch for that bug landed,
> CreatePrintSettings is called every time, so it will run whenever the user
> attempts to do Page Setup or Print.  I'm not convinced that the new behavior is
> correct, but I assume it was done for a good reason.

This was actually bug 270079. Note that 297277 landed in 1.8.

(In reply to comment #2)
> Bug 297277 changed the prototypes of a couple of virtual methods in
> nsPrintOptionsImpl that were overridden in nsPrintOptionsX.  They now use
> nsAString insead of nsString.  The Mac-specific methods were never updated. 
> They need to be updated in order for the Mac WritePrefs to write page setup
> data to the preferences.

I am to blame for that one, though ;)

Comment 5

14 years ago
Comment on attachment 194258 [details] [diff] [review]
v1.0, Update prototypes and initialize print settings from prefs

-nsPrintOptionsX::ReadPrefs(nsIPrintSettings* aPS, const nsString& aPrefName,
PRUint32 aFlags)
+nsPrintOptionsX::ReadPrefs(nsIPrintSettings* aPS, const nsAString& aPrefName,
PRUint32 aFlags)

FWIW, the second argument to ReadPrefs and WritePrefs are mistakenly named
aPrefName. What is actually passed in this argument is the printer name; they
should be changed to something like aPrinterName. (The particular prefs that
are read or written are indicated by aFlags.)
Comment on attachment 194258 [details] [diff] [review]
v1.0, Update prototypes and initialize print settings from prefs

please do rename aPrefName.

r=mano with that fixed.
Attachment #194258 - Flags: review?(bugs.mano) → review+
(Assignee)

Comment 7

14 years ago
I will rename the arguments to aPrinterName in the base class and the Mac extension.

Comment 8

14 years ago
(In reply to comment #7)
> I will rename the arguments to aPrinterName in the base class and the Mac
extension.

I already fixed the naming of the base class in my patch to bug 297277.

Comment 9

14 years ago
Comment on attachment 194258 [details] [diff] [review]
v1.0, Update prototypes and initialize print settings from prefs

>Index: mozilla/gfx/src/mac/nsPrintOptionsX.cpp
>===================================================================

>   nsPrintSettingsX* printSettings = new nsPrintSettingsX; // does not initially ref count
>   NS_ENSURE_TRUE(printSettings, NS_ERROR_OUT_OF_MEMORY);
> 
>   NS_ADDREF(*_retval = printSettings); // ref count
> 
>   rv = printSettings->Init();
>   if (NS_FAILED(rv))
>     NS_RELEASE(*_retval);
>+  else
>+    (void)InitPrintSettingsFromPrefs(*_retval, PR_FALSE,
>+                                     nsIPrintSettings::kInitSaveAll);

I think this would read better with a 'return rv;' in the 'failed' clause.

sr=me with the param renaming.
Attachment #194258 - Flags: superreview?(sfraser_bugs) → superreview+
(Assignee)

Updated

14 years ago
Attachment #194258 - Flags: approval1.8b4?
(Assignee)

Comment 10

14 years ago
Checking in mozilla/gfx/src/mac/nsPrintOptionsX.cpp;
/cvsroot/mozilla/gfx/src/mac/nsPrintOptionsX.cpp,v  <--  nsPrintOptionsX.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in mozilla/gfx/src/mac/nsPrintOptionsX.h;
/cvsroot/mozilla/gfx/src/mac/nsPrintOptionsX.h,v  <--  nsPrintOptionsX.h
new revision: 1.9; previous revision: 1.8
done
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

14 years ago
Attachment #194258 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 11

14 years ago
Fixed on the 1.8 branch.
Keywords: fixed1.8

Comment 12

14 years ago
Does this still need a trunk landing?
(Assignee)

Comment 13

14 years ago
No, fixed on the trunk too.

Comment 14

13 years ago
Unfortunately this bug is still present in Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; de-AT; rv:1.9a1) Gecko/20060112 SeaMonkey/1.5a.

Please reopen.
You need to log in before you can comment on or make changes to this bug.