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

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Mark Mentovai]

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

Comment 1

[Asa Dotzler [:asa]]

Mark, can you post your patch? Do you know what broke here and when?

Comment 2

[Mark Mentovai]

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

Comment 3

[Mark Mentovai]

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.

Comment 4

[Bastiaan Jacques]

(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

[Bastiaan Jacques]

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 6

[Asaf Romano (gone)]

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.

Comment 7

[Mark Mentovai]

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

Comment 8

[Bastiaan Jacques]

(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

[Simon Fraser [no longer active]]

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.

Comment 10

[Mark Mentovai]

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

Comment 11

[Mark Mentovai]

Fixed on the 1.8 branch.

Comment 12

[Josh Aas]

Does this still need a trunk landing?

Comment 13

[Mark Mentovai]

No, fixed on the trunk too.

Comment 14

[Gulliver]

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.