193001 - Use gnome's native print dialog

Status: RESOLVED FIXED

(Core :: Printing: Output, enhancement, P5)

Description

[Chris Lahey]

User-Agent:       Mozilla/5.0 Galeon/1.2.7 (X11; Linux i686; U;) Gecko/20030128
Build Identifier: Mozilla/5.0 Galeon/1.2.7 (X11; Linux i686; U;) Gecko/20030128

I'd like to modify mozilla to use gnome's native print dialog.  This would set
the options for mozilla.  Optimally, mozilla should use gnome-print's rendering
code as described in bug 192999.

Reproducible: Always

Steps to Reproduce:
1. Go to print in menus
Actual Results:  
Xul print dialog

Expected Results:  
native print dialog

Comment 1

[Brendan Eich [:brendan]]

bryner says he'll take this, fwiw.

/be

Comment 2

[Roland Mainz]

I am not sure whether this can really be implemented without loosing existing
functionality (e.g. per-printer options and defaults) and/or rewrite parts of
both Mozilla and the Gnome Print dialog.

For Mozilla we would have at least to rewrite the current "PrinterFeatures" hack
(which is used to store the per-printer capabilities (supported "features" and
paper sizes, plex modes, resolutions, orientations, etc.)) with
nsIPrinterFeatures (e.g. a normal API instead of the prefs hack, this is bug
136058) and hook-up code to feed that information to the Gnome print dialog (and
then work around the bugs in the Gnome print dialog).

I am not sure whether spending huge amounts of work into this is wise (if you
still want to do this please consult me before starting the work) ... the
benefit isn't great and we still would not get rid of the XUL print dialog since
it is used by OS/2 and the Xlib port, too.

Comment 3

[otaylor]

- It's possible to use the gnome-print dialog and then dump a 
  postscript file to it. The gpdf code is an example of doing
  this. So, this is generally very feasible without the
  dependency on bug 192999.

- Capabilities like duplex, etc, can be queried from the
  gnome-print dialog. The set of options supported by the
  gnome-print dialog has a few things not supported by
  mozilla so there would likely have to be a bit of extension
  of the mozilla printing code for optimum integration.
  The main thing I saw here was the ability to do non-collated
  multiple copies. (1,1,2,2 instead of 1,2,1,2)

Cheerleading: this would be a huge win for users; unlike the
XUL dialog, the gnome-print dialog properly interacts
with CUPS on the system to list printers and be aware of the
particular options available on those printers. Plus, the
consistency would be wonderful to have.

Comment 4

[Roland Mainz]

[e-mail] wrote:
> - It's possible to use the gnome-print dialog and then dump a 
>   postscript file to it

That's not enougth for Mozilla. Last time I checked it was not possible to
disable/enable or change single items like "orientation", "print command",
"paper size" etc. dynamically (e.g. per-printer) from within Mozilla code,
something the current XUL print and print job options dialog can do. And this
and other small "nits" are requirements before the Gnome print dialog can be
used.

Comment 5

[otaylor]

What's the user feature under discussion here ... remembering your options
per-printer when selecting between multiple printers?

Obviously, different users have different requirements, but I really
can't see why remembering print options per-printer is a web-browser
required feature, as opposed to a feature that you'd like to have
in general.

(I must be using an old version of the XUL dialog, since my XUL dialog
doesn't even let me *set* the printer other than by passing options
to LPR...)

Comment 6

[Roland Mainz]

[e-mail] wrote:
> What's the user feature under discussion here ... remembering your options
> per-printer when selecting between multiple printers?

I wasn't thinking about that. Mozilla allows that a print driver can teach the
print dialog about what the driver within Mozilla, the printer and the site
policy supports and what not.
For example: If you have a poster printer (e.g. DIN-A0 output format) you really
do not want to allow people to print DIN-A4 on it, right ?

> Obviously, different users have different requirements, but I really
> can't see why remembering print options per-printer is a web-browser
> required feature, as opposed to a feature that you'd like to have
> in general.

See my comment above. The Gnome print dialog cannot be restricted to the set of
options supported by the driver or printer - and feeding the driver or printer
with unsupported options or invalid option combinations will only cause havoc.
Same applies to site policies - admins may want to restrict the available
options for some reason (... continuing my example above... we could buy many
many Sun workstations from the money which is currently spend for the
"accidential" DIN-A4 printouts on our DIN-A0 poster printers).

> (I must be using an old version of the XUL dialog, since my XUL dialog
> doesn't even let me *set* the printer other than by passing options
> to LPR...)

You're likely using one of the RedHat builds of Mozilla. Get an "official" build
from mozilla.org and install the Xprint RPM from http://xprint.mozdev.org/
(don't do that on Debian/Mandrake/etc., the Xprint server is there installed
already by default) and you'll see what I mean.

Comment 7

[Roland Mainz]

[e-mail]:
Instead of using the native Gnome print dialog - what about making the current
XUL print dialog more looking like the Gnome print dialog ? That would be much
easier AFAIK...

Comment 8

[Marco Pesenti Gritti]

> I wasn't thinking about that. Mozilla allows that a print driver can teach the
> print dialog about what the driver within Mozilla, the printer and the site
> policy supports and what not.

Printer capabilities: if I get Owen correctly the gnome print dialog already
deal with it via CUPS, no need for mozilla to worry.

Driver capabilities: if we extend mozilla postscript driver to support all gnome 
print capabilities what sort of problems you expect ?

Site policy: could you point me to docs or code where the possible policies are
defined ?

I dont think it's useful to make the gnome print support all possible
combinations of mozilla drivers but rather to use one of the mozilla drivers
(postscript with an intermediate file could be the easier solution) to be able
to print through gnome print.

Comment 9

[Martin Kretzschmar]

Owen wrote: "It's possible to use the gnome-print dialog and then dump a
postscript file to it. The gpdf code is an example of doing this."

The gpdf code was just stolen from Chris Lahey's galeon patch from Ximian Desktop 2.

I can't remember adding anything interesting on top of that in the gpdf
gnome-print dialog code.

Comment 10

[David Schweiger]

