147605 - Printing / Page settings reset themselves after print (no landscape printing)

Status: VERIFIED FIXED

(Core :: Printing: Output, defect, P3)

Description

[Richard M Davis]

No doubt this is related to Bugs 129033, 83750 among others: it concerns the
persistence and effect of changes to paper orientation in Page Setup. 

Here's what happens to me using RC3 with KDE/RH7.2

1. Change Page Setup/Orientation to Landscape. Close dialog
2. Check Page Setup/Orientation. Shows Landscape selected as expected.
3. Open Print dialog.
4. Do nothing, just close Print dialog.
5. Check Page Setup/Orientation. Shows Portrait selected: wrong, surely?

Alternatively:

4. Confirm print. (Printout will be Portrait orientation, not Landscape, as hoped!)
5. Check Page Setup/Orientation. Shows Portrait selected: surely wrong?

Comment 1

[Roland Mainz]

Confirming and taking myself.

We are punished by the fact that we have to init nsIPrintSettings per printer
but have (currently) no way to check whether the print settings were already
initalised for this printer.

Comment 2

[Roland Mainz]

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

Comment 3

[Roland Mainz]

Accepting bug...

Comment 4

[sujay]

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

Comment 5

[Roland Mainz]

Attachment: Patch (workaround ?!) for 2002-05-24-08-trunk

This patch now remembers the last printer used to initalise the
|nsIPrintSettings| object and only writes into the object if the printer used
for initalisation is not the same as the previous one used to init the object.

However I think this is only a "workaround" - but I am not sure how a "better"
solution would look like... I have to discuss this with rods.

One final solution may look like this:
Split |nsIPrintSettings| into an "application"-part (like "zoom" etc.) and a
"device"-part (like "orientation", "paper name", "copies" etc.).

When a user switches the selected printer both the "application"-part and
"device"-part are saved to the prefs (before modifying the
|nsIPrintAppSettings| and |nsIPrintDeviceSettings| objects) and the
"device"-part is completely reinitalised based on the defaults for this printer
device and prefs for this printer name.

(This problem (bug 147605) would then we "fixed" because we save the prefs
before switching the printer. Selecting the same printer as the previous one
then just picks-up the previously saved settings).

Comment 6

[Roland Mainz]

Attachment: Patch to demonstrate how the correct solution should look like

This patch does the things more or less correctly:
|InitPrintSettingFromPrinter()| now does the following:
1. Before writing to the |nsIPrintSettings| object the printer-specific
attributes from the previous printer are written to the prefs (with the
per-printer prefix) - see |SaveSettingsOfPreviousPrinter()|
2. Next step is to fetch the defaults from the device
3. Final step is to override the device defaults with per-printer prefs if
there are any for this printer

Comments:
- |InitPrintSettingsFromPrefs()| and |InitPrintSettingsFromPrinter()| should
return a flag field (with nsIPrintSettings::kInitSave* flags) indicating what
has been "updated"/"touched" in the |nsIPrintSettings| object
- There should be one |InitPrintSettingsFromPrinterAndPrefs()| which inits the
nsIPrintSettings object with global prefs, device defaults and per-printer
prefs

ToDo:
- Port the changes to the Xlib gfx code

Comment 7

[Roland Mainz]

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

Comment 8

[Roland Mainz]

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

Comment 9

[Louie Zhao]

I test patch 85318 on RedHat7.2
using "Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.0+) Gecko/20020527".  
and on Solaris 8
using "Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.0.0+) Gecko/20020606".

I meet the same problem on both platform.
When I set "Landscape" printing mode, a progress bar came out when printing. But
it made no progress and didn't print.

When I click File->Print again, I got print error "Not Available".

Roland, can you fix it if it's really a bug?

BTW, Is patch 85318 a complete one or a demo version?

Comment 10

[Roland Mainz]

Louie Zhao wrote:
> I meet the same problem on both platform.
> When I set "Landscape" printing mode, a progress bar came out when printing. 
> But it made no progress and didn't print.

AFAIK it did work for me (on a GTK+ Zilla, the patch does _not_ cover the Xlib
code yet) ... but that was a long time ago...

> When I click File->Print again, I got print error "Not Available".

Well, either we leak the print device context or it was still "busy"...

> Roland, can you fix it if it's really a bug?

Yes, I can fix it... hopefully after wednesday...

> BTW, Is patch 85318 a complete one or a demo version?

See comment #6, sections "comments" and "todo"... that's what we need...

Having per-printer nsIPrintSettings objects may be a better idea, but I am not
sure whether rods likes that idea...
The current patch only overwrites the print settings with the printer's defaults
if we choose another printer...

Mhhh... I have to chat with rods and discuss the fix with him.

Comment 11

[Roland Mainz]

