312763 - When changing Page Setup in FF 1.5 b2, the settings are not saved

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Nick Kreeger]

When changing the page setup options in FF 1.5 b2, the settings are not saved
when the dialog is opened back up (File->Page Setup).

Steps to Reproduce:
1. Change Page Setup options
2. Click OK
3. Open back up Page Setup and notice the settings did not save

Comment 1

[Mark Mentovai]

I originally fixed this in bug 306370, it regressed since then.

Comment 2

[Samuel Sidler (old account; do not CC)]

This is a regression since 1.5b1.

Comment 3

[Mark Mentovai]

Attachment: Fix

Regression window, 100306 - 100405.

http://bonsai.mozilla.org/cvsquery.cgi?branch=MOZILLA_1_8_BRANCH&date=explicit&mindate=2005-10-03+05%3A00&maxdate=2005-10-04+06%3A00


This was caused by bug 267422, which removed savePrintSettings following
showPageSetup in printUtils.js.  That was done because kInitSaveNativeData was
thought to be useless (bug 267422 comment 54), which may be the case on other
platforms, but is most definitely not on the Mac.  On the Mac, Page Setup data
is considered a native setting, and preferences like print.print_orientation
aren't used at all.

Comment 4

[Scott MacGregor]

we wouldn't stop ship for this bug. But, if you get a reviewed and trunk
verified patch, come back tomorrow and nominate the patch for branch approval
and we'd probably take it. 

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #3)
> This was caused by bug 267422, which removed savePrintSettings following
> showPageSetup in printUtils.js.  That was done because kInitSaveNativeData was
> thought to be useless (bug 267422 comment 54), which may be the case on other
> platforms, but is most definitely not on the Mac.  On the Mac, Page Setup data
> is considered a native setting, and preferences like print.print_orientation
> aren't used at all.

I don't see where the magic is to handle kInitSaveNativeData:
http://lxr.mozilla.org/mozilla1.8/search?string=kInitSaveNativeData -- is there
some magic that I'm missing somewhere?  I see various references to
MAC_OS_X_PAGE_SETUP_PREFNAME off the PRINTING_PREF_BRANCH in
ReadPageFormatFromPrefs/WritePageFormatToPrefs in nsPrintSettingsX..

Comment 6

[Asaf Romano (gone)]

Comment on attachment 199872 [details] [diff] [review]
Fix

So, two questions here:
1. Do we really need to check for  printSettingsAreGlobal? if one sets is to be
false (that is, print settings per window), the page setup setting wouldn't be
remembered at all.

2. similar questions on the second arg.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #6)
> (From update of attachment 199872 [details] [diff] [review] [edit])
> So, two questions here:
> 1. Do we really need to check for  printSettingsAreGlobal? if one sets is to be
> false (that is, print settings per window), the page setup setting wouldn't be
> remembered at all.

I don't understand the purpose of printSettingsAreGlobal (or rather, of
non-global print settings).  Doing per-window print settings seems pretty
counterintuitive..

Comment 8

[Mark Mentovai]

(In reply to comment #5)
We do this in nsPrintOptionsX::WritePrefs, which isn't getting called at all
after a Page Setup now.  It's actually not gated on kInitSaveNativeData, though
possibly it should be (along with other constants).

Comment 9

[Mark Mentovai]

That makes three of us that don't know what the story is with
use_global_printsettings.  I included it because the old behavior respected that
setting, and I don't want to break anyone that might be relying on it,
especially at this stage of the game.  Considering all of the reinitialization
that goes on now, I would't be surprised if setting use_global_printsettings no
longer has the intended effect.  Possibly, the same goes for save_print_settings.

Comment 10

[Asaf Romano (gone)]

(In reply to comment #7)
> Doing per-window print settings seems pretty counterintuitive..

Prolly why it's not the default. ;)


(In reply to comment #9)
> That makes three of us that don't know what the story is with
> use_global_printsettings.  I included it because the old behavior respected that
> setting, and I don't want to break anyone that might be relying on it,
> especially at this stage of the game.  Considering all of the reinitialization
> that goes on now, I would't be surprised if setting use_global_printsettings no
> longer has the intended effect.  Possibly, the same goes for save_print_settings.

In any case, this patch doesn't affect other platforms (see the XP
implementation of WritePrefs), not relying on it for mac native settings should
be safe, even now.

Comment 11

[Mark Mentovai]

(In reply to comment #10)
> In any case, this patch doesn't affect other platforms (see the XP
> implementation of WritePrefs), not relying on it for mac native settings
> should be safe, even now.

OK, so I can either whack printSettingsAreGlobal and leave kInitSaveNativeData,
or replace kInitSaveNativeData with 0 and leave printSettingsAreGlobal in place.
 My preference is for the former, because kInitSaveNativeData helps the intent
of the code read more easily.  That's this change:

-+      if (gPrintSettingsAreGlobal && gSavePrintSettings) {
++      if (gSavePrintSettings) {

Comment 12

[Asaf Romano (gone)]

Comment on attachment 199872 [details] [diff] [review]
Fix

very well. r=mano with comment 11.

Comment 13

[Mark Mentovai]

Fixed on the trunk.

Comment 14

[Mark Mentovai]

Comment on attachment 199872 [details] [diff] [review]
Fix

Ready for the branch.

Comment 15

[Asa Dotzler [:asa]]

In order to help us evaluate this bug, can you help us by including the
information we included at the top of tinderbox:
* to request drivers' approval, set the attachment flag to approval1.8rc1?.
* include a comment about the value, the risk, and any existing testing.
* land changes and get them verified on the trunk before requesting branch approval.
Thanks.

Comment 16

[Mark Mentovai]

Verified, trunk 101905.  No risk in restoring a line that was present until a
few weeks ago.  The removal and reintroduction of saving in this way after
showing the page setup dialog only affects the Mac.

Comment 17

[Mark Mentovai]

Fixed on the 1.8 branch.

Comment 18

[Mark Mentovai]

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