118889 - Page Setup is broken

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[Simon Fraser [no longer active]]

'Page Setup' on the Mac File menu used to bring up the native printer page setup 
dialog. It now does nothing.

Comment 1

[sairuh (rarely reading bugmail)]

File > Page Setup still comes up on linux.

is this a problem on win32 and/or mac os x?

Comment 2

[sairuh (rarely reading bugmail)]

s/mac os x/mac os 9.x

Comment 3

[sujay]

confirming on MacOS 9.x with 1/7 commercial build also....Page Setup doesn't
bring any dialog up.

Comment 4

[rods (gone)]

over to don

I am not sure how it worked before, but I am sure my checkin probably overrode
something.

Comment 5

[dcone (gone)]

Attachment: Patch to implement the PrintSettings dialog thru the PrintOptions interface.

Comment 6

[rods (gone)]

1) Put a comment above the new includes in nsPrintOptionsImpl.cpp
2) You may want to remove you ifdef NOT_NOW before checkin.
3) Fix the spacing in the makefile.win

r=rods

Comment 7

[Simon Fraser [no longer active]]

Does this show the XP print setup dialog, or the Mac printer driver page setup 
dialog? This bug was filed specifically on the latter, since it's the only way to 
get to some printer-specific options on Mac.

Comment 8

[dcone (gone)]

This will show the Mac native dialog, the Mac overrides the xplatform dialog.

Comment 9

[Marc Attinasi]

Comment on attachment 65430 [details] [diff] [review]
Patch to implement the PrintSettings dialog thru the PrintOptions interface.

sr=attinasi

Comment 10

[Kathleen :Brade]

will this land for 0.9.8?

Comment 11

[Tim Smith]

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

Comment 12

[dcone (gone)]

Attachment: Slightly update patch.. taking out the commented piece, and adding a line to the javascript for the Mac

Comment 13

[Kathleen :Brade]

latest patch looks ok to me; r=brade

Comment 14

[Simon Fraser [no longer active]]

Does the patch fix OS X too?

Comment 15

[dcone (gone)]

I dont think OS X ever had print settings working, I have a bug for that. 
I will look at that next.

Comment 16

[Simon Fraser [no longer active]]

Sure it did; I fixed them at the same time I fixed OS 9.

Comment 17

[dcone (gone)]

Attachment: Had a small bug.. fixed and tested on the Mac & Windows

Comment 18

[Kathleen :Brade]

please fix the comment in utilityOverlay.js

r=brade

Comment 19

[dcone (gone)]

Attachment: Fixed comment... added the OS X fix.

Comment 20

[Kathleen :Brade]

add the read/write prefs calls to the OSX file and then r=brade

Comment 21

[dcone (gone)]

Attachment: Added the Read and Write prefs for OS X stuff.

Comment 22

[Kathleen :Brade]

Comment on attachment 66310 [details] [diff] [review]
Added the Read and Write prefs for OS X stuff.

r=brade

Comment 23

[Simon Fraser [no longer active]]

Comment on attachment 66310 [details] [diff] [review]
Added the Read and Write prefs for OS X stuff.

Comments:

 REQUIRES=   xpcom \
+			windowwatcher \
+			dom \

This scares me, greatly (see below).

+nsPrintOptions::ShowPrintSetupDialog(nsIPrintSettings *aPS)
...
+     rv = wwatch->OpenWindow(parent,
"chrome://communicator/content/printPageSetup.xul",
+		    "_blank", "chrome,modal,centerscreen", array,
+		    getter_AddRefs(newWindow));

I think that this is the wrong way to go about this. gfx is a low-level DLL,
and as such should not be doing things like bringing up XUL windows (think
about embedders). It should also not have dependencies on DOM and
WindowWatcher. If you have to throw a XUL dialogs, this should be done from JS
code, and the JS that runs the dialog should interact with the gfx interfaces
for print settings. I really don't think that this code should be here.

Comment 24

[Roland Mainz]

sfraser:
gfx/src/gtk/ and other platforms use windowwatcher/dom since eternity, too - and
noone complained about that yet...

Comment 25

[dcone (gone)]

The problem is.. this is needed for the .98 build to get the dialogs working.. 
Those includes for the Makefile are also dependencies in the GFX/SRC/Windows 
directories.. so your concerns.. may be valid.. but its a bigger problem than 
just me using them.

That problem can be addressed later.. I would hope.. but in the mean time.. this 
is the only way to get the print settings dialogs working again.. for the  .98 
release.

Comment 26

[Simon Fraser [no longer active]]

What is needed for 0.9.8? Just the Mac page setup dialog fixes, or the Windows
changes too?

