Richard M Davis Reporter
	Description • 23 years ago

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?

	Roland Mainz Assignee
	Comment 1 • 23 years ago

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.

	Roland Mainz Assignee
	Comment 2 • 23 years ago

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

	Roland Mainz Assignee
	Comment 3 • 23 years ago

Accepting bug...

	sujay
	Comment 4 • 23 years ago

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

	Roland Mainz Assignee
	Comment 5 • 23 years ago

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

	Roland Mainz Assignee
	Comment 6 • 23 years ago

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

	Roland Mainz Assignee
	Comment 7 • 23 years ago

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

	Roland Mainz Assignee
	Comment 8 • 23 years ago

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

	Louie Zhao
	Comment 9 • 23 years ago

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?

	Roland Mainz Assignee
	Comment 10 • 23 years ago

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.

	Roland Mainz Assignee
	Comment 11 • 23 years ago

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

	Roland Mainz Assignee
	Comment 12 • 23 years ago

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

	Adrian Ulrich
	Comment 13 • 23 years ago

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

	Jay Yan
	Comment 14 • 23 years ago

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.

	Jay Yan
	Comment 15 • 23 years ago

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.

	Roland Mainz Assignee
	Comment 16 • 23 years ago

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

	Roland Mainz Assignee
	Comment 17 • 23 years ago

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

	Roland Mainz Assignee
	Comment 18 • 23 years ago

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.

	Jay Yan
	Comment 19 • 23 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 23 years ago

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

	Jay Yan
	Comment 21 • 23 years ago

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.
 

	Roland Mainz Assignee
	Comment 22 • 23 years ago

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.

	Roland Mainz Assignee
	Comment 24 • 23 years ago

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

	Roland Mainz Assignee
	Comment 25 • 23 years ago

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

	Pete Zha
	Comment 26 • 23 years ago

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

	Johnny Stenback (:jst)
	Comment 28 • 23 years ago

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

	Jaime Rodriguez, Jr.
	Comment 29 • 23 years ago

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

	Jaime Rodriguez, Jr.
	Comment 30 • 23 years ago

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

	Pete Zha
	Comment 31 • 22 years ago

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.

	Asa Dotzler [:asa]
	Comment 32 • 22 years ago

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

	Pete Zha
	Comment 33 • 22 years ago

checked in trunk

	Jay Yan
	Comment 34 • 22 years ago

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

	Jay Yan
	Comment 35 • 22 years ago

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

	Jay Yan
	Comment 36 • 22 years ago

cvs log on oembranch added.

	sujay
	Comment 37 • 22 years ago

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

	Margaret Chan
	Comment 38 • 22 years ago

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.

	Roland Mainz Assignee
	Comment 39 • 22 years ago

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

	Roland Mainz Assignee
	Comment 40 • 22 years ago

Nominating for 1.0.2-branch...