[reminder (for me)]
I discussed the issue with rods few secs ago, he said "go" for the creation of
the per-printer nsPrintSettings patch...

Comment 12

[Roland Mainz]

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

Comment 13

[Adrian Ulrich]

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

Comment 14

[Jay Yan]

hi, gisburn, I found 1.0 branch has also this bug, how is your xlib gfx related
code going on now, any plan about trunk and branch for this bug? thanks.

Comment 15

[Jay Yan]

Roland:
your patch only works when you print one page, when the document is larger than
1 page, you will meet with the same problem as Louie' comment #9.

Comment 16

[Roland Mainz]

I am working on a new patch... I need 2-3 days...

Comment 17

[Roland Mainz]

Attachment: Test patch for 2002-06-29-08-trunk

Another test patch (mainly for reference that the model of having global,
per-printer nsIPrintSettings objects will not work since we would have to
change frozen interfaces (hurray! ;-( ).

I am getting grey hair with this print settings stuff (and noone pays me for
the pain... ;-( ) ...

Comment 18

[Roland Mainz]

OK, new strategy, learning from my previous failure:
I am going to cut nsIPrintSettings into two pieces, one application-specific
part (|nsIPrintSettings|) and one device-specific part
(|nsIPrintDeviceSettings|).
We will still have one global |nsIPrintSettings| (therefore we won't need to
change any frozen interfaces) but multiple per-printer |nsIPrintDeviceSettings|
objects.

Advantage of this design:
- We are fixing this bug
- We are getting a clean appoach for per-printer settings (since we're
seperating application settings like shrink-to-fit from device settings (like
device resolution, paper size, page orientation, etc.)).
- We do not need to touch frozen interfaces [hopefully, but I am not going to
gurantee this now before I have a working patch... :) ]

Disadvantage:
- Lots of work.... ;-(
- It may require discussion which attributes are device specific and which are
application specific.

Comment 19

[Jay Yan]

Roland, since this bug has very high visibility and we want to have it fixed in
coming Netscape7.0, I have to kick your butt again.
How is your implementation of dividing interface into two part? do you need
help? As I know, The schedule of Netscape7.0 is very urgent, according to the
plan, today is the branch date, and code freezing date is very soon, Sun's
schedule is very close with Netscape's schedule, 
I noticed that the target milestone of this bug is mozilla1.1 beta, is it possible?

Can I recommend three alternative solution?

1 checkin your current patch which has global printing setting into trunk and
branch, in order that Netscape7.0 can print using Lanscape mode. and at the same
time open new bugs to enhance printing setting, such as multiple per-printer
setting, store user's printing setup perference, etc.

2 If you can finish these "a lot of work" by yourself before Netscape7.0's code
freezing, let you get your hair gray to white without payment:-)

3 Have us or others help you on the implementation of your new strategy.

Since the Netscape7.0 schedule is urgent,  I prefer option 1, 

What is your idea?  Roland?

and What is your idea, Rods?

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[Jay Yan]

Thanks Boris for nominating this bugs as showstopper candidate, I also think
this is a show stopper, 
but I do not know what rods, dcone and Roland are thinking about it. and I do
not know how it is going on there.:-(
any comments? thanks.
 

Comment 22

[Roland Mainz]

Jay Yan wrote:
> Roland, since this bug has very high visibility and we want to have it fixed 
> in coming Netscape7.0, I have to kick your butt again.
> How is your implementation of dividing interface into two part? do you need
> help? As I know, The schedule of Netscape7.0 is very urgent, according to the
> plan, today is the branch date, and code freezing date is very soon, Sun's
> schedule is very close with Netscape's schedule,

;-(

> I noticed that the target milestone of this bug is mozilla1.1 beta, is it 
> possible?

Not really. 1.1beta is out.

> Can I recommend three alternative solution?
> 
> 1 checkin your current patch which has global printing setting into trunk and
> branch, in order that Netscape7.0 can print using Lanscape mode. and at the
same
> time open new bugs to enhance printing setting, such as multiple per-printer
> setting, store user's printing setup perference, etc.
> 
> 2 If you can finish these "a lot of work" by yourself before Netscape7.0's
code
> freezing, let you get your hair gray to white without payment:-)

Well, I guess I have a workaround (I am not sure whether it will work but I can
file the patch).

> 3 Have us or others help you on the implementation of your new strategy.
> 
> Since the Netscape7.0 schedule is urgent,  I prefer option 1, 
> 
> What is your idea?  Roland?

My problem is that I do not want to modify the frozen interfaces in
nsIWebBrowserPrint&co. (I am sure some people will cut my head if I start to do
this)
The problem here is very simple:
We init the nsIPrintSettings object each time we change the printer. The
problem: We overwrite user selections with this initalisation (which is
unavoidable, otherwise are breaking existing functionality elsewhere).

AFAIK one of the ideal solutions would look like this:
- We have multiple methods to init a nsIPrintSettings object
(InitPrintSettingsFromPrefs(), InitPrintSettingsFromPrinter()) etc. but we
_must_ only expose _one_ function - to obtain a fully initalised
|nsIPrintSettings| object (which is only valid for the given printer):
|nsIPrintSettings *GetNewPrintSettingsObjectForPrinter( const char
*printerName)|
(all the InitPrintSettings*-methods should be private and not accessible to
embeddors or javascript).
- nsIPrintSettingsService would provide an API to manage a list of these
per-printer nsIPrintSettings objects (store such an object and find it later via
the printer name).
- |nsIPrintSettings| gets a new operator to copy certain fields from between two
|nsIPrintSettings| objects

Splitting the |nsIPrintSettings| object into an application-spefic part and a
device-specific part would be nice, too.

Comment 23

[Roland Mainz]

Attachment: Workaround forces that printdialog.js will init the global printsettings object only if we change the printer name

Comment 24

[Roland Mainz]

The workaround fixes the "can't print landscape"-issue, but IMHO we should file
a bug to get the API fixed to avoid that anyone hits the same issue in the
future.

Known bugs of the workaround:
- The workaround will not work in the scenario where a user selects "landscape"
and then changes the printer name (changing the printer name will always reset
the printsettings object).

Comment 25

[Roland Mainz]

Requesting r=/sr=/a{trunk}= etc.

Lets test it a week on "trunk" first before we apply this to the 1.0.1 branch,
please...

Comment 26

[Pete Zha]

Comment on attachment 92671 [details] [diff] [review]
Workaround forces that printdialog.js will init the global printsettings object only if we change the printer name

>+    printService.initPrintSettingsFromPrefs(gPrintSettings, true, flags);
>+    
>+    dump("setPrinterDefaultsForSelectedPrinter: init\n");
>+  }
>+  else
>+  {
this line should use this style, just same as other place in this file.
if (foo) {
} else {
}
And I'm not sure dump is necessary since we don't need them in runtime.
Other than above r=pete

Comment 27

[Roland Mainz]

Attachment: New workaround for 2002-07-18-08-trunk per petez's review comment which forces that printdialog.js will init the global printsettings object only if we change the printer name

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 92674 [details] [diff] [review]
New workaround for 2002-07-18-08-trunk per petez's review comment  which forces that printdialog.js will init the global printsettings object only if we change the printer name

sr=jst

Comment 29

[Jaime Rodriguez, Jr.]

kevin: can you take a look at this one, and add an ADT impact to the status
whiteboard? thanks!

Comment 30

[Jaime Rodriguez, Jr.]

removing adt1.0.1, since this has not landed on the trunk.

Comment 31

[Pete Zha]

Comment on attachment 92674 [details] [diff] [review]
New workaround for 2002-07-18-08-trunk per petez's review comment  which forces that printdialog.js will init the global printsettings object only if we change the printer name

r=pete
ok, we just need request approval for this bug.

Comment 32

[Asa Dotzler [:asa]]

Comment on attachment 92674 [details] [diff] [review]
New workaround for 2002-07-18-08-trunk per petez's review comment  which forces that printdialog.js will init the global printsettings object only if we change the printer name

a=asa (on behalf of drivers) for checkin to 1.1

Comment 33

[Pete Zha]

checked in trunk

Comment 34

[Jay Yan]

Thanks, Roland and Pete.
Since it is a showstopper for 7.0, I am nominating this patch to be checked into
OEM_BRANCH, mark it as BranchOEM.

Jim, please help to approve it. thanks a lot.
  
(BTW, who can tell me the meaning of "[ADT2]" in Whiteboard? thanks.) 

Comment 35

[Jay Yan]

Checked in to oembranch.(a= Jim)

Jim: Sorry for failing to write cvs log becasue of a typo.
http://bonsai.mozilla.org/cvsquery.cgi?module=allrepositories&branch=&dir=&file=&who=jay.yan%25sun.com&sortby=Date&hours=2&date=week

Comment 36

[Jay Yan]

cvs log on oembranch added.

Comment 37

[sujay]

verified on 8/16 linux trunk...someone else(either Jay/Pete) will have
to verify on OEM branch.

Comment 38

[Margaret Chan]

I can finally print out my page in landscape on my 8/15 build of the OEM
branch.  Marked it verified for the OEM branch.

Comment 39

[Roland Mainz]

Unfortunately this patch regressed something - see bug 166217 ("Print settings
on Linux are saved at shutdown but not read at next start") ... ;-(

Comment 40

[Roland Mainz]

Nominating for 1.0.2-branch...