Comment 27

[dcone (gone)]

Those dependencies that I used are not new to GFX, so inorder to fix what your 
saying.. it has to be fixed a bunch of other places.  

The Mac dialogs do not work right now.. and this is the only way to fix it.. by 
redoing how all the platforms put up the dialog.

Comment 28

[Kathleen :Brade]

Simon--the changes in
xpfe/communicator/resources/content/mac/platformCommunicatorOverlay.xul would be
enough to show the XP dialog (it's pretty bad; no window title bar, etc.).

Also, Don and I have discussed an additional change to the above file to handle
the print dropdown list (on the toolbar) as well.  He will make the same change
twice in that file (File menu is changed in existing patch, now he'll also
change dropdown list for toolbar).

Comment 29

[Simon Fraser [no longer active]]

I don't see why a small reworking to bring up the JS from XUL, rather than the 
gfx code, is hard. None of the other code has to change, right?

Comment 30

[dcone (gone)]

ShowPrintSetupDialog is the backstop.. all platforms will bring that up if there 
in not a native implementation.  If I reworked it.. the xul and js  for each 
platform would have to be different.. and call different routines.. right?
The ShowXPPrintDialog is itself dependent on those routines, or files.  I think 
that the patch is the clean way to solve the problem of the printsettings 
dialog, and then the next problem that your worried about is breaking that 
dependency.  Reworking to bring up the JS from XUL does not break this 
dependency, it just re-arranges it.  GFX the way is stands now.. depends on 
those files.. and this patch does not add to it.. it just keeps it the same.  
So I think that putting this patch in.. then solving the problem of how to break 
that dependency is the correct thing to do.   

The patch has a single place it calls to bring up a dialog.. and each platform 
can override this.. I think thats a great way to do it.  I did not add a 
dependency.. it will be there no matter if this patch goes in or not.  

Comment 31

[Simon Fraser [no longer active]]

> ShowPrintSetupDialog is the backstop.. all platforms will bring that up if there 
> in not a native implementation.  If I reworked it.. the xul and js  for each 
> platform would have to be different.. and call different routines.. right?

I think what you need is a method on nsIPrintOptions that is 
"HasNativePageSetupDialog" or something. Then your JS can look like:

  if (printOptionsService.hasNativePageSetupDialog)
    printOptionsService.ShowPrintSetupDialog(printSettings);
  else
    window.openDialog(parent, "chrome://communicator/content/
printPageSetup.xul"........)

[note that your .idl should use leading lowercase method names, so this should
be printOptionsService.showPrintSetupDialog(printSettings)]

> The ShowXPPrintDialog is itself dependent on those routines, or files.  I think 
> that the patch is the clean way to solve the problem of the printsettings 
> dialog, and then the next problem that your worried about is breaking that 
> dependency.  Reworking to bring up the JS from XUL does not break this 
> dependency, it just re-arranges it.

How so? You're making the gfx code require windowwatcher and dom header files at 
build time, and DLLs at runtime (to be functional). That's made explicit by this 
change:

REQUIRES=   xpcom \
+                       windowwatcher \
+                       dom \
             string \

It also hard-codes in C++ a url to a chrome file, thus making assumptions about 
the structure of JAR files. Moving the dialog invocation into C++ avoids all 
these problems.

>  GFX the way is stands now.. depends on 
> those files.. and this patch does not add to it.. it just keeps it the same.

Not true, as I point out above.

> So I think that putting this patch in.. then solving the problem of how to break 
> that dependency is the correct thing to do.   
> 
> The patch has a single place it calls to bring up a dialog.. and each platform 
> can override this.. I think thats a great way to do it.  I did not add a 
> dependency.. it will be there no matter if this patch goes in or not.  

But you added to the REQUIRES in the makefile, thus adding a dependency! That's 
what REQUIRES is all about.

Comment 32

[dcone (gone)]

There is a routine.. in gfx/src/windows.. that makefile also requires those
fields.. hence GFX requires the same files.. All is did is promote that routine
in the gfx/src/windows directory to the gfx/src directory.  That other routine..
will disapear.. when rods determines it is no longer needed because I moved it
to the GFX/src nsPrintOptionsImp class.  The other file it is in.. that requires
those same files is gfx/src/windows/nsDeviceContextSpecWin.  So I did not add
depedencies to gfx.. just moved those same dependecies up one directory.

I did not make these dependencies.. they are all over.. and GFX has those same
dependencies all over.  If I actually did something.. or added a dependecy that
was not there.. I would agree.. but I did not.  

I would agree these (all the dependecies) should be cleared up.. but that does
not mean that this patch is incorrect.. I think its the right way.. I just think
that the windowwatcher is the only way to get some functionality that is needed..
And that will not go away even if I redo the patch the way you suggest.  That
dependecy is STILL there.. is just in the gfx/src/windows directory instead of
the grx/src directory.  That does not include the other dependecies in the
gfx/src/gtk directory.. and I would bet.. there are plenty more instances of
this kind of thing.  

Dont get me wrong.. I agree.. these have to be fixed.. but the current way that
nsPrintOptions and nsPrintSettings work.. those dependencies will not go away,
they are there currently.. I just moved it a directory higher.

Comment 33

[dcone (gone)]

Just to make things more clear.. all I really did.. was move some functionality 
in GFX up a directory level from the platforms to the Impl.. I did not add.. I 
did that so I would be able to expose the functions needed to put up the dialog 
settings on Mac.  To rid GFX of those dependecies is not the goal of this bug.. 
and I did not add those dependencies.. those were there already.. just in a 
different spot.. but still there.

Comment 34

[Simon Fraser [no longer active]]

What if we checked this patch into 0.9.8, and do the right thing on the trunk?

Comment 35

[dcone (gone)]

I will take a sr for the branch only.. and look into figuring out how to get rid 
of that dependency.. but remember.. this patch does not cause that dependency.. 
the Checkin Rods did a month ago.. created it.  

I still think this patch accomplishes the task of xplatform print settings.. and 
overiding xp with native dialogs the correct way from an engineering 
standpoint.. and we need to solve the dependency issue another way. But I will 
take what I can get for .98 :)

So please do sr.. it for .98.. and I will work with you getting a solution we 
both like for the trunk.

Comment 36

[Simon Fraser [no longer active]]

Comment on attachment 66310 [details] [diff] [review]
Added the Read and Write prefs for OS X stuff.

sr=sfraser for 0.9.8 branch checkin only.

Comment 37

[Kevin McCluskey (gone)]

I think we should create a separate bug to cleanup the dependencies that are
created by the cross platform print dialog. Regardless of where the dependecies
are created (gfx/src as ocurs in this patch) or in both (gfx/src/windows,
gfx/src/gtk) as it is currently on the trunk, we are in a sitution where the gfx
module can not be built without these other modules.  Getting the functionality
in PageSetup working for as many user's as possible is critical right now, so we
can get additional testing. I don't think we should hold up getting this fix in
until the dependencies have been removed.  I think the depedency bug should have
a high priority however, since too many dependencies have crept into gfx over
time and adding a couple of more just compounds the problem and sends the wrong
message. 

Comment 38

[Asa Dotzler [:asa]]

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

Comment 39

[dcone (gone)]

checked into the .98 branch

Comment 40

[Asa Dotzler [:asa]]

removing keyword. fix verified on the branch.

Comment 41

[dcone (gone)]

I have talked with Rod and Kevin about this issue.  That dependency.. was 
introduced by Rods checkin a month earlier.  He has a bug.. 
http://bugzilla.mozilla.org/show_bug.cgi?id=115136  that addresses that issue, 
and the way we bring up the XP dialog will not change.  So.. I think this 
patch should be allowed to be checked in on the trunk and that dependency issue 
should be fixed by bug 115136.  If I dont check this in..the Mac print 
settings will be broken.. and that dependency will still live on in the 
gfx/src/windows directory.. basically a lose lose situation.  I could come up 
with a patch that just puts up the Mac dialog..and jump through alot of hoops.. 
but that dependency would still be there.. and.. I would have added some code 
that is not the proper xp way to bring up Native or XP print settings dialogs.

Comment 42

[Simon Fraser [no longer active]]

I'd like to hear about the solution to bug 115136 before allowing this patch in 
on the trunk.

Comment 43

[dcone (gone)]

That solution is for Rod.  I guess print settings wont work for a while on the 
Mac.

Comment 44

[Fabian respighi]

Windows ME. trying to change the aspect of the printer page. File/Page
Setup/Header Right: it is impossible (for me) to write anything there.

Comment 45

[Simon Fraser [no longer active]]

Comment on attachment 66310 [details] [diff] [review]
Added the Read and Write prefs for OS X stuff.

OK, let's get this in on the trunk, and make sure that 115136 removes the bogus
dependencies.

Comment 46

[Kevin McCluskey (gone)]

Marking nsbeta1+. Setting milestone to Moz0.9.9

Comment 47

[dcone (gone)]

fixed and checked in.

Comment 48

[sujay]

okay it works now.

verified using 2/13 trunk build.