(In reply to comment #3)
> - Capabilities like duplex, etc, can be queried from the
>   gnome-print dialog. The set of options supported by the
>   gnome-print dialog has a few things not supported by
>   mozilla so there would likely have to be a bit of extension
>   of the mozilla printing code for optimum integration.
>   The main thing I saw here was the ability to do non-collated
>   multiple copies. (1,1,2,2 instead of 1,2,1,2)

XPrint supports duplex, simplex, etc, from the Mozilla print dialog. What is the
gain here?

> Cheerleading: this would be a huge win for users; unlike the
> XUL dialog, the gnome-print dialog properly interacts
> with CUPS on the system to list printers and be aware of the
> particular options available on those printers. Plus, the
> consistency would be wonderful to have.

CUPS, LPRng, etc, are supported by XPrint, too. Again what is the gain here?

Comment 11

[David Schweiger]

(In reply to comment #8)
> Driver capabilities: if we extend mozilla postscript driver to support all gnome 
> print capabilities what sort of problems you expect ?
Extending the mozilla postscript driver is useless, many Linux distributions
already build without it and they do not care about that code - too buggy, too
insecure. XPrint printing has proven to be the better print solution

Comment 12

[Marco Pesenti Gritti]

The target of this bug, together with 192999, is to integrate mozilla printing
with gnome-print. Gnome print is composed of a frontend (libgnomeprintui) and a
backend (libgnomeprint). To be able to use the dialog (and his functionalities,
it's not just matter of look and feel) you have to use the backend to do the
actual print.
Now there are two ways to use this backend. Dump it a poscript file or add
gfx/src/gnomeprint (bug 192999).
Using the native dialog you gain consistent look and feel, consistent
interaction, functionalities (for example proper interaction with CUPS owen
mentioned)

Comment 13

[chris hofmann]

any chances of getting something working by 1.8a?

Comment 14

[Roland Mainz]

chris hofmann wrote:
> any chances of getting something working by 1.8a?

Not unless the Gnome print dialog itself gets some enhanchements. It's still a
far far way from integrating it into Mozilla without loosing lots of
functionality.

Comment 15

[Roland Mainz]

[e-mail] wrote:
> Using the native dialog you gain consistent look and feel, consistent
> interaction, functionalities

What do you want exactly ? Working, usefull functionality or a "consistent look
and feel" where half the functionality is not working ?

> (for example proper interaction with CUPS owen mentioned)

What about platforms or Linux distributions which do not use CUPS ? Do you just
"ignore" them with the comment "you have to use CUPS" or will you support them,
too ?

Comment 16

[Roland Mainz]

[e-mail] wrote:
> > I wasn't thinking about that. Mozilla allows that a print driver can teach 
> > the
> > > print dialog about what the driver within Mozilla, the printer and the 
> > site policy supports and what not.
>
> Printer capabilities: if I get Owen correctly the gnome print dialog already
> deal with it via CUPS, no need for mozilla to worry.

Erm... what about platforms which do not have CUPS ? And the way how CUPS
handles the way is very very incomplete.

> Driver capabilities: if we extend mozilla postscript driver to support all 
> gnome  print capabilities what sort of problems you expect ?

See David Schweigers's comment #11 about the mozilla postscript driver. I am not
sure whether it is a bright idea to extend that code and wallpaper another "fix"
on top of it. I already said a couple of _years_ (at least two years per bug
119491 comment #15) ago that the the postscript print module has very serious
design problems and should be re-done from scratch.
And nothing changed since that except braindead wallpapering over bugs.

> Site policy: could you point me to docs or code where the possible policies 
> are defined ?

I mean "site policies" as described in the original CDEnext docs (they're based
on the ISO print stuff) - basically you have a way to restrict, extend or
override certain printer attributes without the need to change the printer
description (such as a PPD). A basic example is to have a rule which defines
that a certain set of printers is only allowed to print DIN-A4 in the main tray
(and the user cannot override that locally - the rules are set on the server
side).

> I dont think it's useful to make the gnome print support all possible
> combinations of mozilla drivers but rather to use one of the mozilla drivers
> (postscript with an intermediate file could be the easier solution) to be able
> to print through gnome print.

There is the "minor" problem that Xprint doesn't output PostScript files (unless
you print to a file), the jobs are generated on the Xprint server side (which
can be on another box) - the local application don't see any PostScript data at
all (which is very usefull for applications running on handhelds since all the
heavywheight stuff is done on the server side).

Comment 17

[Marco Pesenti Gritti]

> What do you want exactly ? Working, usefull functionality or a "consistent look
> and feel" where half the functionality is not working ?

I want mozilla to support the limited set of functionalities of the GNOME print
system, I dont need more than that. If gnome print doesnt support a feature
there is not much value for GNOME users to have it available in the browser
(unless it's a browser specific feature).
It's more important to be able to interact consistently with the print system. 

Right now we are using two different print systems in GNOME and mozilla. That
cause differences in the user interface and, more importantly, it adds
complexity to the conceptual model the user build about the desktop print
system. That's a learning cost for the majority of GNOME users, while CDEnext
site policies would be useful to a minority (and what would they gain anyway if
mozilla is the only desktop application to support them ?).

> What about platforms or Linux distributions which do not use CUPS ? Do you just
> "ignore" them with the comment "you have to use CUPS" or will you support them,
> too ?

I'm not advocating to discontinue xprint support or the XUL dialog,
distributions that likes can continue to use it.

I just want using gnome printing system to be one of the options. Ideally we
would support libgnomeprint as backend and libgnomeprintui as frontend. Though
libgnomeprint drawing api is probably going to be deprecated (see
http://bugzilla.gnome.org/show_bug.cgi?id=141241) so adding a backend could be
waste of time.

On the long time the situation is pretty clear to me. GNOME and mozilla should
be able to use the same drawing api (Cairo?). (Obviously mozilla could continue
to support different backends if desired). Then supporting the remaining part of
libgnomeprint(ui) would be easy.

On the short time we can just try to hide the print system differences to the user:

1 reimplementing libgnomeprintui dialog in XUL
2 using libgnomeprint just as a transport mechanism and libgnomeprintui native
dialog.

In any case I think the libgnomeprintui GUI should be split in a setup and a
print part. That seem more sane and would integrate better with mozilla. I'll
see if I can get someone to do it.

To me the pro of 2 are: 

- we start to work on an unified user interface, rather then maintaining a fork
and having to sync it
- we are using gnome print for most of the work so it would probably result in
better integration
- we get nearer to the long term goal, only drawing api is left

The con is that we would have to use the postscript backend, which is apparently
a mess.

I think 1.8a target is not realistic unfortunately. I propose to approach the
problem this way.

1 Separate print and setup in libgnomeprintui
2 Make a list of the remaining incosistencies between XUL and native dialog
3 See if it's possible to fix these and how much work would be compared to use
the native dialog

I can deal with 1 and 2. I'll probably need some help on 3.

In the mean time we should really look at using a common drawing api on the long
run. I'd open a bug about it and cc GNOME/mozilla maintainers.
At the moment gnome printing system is not fully defined. That's a shame because
we cant just add support for it in mozilla. Though it's also an opportunity
because there is space to make it evolve in a direction that satisfies both
projects.

What do you think ? Hopefully this is more constructive than my previous rants
about incosistencies ;)

Comment 18

[Marco Pesenti Gritti]

> There is the "minor" problem that Xprint doesn't output PostScript files (unless
> you print to a file), the jobs are generated on the Xprint server side (which
> can be on another box) - the local application don't see any PostScript data at
> all (which is very usefull for applications running on handhelds since all the
> heavywheight stuff is done on the server side).

I fear we are still not on the same page here, probably my fault in explaining
it. What I ideally want is:

--enable-xprint
--enable-gnomeprint

When gnomeprint is enabled we could also use the native print dialog, when
xprint is enabled we would use the XUL one.
I dont think there is interest to make libgnomeprintui a generic dialog that can
be used by different print systems.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #18)
> When gnomeprint is enabled we could also use the native print dialog, when
> xprint is enabled we would use the XUL one.

what if both are enabled? or is that not possible?

Comment 20

[chris hofmann]

yeah, I guess one question is could both printing dialogs, and backends be
available a runtime and enabled via a pref (hidden?).   That would allow both
distributors and users to make the selection about what printing features they
need until we get both to the state of comparable features.

I'm pretty sure that for the linux desktop get wide enterprise and business
deployment things page orientation (portrait and landscape) and paper size
controls are going to be required.  Mozilla and open office are going to need
that feature for producing spreadsheets, wide tables, powerpoint like preso's
and legal docs.   We need to figure out how to get those on the gnome roadmap if
they aren't there now.

it would be great if someone could go down the list of features available in the
 XUL based print dialog, the gnome print dialog, the open office print dialog,
and maybe m$ office print dialogs to make sure the gnome print dialog and
supporting backend is on track all the competive features it needs if it doesn't
have them already...

I think it would be really helpful to have a clear view of that to move forward
and an accurate and up to date chart might look something like...

                      moz-xul   gnome  open-office   office

 page orientation        x
 page size               x
 feature c
 feature d
 ...
 ...

can someone volenteer to put that together with some of the data that is
scattered among the comments in this bug and a look at the latest versions?

Comment 21

[Marco Pesenti Gritti]

(In reply to comment #20)
> yeah, I guess one question is could both printing dialogs, and backends be
> available a runtime and enabled via a pref (hidden?).   That would allow both
> distributors and users to make the selection about what printing features they
> need until we get both to the state of comparable features.

Yeah, that sounds feasible to me.

>                       moz-xul   gnome  open-office   office
> 
>  page orientation        x
>  page size               x
>  feature c
>  feature d
>  ...
>  ...
> 
> can someone volenteer to put that together with some of the data that is
> scattered among the comments in this bug and a look at the latest versions?

Yeah, I'll see what I can do. In general I dont expect gnome print to lack
important features and I'm sure we can put them on GNOME roadmap if there are.

What worries me most atm is the drawing api business, but let's deal with things
one at a time.

Comment 22

[chris hofmann]

thanks marco.  does anyone know what evolution uses?  the UI seems to have a
pretty broad/good set of UI printing features along the lines that apps like
mozilla and OO might need.

Comment 23

[Marco Pesenti Gritti]

Evolution uses gnome print. (Other apps that use gnome print are gnumeric and gedit)

Comment 24

[Roland Mainz]

[e-mail] wrote:
> > What do you want exactly ? Working, usefull functionality or a "consistent 
> > look and feel" where half the functionality is not working ?
> 
> I want mozilla to support the limited set of functionalities of the GNOME 
> print system,
> I dont need more than that. If gnome print doesnt support a feature
> there is not much value for GNOME users to have it available in the browser
> (unless it's a browser specific feature).

The Gnome print dialog can easily be extended to match the current features in
the Mozilla XUL/CDEnext/Windows dialogs... that's not hard... :)
I am willing to cooperate here and help marco with some specs etc. to make the
Gnome print dialog more flexible (e.g. that it can support more than one print
system - the CDEnext and KDE print dialogs are already able to do that since
years...).

> Right now we are using two different print systems in GNOME and mozilla. That
> cause differences in the user interface and, more importantly, it adds
> complexity to the conceptual model the user build about the desktop print
> system. That's a learning cost for the majority of GNOME users, while CDEnext
> site policies would be useful to a minority (and what would they gain anyway 
> if mozilla is the only desktop application to support them ?).

The site policies (and other features like autoconfiguration) aren't
CDEnext-specific, they are coming from the Xprint side (most of the major design
documents about Xprint were written for the CDEnext project so please forgive me
when I start to mix both terms here wildly), so far this means Mozilla, Motif2,
Lesstif, OO and Qt-based applications can employ these features.

[snip]
> On the long time the situation is pretty clear to me. GNOME and mozilla should
> be able to use the same drawing api (Cairo?). (Obviously mozilla could 
> continue to support different backends if desired)

Well, there is actually no need to make an extra Cairo _backend_ (we may better
put it somewhere in the _middle_ :) ... the Xprint server side supports the
OpenGL API and allowes to plug-in PostScript code fragments, too - so either
native Cairo-->OpenGL-->Xprint will work or those parts of a document (like SVG
plugins) which need it can generate PostScript code fragments (assuming the
destination PDL allows PS embedding, otherwise the OpenGL path should be used
(which is better anyway since it can support transparency)).

> Then supporting the remaining part of libgnomeprint(ui) would be easy.
> 
> On the short time we can just try to hide the print system differences to the 
> user:
> 
> 1 reimplementing libgnomeprintui dialog in XUL
> 2 using libgnomeprint just as a transport mechanism and libgnomeprintui native
> dialog.

[1] is much easier for now, otherwise we will have do deal with the internals of
the mozilla print settings system and that's something which can cause _lots_ of
bugs - the code has become an horrible minefield and doing changes there usually
requires testing against the last 10 closed bug reports in that area... ;-(

BTW: When touching the XUL print dialog we have to sync with the OS/2 people
since they use the dialog, too.

> In any case I think the libgnomeprintui GUI should be split in a setup and a
> print part. That seem more sane and would integrate better with mozilla. I'll
> see if I can get someone to do it.

Umpf... do you mean something like "Print dialog (main)" and "Print dialog
(extra options)" ?

> To me the pro of 2 are: 
> 
> - we start to work on an unified user interface, rather then maintaining a 
> fork and having to sync it
> - we are using gnome print for most of the work so it would probably result in
> better integration
> - we get nearer to the long term goal, only drawing api is left
> 
> The con is that we would have to use the postscript backend, which is 
> apparently a mess.

Yeah... better we use [1] for now and work on making the Gnome print GUI more
flexible that it can support more than one print backend (as I said... KDE's
print dialog already has that feature... :)

> I think 1.8a target is not realistic unfortunately.

Chofmann ?

> I propose to approach the problem this way.
> 
> 1 Separate print and setup in libgnomeprintui
> 2 Make a list of the remaining incosistencies between XUL and native dialog
> 3 See if it's possible to fix these and how much work would be compared to use
> the native dialog
> 
> I can deal with 1 and 2. I'll probably need some help on 3.

[1] depend on what you are trying to do, [2] should also take the work and
experience of the KDE and CDEnext designs into account (don't think about the
windows print dialog, IMHO we should avoid that design of the implementation
details).

Comment 25

[chris hofmann]

Attachment: xul(PS-only)/gnome/office feature comparison

Comment 26

[Roland Mainz]

chris hofmann wrote:
> Created an attachment (id=148229)
> xul/gnome/office feature comparison

That chart is incomplete. The Mozilla XUL dialog has "plex mode" ("simplex",
"duplex", "tumble" and custom attributes), "paper source" and "find printerr"
capabilities when Xprint is used ("orientation" is in available in the "page
setup" dialog).

Comment 27

[Roland Mainz]

Attachment: Mozilla-XUL/Gnome/Office feature comparison

Updated chart, on demand I can add CDEnext and KDE dialogs, too.

Comment 28

[Marco Pesenti Gritti]

> > In any case I think the libgnomeprintui GUI should be split in a setup and a
> > print part. That seem more sane and would integrate better with mozilla. I'll
> > see if I can get someone to do it.

See the comparation chart. Some features in mozilla are in the Page Setup
dialog, and they are in the Print dialog for gnome print. That's an incosistency
we need to deal with.

Comment 29

[piers]

Screenshots of various print dialogs for Firefox/Win, Mozilla/Linux, Internet 
Explorer/Win and Gnumeric (libgnomeprint[ui]):

http://www.sparknet.pwp.blueyonder.co.uk/print/print.html

Comment 30

[Marco Pesenti Gritti]

> The Gnome print dialog can easily be extended to match the current features in
> the Mozilla XUL/CDEnext/Windows dialogs... that's not hard... :)
> I am willing to cooperate here and help marco with some specs etc. to make the
> Gnome print dialog more flexible (e.g. that it can support more than one print
> system - the CDEnext and KDE print dialogs are already able to do that since
> years...).

It sounds reasonable to work on a more complete design of the dialog. (I may try
to get a proper usability review on it while I'm at it too). libgnomeprintui
dialog seem to be bound to libgnomeprint (it requires a GnomePrintJob). So, in
my limited knowledge, it's not possible to make it support multiple backends.
Though there is apparently some interest in having a dialog in gtk itself
(generic user interface only).

http://mail.gnome.org/archives/gtk-devel-list/2004-April/msg00116.html

Maybe that's the way to go. Anyway I'll try to work on a design and figure out
if we can make it independent from the backend.
 
> Well, there is actually no need to make an extra Cairo _backend_ (we may better
> put it somewhere in the _middle_ :) ... the Xprint server side supports the
> OpenGL API and allowes to plug-in PostScript code fragments, too - so either
> native Cairo-->OpenGL-->Xprint will work or those parts of a document (like SVG
> plugins) which need it can generate PostScript code fragments (assuming the
> destination PDL allows PS embedding, otherwise the OpenGL path should be used
> (which is better anyway since it can support transparency)).

Xprint is interesting to me only if we can adopt it as a common print system for
GNOME/Mozilla. I noticed it's now hosted at freedesktop.org, there has been
already discussions about adopting it in GNOME? What's the situation with KDE,
is it one of the supported backends, the only one?
Sorry, I need to be educated, print isnt quite my field;)

Comment 31

[Leon Sha]

Printing support was added in GTK+ 2.10.
The API Reference Manual is here.
http://developer.gnome.org/doc/API/2.0/gtk/gtk-High-level-Printing-API.html
Seems it has a relationship with cairo.
Can we start to implemente this on cairo based mozilla?

Comment 32

[Leon Sha]

I have just tried the Gtk Print API in gtk+2.10.
It is quite simple and cool. An example can be found here.
http://cvs.gnome.org/viewcvs/gtk%2B/tests/print-editor.c?view=markup
There are two ways to impletement this in mozilla.
1. Just use the GTK Print API dialog part.
http://developer.gnome.org/doc/API/2.0/gtk/GtkPrintUnixDialog.html
It's maybe simple to do. But I don't think it is a right way.
2. Use the GTK Print high level API 
http://developer.gnome.org/doc/API/2.0/gtk/gtk-High-level-Printing-API.html
It may be need a big change in mozilla.

Comment 33

[Ghee Teo]

Now that GNOME printing support have progressed moving from lingnomeprint[ui] to
GTK+, is it time know to reconsider this RFE?

The new GTK+ printing API as listed by Leon Sha now provides a set of printing
commands with pango/cairo rendering supports should address the following problems:

- GTK+, a common toolkit layer for Mozilla/Firefox/ThurderBird
- A Print Dialog that is fairly complete and provided hook for customization
- A CUPS backend already in place, and folks at Sun is working on a PAPI (Printing API of OpenStandard, http://www.freestandards.org/en/OpenPrinting)
backend

Benefits of RFE:
- consistent user experiences on printing dialog across gtk+ based apps [1]
- proper interaction with CUPS/PAPI systems [2]
- Better interaction with CUPS/PAPI backends allow better interaction with addition of new printer or browsing of printers.

Pros:
- site policies restrictions still need to be addressed.

So what do you think?

[1] Okay. This may take a while for many of the gtk+ apps to adopt the new dialog and API, but that is the goal :)
[2] This is pretty extensible, so it can be more backend in the future.

Comment 34

[Darren Kenny]

I would like to add my voice to this too.

While I undestand the original evaluation, as Ghee mentioned, there has been a re-architecture of the printing situation in GTK+ - enough to warrant a re-evaluation at least.

Is there anyone from the Mozilla side that could look into this?

Thanks,

Darren.

Comment 35

[Martin Meyer]

There was a HOWTO sent recently to Gnome's desktop-devel list for migrating to the GTK+ print dialog.  It gives some pretty good instructions for migrating to the new dialog, and I'm sure the Gnome people would be willing to help get this done if anyone asked. The message/HOWTO can be found at:

http://mail.gnome.org/archives/desktop-devel-list/2006-December/msg00069.html

Comment 36

[Kenneth Herron]

Attachment: Work in progress - GTK+ print dialog

Work in progress. This displays the GTK+ print dialog instead of the built-in XUL dialog for print requests. It's pretty rough; for example, after dismissing the dialog it crashes while trying to read back the paper size (and there's no place to select a paper size within the dialog).

Comment 37

[Ghee Teo]

hmm, paper size typically is loaded by the backend support library from the PPD file I think. How is the existing XUL dialog get its list of available paper size?

Keep up the Good work!

Comment 38

[Kenneth Herron]

Attachment: Updated work in progress

This still doesn't work. Apparently I'm not doing either cairo or glib refcounting correctly. However, it includes code both to display the GTK print dialog, and to use the GTK printing system to create the print job, instead of working directly with PS/PDF and CUPS.

Comment 39

[Michael Ventnor]

Kenneth, are you still actively working on this?

Comment 40

[Kenneth Herron]

(In reply to comment #39)
> Kenneth, are you still actively working on this?

I've been busy lately. I have some vacation time coming up, and I hope to make some more progress on this. I wouldn't turn down some help, though.

Comment 41

[Kenneth Herron]

Attachment: Work in progress #3

The previous patch wasn't working because it was taking a cairo surface created by the gtk printing code and manipulating it through thebes. Libgtk was using the system's version of cairo, while thebes uses the copy that's built in to gecko. It seems that approach won't be feasible for the time being.

This patch prints through gtk the the same way we print through other printing systems. It writes the print job to a temporary file and then prints that. I have only tested with print-to-file, but it seems to work properly, including the ability to select PDF or PS. There's still no way to select a paper size from the print dialog.

Comment 42

[Michael Ventnor]

Attachment: Patch

Kenneth, is it OK if I post my patch? It is a continuation of your WIP #3 and I have been working on it full-time as part of my internship.

All of the Firefox-centric options have been moved to the print dialog in a custom tab ported to GTK. This allows us to use the native Page Setup dialog as well which allows us to use all the quirky GTK page sizes and margins. Everything seems to work OK.

The reason the majority of the dialog code is in widget/ and not embedding/ is that printingui/unixshared is shared among all Unixes as the name implies, whereas this dialog is GTK specific.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+#ifndef __gen_nsIPrintSessionGTK_h__
+#define __gen_nsIPrintSessionGTK_h__

Identifiers with two leading underscores are reserved for the compiler

+#ifndef __gen_nsIPrintSession_h__
+#include "nsIPrintSession.h"
+#endif

Don't wrap #includes like this, it's pointless

+/* Use this macro when declaring classes that implement this interface. */
+#define NS_DECL_NSIPRINTSESSIONGTK \
+  virtual void SetNativePageSetup(GtkPageSetup *); \
+  virtual void SetNativePrintSettings(GtkPrintSettings *); \
+  virtual void SetNativePrinter(GtkPrinter *); \
+  virtual GtkPageSetup *GetNativePageSetup(); \
+  virtual GtkPrintSettings *GetNativePrintSettings(); \
+  virtual GtkPrinter *GetNativePrinter();

Don't need this macro, this is only one implementation of the interface

+// If you add any here remember to set them to NULL in Show()
+static GtkWidget* radio_as_laid_out;
+static GtkWidget* radio_selected_frame;
+static GtkWidget* radio_separate_frames;
+static GtkWidget* shrink_to_fit_toggle;
+static GtkWidget* print_bg_colors_toggle;
+static GtkWidget* print_bg_images_toggle;
+static GtkWidget* selection_only_toggle;
+static GtkWidget* header_left_dropdown;
+static GtkWidget* header_center_dropdown;
+static GtkWidget* header_right_dropdown;
+static GtkWidget* footer_left_dropdown;
+static GtkWidget* footer_center_dropdown;
+static GtkWidget* footer_right_dropdown;

Put all these plus the static functions into a real object with fields, methods and a destructor

+static nsresult ImportSettings(GtkPrintUnixDialog *, nsIPrintSettings *);
+static nsresult ExportSettings(GtkPrintUnixDialog *, nsIPrintSettings *);

Write parameter names

+static GtkWindow *
+get_gtk_window_for_nsiwidget(nsIWidget *widget)

This function must already exist in nsWindow, use that

+static void
+moz_destroy_custom_strings()
+{
+  char* custom_string_buffer;
+
+  custom_string_buffer = (char*) g_object_get_data(G_OBJECT(header_left_dropdown), "custom-text");
+  if (custom_string_buffer)
+    g_free(custom_string_buffer);

Don't do this, just use g_object_set_data_full and pass in a destructor function that cleans up the string. You may be able to just pass in g_free.

+static char*
+moz_convert_index_to_string(GtkWidget* dropdown)
+{
+  switch (gtk_combo_box_get_active(GTK_COMBO_BOX(dropdown))) {
+    case 1:
+      return "&T";
+    case 2:

Don't use magic numbers for the header/footer types, use an enum. And have an array with the tags, e.g.
static const char headerFooterTags[6][4] = { "", "&T", etc };

+  nsDependentString currStrWrapper = nsDependentString(currentString);
+  if (currStrWrapper.EqualsLiteral(""))
+    gtk_combo_box_set_active(GTK_COMBO_BOX(dropdown), 0);
+  else if (currStrWrapper.EqualsLiteral("&T"))
+    gtk_combo_box_set_active(GTK_COMBO_BOX(dropdown), 1);

You can use the headerFooterTags array here with a loop.

+  GtkWidget* prompt_dialog = gtk_dialog_new_with_buttons(NS_ConvertUTF16toUTF8(intlString).get(), NULL, GTK_DIALOG_MODAL,

Create a helper class derived from NS_ConvertUTF16toUTF8 that takes a bundle and a const char* string name, grabs the string from the bundle and converts it to UTF8.

+  gtk_box_pack_start(GTK_BOX(custom_vbox), custom_label, FALSE, FALSE, 0);
+  gtk_box_pack_start(GTK_BOX(custom_vbox), custom_entry, FALSE, FALSE, 5);
+  GtkWidget* custom_hbox = gtk_hbox_new(FALSE, 2);
+  gtk_box_pack_start(GTK_BOX(custom_hbox), question_icon, FALSE, FALSE, 0);
+  gtk_box_pack_start(GTK_BOX(custom_hbox), custom_vbox, FALSE, FALSE, 10);

Document or better still create named constants for these magic 0, 5, 10 numbers

+    const char* response_text = (const char*) gtk_entry_get_text(GTK_ENTRY(custom_entry));

Why do you have to cast to const char* here?

+  printBundle->GetStringFromName(NS_LITERAL_STRING("hfBlank").get(), getter_Copies(intlString));
+  gtk_combo_box_append_text(GTK_COMBO_BOX(dropdown), NS_ConvertUTF16toUTF8(intlString).get());
+  printBundle->GetStringFromName(NS_LITERAL_STRING("hfTitle").get(), getter_Copies(intlString));
+  gtk_combo_box_append_text(GTK_COMBO_BOX(dropdown), NS_ConvertUTF16toUTF8(intlString).get());

Use a loop over a const char string array instead of all this code.

moz_construct_header_footer_options should also use loops and arrays instead of writing out lots of code.

more coming...

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

There's a whole lot of code that should be checking nsresults in the Import and Export functions.

+  else if (nsDependentCString(fmtGTK).EqualsIgnoreCase("pdf"))

Use stricmp

+static inline void

Don't make functions "static inline void", just "static", the compiler will do the right thing

+  nsCAutoString gtkPrinter;
+  CopyUTF16toUTF8(geckoPrinter, gtkPrinter);

I think you can just write
  NS_ConvertUTF16toUTF8 gtkPrinter(geckoPrinter);

+    offset = 5;

Too magical, write NS_ARRAY_LENGTH("CUPS/")-1 or something

+  nsAutoString nmPrinter;
+  CopyUTF8toUTF16(gtk_print_settings_get_printer(aSettings), nmPrinter);

Just use NS_ConvertUTF8toUTF16(gtk_print_settings_get_printer(aSettings)).get()

+    nsCAutoString geckoPaper;
+    CopyUTF16toUTF8(nmPaper, geckoPaper);

Similar here

+  nsAutoString name;
+  CopyUTF8toUTF16(gtk_paper_size_get_name(szPaper), name);
+  aNS->SetPaperName(name.get());

ditto

+  if (szFromSettings)
+    gtk_paper_size_free(szPaper);

Makes more sense to have "if (szPaper)"

+          if (lstRanges[ii].start < start) start = lstRanges[ii].start;
+          if (lstRanges[ii].end   < end)   end =   lstRanges[ii].end;

Just use "start = PR_MIN(...); end = PR_MAX(...);"

+      GtkPageRange* lstRanges(gtk_print_settings_get_page_ranges(aSettings,
+                                                                 &ctRanges));

Use = instead of copy construction

Is it true that when ctRanges is 0, we don't need to free lstRanges?

nsPrintJobGtkGTK is a horrible name. Let's just call it nsPrintJobGTK.

struct PrintJobGtkDone is probably overkill, you could use a static function for the callback and just the nsILocalFile* for the callback data, just NS_RELEASE it. You'll have to get the nsCOMPtr to forget it to keep the refcounts balanced.

So there's no way to share these strings with other print dialog implementations?

Comment 45

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #43)
> +/* Use this macro when declaring classes that implement this interface. */
> +#define NS_DECL_NSIPRINTSESSIONGTK \
> +  virtual void SetNativePageSetup(GtkPageSetup *); \
> +  virtual void SetNativePrintSettings(GtkPrintSettings *); \
> +  virtual void SetNativePrinter(GtkPrinter *); \
> +  virtual GtkPageSetup *GetNativePageSetup(); \
> +  virtual GtkPrintSettings *GetNativePrintSettings(); \
> +  virtual GtkPrinter *GetNativePrinter();
> 
> Don't need this macro, this is only one implementation of the interface

It's the only implementation of this interface in the moz tree, but that doesn't mean you shouldn't provide the macro! (E.g. epiphany might want to implement that interface in its own print session class.)

Comment 46

[Kenneth Herron]

(In reply to comment #42)
> Kenneth, is it OK if I post my patch? It is a continuation of your WIP #3 and I
> have been working on it full-time as part of my internship.

Michael, that's fine. It's clear you can spend more time on this than I can. It's nice to see Mozilla expend some resources on this.

My next change would have been to have the GtkPrinter, GtkPrintSettings, &
GtkPageSetup be owned by the nsIPrintSettings object instead of the
nsIPrintSession. Gecko supports printing without displaying a print dialog
first, so the code must be able to construct the GTK objects from the gecko
print settings and then use them to print. This would also simplify saving &
loading a serialized GtkPrintSettings when the nsIPrintSettings object is saved
or loaded from prefs.

Comment 47

[Mike Shaver (:shaver emeritus)]

(In reply to comment #43)
> +#ifndef __gen_nsIPrintSessionGTK_h__
> +#define __gen_nsIPrintSessionGTK_h__
> 
> Identifiers with two leading underscores are reserved for the compiler

I suspect he was just following the pattern used by xpidl in its generated headers, misleading though its example may be.

Comment 48

[Michael Ventnor]

Attachment: Patch 2

This patch is a doozy. It implements nsIPrintettingsGTK for storing native data thus allowing silent printing to work with sane defaults, since defaults are in the constructor. A lot of dialog construction code was abstracted out, and the device context spec code was just about rewritten to become completely dependent upon the GTK print API.

This probably means that CUPS no longer holds a monopoly on our Linux print service (yay!) but it means breaking things for poor bz again :( I included code to return early in the event of an old GTK version so we gracefully fail instead of crash.

This also means that nsPrintJobGTK and nsPrintJobFactoryGTK are no longer used! Yay!

Comment 49

[Christian Persch (GNOME) (away; not receiving bug mail)]

Without a screenshot it's hard to tell what's going on, but I think you have all the spacings and paddings wrong, and you're not setting the combos as menmonic targets of the labels (actually, you don't even have mnemonics on the strings).

+    char* custom_string = ToNewCString(currStrWrapper);
+    g_object_set_data_full(G_OBJECT(dropdown), "custom-text", custom_string, (GDestroyNotify) g_free);

ToNewCString is not compatible with g_free.

+#if GTK_CHECK_VERSION(2,12,0)
+                        | GTK_PRINT_CAPABILITY_NUMBER_UP
+#endif

I don't see code anywhere in the print engine to handle n-up printing?

+  mPrinter = gtk_printer_new(gtk_printer_get_name(rhs.mPrinter),
+                             gtk_printer_get_backend(rhs.mPrinter),
+                             gtk_printer_is_virtual(rhs.mPrinter));

Erm... g_object_ref (rhs.mPrinter) ?

---

Will this patch interfere in any way with gtk-based embedders that use the "PostScript/default" printer to print to file ?

Comment 50

[Michael Ventnor]

I believe the padding is right, since a lot of those values were borrowed from Epiphany's glade files. It looks correct to me.

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+// If you add any here remember to set them to NULL in Show()
+static GtkWidget* radio_as_laid_out;
+static GtkWidget* radio_selected_frame;
+static GtkWidget* radio_separate_frames;
+static GtkWidget* shrink_to_fit_toggle;
+static GtkWidget* print_bg_colors_toggle;
+static GtkWidget* print_bg_images_toggle;
+static GtkWidget* selection_only_toggle;
+static GtkWidget* header_left_dropdown;
+static GtkWidget* header_center_dropdown;
+static GtkWidget* header_right_dropdown;
+static GtkWidget* footer_left_dropdown;
+static GtkWidget* footer_center_dropdown;
+static GtkWidget* footer_right_dropdown;

I asked for these (plus the static functions that use them) to be made fields of a C++ object. That object should also store a reference to the string bundle so we don't have to get it more than once. moz_get_utf8_from_bundle could be a method in that object so we don't have to pass a bundle parameter, simplifying the code more.

+  if (index == 6)

Make a #define CUSTOM_HEADER_FOOTER NS_ARRAY_LENGTH(header_footer_tags) and use it here.

Also, assert that index <= CUSTOM_HEADER_FOOTER.

+  if (gtk_combo_box_get_active(changed_box) != 6)

Use #defined value

+    gtk_editable_select_region(GTK_EDITABLE(custom_entry), 0, strlen(current_text) - 1);

why - 1?

+    g_object_set_data_full(G_OBJECT(changed_box), "custom-text", strdup(response_text), (GDestroyNotify) g_free);

free, not g_free

+  for (int i = 0; i < 7; i++) {

NS_ARRAY_LENGTH(hfOptions)

+  for (int i = 0; i < 6; i++) {

NS_ARRAY_LENGTH(header_footer_tags)

Instead of currStrWrapper, use strcmp to compare with currentString and use strdup to copy the string.

moz_construct_header_footer_options doesn't use the loops I wanted.

rv checking is still missing.

+    mPageSetup = NULL;
+  }
+  if (mPrintSettings) {
+    g_object_unref(mPrintSettings);
+    mPrintSettings = NULL;
+  }
+  if (mPrinter) {
+    g_object_unref(mPrinter);
+    mPrinter = NULL;

No need to null these out in the destructor.

+hfBlank=--blank--
+hfTitle=Title
+hfURL=URL
+hfDate=Date/Time
+hfPage=Page #
+hfPageTotal=Page # of #
+hfCustom=Custom...
+customPrompt=Please enter your custom header/footer text

Make these more meaningful for localizers --- expand "hf" to headerFooter, and customHeaderFootPrompt.

Christian, I think this should be good for GTK embedders ... you just have to call nsIPrintOptions::CreatePrintSettings, QI to nsIPrintSettingsGTK, then you can fill in the gtkPageSetup, gtkPrintSettings and gtkPrinter to your liking, and nsDeviceContextSpecGTK will use them when it does gtk_print_job_new.

Comment 52

[Michael Ventnor]

Attachment: Patch 3

Addresses remaining review comments, hopefully.

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess there is a problem, which is that old code that set up the nsIPrintSettings to e.g. print to file via the printToFile attribute and toFileName attribute, will no longer work. So a lot of those attributes need to be reimplemented in nsPrintSettingsGTK to read and write the Gtk objects.

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In fact, nsPrintSettingsGTK should perhaps not extend nsPrintSettings, since we're changing the implementation to store most of its data in Gtk objects instead of fields of the nsPrintSettings class.

Comment 55

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #53)
> I guess there is a problem, which is that old code that set up the
> nsIPrintSettings to e.g. print to file via the printToFile attribute and
> toFileName attribute, will no longer work. So a lot of those attributes need to
> be reimplemented in nsPrintSettingsGTK to read and write the Gtk objects.

Epiphany uses exactly this method to print (print-to-file, then process itself), so this would be a problem for us.

(In reply to comment #50)
> I believe the padding is right, since a lot of those values were borrowed from
> Epiphany's glade files. It looks correct to me.

(I assume you _want_ to make this dialogue HIG compliant? If not, disregard the following.)

It doesn't look correctly to me.
- the labels are missing mnemonics.
- the spacings and paddings are wrong. In fact, you should almost never have to use paddings if you set up the spacings and border widths correctly (except the paddings in the GtkAlignments).

Could you make a screenshot of the custom tab?

Comment 56

[Michael Ventnor]

Attachment: Patch 4

This will give the labels mnemonics and will move a lot of settings transfer code from nsPrintDialogGTK to nsPrintSettingsGTK, so that when you set something with one of the nsPrintSettings functions, the internal GTK object is updated too so it is reflected in the GTK print job.

Print-to-file has moved back to manual handling for embedders.

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Seems like you can get rid of all the Import... and Export... functions by relying on nsPrintSettingsGTK::Set*() and Get*() methods operating on its internal GTK objects correctly. You'll have to stop creating your own GTK objects in ShowPageSetup though.

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

[more offline discussion]

We'll maintain our own outputFormat variable. That just tells us whether to override GTK's idea of what the format should be when we finally do the printing.

We'll also keep our own printToFile variable. There doesn't seem to be a foolproof way to tell GtkPrintSettings to print to a file. However, it looks like we can store the file name in the print settings, so we should.

Thus, most of ExportPrintToFile (the file name parts) will move to nsPrintSettingsGTK::GetToFileName. The format-checking parts will move to nsDeviceContextSpecG::GetSurfaceForPrinter. The rest of ExportPrinter will move to nsPrintSettingsGTK::GetPrinterName. nsPrintDialogWidgetGTK::ExportPageRange will move to nsPrintSettingsGTK::Get*PageRange (or just go away since you already have an implementation of those functions). 

You should document in nsPrintSettingsGTK which variables we're storing for ourselves and explain that for the rest, the Gtk objects contain "the truth".

+  case GTK_PRINT_PAGES_CURRENT:
+    // map this to selection for now

I think this should probably map to the first page or something.

Comment 59

[Michael Ventnor]

Attachment: Patch 5

Please be the last one; no more comments...

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

"Print selection" should be a boolean field of nsPrintSettingsGTK, meaning "override the page ranges and just print the selection". Don't forget to initialize the dialog properly using that field.

ExportPageRange mostly isn't needed. Let's keep the page ranges in the GTK object and extract them from GetStartPageRange/GetEndPageRange. Ditto for the GtkPrintPages option.

  if (gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(selection_only_toggle)))
+    geckoPages = nsIPrintSettings::kRangeSelection;

Store the value of this toggle in nsPrintSettingsGTK. Then when someone calls GetPrintRange, compute the nsIPrintSettings::kRange* value to be returned.

+  PRUnichar* header_footer_str[3];
+
+  aSettings->GetHeaderStrLeft(&header_footer_str[0]);
+  aSettings->GetHeaderStrCenter(&header_footer_str[1]);
+  aSettings->GetHeaderStrRight(&header_footer_str[2]);

These are leaking, right?

+  aSettings->GetFooterStrLeft(&header_footer_str[0]);
+  aSettings->GetFooterStrCenter(&header_footer_str[1]);
+  aSettings->GetFooterStrRight(&header_footer_str[2]);

Same here.

Need to break lines at 80 chars.

+void
+nsPrintDialogWidgetGTK::ExportPrinter(nsIPrintSettings *aNS, GtkPrintSettings *aSettings)
+{
+  aNS->SetOutputFormat(nsIPrintSettings::kOutputFormatNative);

Why do this? We should leave the output format alone.

+  // Is this print-to-file?
+  if (gtk_print_settings_get(aSettings, GTK_PRINT_SETTINGS_OUTPUT_URI)) {
+    aNS->SetPrintToFile(PR_TRUE);
+    return;
+  }

Why do this? I don't think this is needed. GetPrintToFile should just return the "print to file override" set by SetPrintToFile. This may not even be correct if our own code sets GTK_PRINT_SETTINGS_OUTPUT_URI for a printer that's not print-to-file.

+  aNS->SetPrintToFile(PR_FALSE);

Don't do this either.

+  aNS->SetPrinterName(NS_ConvertUTF8toUTF16(gtk_print_settings_get_printer(aSettings)).get());

And don't do this, the settings' internal GTK object is already correct!

+  // Now newPageSetup has a refcount of 2 (SetGtkPageSetup will addref), put it to 1 so if
+  // this gets replaced we don't leak.
+  g_object_unref(newPageSetup);

Don't we need to unref gtkSettings as well?

+  switch (format) {
+    case nsIPrintSettings::kOutputFormatPDF:
+      surface = new gfxPDFSurface(stream, gfxSize(width, height));
+      break;
+    case nsIPrintSettings::kOutputFormatPS:
+      surface = new gfxPSSurface(stream, gfxSize(width, height));
+      break;
+    case nsIPrintSettings::kOutputFormatNative:
+    default:
+    {
+      nsCOMPtr<nsIPrintSettingsGTK> printSettingsGTK(do_QueryInterface(mPrintSettings));
+      if (mToPrinter) {
+        GtkPrinter* thePrinter;
+        printSettingsGTK->GetGtkPrinter(&thePrinter);
+        if (gtk_printer_accepts_pdf(thePrinter))
+          surface = new gfxPDFSurface(stream, gfxSize(width, height));
+        else
+          surface = new gfxPSSurface(stream, gfxSize(width, height));
+      } else {
+        GtkPrintSettings* theSettings;
+        printSettingsGTK->GetGtkPrintSettings(&theSettings);
+        const gchar* fmtGTK = gtk_print_settings_get(theSettings, GTK_PRINT_SETTINGS_OUTPUT_FILE_FORMAT);
+        if (!fmtGTK) // This can be null, make PS the default
+          surface = new gfxPSSurface(stream, gfxSize(width, height));
+        else if (nsDependentCString(fmtGTK).EqualsIgnoreCase("pdf"))
+          surface = new gfxPDFSurface(stream, gfxSize(width, height));
+        else
+          surface = new gfxPSSurface(stream, gfxSize(width, height));
+      }

First compute whether you want to use PDF or PS, *then* do your "new gfxPDFSurface/gfxPSSurface" calls so we only need one each. Declare "gfxSize size(width, height)" outside too so you share that.

+    return NS_ERROR_NOT_AVAILABLE;  // NS_ERROR_UPGRADE_YOUR_GODDAMN_GTK_LIBRARY

Please control yourself.

+    mPrintSettings->GetToFileName(&targetPath);

Don't leak targetPath. Hmm, maybe here and elsewhere we should be using nsXPIDLString and getter_Copies? I think so.

Why do you think toFileName is supposed to be a file: URI? Looks to me like it's meant to be a regular file path.

+    else if (geckoPaper.EqualsIgnoreCase("Legal"))

This should be "legal"

+  nsPrintSettings::GetOrientation(&geckoOrient);

Why do we need this (and others like it) in PreloadPageSetup? Won't it always be the default (portrait, in this case)?

+  // We make a "custom-ified" copy of the paper size so it can be changed later.
+  mPaperSize = gtk_paper_size_new_custom(gtk_paper_size_get_name(szPaper),
+                                         gtk_paper_size_get_display_name(szPaper),
+                                         gtk_paper_size_get_width(szPaper, GTK_UNIT_INCH),
+                                         gtk_paper_size_get_height(szPaper, GTK_UNIT_INCH),
+                                         GTK_UNIT_INCH);

Use a helper function to avoid duplicating this code (there are 3 copies).

+  if (ctRanges >= 1)
+    *aEndPageRange = lstRanges[0].end + 1;

Shouldn't you return the end of the *last* range?

+  // Keep the original name to return in the Getter
+  mPrinter.Assign(aPrinter);

Don't need this, since you aren't using mPrinter any more

+  nsCAutoString gtkPrinter;
+  CopyUTF16toUTF8(aPrinter, gtkPrinter);

use "NS_ConvertUTF16toUTF8 gtkPrinter(...)"

+  else if (gtkPaperName.EqualsIgnoreCase("Legal"))

"legal"

That's all!

Comment 61

[Michael Ventnor]

(In reply to comment #60)

> +void
> +nsPrintDialogWidgetGTK::ExportPrinter(nsIPrintSettings *aNS, GtkPrintSettings
> *aSettings)
> +{
> +  aNS->SetOutputFormat(nsIPrintSettings::kOutputFormatNative);
> 
> Why do this? We should leave the output format alone.

No, the print dialog is just another consumer of the nsIPrintSettings code so it's in control of the object, and it needs to make sure that the output is sent in the format that it thinks is correct, which is GTK's autodetection.

> +  aNS->SetPrintToFile(PR_FALSE);
> 
> Don't do this either.

We have to since print-to-file is on by default, and since I put back the manual spool file copying for the embedders' sake, if I don't unset this it will never print.

> + 
> aNS->SetPrinterName(NS_ConvertUTF8toUTF16(gtk_print_settings_get_printer(aSettings)).get());
> 
> And don't do this, the settings' internal GTK object is already correct!
> 
> +  // Now newPageSetup has a refcount of 2 (SetGtkPageSetup will addref), put
> it to 1 so if
> +  // this gets replaced we don't leak.
> +  g_object_unref(newPageSetup);
> 
> Don't we need to unref gtkSettings as well?

No, its a direct pointer to the internal nsIPrintSettingsGTK private object.
> 
> +    return NS_ERROR_NOT_AVAILABLE;  //
> NS_ERROR_UPGRADE_YOUR_GODDAMN_GTK_LIBRARY
> 
> Please control yourself.

It'll be hard but I'll try. :)

> +    mPrintSettings->GetToFileName(&targetPath);
> 
> Don't leak targetPath. Hmm, maybe here and elsewhere we should be using
> nsXPIDLString and getter_Copies? I think so.
> 
> Why do you think toFileName is supposed to be a file: URI? Looks to me like
> it's meant to be a regular file path.

I used printf's to find out that its a file:// URI which is unsurprising since GTK uses those a lot. If I pass that to the old NS_NewNativeLocalFile I get an error.

Comment 62

[Jeff Walden [:Waldo]]

> +    else if (geckoPaper.EqualsIgnoreCase("Legal"))
> +  else if (gtkPaperName.EqualsIgnoreCase("Legal"))

I'm pretty sure that this causes assertions in debug builds and flat-out doesn't work.  If you're not running a debug build, you probably should be; if you are, I guess you just didn't hit these edge cases while doing manual testing (understandable given how much surface area there is to these changes).

You might want to have a QA person or two thoroughly exercise this to look for glitches in it, or perhaps ask for them to have people attack it at a test day or something after it lands (not sure how the whole "printing" part of that would work without asking people to print lots of garbage, tho).

Comment 63

[Kenneth Herron]

Michael, just a few comments if you don't mind:

1) You should update the initial developer and copyright date on the new files that you're adding.

2) You could remove the definition of |nsDeviceContextSpecGTK::GetPrintSettings|. I added it because my code needed it, but yours doesn't.

3) After printing, what deletes the spool file? You also should delete the spool file if the print operation doesn't complete for some reason.

4) Is it necessary to special-case print-to-file in |nsDeviceContextSpecGTK::EndDocument|? Letting GTK handle print-to-file worked fine for me.

5) A temporary spool file--at least one intended for a printer--should be created with limited permissions, ie 0600. But IMO the user will expect print-to-file to result in a file with permissions reflecting his umask, eg 0644 if his umask is 0022. simply calling |MoveTo| on the spool file won't accomplish this.

Comment 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks for the comments kherron!

> 4) Is it necessary to special-case print-to-file in
> |nsDeviceContextSpecGTK::EndDocument|? Letting GTK handle print-to-file worked
> fine for me.

The issue is if an embedder calls SetPrintToFile/SetToFileName. There doesn't seem to be a way to tell GTK to print to a file. In fact, it might not even be configured with a "print to file" printer so it might not be able to do it. So we need our own support code for that case. In the latest version of the patch we only use our code for that embedding case --- if the user selects "print to file" in the GTK dialog, GTK's code is used.

Comment 65

[Michael Ventnor]

Attachment: Patch 6

Attempt to address all comments.

Comment 66

[Christian Persch (GNOME) (away; not receiving bug mail)]

+  // Is this print-to-file?
+  if (gtk_print_settings_get(aSettings, GTK_PRINT_SETTINGS_OUTPUT_URI)) {
+    aNS->SetPrintToFile(PR_TRUE);
+    return;
+  }

That's not the right way to detect print-to-file. Unfortunately, there is no way to detect whether print-to-file is requested in the print settings; the way gtk works is that in the to-file case the 'file' _printer_ is used. See http://bugzilla.gnome.org/show_bug.cgi?id=345590 .

Comment 67

[Michael Ventnor]

(In reply to comment #66)
> +  // Is this print-to-file?
> +  if (gtk_print_settings_get(aSettings, GTK_PRINT_SETTINGS_OUTPUT_URI)) {
> +    aNS->SetPrintToFile(PR_TRUE);
> +    return;
> +  }
> 
> That's not the right way to detect print-to-file. Unfortunately, there is no
> way to detect whether print-to-file is requested in the print settings; the way
> gtk works is that in the to-file case the 'file' _printer_ is used. See
> http://bugzilla.gnome.org/show_bug.cgi?id=345590 .
> 

That code is gone in the latest patch.

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  // Gets/Sets the printer name in the GtkPrintSettings. If no rpinter name is specified there,

Typo "rpinter"

+  nsresult _Clone(nsIPrintSettings **_retval);
+  nsresult _Assign(nsIPrintSettings *aPS);

Make them virtual to indicate that they override parent class methods. (Not strictly needed but it helps the reader.)

+  nsCAutoString url(gtk_print_settings_get(mPrintSettings, GTK_PRINT_SETTINGS_OUTPUT_URI));

This should be an nsDependentCString.

+  if (!aToFileName || nsDependentString(aToFileName).IsEmpty()) {

Might as well just check aToFileName[0] == 0.

     surface = new gfxPDFSurface(stream, gfxSize(width, height));
-  } else {
+  else
     surface = new gfxPSSurface(stream, gfxSize(width, height));

Use surfaceSize.

Also "if (foo) blah; else corp;" isn't our style. Use braces please (or a ? : operator when reasonable).

+  nsresult PreloadPageSetup();
+

Take this out.

+    if (mToPrinter && !gtk_print_settings_get(theSettings, GTK_PRINT_SETTINGS_OUTPUT_URI)) {

Why this OUTPUT_URI check? We shouldn't need it.

The only remaining issue I'm concerned about is whether mIsInitedFromPrinter and mIsInitedFromPrefs are being set properly. mIsInitedFromPrefs looks like it's just used for some lame-o optimization, so that doesn't matter. But I don't really know about mIsInitedFromPrinter. kherron, can you help?

Comment 69

[Michael Ventnor]

Attachment: Patch 6 nits fixed

To me that variable is also something that we don't need to worry about.

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, I think mIsInitedFromPrinter is mainly used by our XUL print dialogs. But I think for safety we should only clear mIsInitedFromPrinter and mIsInitedFromPrefs if the printer name is actually changing. So you'll want to call gtk_print_settings_set_printer and check.

Comment 71

[Michael Ventnor]

Attachment: Patch 6 more nits fixed

Put back modifications of those variables when needed.

Comment 72

[Michael Ventnor]

Comment on attachment 297481 [details] [diff] [review]
Patch 6 more nits fixed

Requesting a1.9; We need to do all we can to improve the printing experience (printing web pages is a surprisingly common task!) and that includes killing off the asinine XUL print dialog on GTK.

Comment 73

[Michael Ventnor]

(In reply to comment #72)
> (From update of attachment 297481 [details] [diff] [review])
> Requesting a1.9; We need to do all we can to improve the printing experience
> (printing web pages is a surprisingly common task!) and that includes killing
> off the asinine XUL print dialog on GTK.
> 

At Roc's request I must seriously stress the horrific nature of our current setup. Things are so bad that changing advanced options requires typing in a custom _lpr command_ in a special text field. Unlike GNOME's dialog, almost nothing is presented pictorially so a lot of the time you have no idea of the effects of the values you are changing, and this switch gives us easy access to features that even Mozilla's system doesn't support yet GTK gives us for free due to the trivial rewrite of nsDeviceContextSpecG.cpp (which is a very small file).

CUPS will no longer have a monopoly on our GTK printing system which makes support of our GTK builds much easier to do on non-UNIX systems.

We bought a sexy new printer for the office so this has received a large amount of testing by me already, and I did design this patch to smoothly integrate things as much as possible.

Comment 74

[Mike Schroepfer]

Comment on attachment 297481 [details] [diff] [review]
Patch 6 more nits fixed

Michael this is awesome.  You ready to catch any follow-ups/regressions?

Comment 75

[Michael Ventnor]

(In reply to comment #74)
> (From update of attachment 297481 [details] [diff] [review])
> Michael this is awesome.  You ready to catch any follow-ups/regressions?
> 

I can only do my best! :-)

Comment 76

[Reed Loden [:reed]]

Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1912; previous revision: 1.1911
done
Checking in config/system-headers;
/cvsroot/mozilla/config/system-headers,v  <--  system-headers
new revision: 3.33; previous revision: 3.32
done
Checking in embedding/components/printingui/src/unixshared/nsPrintingPromptService.cpp;
/cvsroot/mozilla/embedding/components/printingui/src/unixshared/nsPrintingPromptService.cpp,v  <--  nsPrintingPromptService.cpp
new revision: 1.19; previous revision: 1.18
done
RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/global/gnomeprintdialog.properties,v
done
Checking in toolkit/locales/en-US/chrome/global/gnomeprintdialog.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global/gnomeprintdialog.properties,v  <--  gnomeprintdialog.properties
initial revision: 1.1
done
Checking in toolkit/locales/jar.mn;
/cvsroot/mozilla/toolkit/locales/jar.mn,v  <--  jar.mn
new revision: 1.45; previous revision: 1.44
done
Checking in layout/printing/nsPrintEngine.cpp;
/cvsroot/mozilla/layout/printing/nsPrintEngine.cpp,v  <--  nsPrintEngine.cpp
new revision: 1.159; previous revision: 1.158
done
Checking in widget/public/Makefile.in;
/cvsroot/mozilla/widget/public/Makefile.in,v  <--  Makefile.in
new revision: 1.116; previous revision: 1.115
done
RCS file: /cvsroot/mozilla/widget/public/nsIPrintDialogService.h,v
done
Checking in widget/public/nsIPrintDialogService.h;
/cvsroot/mozilla/widget/public/nsIPrintDialogService.h,v  <--  nsIPrintDialogService.h
initial revision: 3.1
done
RCS file: /cvsroot/mozilla/widget/public/nsIPrintSettingsGTK.idl,v
done
Checking in widget/public/nsIPrintSettingsGTK.idl;
/cvsroot/mozilla/widget/public/nsIPrintSettingsGTK.idl,v  <--  nsIPrintSettingsGTK.idl
initial revision: 3.1
done
Checking in widget/public/nsWidgetsCID.h;
/cvsroot/mozilla/widget/public/nsWidgetsCID.h,v  <--  nsWidgetsCID.h
new revision: 3.51; previous revision: 3.50
done
Checking in widget/src/gtk2/Makefile.in;
/cvsroot/mozilla/widget/src/gtk2/Makefile.in,v  <--  Makefile.in
new revision: 1.71; previous revision: 1.70
done
Checking in widget/src/gtk2/nsDeviceContextSpecG.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsDeviceContextSpecG.cpp,v  <--  nsDeviceContextSpecG.cpp
new revision: 1.96; previous revision: 1.95
done
Checking in widget/src/gtk2/nsDeviceContextSpecG.h;
/cvsroot/mozilla/widget/src/gtk2/nsDeviceContextSpecG.h,v  <--  nsDeviceContextSpecG.h
new revision: 1.42; previous revision: 1.41
done
RCS file: /cvsroot/mozilla/widget/src/gtk2/nsPrintDialogGTK.cpp,v
done
Checking in widget/src/gtk2/nsPrintDialogGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsPrintDialogGTK.cpp,v  <--  nsPrintDialogGTK.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/widget/src/gtk2/nsPrintDialogGTK.h,v
done
Checking in widget/src/gtk2/nsPrintDialogGTK.h;
/cvsroot/mozilla/widget/src/gtk2/nsPrintDialogGTK.h,v  <--  nsPrintDialogGTK.h
initial revision: 1.1
done
Removing widget/src/gtk2/nsPrintJobFactoryGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsPrintJobFactoryGTK.cpp,v  <--  nsPrintJobFactoryGTK.cpp
new revision: delete; previous revision: 1.1
done
Removing widget/src/gtk2/nsPrintJobFactoryGTK.h;
/cvsroot/mozilla/widget/src/gtk2/nsPrintJobFactoryGTK.h,v  <--  nsPrintJobFactoryGTK.h
new revision: delete; previous revision: 1.2
done
Removing widget/src/gtk2/nsPrintJobGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsPrintJobGTK.cpp,v  <--  nsPrintJobGTK.cpp
new revision: delete; previous revision: 1.4
done
Removing widget/src/gtk2/nsPrintJobGTK.h;
/cvsroot/mozilla/widget/src/gtk2/nsPrintJobGTK.h,v  <--  nsPrintJobGTK.h
new revision: delete; previous revision: 1.2
done
Checking in widget/src/gtk2/nsPrintOptionsGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsPrintOptionsGTK.cpp,v  <--  nsPrintOptionsGTK.cpp
new revision: 1.4; previous revision: 1.3
done
Checking in widget/src/gtk2/nsPrintOptionsGTK.h;
/cvsroot/mozilla/widget/src/gtk2/nsPrintOptionsGTK.h,v  <--  nsPrintOptionsGTK.h
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/widget/src/gtk2/nsPrintSettingsGTK.cpp,v
done
Checking in widget/src/gtk2/nsPrintSettingsGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsPrintSettingsGTK.cpp,v  <--  nsPrintSettingsGTK.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/widget/src/gtk2/nsPrintSettingsGTK.h,v
done
Checking in widget/src/gtk2/nsPrintSettingsGTK.h;
/cvsroot/mozilla/widget/src/gtk2/nsPrintSettingsGTK.h,v  <--  nsPrintSettingsGTK.h
initial revision: 1.1
done
Checking in widget/src/gtk2/nsWidgetFactory.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsWidgetFactory.cpp,v  <--  nsWidgetFactory.cpp
new revision: 1.40; previous revision: 1.39
done

Comment 77

[Ginn Chen]

"nsDeviceContextSpecG.cpp", line 678: Error: nsISupports::AddRef() is not accessible from nsDeviceContextSpecGTK::EndDocument().

Should it be NS_ADDREF(mSpoolFile.get()) ?

Comment 78

[Reed Loden [:reed]]

(In reply to comment #77)
> "nsDeviceContextSpecG.cpp", line 678: Error: nsISupports::AddRef() is not
> accessible from nsDeviceContextSpecGTK::EndDocument().
> 
> Should it be NS_ADDREF(mSpoolFile.get()) ?

Checking in widget/src/gtk2/nsDeviceContextSpecG.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsDeviceContextSpecG.cpp,v  <--  nsDeviceContextSpecG.cpp
new revision: 1.97; previous revision: 1.96
done

Comment 79

[Ginn Chen]

It works.

Comment 80

[Reed Loden [:reed]]

This causes the printing test from bug 396024 to crash during mochitest.

Part of the stack:
(gdb) bt full
#0  0xffffe410 in __kernel_vsyscall ()
No symbol table info available.
#1  0xb73e8e96 in nanosleep () from /lib/tls/i686/cmov/libc.so.6
No symbol table info available.
#2  0xb73e8ca7 in sleep () from /lib/tls/i686/cmov/libc.so.6
No symbol table info available.
#3  0xb7f0ee03 in ah_crap_handler (signum=11) at nsSigHandlers.cpp:149
No locals.
#4  0xb7f23251 in nsProfileLock::FatalSignalHandler (signo=11) at nsProfileLock.cpp:216
        oldact = (sigaction *) 0xb749bff4
#5  <signal handler called>
No symbol table info available.
#6  0xb5b95589 in DocumentViewerImpl::PrintPreview (this=0x8ccb900, 
    aPrintSettings=0x8ccc488, aChildDOMWin=0x0, aWebProgressListener=0x8c824d0)
    at /home/reed/mozilla/builds/mozilla/layout/base/nsDocumentViewer.cpp:3537
        rv = <value optimized out>
        xulDoc = {mRawPtr = 0x0}
        docShell = {mRawPtr = 0x0}
        presShell = {mRawPtr = 0x0}
#7  0xb7e0265d in NS_InvokeByIndex_P ()
    at /home/reed/mozilla/builds/mozilla/xpcom/reflect/xptinfo/src/xptiInterfaceInfo.cpp:73
        nsIInterfaceInfo::COMTypeInfo<int>::kIID = {m0 = 559791620, m1 = 38055, 
  m2 = 4562, m3 = "�X\000\200_\212]�"}
        nsISupports::COMTypeInfo<int>::kIID = {m0 = 0, m1 = 0, m2 = 0, 
  m3 = "�\000\000\000\000\000\000F"}

Comment 81

[Reed Loden [:reed]]

Glancing at the numbers, this actually might be a Ts win, too! :)

Comment 82

[Martijn Wargers (dead)]

The mochitest for bug 396024 is testing print preview code, but doesn't really work right now, because no printers are installed on the tinderboxes.
So it might be good to test if the patch doesn't cause a crash when no printers are installed.
Maybe bug 408355 is related?

Otherwise, I don't know why it's causing the crash. I guess you might want to disable the mochitest then, although I would prefer it if the crash was fixed.

Comment 83

[Michael Ventnor]

Attachment: Fix test failure

This special-cases print preview in GetSurfaceForPrinter so we don't try to detect the format capabilities of a non-existent printer when no printer is installed.

I think this is the problem, I can't really tell. But if no printers are on Tinderbox this seems likely.

Comment 84

[Michael Ventnor]

Comment on attachment 297600 [details] [diff] [review]
Fix test failure

This doesn't fix it.

Comment 85

[Michael Ventnor]

Attachment: Fix test failure

It took a while but I finally figured out the test failure. Yay! It turns out it was an ugly race condition in the event loop which caused one of the frames to be destroyed before we got the chance to show the print preview. This is fixed by moving the printer enumeration (which causes the probing of the event loop) to a much later location.

Some code had to be changed to make sure this move didn't cause more crashes. I also added a failure return where the crash happened. We return early when we detect the absence of other important global variables, so why not the mContainer? This will make print and print preview much safer.

Comment 86

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is definitely an improvement, but we're potentially just moving the crash to code that creates the devicecontextspec, namely nsPrintEngine::DoCommonPrint. We need to look at the "silent printing" APIs and figure out which one we should make potentially spin the event loop.

Or else we could do something a little more outrageous and maintain the default printer in nsToolkitGTK, by re-getting the default printer periodically and asynchronously.

Comment 87

[Kenneth Herron]

(In reply to comment #86)
> Or else we could do something a little more outrageous and maintain the default
> printer in nsToolkitGTK, by re-getting the default printer periodically and
> asynchronously.

As I understand it, neither the GTK nor CUPS layer caches printers, so each printer enumeration can result in queries to remote print servers. See eg <http://bugzilla.gnome.org/show_bug.cgi?id=370069> and <http://bugzilla.gnome.org/show_bug.cgi?id=508905>.

Comment 88

[Michael Ventnor]

(In reply to comment #86)
> This is definitely an improvement, but we're potentially just moving the crash
> to code that creates the devicecontextspec, namely
> nsPrintEngine::DoCommonPrint. We need to look at the "silent printing" APIs and
> figure out which one we should make potentially spin the event loop.

The problem is there is no "silent printing API's", it all takes the same path. Its just that silent printing skips the code that shows the print dialog. The reason I put it there is to do it as least as possible. We don't need it for print preview and the print dialog will give us a GtkPrinter anyway. I think this is the safest place to put it, and with the extra failure checks in nsDocumentViewer it will become even safer.

> Or else we could do something a little more outrageous and maintain the default
> printer in nsToolkitGTK, by re-getting the default printer periodically and
> asynchronously.

We should never do anything that can encourage races, in this case between nsToolkitGTK and nsDeviceContextSpecGTK.

Comment 89

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #88)
> I think
> this is the safest place to put it, and with the extra failure checks in
> nsDocumentViewer it will become even safer.

Those checks may block that particular crash, but processing events during nsDeviceContextSpecGTK::Init is still BAD BAD BAD scary stuff.

> > Or else we could do something a little more outrageous and maintain the
> > default printer in nsToolkitGTK, by re-getting the default printer
> > periodically and asynchronously.
> 
> We should never do anything that can encourage races, in this case between
> nsToolkitGTK and nsDeviceContextSpecGTK.

What races? You're worried about the default printer changing or going away between the poll and the print?

How do things work today? If CUPS already does some kind of expensive checking of remote printers, do we just block completely? do we process events during that time? if we process events, during which print API call? Someone please read the code so I don't have to.

Comment 90

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This seems lame, GTK must have some local knowledge of what the default printer is, but there doesn't seem any way to get that directly.

As a mega-lame solution to "silent printing", could we open the print dialog and then instantly close it as if the user had clicked OK?

Comment 91

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We decided to add a new SetupSilentPrinting method to nsIPrintSettings that will be called by nsPrintEngine::DoCommonPrint at the place where it currently calls ShowPrintDialog for non-silent printing, which spins the event loop. SetupSilentPrinting will be allowed to spin the event loop. This should add no new event-loop hazards, because if the print.always_print_silent preference is created and set to false, this code would always pop up the dialog and spin the event loop even for clients that set silentPrinting to true explicitly.

Comment 92

[Michael Ventnor]

Attachment: Attempt to constrain the event loop run

Yeah, this seems to work.

Comment 93

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 298171 [details] [diff] [review]
Attempt to constrain the event loop run

+  /**
+   * We call this function so that anything that requires a run of the event loop
+   * can do so safely. The print dialog runs the event loop but in silent printing
+   * that doesn't happen.
+   */

Please document that either SetupSilentPrinting or ShowPrintDialog (but not both) must be called by the print engine when actually printing.

+        // XXX Some platforms allow the user to change the ShrinkToFit option so change it here too

How about
"// The user might have changed shrink-to-fit in the print dialog, so update our copy of its state"
(shouldn't be XXX since there is not a known probem with this)

Comment 94

[Michael Ventnor]

Attachment: Comment fix

Comment 95

[Michael Ventnor]

Attachment: ...And rev the UUID

I'm so bad at remembering to do this :(

Comment 96

[Michael Ventnor]

Attachment: ...And update to tip

Comment 97

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I just realized that nsIPrintSettingsGTK shouldn't be IDL. In fact, it shouldn't even exist. The code should just use nsPrintSettingsGTK directly.

Comment 98

[Michael Ventnor]

Attachment: Address Roc's literally last-second interface rewrite comment

When this gets checked in, the files nsPrintJobFactoryGTK.cpp and nsPrintJobGTK.cpp must be removed from widget/src/gtk2/.

Comment 99

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 298194 [details] [diff] [review]
Address Roc's literally last-second interface rewrite comment

+#define NS_IPRINTSETTINGSGTK_IID \

call it NS_PRINTSETTINGSGTK_CID

Comment 100

[Michael Ventnor]

Attachment: Address picky comment

Comment 101

[Reed Loden [:reed]]

Attachment: Include file removals and remove some other old code (checked in)

Comment 102

[Reed Loden [:reed]]

Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1916; previous revision: 1.1915
done
Checking in config/system-headers;
/cvsroot/mozilla/config/system-headers,v  <--  system-headers
new revision: 3.35; previous revision: 3.34
done
Checking in embedding/components/printingui/src/unixshared/nsPrintingPromptService.cpp;
/cvsroot/mozilla/embedding/components/printingui/src/unixshared/nsPrintingPromptService.cpp,v  <--  nsPrintingPromptService.cpp
new revision: 1.21; previous revision: 1.20
done
Checking in toolkit/locales/en-US/chrome/global/gnomeprintdialog.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global/gnomeprintdialog.properties,v  <--  gnomeprintdialog.properties
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/locales/jar.mn;
/cvsroot/mozilla/toolkit/locales/jar.mn,v  <--  jar.mn
new revision: 1.47; previous revision: 1.46
done
Checking in layout/printing/nsPrintEngine.cpp;
/cvsroot/mozilla/layout/printing/nsPrintEngine.cpp,v  <--  nsPrintEngine.cpp
new revision: 1.161; previous revision: 1.160
done
Checking in layout/base/nsDocumentViewer.cpp;
/cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v  <--  nsDocumentViewer.cpp
new revision: 1.561; previous revision: 1.560
done
Checking in widget/src/xpwidgets/nsPrintSettingsImpl.cpp;
/cvsroot/mozilla/widget/src/xpwidgets/nsPrintSettingsImpl.cpp,v  <--  nsPrintSettingsImpl.cpp
new revision: 1.42; previous revision: 1.41
done
Checking in widget/public/Makefile.in;
/cvsroot/mozilla/widget/public/Makefile.in,v  <--  Makefile.in
new revision: 1.119; previous revision: 1.118
done
Checking in widget/public/nsIPrintDialogService.h;
/cvsroot/mozilla/widget/public/nsIPrintDialogService.h,v  <--  nsIPrintDialogService.h
new revision: 3.3; previous revision: 3.2
done
Checking in widget/public/nsIPrintSettings.idl;
/cvsroot/mozilla/widget/public/nsIPrintSettings.idl,v  <--  nsIPrintSettings.idl
new revision: 1.44; previous revision: 1.43
done
Checking in widget/public/nsWidgetsCID.h;
/cvsroot/mozilla/widget/public/nsWidgetsCID.h,v  <--  nsWidgetsCID.h
new revision: 3.53; previous revision: 3.52
done
Checking in widget/src/gtk2/Makefile.in;
/cvsroot/mozilla/widget/src/gtk2/Makefile.in,v  <--  Makefile.in
new revision: 1.73; previous revision: 1.72
done
Checking in widget/src/gtk2/nsDeviceContextSpecG.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsDeviceContextSpecG.cpp,v  <--  nsDeviceContextSpecG.cpp
new revision: 1.99; previous revision: 1.98
done
Checking in widget/src/gtk2/nsDeviceContextSpecG.h;
/cvsroot/mozilla/widget/src/gtk2/nsDeviceContextSpecG.h,v  <--  nsDeviceContextSpecG.h
new revision: 1.44; previous revision: 1.43
done
Removing widget/src/gtk2/nsIPrintJobGTK.h;
/cvsroot/mozilla/widget/src/gtk2/nsIPrintJobGTK.h,v  <--  nsIPrintJobGTK.h
new revision: delete; previous revision: 1.2
done
Checking in widget/src/gtk2/nsPrintDialogGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsPrintDialogGTK.cpp,v  <--  nsPrintDialogGTK.cpp
new revision: 1.3; previous revision: 1.2
done
Checking in widget/src/gtk2/nsPrintDialogGTK.h;
/cvsroot/mozilla/widget/src/gtk2/nsPrintDialogGTK.h,v  <--  nsPrintDialogGTK.h
new revision: 1.3; previous revision: 1.2
done
Removing widget/src/gtk2/nsPrintJobFactoryGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsPrintJobFactoryGTK.cpp,v  <--  nsPrintJobFactoryGTK.cpp
new revision: delete; previous revision: 1.3
done
Removing widget/src/gtk2/nsPrintJobFactoryGTK.h;
/cvsroot/mozilla/widget/src/gtk2/nsPrintJobFactoryGTK.h,v  <--  nsPrintJobFactoryGTK.h
new revision: delete; previous revision: 1.4
done
Removing widget/src/gtk2/nsPrintJobGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsPrintJobGTK.cpp,v  <--  nsPrintJobGTK.cpp
new revision: delete; previous revision: 1.6
done
Removing widget/src/gtk2/nsPrintJobGTK.h;
/cvsroot/mozilla/widget/src/gtk2/nsPrintJobGTK.h,v  <--  nsPrintJobGTK.h
new revision: delete; previous revision: 1.4
done
Checking in widget/src/gtk2/nsPrintOptionsGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsPrintOptionsGTK.cpp,v  <--  nsPrintOptionsGTK.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in widget/src/gtk2/nsPrintOptionsGTK.h;
/cvsroot/mozilla/widget/src/gtk2/nsPrintOptionsGTK.h,v  <--  nsPrintOptionsGTK.h
new revision: 1.5; previous revision: 1.4
done
Checking in widget/src/gtk2/nsPrintSettingsGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsPrintSettingsGTK.cpp,v  <--  nsPrintSettingsGTK.cpp
new revision: 1.3; previous revision: 1.2
done
Checking in widget/src/gtk2/nsPrintSettingsGTK.h;
/cvsroot/mozilla/widget/src/gtk2/nsPrintSettingsGTK.h,v  <--  nsPrintSettingsGTK.h
new revision: 1.3; previous revision: 1.2
done
Checking in widget/src/gtk2/nsWidgetFactory.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsWidgetFactory.cpp,v  <--  nsWidgetFactory.cpp
new revision: 1.42; previous revision: 1.41
done

Comment 103

[Michael Ventnor]

Attachment: Possible Crash follow-up (checked in)

This may be the cause of some people's crashes lately, see if this fixes anything.

Comment 104

[Reed Loden [:reed]]

Checking in widget/src/gtk2/nsPrintSettingsGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsPrintSettingsGTK.cpp,v  <--  nsPrintSettingsGTK.cpp
new revision: 1.4; previous revision: 1.3
done

Comment 105

[Fredrik Larsson (:nossralf)]

For reference, the "crashes on File > Page Setup" bug is bug 413414. (I hope this doesn't qualify as bug spam.)

Comment 106

[Michael Ventnor]

Attachment: Followup 2: HIG fixes

This will make the custom text dialog obey the HIG more by switching the order of the buttons, and marking one as the default for when you press Enter.

Comment 107

[Reed Loden [:reed]]

Checking in widget/src/gtk2/nsPrintDialogGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsPrintDialogGTK.cpp,v  <--  nsPrintDialogGTK.cpp
new revision: 1.4; previous revision: 1.3
done

Comment 108

[Ian Spence]

This is just a nitpick, but it'd be nice if the page setup and other dialogs had a firefox icon (well, the icon of the program using the toolkit)