Page Setup is broken

VERIFIED FIXED in mozilla0.9.9

Status

()

VERIFIED FIXED
17 years ago
5 years ago

People

(Reporter: sfraser_bugs, Assigned: dcone)

Tracking

({regression})

Trunk
mozilla0.9.9
PowerPC
All
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

17 years ago
'Page Setup' on the Mac File menu used to bring up the native printer page setup 
dialog. It now does nothing.
(Reporter)

Updated

17 years ago
Keywords: regression
Keywords: nsbeta1
File > Page Setup still comes up on linux.

is this a problem on win32 and/or mac os x?
s/mac os x/mac os 9.x
OS: Mac System 9.x → MacOS X

Comment 3

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

Comment 4

17 years ago
over to don

I am not sure how it worked before, but I am sure my checkin probably overrode
something.
Assignee: rods → dcone
(Assignee)

Comment 5

17 years ago
Created attachment 65430 [details] [diff] [review]
Patch to implement the PrintSettings dialog thru the PrintOptions interface.

Comment 6

17 years ago
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
(Reporter)

Comment 7

17 years ago
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.
(Assignee)

Comment 8

17 years ago
This will show the Mac native dialog, the Mac overrides the xplatform dialog.

Comment 9

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

sr=attinasi
Attachment #65430 - Flags: superreview+

Comment 10

17 years ago
will this land for 0.9.8?
OS: MacOS X → All

Comment 11

17 years ago
*** Bug 120707 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 12

17 years ago
Created attachment 66177 [details] [diff] [review]
Slightly update patch.. taking out the commented piece, and adding a line to the javascript for the Mac

Comment 13

17 years ago
latest patch looks ok to me; r=brade
(Reporter)

Comment 14

17 years ago
Does the patch fix OS X too?
(Assignee)

Comment 15

17 years ago
I dont think OS X ever had print settings working, I have a bug for that. 
I will look at that next.
(Reporter)

Comment 16

17 years ago
Sure it did; I fixed them at the same time I fixed OS 9.
(Assignee)

Comment 17

17 years ago
Created attachment 66188 [details] [diff] [review]
Had a small bug.. fixed and tested on the Mac & Windows
Attachment #65430 - Attachment is obsolete: true
Attachment #66177 - Attachment is obsolete: true

Comment 18

17 years ago
please fix the comment in utilityOverlay.js

r=brade
(Assignee)

Comment 19

17 years ago
Created attachment 66302 [details] [diff] [review]
Fixed comment... added the OS X fix.

Comment 20

17 years ago
add the read/write prefs calls to the OSX file and then r=brade
(Assignee)

Comment 21

17 years ago
Created attachment 66310 [details] [diff] [review]
Added the Read and Write prefs for OS X stuff.
Attachment #66188 - Attachment is obsolete: true
Attachment #66302 - Attachment is obsolete: true

Comment 22

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

r=brade
Attachment #66310 - Flags: review+
(Reporter)

Comment 23

17 years ago
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.
Attachment #66310 - Flags: needs-work+

Comment 24

17 years ago
sfraser:
gfx/src/gtk/ and other platforms use windowwatcher/dom since eternity, too - and
noone complained about that yet...
(Assignee)

Comment 25

17 years ago
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.
(Reporter)

Comment 26

17 years ago
What is needed for 0.9.8? Just the Mac page setup dialog fixes, or the Windows
changes too?
(Assignee)

Comment 27

17 years ago
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

17 years ago
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).
(Reporter)

Comment 29

17 years ago
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?
(Assignee)

Comment 30

17 years ago
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.  
(Reporter)

Comment 31

17 years ago
> 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.
(Assignee)

Comment 32

17 years ago
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.
(Assignee)

Comment 33

17 years ago
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.
(Reporter)

Comment 34

17 years ago
What if we checked this patch into 0.9.8, and do the right thing on the trunk?
(Assignee)

Comment 35

17 years ago
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.
(Reporter)

Comment 36

17 years ago
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.
Attachment #66310 - Flags: superreview+
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

17 years ago
a=asa (on behalf of drivers) for checkin to 0.9.8
Keywords: mozilla0.9.8+
(Assignee)

Comment 39

17 years ago
checked into the .98 branch

Comment 40

17 years ago
removing keyword. fix verified on the branch.
Keywords: mozilla0.9.8+
(Assignee)

Comment 41

17 years ago
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.
(Reporter)

Comment 42

17 years ago
I'd like to hear about the solution to bug 115136 before allowing this patch in 
on the trunk.
(Assignee)

Comment 43

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

Comment 44

17 years ago
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.
(Reporter)

Comment 45

17 years ago
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.
Attachment #66310 - Flags: needs-work+
Marking nsbeta1+. Setting milestone to Moz0.9.9
Keywords: nsbeta1 → nsbeta1+
Target Milestone: --- → mozilla0.9.9
(Assignee)

Comment 47

17 years ago
fixed and checked in.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 48

17 years ago
okay it works now.

verified using 2/13 trunk build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.