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

bryner says he'll take this, fwiw. /be

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.

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

otaylor@redhat.com 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.

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

otaylor@redhat.com 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.

otaylor@redhat.com: 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...

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

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.

(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?

(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

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)

any chances of getting something working by 1.8a?

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.

marco@gnome.org 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 ?

marco@gnome.org 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).

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

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

(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?

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?

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

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.

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

marco@gnome.org 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).

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

Updated chart, on demand I can add CDEnext and KDE dialogs, 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. 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.

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

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

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?

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.

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.

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.

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

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

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!

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.

Kenneth, are you still actively working on this?

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

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.

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.

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

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?

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

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

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

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!

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 ?

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

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

Addresses remaining review comments, hopefully.

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.

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.

(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?

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.

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.

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

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

"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!

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

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

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.

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.

Attempt to address all comments.

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

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

+ // 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?

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

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.

Put back modifications of those variables when needed.

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.

(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 on attachment 297481 [details] [diff] [review] Patch 6 more nits fixed Michael this is awesome. You ready to catch any follow-ups/regressions?

(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! :-)

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

"nsDeviceContextSpecG.cpp", line 678: Error: nsISupports::AddRef() is not accessible from nsDeviceContextSpecGTK::EndDocument(). Should it be NS_ADDREF(mSpoolFile.get()) ?

(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

It works.

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"}

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

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.

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 on attachment 297600 [details] [diff] [review] Fix test failure This doesn't fix it.

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.

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.

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

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

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

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?

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.

Yeah, this seems to work.

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)

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

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

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

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

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

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

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

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

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.

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

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)