693230 - Implement nsIPrintDialogService for Windows

Status: RESOLVED DUPLICATE of bug 1405210

(Toolkit :: Printing, defect)

Description

[Justin Dolske [:Dolske]]

Bug 629500 is a massive refactoring of our printing code. The one piece preventing it from eventually landing is that it needs a implementation of nsIPrintDialogService for Windows.

Patch 13/19's nsIPrintDialogService :: ShowPageSetup() is, I think, the only thing that needs implemented. I think it "just" needs to bring up a native page setup dialog, converting options to/from the provided nsIPrintSettings.

This should be a nice exercise in Gecko native code for Jared, even though there's a lot of related churn. I suspect the existing code in bug 629500 has bitrotted, I'd suggest either waiting until Zach posts an updated patch, or pull an old version of the tree and work with his patches on top of that. [Ideally as part of Zach's update he'll provide an Hg bundle, so that you don't have to manually apply 19 patches!]

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Sorry for the delay. I plan on working on this bug this week.

Comment 2

[Zack Weinberg (:zwol)]

Thanks.  There's been significant churn, so I'll post an updated bundle for bug 629500 later today.  Meanwhile, if you have some spare cycles, I could really use some help pinning down the Windows XP tryserver failures in bug 702678 - I don't have anything with XP on it anymore, and I don't have the day it would take to spin up a VM.

Comment 3

[Zack Weinberg (:zwol)]

https://bugzilla.mozilla.org/attachment.cgi?id=591327 is an up-to-date bundle for bug 629500's changes.  Note that I have not yet gotten a good tryserver cycle on Windows, but hopefully http://tbpl.mozilla.org/?tree=Try&rev=1437387551c4 (corresponding to this bundle) will be it.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP of patch for bug 693230

This patch gets the Page Setup dialog added but the settings that are changed are not persisted yet.

There is also some work to bring this dialog to feature parity with the current Page Setup dialog, although I'm not sure what the requirements are when it comes to feature parity here.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP of patch for bug 693230

Removed an erroneous return in ShowPageSetup.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP of patch for bug 693230

Brian: I know you might not be too familiar with the printing code (I don't know who is...), but maybe you could help me out here? The settings that are changed in the page setup dialog aren't getting persisted. Does anything jump out at you?

Comment 7

[Zack Weinberg (:zwol)]

Did page setup settings persist in the old state of the code? (pre-629500)  There are a bunch of old bugs that boil down to 'print settings don't persist' (e.g. bug 446041, bug 526811).

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Zack Weinberg (:zwol) from comment #7)
> Did page setup settings persist in the old state of the code? (pre-629500) 
> There are a bunch of old bugs that boil down to 'print settings don't
> persist' (e.g. bug 446041, bug 526811).

In today's nightly, you can open Print Preview, then choose Page Setup, and change to Landscape. When exiting page setup, the preview now shows Landscape mode.

With this patch applied, the preview isn't updated to reflect the new Landscape mode.

Comment 9

[Zack Weinberg (:zwol)]

Oh, okay, that's rather worse than what I was talking about.  Unfortunately, I can't tell you what's wrong, either. :-/

Comment 10

[Brian R. Bondy [:bbondy]]

Comment on attachment 591979 [details] [diff] [review]
WIP of patch for bug 693230

Review of attachment 591979 [details] [diff] [review]:
-----------------------------------------------------------------

Just a question to see what happens... where is this setup function being called? 

Can I have a consolidated patch that I can look at the new code? 
It's hard to see because I don't have the related code that this patch was changed from to look at.

::: widget/windows/nsPrintDialogWin.cpp
@@ +219,5 @@
>      // Setup Orientation
>      PRInt32 orientation;
>      aPrintSettings->GetOrientation(&orientation);
> +    aDevMode->dmOrientation = orientation == nsIPrintSettings::kPortraitOrientation ? DMORIENT_PORTRAIT :
> +                                                                                      DMORIENT_LANDSCAPE;

What happens if you hardcode landscape here?

@@ +244,5 @@
>  
>    aPrintSettings->SetIsInitializedFromPrinter(true);
>    if (aDevMode->dmFields & DM_ORIENTATION) {
> +    PRInt32 orientation  = aDevMode->dmOrientation == DMORIENT_PORTRAIT ? nsIPrintSettings::kPortraitOrientation :
> +                                                                          nsIPrintSettings::kLandscapeOrientation;

Ditto

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: nsPrintDialogWin.cpp

(In reply to Brian R. Bondy [:bbondy] from comment #10)
> Comment on attachment 591979 [details] [diff] [review]
> WIP of patch for bug 693230
> 
> Review of attachment 591979 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a question to see what happens... where is this setup function being
> called? 

This setup function is called from CreateGlobalDevModeAndInit().

> Can I have a consolidated patch that I can look at the new code? 
> It's hard to see because I don't have the related code that this patch was
> changed from to look at.

The entire patch series that this relies on is pretty large (see bug 629500), however just viewing nsPrintDialogWin.cpp may be enough. I've attached it to this bug.
 
> ::: widget/windows/nsPrintDialogWin.cpp
> @@ +219,5 @@
> >      // Setup Orientation
> >      PRInt32 orientation;
> >      aPrintSettings->GetOrientation(&orientation);
> > +    aDevMode->dmOrientation = orientation == nsIPrintSettings::kPortraitOrientation ? DMORIENT_PORTRAIT :
> > +                                                                                      DMORIENT_LANDSCAPE;
> 
> What happens if you hardcode landscape here?

Hardcoding landscape here causes the page setup dialog to open with the landscape radio button checked. So this looks good.

> @@ +244,5 @@
> >  
> >    aPrintSettings->SetIsInitializedFromPrinter(true);
> >    if (aDevMode->dmFields & DM_ORIENTATION) {
> > +    PRInt32 orientation  = aDevMode->dmOrientation == DMORIENT_PORTRAIT ? nsIPrintSettings::kPortraitOrientation :
> > +                                                                          nsIPrintSettings::kLandscapeOrientation;
> 
> Ditto

Hardcoding landscape here doesn't seem to have an effect when the print preview is reloaded. In other words, the page stays in portrait mode.

Comment 12

[Brian R. Bondy [:bbondy]]

I'm not familiar with the printing code but since hardcoding the first aDevMode->dmOrientation works I don't think the problem is a Windows specific one.  

It sounds like the settings are not being saved or loaded correctly.  Try debugging the following functions to see if the orientation gets loaded and saved correctly or not.

layout/printing/nsPrintEngine.cpp
> nsPrintEngine::GetGlobalPrintSettings(nsIPrintSettings **aGlobalPrintSettings)

From: /widget/xpwidgets/nsPrintOptionsImpl.cpp

> nsPrintOptions::SavePrintSettingsToPrefs(nsIPrintSettings *aPS, 
>                                          bool aUsePrinterNamePrefix, 
>                                          PRUint32 aFlags)

And: 
 
> nsPrintOptions::InitPrintSettingsFromPrinter(const PRUnichar *aPrinterName,
>                                              nsIPrintSettings *aPrintSettings)

Comment 13

[Zack Weinberg (:zwol)]

For what it's worth, the equivalent manipulation appears to work on Linux/Gnome.  Have a look at http://mxr.mozilla.org/mozilla-central/source/widget/gtk2/nsPrintDialogGTK.cpp#602 ?

Comment 14

[Brian R. Bondy [:bbondy]]

Strange, so I guess debug on Windows to see if the save/load works as expected.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

This is very low on my priority list due to the Australis work that I'm doing. If someone else would like to help out here that would be much appreciated.

Comment 16

[Michal Jaskurzynski]

Attachment: fixed patch

I fixed issue with saving print settings. There was problem with settings name. Some modules uses printer name prefix, some not. I changed saving settings with printer prefix to false in setup dialog and preview dialog, because there is no association with printer on Windows.
There is another problem with saving margins. There aren't saved on nsPrintDialogServiceWin::ShowPageSetup. I insert saving fixed values to Print Settings but as a result empty page is generated on print preview.

Comment 17

[Michal Jaskurzynski]

Attachment: fixed patch

I solved problem with margins and now everything works.

Comment 18

[Zack Weinberg (:zwol)]

Comment on attachment 653042 [details] [diff] [review]
fixed patch

The changes to printPreviewBindings.xml and printUtils.js make me nervous, as those are used cross-platform; could you explain please why they are necessary?  If you have reason to believe that they won't break Mac or Linux that would be good to hear, too.

Unfortunately I am in the middle of a cross-country move and will not have time to hack on Mozilla for at least two weeks, but I'm delighted to see this coming unjammed finally.

Comment 19

[Michal Jaskurzynski]

Attachment: fixed patch

Changes I made in printPreviewBindings.xml and printUtils.js disables saving print settings with default printer prefix. That was the reason why settings from page setup desn't affect print preview and printing. In this patch I removed saving default printer to settings in page setup and now all settings have the same name without changing printPreviewBindings.xml. 
However I think that printSettings.kInitSaveNativeData flag in printUtils.js should be change to printSettings.kInitSaveAll. I don't know what is this flag used for, but it want save settings to prefs because it sets only one bit. Moreover this flag is not used in nsPrintSettingsService. Can anybody explain why this flag was used instead of kInitSaveAll.

Comment 20

[Michal Jaskurzynski]

Attachment: fixed patch

Comment 21

[Zack Weinberg (:zwol)]

Persistent print settings are pretty much broken everywhere, and also nobody knows how that feature was meant to work anymore.  Rather than worry about whether individual calls to save print data use the right kInitSaveWhatever flags, I think the right way to assess this patch is black-box testing to confirm that Page Setup settings persist over the lifetime of one browser process, and are applied to subsequent print jobs, and Print dialog box settings are applied to *that* print job, on all the tier 1 platforms.

(It has been my intention since I started hacking on this stuff to take my trusty chain saw to the print settings code ... as soon as we get through bug 629500 and its dependencies.  The patch stack is difficult enough to work with already.)

Comment 22

[Brian R. Bondy [:bbondy]]

Hi guys, last week rstrong asked me to take a look at how much work would be needed to close this out, and I'm just getting around to it now. 

Can you describe what patches I need to get started with this on my own machine?  Do I need any patches from other bugs? How about the 3 patches in this bug?

Comment 23

[Zack Weinberg (:zwol)]

(In reply to Brian R. Bondy [:bbondy] from comment #22)
> Hi guys, last week rstrong asked me to take a look at how much work would be
> needed to close this out, and I'm just getting around to it now. 

Awesome!  I have been thinking for a couple months about trying to find the time to set up a Windows development environment and finish it myself, but I am sure it will be faster and less painful for someone who actually knows how to write Windows GUI code.

Please don't hesitate to bug me about any problems you run into.

> Can you describe what patches I need to get started with this on my own
> machine?  Do I need any patches from other bugs? How about the 3 patches in
> this bug?

I think attachment 653453 [details] [diff] [review] represents the current state of play, but you should only take it as a guideline, because I think some of the things in there are wrong, and because if you try to use it directly you're going to need a bunch of patches from bug 629500 which have probably bitrotted by now.

Instead, you want to work relative to the current state of the code, so you want to be modifying embedding/components/printingui/src/win/nsPrintDialogUtil.cpp and .../nsPrintingPromptService.cpp.  Specifically, nsPrintDialogUtil.cpp should grow a NativeShowPageSetup function that invokes ::PageSetupDlgW (I *think* that's the right Win32 API) and then translates the information returned by the OS into our own data structures.  nsPrintingPromptService::ShowPageSetup (in the same directory) should then use NativeShowPageSetup rather than what it does now.  That would be enough to close *this* bug.

I'll then move all the code around in bug 629500 and I'll have to figure out what to do about bug 650960.  If you have spare cycles to help with either of those (especially the latter) that would be appreciated, but it is not necessary.  And what would be even more helpful is if you could *modernize* NativeShowPrintDialog while you were in there.  To my not-terribly-educated eye it looks like it was written in the era of Windows 95 and probably isn't doing things the way Vista or maybe even XP would like you to do things.

Comment 24

[Oskar Eisemuth]

Is there any progress here?

As the printing system for my need is totally broken, 
it would be nice to know if the overall status changed here, specially bug 629500.

I am quite not sure about the direction, having a pdf viewer in core but the printing system is totally borked.

Comment 25

[Zack Weinberg (:zwol)]

There has been no progress.  We are waiting for someone who knows Win32 to show up and write the code.  That is the only thing that will help here.

Philipp, Guillaume: I don't know if this is the right venue, but maybe this should get stacked onto bug 950073?

Comment 26

[u428464]

(In reply to Zack Weinberg (:zwol) from comment #25)
> Philipp, Guillaume: I don't know if this is the right venue, but maybe this
> should get stacked onto bug 950073?

Philipp is in charge for the backlog. ;)

Comment 27

[Oskar Eisemuth]

Looking at current code and the patch, I am not sure about the direction. 

Actually SetupDevModeFromSettings in widget/windows/nsDeviceContextSpecWin.cpp does already transforming Settings to DEVMODE, and already existing DEVMODE could be used to init the Win32 PageSetupDlg.
The opposite direction should be doable with SetPrintSettingsFromDevMode, so should the code reinvent the wheel? 
Not sure if it's desired that DEVMODE gets manipulated directly. 

Note: Actually most Win32 Applications provide some form of a extended or actually using their own window for print settings. 

http://people.redhat.com/~alexl/print_dialogs/Windows/windows.html

Comment 28

[Zack Weinberg (:zwol)]

(In reply to Oskar Eisemuth from comment #27)
> Looking at current code and the patch, I am not sure about the direction. 
> 
> Actually SetupDevModeFromSettings in
> widget/windows/nsDeviceContextSpecWin.cpp does already transforming Settings
> to DEVMODE, and already existing DEVMODE could be used to init the Win32
> PageSetupDlg.
> The opposite direction should be doable with SetPrintSettingsFromDevMode, so
> should the code reinvent the wheel? 
> Not sure if it's desired that DEVMODE gets manipulated directly. 

Regarding how the code works, do whatever makes most sense to you.

> Note: Actually most Win32 Applications provide some form of a extended or
> actually using their own window for print settings. 

So the immediate goal of this specific bug is to remove the "own window" that we have right now, because it is awkwardly implemented and needs to know things (like the complete list of configured printers) that Windows would rather we not ask for (see the dire warnings under "Remarks" at http://msdn.microsoft.com/en-us/library/windows/desktop/dd162692%28v=vs.85%29.aspx ).  I don't care what it gets replaced with, as long as it isn't XUL-based.  I personally think the best way forward would be to use the stock "PageSetupDlg" dialog box, perhaps with customizations, but if customizations get dropped for now and put back in bug 650957 that would be fine by me.

Comment 29

[Oskar Eisemuth]

The xul pagesetup for win32 can't select printers. Without doing a printout it's not possible to change the default printer under win32. (except about:config)

When firing print, you can select a printer in the opened native dialog and get to native settings of the printer driver.

At least my Print Page Setup dialog shows this on win32, and nothing else:
source/toolkit/components/printing/content/printPageSetup.xul

I think I have seen a one used for CUPS/Linux that isn't used when Firefox uses GTK.

Comment 30

[Zack Weinberg (:zwol)]

(In reply to Oskar Eisemuth from comment #29)
> The xul pagesetup for win32 can't select printers. Without doing a printout
> it's not possible to change the default printer under win32. (except
> about:config)
> 
> When firing print, you can select a printer in the opened native dialog and
> get to native settings of the printer driver.
> 
> At least my Print Page Setup dialog shows this on win32, and nothing else:
> source/toolkit/components/printing/content/printPageSetup.xul

Yes, these things are true.  What do you think should be done about them?

> I think I have seen a one used for CUPS/Linux that isn't used when Firefox
> uses GTK.

That code is slated to be removed in bug 629500.  It never worked very well.  I think it winds up using the same XUL-based dialogs.

Comment 31

[Oskar Eisemuth]

Win32 would gain a way to select a paper size and paper bin  (not sure if the later is stored correctly)

To have all old functionality, however the dialog needs to extended to get header and footer settings, zoom and background settings.

This needs more info from UI people, as said there is no standard way. Simple "page setup" in a search engine shows the problem quite well, a ton of individual settings window. The current UI design goes back to Bug 113917 I think.

Considering there a lot help pages describing the current layout, it may be better to keep the xul dialog for win32.

Comment 32

[Zack Weinberg (:zwol)]

"More info from UI people" is bug 650957.  As things only barely work right now, I think losing features temporarily would be okay.

Keeping the XUL dialog is really not an option.  There's so much code cleanup that only becomes possible if it goes away.

Comment 33

[Steffen Wilberg]

(In reply to Zack Weinberg (:zwol) from comment #28)
> So the immediate goal of this specific bug is to remove the "own window"
> that we have right now, because it is awkwardly implemented and needs to
> know things (like the complete list of configured printers) that Windows
> would rather we not ask for (see the dire warnings under "Remarks" at
> http://msdn.microsoft.com/en-us/library/windows/desktop/dd162692%28v=vs.
> 85%29.aspx ).
Is this still a huge problem after bug 701479 has been fixed?
How does Chrome handle this, which has print, print preview and page setup integrated into a single tab-modal sheet?

> I don't care what it gets replaced with, as long as it isn't
> XUL-based.
I'd get rid of the XUL page setup dialog as well, but replace it with a XUL print preview + page setup + print sheet...

> I personally think the best way forward would be to use the
> stock "PageSetupDlg" dialog box, perhaps with customizations,
The native page setup dialogs on Windows are a mess. I haven't seen any two Windows applications with the same page setup dialog, and that includes the apps bundled with Windows like Notepad, Wordpad and Paint.

> but if customizations get dropped for now and put back in bug 650957 that would be
> fine by me.
Like page scaling, print background colors, frame options, and customizing the page header and footer?
That would leave setting page orientation, and margin size. The former is available in Print Preview as well. We don't include paper size in the current xul page setup dialog. So there's basically nothing you would get from a native page setup dialog. That dialog is useless without the additions.

Comment 34

[Steffen Wilberg]

Attachment: screenshot of the XUL Page Setup dialog on Windows

Just so we're talking about the same thing, here's a screenshot of the XUL Page Setup dialog on Windows.