Created attachment 100911 [details] Patch to add printing support to pdftocairo in Win32 Hello all! Printing a PDF unattendedly in Linux is easy. But in MS-Windows, it is not. I've been looking for a solution for ages and found nothing! Then, I realized that "pdftocairo" just renders a PDF to a Cairo contexts, and a Cairo context in Win32 can be a printer. I've written a patch (for 0.26.1) that adds that functionality to "pdftocairo". It still misses documentation changes, but I'm willing to add that too, if/when it is accepted. Basically, it adds two command line options: -prn: Instead of an image, it should render the output to a printer. The name of the printer will be "output-file", or the default printer if no name is specified. -prnsource <src>: The paper source of the printer. It corresponds to the DMBIN_* enumeration from Windows headers, so: 0=default, 4=manual, 7=auto. The default is "default". -- Rodrigo Rivas
Created attachment 103579 [details] [review] New version of the patch with new command line style Hi! From Adrian Johnson's comments in the mailing list, I've updated the patch. The printing code is exactly the same, but the command line options are different. Now: -print: Print to a system printer, instead of a file. -printer <name>: Use printer <name>. If not specified, it uses the default system printer. -printopt <options>: Configure the printer using <options>. It is a comma separated list of options, of the form <opt1>=<val1>[,<optN>=<valN>]*. Currently, there are only the following options in -printopt: * 'source': Choose the paper source. The value is one of: 'upper', 'onlyone', 'lower', 'middle', 'manual', 'envelope', 'envmanual', 'auto', 'tractor', 'smallfmt', 'largefmt', 'largecapacity', 'formsource' or an integer constant for printer specific sources. * 'duplex': Set the duplex mode. One of: 'simplex', 'horizontal', 'vertical'. Some random comments: * The linker needs the `-lwinspool` option. I've modified configure.ac to add it to every Win32 command. It should be harmless, but there may be other better options. * The '-printopt duplex' option somewhat overlaps with the '-dupex' option, but I'm not sure if it is wise to mix them ('-dupex' is a boolean, but -'-printopt duplex' has three values). * There are many more '-printopt' to be added, and some overlap with older options, such as '-duplex' and '-paper' and maybe others. I don't know exactly what should be done with them. * If we add more '-printopt', then it may be prudent to split the parsing functions into a separated file. That may complicate the Makefile, but make the code easier to work with. I have no preference about it... * There is a bug in the latest cairo release (1.12.16, maybe others too) that prevents the Win32 printer backend to work at all. I've sent a patch at [1] that seems to fix it. Thank you! -- Rodrigo Rivas [1]: https://bugs.freedesktop.org/show_bug.cgi?id=81709
Thanks for implementing the suggested print options. This should make it easier to later add new print options. > * The '-printopt duplex' option somewhat overlaps with the '-dupex' > option, but I'm not sure if it is wise to mix them ('-dupex' is a > boolean, but -'-printopt duplex' has three values). > * There are many more '-printopt' to be added, and some overlap with > older options, such as '-duplex' and '-paper' and maybe others. I > don't know exactly what should be done with them. You only need to add what you need. Other options can be added later. Where there is overlap with existing options, the existing options should be used to set the printing options. Users expect these options to work across all output formats. If an existing option does not support all the functionality we can make the -printopt version override the old one (maybe with a warning about duplicated options). > * If we add more '-printopt', then it may be prudent to split the > parsing functions into a separated file. That may complicate the > Makefile, but make the code easier to work with. I have no > preference about it... I would like the win32 code to be in a separate file (eg pdftocairowin32.cc). This keeps windows.h (which tends to pollute the namespace) out of pdftocairo.cc and makes the code easier to read. For example the changes to the beginDocument() function can be reduced to: if (print) { #ifdef CAIRO_HAS_WIN32_SURFACE win32BeginDocument(printer); #endif } else if (printing) { .... } The #ifdefs should test for CAIRO_HAS_WIN32_SURFACE instead of _WIN32 since building on windows does not mean that cairo has the win32 backend compiled in. I'm not sure how easy it is to make the build system compile pdftocairowin32.cc only when CAIRO_HAS_WIN32_SURFACE is defined. Probably the easiest thing is to just wrap pdftocairowin32.cc in a #ifdef CAIRO_HAS_WIN32_SURFACE. This is what we do in the cairo source. I recommend using git diff or at least turn on the -p option of diff so I can see what function each change is in. I don't like the use of an integer in the source option. Why would we need to support a number that is not one the #defines for DEVMODE? It would be nicer if parseSource used a table instead of multiple if statements for matching the source. eg see standardMedia in PSOutputDev.cc. When setting printer options in devmode you are setting the corresponding flag in dmFields. My interpretation of the DEVMODE documentation is that the devmode structure for a printer only supports setting the options indicated in the dmFields member. ie you should not be changing dmFields but instead check to see if an option is supported in dmFields before setting the option. The paper size in devmode should be set to the values in paperWidth and paperHeight. Looking at devmode we should be able to just convert the width/height to units of 0.1mm and put it in dmPaperWidth/dmPaperHeight. The paper size is also used to scale and translate the pdf page depending on the -expand/-noshrink/nocenter options. For this to work correctly the paper width and height returned by getOutputSize() should match the device context. ie the values obtain from GetDeviceCaps should be used. Note that the windows printing device context positions the origin at the top left of the printable area. This works well for the default pdftocairo settings where large pages are shrunk to fit the paper size. If -noshrink is used it would be better to use the full width and height of the paper. ie printing an A4 pdf to A4 paper with -noshrink should not scale the document even if it means any content outside the printable area will be lost. I would recommend doing the scaling from physical size to points on the hdc. This avoid the need make win32 specific changes to the cairo matrix transformation code. I suggest something like this after creating the hdc: XFORM xform; double xOffset, yOffset; double xPointsToDev = GetDeviceCaps (hdc, LOGPIXELSX)/72.0; double yPointsToDev = GetDeviceCaps (hdc, LOGPIXELSY)/72.0; if (noShrink) { outputPaperWidth = GetDeviceCaps (hdc, PHYSICALWIDTH)/xPointsToDev; outputPaperHeight = GetDeviceCaps (hdc, PHYSICALHEIGHT)/yPointsToDev; xOffset = GetDeviceCaps (hdc, PHYSICALOFFSETX); yOffset = GetDeviceCaps (hdc, PHYSICALOFFSETY); } else { paperWidth = GetDeviceCaps (hdc, HORZRES)/xPointsToDev; paperHeight = GetDeviceCaps (hdc, VERTZRES)/yPointsToDev; xOffset = 0; yOffset = 0; } SetGraphicsMode (hdc, GM_ADVANCED); xform.eM11 = xPointsToDev; xform.eM12 = 0; xform.eM21 = 0; xform.eM22 = yPointsToDev; xform.eDx = xOffset; xform.eDy = yOffset; SetWorldTransform (hdc, &xform); and use the outputPaperWidth/Height in getOutputSize(). EndDoc and DeleteDC should be called after cairo_surface_finish and cairo_surface_destroy. The pdftocairo.1 man page needs to be updated to include the new options. I suggest including -print, -printer, and -printopt in the OPTIONS section noting that they are windows only. Put the details of the -printopt supported options in a new section named WINDOWS PRINT OPTIONS. In DOCINFO put the actual pdf filename if available. If reading from standard input I suggest using the document name "pdftocairo <stdin>". Win32 calls that have both ansi and unicode versions should have an "A" or "W" to specify which one we are using. eg DeviceCapabilitiesA. The "Error: one of the output format ..." error message should only include "-print" if this option has been compiled in.
> xform.eDx = xOffset; > xform.eDy = yOffset; This should be xform.eDx = -xOffset; xform.eDy = -yOffset;
(In reply to comment #2) > You only need to add what you need. Other options can be added later. > > Where there is overlap with existing options, the existing options > should be used to set the printing options. Users expect these options > to work across all output formats. > > If an existing option does not support all the functionality we can > make the -printopt version override the old one (maybe with a warning > about duplicated options). > Ok. Actually, I only need the 'source' option. The 'duplex' was just for show, precisely because it overlapped. The override with warning is a nice idea. > I'm not sure how easy it is to make the build system compile > pdftocairowin32.cc only when CAIRO_HAS_WIN32_SURFACE is > defined. Probably the easiest thing is to just wrap pdftocairowin32.cc > in a #ifdef CAIRO_HAS_WIN32_SURFACE. This is what we do in the cairo > source. I think you would have to write a autoconf test. It is not difficult but probably not worth it. What we could do is to add "AC_SUBST(WIN32)" to configure.ac so that the -lwinspool and the pdftocairowin32.cc are only considered in Windows, even if the code is guarded by CAIRO_HAS_WIN32_SURFACE. > I don't like the use of an integer in the source option. Why would we > need to support a number that is not one the #defines for DEVMODE? The docs say: dmDefaultSource: [...] This member can be one of the following values, or it can be a device-specific value greater than or equal to DMBIN_USER. I think that it means that a printer driver is free to define its own values >= DMBIN_USER (256). I don't know how frequent they do that, but an advanced user may want to use source 258, and that does not have a name. Hey, maybe even printer manufacturers will use that, some day... Also, there are a few other fields with *_USER values. > It would be nicer if parseSource used a table instead of multiple if > statements for matching the source. eg see standardMedia in > PSOutputDev.cc. Yeah, no problem. > When setting printer options in devmode you are setting the > corresponding flag in dmFields. My interpretation of the DEVMODE > documentation is that the devmode structure for a printer only supports > setting the options indicated in the dmFields member. ie you should > not be changing dmFields but instead check to see if an option is > supported in dmFields before setting the option. I'm not sure. In windows it is frequent to have a bitfield in a structure for the fields you set, so that the unset ones get default values. See for example the STARTUPINFO struct. The issue with DEVMODE is that I'm initializing it with the default values from the printer driver, so if the flag is not set in the first place it is probably because the driver does not support it. Except for mutual incompatible flags, such as DM_PAPERSIZE and DM_PAPERLENGTH/DM_PAPERWIDTH. Anyway, we only add options explicitly set in the command line, so I don't see any reason to check. If the user wants it, just do it. The proper function for checking those would be 'DeviceCapabilities()'. > The paper size in devmode [... a lot of text and code ...] Ok, it looks fine. I'll check that and comment later. > EndDoc and DeleteDC should be called after cairo_surface_finish and > cairo_surface_destroy. That was my first idea, but the first time I did it, a few months ago, it crashed. Maybe now it is corrected, I will try again. > The pdftocairo.1 man page needs to be updated [...] Oh, the docs. Sure. > In DOCINFO put the actual pdf filename if available. If reading from > standard input I suggest using the document name "pdftocairo <stdin>". Nice. > Win32 calls that have both ansi and unicode versions should have an > "A" or "W" to specify which one we are using. eg DeviceCapabilitiesA. Well, the default one is the "A" unless UNICODE is defined. If it were a library, then of course, but it is a program: it is up to us to define UNICODE or not... Anyway, if you prefer, I'll add the A or W everywhere. > > The "Error: one of the output format ..." error message should only > include "-print" if this option has been compiled in. Yes, but the ifdefs would be ugly. Maybe it can be done as: fprintf(stderr, "Error: one of the output format options (-png, -jpeg, -ps, -eps, -pdf," #ifdef CAIRO_HAS_WIN32_SURFACE " -print," #endif " -svg) must be used.\n"); If you want, I'll prepare a more complete patch in a few days. Thank you!
(In reply to comment #2) > If an existing option does not support all the functionality we can > make the -printopt version override the old one (maybe with a warning > about duplicated options). Done! > I would like the win32 code to be in a separate file... Done! > The #ifdefs should test for CAIRO_HAS_WIN32_SURFACE instead of _WIN32... Done! > I'm not sure how easy it is to make the build system compile > pdftocairowin32.cc only when CAIRO_HAS_WIN32_SURFACE is > defined... I simply added `#include "pdftocairowin32.cc"` to the main source. Some people will frown upon it, but I think it fits nicely the one-source-file-but-not-quite character of pdftocairo. Moreover, this way we avoid the need to extern-declare all the globals and functions. > I recommend using git diff... Done! > It would be nicer if parseSource used a table... Done! > The paper size in devmode should be set to the values in paperWidth > and paperHeight... Done, I think. > The paper size is also used to scale and translate... the pdf page > depending on the -expand/-noshrink/nocenter options. For this to work > correctly the paper width and height returned by getOutputSize() > should match the device context. ie the values obtain from > GetDeviceCaps should be used. Sorry, I get lost with all these options and transformations. I tried to do what you suggested but I couldn't get it right. Sometimes the text is fine but the images go crazy, sometimes is the other way around. The only way that I got it to work for every PDF I tested is the one in the original patch: modifying the cairo matrix. I suspect that cairo may be playing with the HDC transformation too, but I didn't check it... > Note that the windows printing device context positions the origin at > the top left of the printable area. This works well for the default > pdftocairo settings where large pages are shrunk to fit the paper > size. If -noshrink is used it would be better to use the full width > and height of the paper. ie printing an A4 pdf to A4 paper with > -noshrink should not scale the document even if it means any content > outside the printable area will be lost. > > I would recommend doing the scaling from physical size to points on > the hdc. This avoid the need make win32 specific changes to the cairo > matrix transformation code. I suggest something like this after creating the > hdc: > > [code snip] > > and use the outputPaperWidth/Height in getOutputSize(). I tried that, too, but it didn't work. One problem is that `getOutputSize()` is called *before* CreateDC(), but all these computations require the HDC. So you have a kind of chicken-egg situation... > EndDoc and DeleteDC should be called after cairo_surface_finish and > cairo_surface_destroy. I tried moving it, but I fails badly. The problem is that after `cairo_surface_finish()` calling `cairo_win32_surface_get_dc()` will return NULL. I don't know if the HDC is really destroyed, or if it would leak. With a display HDC it would be bad enough, but with a printer HDC it is terrible, as the printer job may never finish. > The pdftocairo.1 man page needs to be updated... Done! I'm notably bad at writing man pages, so feel free to improve. > In DOCINFO put the actual pdf filename if available. If reading from > standard input I suggest using the document name "pdftocairo <stdin>". Done! > Win32 calls that have both ansi and unicode versions should have an > "A" or "W" to specify which one we are using. eg DeviceCapabilitiesA. I don't see the point, but it is easy, so, done! I also added a check to the return value of `StartDocA()` because it will return an error with the XPS driver if the user presses "Cancel" in the "Save File" dialog. > The "Error: one of the output format ..." error message should only > include "-print" if this option has been compiled in. That would be easy enough, but I feel it a bit unfair. -ps, -eps, -pdf and -svg options can also be disabled at compile time but they are unconditionally listed. (Just half-kidding). -- That all said, there is still room for improvement. For example, I found that there is that function ResetDC(), that can be used just before `StartPage()` to change the DEVMODE for each page. That is way beyond my original intents, but it is nice to know.
Created attachment 103702 [details] [review] New patch version with many improvements Added attachment with new version of the patch.
Created attachment 103703 [details] [review] New patch version with many improvements Please, ignore the previous attachment. This is the real thing.
I finally found some time to look at this bug again. I will attach some patches that fix a number of issues. Have a look and check that I have not broken anything. The major fix is the for the page size issue. I have fixed it to make all the various options (-paper, -origpapersizes, -noshrink, -nocenter) work. I had the same issue you mentioned with images not appearing. I have fixed it in cairo. The patch is: http://cgit.freedesktop.org/cairo/commit/?id=0aa43ed886c0f8468a21a470f2f024bd4d8a4513 I didn't like #including pdftocairo-win32.cc so I changed it to be a standalone .cc file. I also added the ability to print to a file by specifying an output file on the command line. The only remaining issue I have is with the numeric value for the source option. If we are going to support non standard sources we should use DeviceCapabilities() with DC_BINNAMES to obtain the names and allow the user to enter the text name of the source. Otherwise I suggest removing the numeric value capability. I also noticed in the documentation for DocumentProperties() at http://msdn.microsoft.com/en-us/library/windows/desktop/dd183576%28v=vs.85%29.aspx that we are not doing step 5 in the "To make changes to print settings that are local to an application, an application should follow these steps:" section. It seems to work without the extra call to DocumentProperties() but I am wondering if we should do exactly what the documentation says to do to change the print settings.
Created attachment 106263 [details] [review] make autogen.sh work with variables with spaces
Created attachment 106264 [details] [review] Don't use -fPIC on mingw
Created attachment 106265 [details] [review] fix compile error
Created attachment 106266 [details] [review] Update .gitignore
Created attachment 106267 [details] [review] Allow an output file for win32 printing to be specified
Created attachment 106268 [details] [review] make pdftocairo-win32.cc a standalone .cc file
Created attachment 106269 [details] [review] fix page size selection
Created attachment 106270 [details] [review] win32 hdc should be destroyed after the cairo surface
Created attachment 106271 [details] [review] ensure output_file is NULL when printing to win32
(In reply to comment #8) > I finally found some time to look at this bug again. I will attach some > patches that fix a number of issues. Have a look and check that I have not > broken anything. Great news! > The major fix is the for the page size issue. I have fixed it to make all > the various options (-paper, -origpapersizes, -noshrink, -nocenter) work. I > had the same issue you mentioned with images not appearing. I have fixed it > in cairo. The patch is: > > http://cgit.freedesktop.org/cairo/commit/ > ?id=0aa43ed886c0f8468a21a470f2f024bd4d8a4513 That proves that the Windows' backends need more love... BTW, I'm preparing a easier and more modular rewrite of my MinGW cross-compilation suite. I'll add it to github once it is ready for showing. > The only remaining issue I have is with the numeric value for the source > option. If we are going to support non standard sources we should use > DeviceCapabilities() with DC_BINNAMES to obtain the names and allow the user > to enter the text name of the source. Otherwise I suggest removing the > numeric value capability. As you wish. I don't have a real need for that. Maybe in the real world these custom BIN values are seldom used, I don't know. If you don't like the numbers, probably it is not worth doing anything until someone really needs it. > I also noticed in the documentation for DocumentProperties() at > http://msdn.microsoft.com/en-us/library/windows/desktop/dd183576%28v=vs. > 85%29.aspx > that we are not doing step 5 in the "To make changes to print settings that > are local to an application, an application should follow these steps:" > section. It seems to work without the extra call to DocumentProperties() but > I am wondering if we should do exactly what the documentation says to do to > change the print settings. Interesting... I really don't what is this last call for. Maybe it is for user selection validation, or something like that. But that could easily been done at `CreateDC()`... But now that we are into it, would it make sense to add a CLI option to show the printer property sheet? That is, to call `DocumentProperties()` with the `DM_IN_PROMPT` flag. I think that it could be done easily. Add the call from step 5 of the reference doc, but just add the `DM_IN_PROMPT` flag iff the `-prnproperties` (or whatever) option is used.
Created attachment 106366 [details] [review] improve option parsing Remove the source numeric value and fix a bug (option not incremented).
(In reply to comment #18) > > I also noticed in the documentation for DocumentProperties() at > > http://msdn.microsoft.com/en-us/library/windows/desktop/dd183576%28v=vs. > > 85%29.aspx > > that we are not doing step 5 in the "To make changes to print settings that > > are local to an application, an application should follow these steps:" > > section. It seems to work without the extra call to DocumentProperties() but > > I am wondering if we should do exactly what the documentation says to do to > > change the print settings. > > Interesting... I really don't what is this last call for. Maybe it is for > user selection validation, or something like that. But that could easily > been done at `CreateDC()`... I found another interesting example: http://support.microsoft.com/kb/167345/en-us I suggest we use this as a template for modifying the print settings. > But now that we are into it, would it make sense to add a CLI option to show > the printer property sheet? That is, to call `DocumentProperties()` with the > `DM_IN_PROMPT` flag. I think that it could be done easily. Add the call from > step 5 of the reference doc, but just add the `DM_IN_PROMPT` flag iff the > `-prnproperties` (or whatever) option is used. Interesting. But I would take it one step further and display the print dialog. Then the user can select the printer as well as change the print settings. eg -printdlg opens the print dialog. One problem would be the page size selection. On the command line not specifying the paper size means use whatever page size is in the PDF. This also supports printing PDFs with different page sizes (assuming the printer also has the different paper sizes available). Using the print dialog would require adding an extra option to the dialog to specify "select paper size using PDF page size". I found another issue with the code. I don't like the names of the duplex options "simplex", "horizontal", and "vertical". Most print dialogs I've seen use options like "one sided" or "off", "long edge" and "short edge". I suggest we use the names "off", "long", and "short".
In reply to comment #20) > (In reply to comment #18) > > Interesting... I really don't what is this last call for. Maybe it is for > > user selection validation, or something like that. But that could easily > > been done at `CreateDC()`... > > I found another interesting example: > > http://support.microsoft.com/kb/167345/en-us > > I suggest we use this as a template for modifying the print settings. From the linked article, in a comment to the 3rd call to DocumentProperties(): This gives the driver an opportunity to update any private portions of the DevMode structure. So it is probably wise to call it. Anyway, I don't see nothing else particularly interesting, except the use of `dmFields` to check for supported functionality that directly contradicts the documentation: NOTE: A flag in a DEVMODE's dmFields member is only an indication that a printer uses the associated structure member. [...] To determine the supported settings of a DEVMODE's field, applications should use DeviceCapabilities(). Although information from (the old parts of) MSDN is usually accurate, the sample code pieces are not top quality. I would not trust them too much. And I wouldn't bother using `DeviceCapabilities()`. If the user selects an unsuported feature of the printer, it will either fail or print without that feature. If we checked it, we would do more or less the same, so what's the point? Being a CLI utility, not a GUI, I think that the uses is expected to know what they are doing. > > But now that we are into it, would it make sense to add a CLI option to show > > the printer property sheet? That is, to call `DocumentProperties()` with the > > `DM_IN_PROMPT` flag. I think that it could be done easily. Add the call from > > step 5 of the reference doc, but just add the `DM_IN_PROMPT` flag iff the > > `-prnproperties` (or whatever) option is used. > > Interesting. But I would take it one step further and display the print > dialog. Then the user can select the printer as well as change the print > settings. eg -printdlg opens the print dialog. Yeah, "-printdlg" would replace "-printer <printername>" but "-printproperties" would be a different option. It does not make sense to use "-print -printdlg -printproperties" because the "Select Printer" dialog has a button to configure the selected printer. But it does make sense to use "-print -printer "My Printer" -printproperties" to show the "Properties" dialog of the given printer, doesn't it? > One problem would be the page size selection. On the command line not > specifying the paper size means use whatever page size is in the PDF. This > also supports printing PDFs with different page sizes (assuming the printer > also has the different paper sizes available). Using the print dialog would > require adding an extra option to the dialog to specify "select paper size > using PDF page size". AFAIK, adding an extra option to the properties dialog is next to impossible. This dialog is created by the printer driver, so it is different for each model of printer. There is no template to modify, as in `OpenFileDialog`. You could use a dummy paper size as a flag, but that would be a horrible hack and I have the impression that the printer driver will overwrite it anyway. It will be much easier to add an option to the CLI: something along the lines of `-prnignoresizefromdlg`. Or if you prefer the opposite `-prnusesizefromdlg`. Alternatively, you could use the size from the dialog only if there is a paper size in the command line. The option in the dlg would be initialized to the value from the CL. If no paper size in the CL, then the paper size from the dlg is ignored. Hackish, but I kind of like it. I've just checked what MS-Word 2013 does: it ignores the selection of the paper size in the Printer Properties Dialog and it uses whatever paper size is configured in the document. But then, it does not have scale or cropping options either...
Created attachment 106547 [details] [review] rename duplex values
Created attachment 106548 [details] [review] Add third call to DocumentProperties
I suggest we open a separate bug for the print dialog/print properties feature. I don't won't hold up the current CLI work from being committed while we think about how best to implement dialogs. I think the current patches are ready for final review. Carlos do you want to have a look at these patches?
(In reply to comment #23) > Created attachment 106548 [details] [review] [review] > Add third call to DocumentProperties You are not checking for errors in this third call to DocumentProperties. I think it would be wise to add an 'if/printf/exit(99)' for that, just in case.
Created attachment 106592 [details] [review] Fix error messages Add the missing error check for DocumentProperties and fix some existing error messages that were missing the \n.
Comment on attachment 106263 [details] [review] make autogen.sh work with variables with spaces Review of attachment 106263 [details] [review]: ----------------------------------------------------------------- Looks good.
Comment on attachment 106264 [details] [review] Don't use -fPIC on mingw Review of attachment 106264 [details] [review]: ----------------------------------------------------------------- Ok.
Comment on attachment 106265 [details] [review] fix compile error Review of attachment 106265 [details] [review]: ----------------------------------------------------------------- I think you can push these trivial fixes unreviewed, specially build fixes :-)
Comment on attachment 106266 [details] [review] Update .gitignore Review of attachment 106266 [details] [review]: ----------------------------------------------------------------- Sure
(In reply to comment #29) > Comment on attachment 106265 [details] [review] [review] > fix compile error > > Review of attachment 106265 [details] [review] [review]: > ----------------------------------------------------------------- > > I think you can push these trivial fixes unreviewed, specially build fixes > :-) Oh, I see now that this is a fix for the first patch, I'll review that one first.
Comment on attachment 103703 [details] [review] New patch version with many improvements Review of attachment 103703 [details] [review]: ----------------------------------------------------------------- ::: utils/pdftocairo-win32.cc @@ +1,3 @@ > +#include <cairo-win32.h> > + > +static void win32GetFitToPageTransform(cairo_matrix_t *m) This is a private method of this file, I don't think we need to use the win32 prefix for this. @@ +135,5 @@ > + } > + } > +} > + > +static void win32BeginDocument(GooString *outputFileName, double w, double h) I would could this something like createPrintingSurface so that we don't need to use the win32 prefix, this is called by beingDocument after all, not instead of beginDocument. And I would make it return the surface instead. @@ +137,5 @@ > +} > + > +static void win32BeginDocument(GooString *outputFileName, double w, double h) > +{ > + if (!print) And this could be checked by the caller @@ +140,5 @@ > +{ > + if (!print) > + return; > + > + if (printer.getCString()[0] == 0) I think if (printer.getLength() == 0) is easier to read. @@ +141,5 @@ > + if (!print) > + return; > + > + if (printer.getCString()[0] == 0) > + { Move this to the previous line, for consistency with the rest of the code. @@ +149,5 @@ > + GetDefaultPrinterA(devname, &szName); > + printer.Set(devname); > + gfree(devname); > + } > + char *cPrinter = printer.getCString(); This should probably be const char * @@ +154,5 @@ > + > + //Query the size of the DEVMODE struct > + LONG szProp = DocumentPropertiesA(NULL, NULL, cPrinter, NULL, NULL, 0); > + if (szProp < 0) > + { Move this to the previous line @@ +164,5 @@ > + devmode->dmSize = sizeof(DEVMODEA); > + devmode->dmSpecVersion = DM_SPECVERSION; > + //Load the current default configuration for the printer into devmode > + if (DocumentPropertiesA(NULL, NULL, cPrinter, devmode, NULL, DM_OUT_BUFFER) < 0) > + { Ditto. @@ +170,5 @@ > + exit(99); > + } > + fillCommonPrinterOptions(devmode, w, h); > + fillPrinterOptions(devmode); > + HDC hdc = CreateDCA(NULL, cPrinter, NULL, devmode); I have no idea what DCA stands for, but maybe all previous code could be moved to a helper function that initialize the printer and returns the HDC @@ +173,5 @@ > + fillPrinterOptions(devmode); > + HDC hdc = CreateDCA(NULL, cPrinter, NULL, devmode); > + gfree(devmode); > + if (!hdc) > + { Ditto. @@ +182,5 @@ > + DOCINFOA docinfo; > + memset(&docinfo, 0, sizeof(docinfo)); > + docinfo.cbSize = sizeof(docinfo); > + if (outputFileName->cmp("fd://0") == 0) > + docinfo.lpszDocName = outputFileName->getCString(); Fixed by Adrian in a follow up patch, it would be better to fix it here before instead of landing a patch that break the build. @@ +194,5 @@ > + > + surface = cairo_win32_printing_surface_create(hdc); > +} > + > +static void win32BeginPage(double w, double h) Ah, I see all are static, but included in the other file. I would rather add a header for the public methods, and include the header from pdtocairo instead. ::: utils/pdftocairo.cc @@ +77,4 @@ > static GBool ps = gFalse; > static GBool eps = gFalse; > static GBool pdf = gFalse; > +static GBool print = gFalse; I find this a bit confusing, because we also have printing variable. I would use something like printingToWinPrinter or something like that @@ +485,5 @@ > // shrink to fit > cairo_matrix_scale (m, scale, scale); > } > +#ifdef CAIRO_HAS_WIN32_SURFACE > + win32GetFitToPageTransform(m); I see now this was actually "public", but again this is not the getFitToPageTransform for win32, no? I would use a better name that express what the function actually does
Comment on attachment 106267 [details] [review] Allow an output file for win32 printing to be specified Review of attachment 106267 [details] [review]: ----------------------------------------------------------------- why GSView? :-P Patch LGTM ::: utils/pdftocairo-win32.cc @@ +188,5 @@ > + docinfo.lpszDocName = inputFileName->getCString(); > + } > + if (outputFileName) { > + docinfo.lpszOutput = outputFileName->getCString(); > + } Why have you added braces everywhere? ::: utils/pdftocairo.cc @@ +741,5 @@ > } > > + if (print) { > + return NULL; // No output file means print to printer > + } Ditto.
Comment on attachment 106268 [details] [review] make pdftocairo-win32.cc a standalone .cc file Review of attachment 106268 [details] [review]: ----------------------------------------------------------------- Indeed, but I prefer if this is squashed in the first patch instead of landing a follow up commit
(In reply to comment #34) > Comment on attachment 106268 [details] [review] [review] > make pdftocairo-win32.cc a standalone .cc file > > Review of attachment 106268 [details] [review] [review]: > ----------------------------------------------------------------- > > Indeed, but I prefer if this is squashed in the first patch instead of > landing a follow up commit It would be neater but the first patch is by Rodrigo and the followup by myself. We should maintain the correct authorship in each commit.
Comment on attachment 106269 [details] [review] fix page size selection Review of attachment 106269 [details] [review]: ----------------------------------------------------------------- ::: utils/pdftocairo-win32.cc @@ +227,5 @@ > + *h = GetDeviceCaps (hdc, PHYSICALHEIGHT)*72.0/y_dpi; > + } else { > + *w = GetDeviceCaps (hdc, HORZRES)*72.0/x_dpi; > + *h = GetDeviceCaps (hdc, VERTRES)*72.0/y_dpi; > + } Could we move all this to a separate function that returns the width/height to be called before beginPage only for win32? That way we don't need to change beginPage in pdftocairo.cc
Comment on attachment 106270 [details] [review] win32 hdc should be destroyed after the cairo surface Review of attachment 106270 [details] [review]: ----------------------------------------------------------------- This could probably be squashed in a previous commit too.
Comment on attachment 106271 [details] [review] ensure output_file is NULL when printing to win32 Review of attachment 106271 [details] [review]: ----------------------------------------------------------------- And this one as well
(In reply to comment #36) > Comment on attachment 106269 [details] [review] [review] > fix page size selection > > Review of attachment 106269 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: utils/pdftocairo-win32.cc > @@ +227,5 @@ > > + *h = GetDeviceCaps (hdc, PHYSICALHEIGHT)*72.0/y_dpi; > > + } else { > > + *w = GetDeviceCaps (hdc, HORZRES)*72.0/x_dpi; > > + *h = GetDeviceCaps (hdc, VERTRES)*72.0/y_dpi; > > + } > > Could we move all this to a separate function that returns the width/height > to be called before beginPage only for win32? That way we don't need to > change beginPage in pdftocairo.cc We don't know the actual width/height until the new size is selected. In win32BeginPage((), fillPagePrinterOptions() sets the requested page size. Then ResetDCA() resets the device context to the requested size. Then we get the actual paper size from the device context which may be different to the requested size. The actual size is returned to pdftocairo.cc to ensure the transformations in getFitToPageTransform() are correct.
Created attachment 106997 [details] [review] Add support for printing to a Windows printer from pdftocairo I squashed the compile fix into Rodrigo's patch since it is an obvious typo.
Created attachment 106998 [details] [review] Allow an output file for win32 printing to be specified
Created attachment 106999 [details] [review] make pdftocairo-win32.cc a standalone .cc file
Created attachment 107000 [details] [review] fix a number of bugs in win32 printing Squash all the various fixes into one commit.
These updated patches fix all the review comments except comment 37 which as I explain in comment 39 I don't think it can be easily changed. I've rolled the one line compile fix into Rodrigo's commit since it is an obvious typo that slipped through. I've kept the rest of my changes as separate commits to preserve the authorship of the commits. All the bug fix commits have been squashed into one commit.
Created attachment 107345 [details] [review] setupdlg option to show printer properties dialog This adds a -setupdlg option that displays the printer properties dialog before printing.
Created attachment 107356 [details] [review] Add -printdlg option to show print dialog This adds a -printdlg output mode for windows printing that shows the print dialog. The same additional controls that are in the evince print dialog (scale, center, and orig page size) have been added to the print dialog so that all print options can be modified without using the command line options.
Rodrigo, do you plan to update your patch after my review?
(In reply to Carlos Garcia Campos from comment #47) > Rodrigo, do you plan to update your patch after my review? Emmm. Frankly speaking, I'm not sure what I'm supposed to do. Don't get me wrong, I'm more than willing to do whatever is required to get this commited, but there are now 9 patches in this page, some of those have already been commited to "master". Should I merge all the remaining patches into one, absorbing the original work by Adrian? That would mess the authorship of each piece of code. Not that I care too much about these things, but it's better to err on excess. Or should I just update my original patch? Note that my version has already been modified by Adrian to fix some issues, I'm not sure to what extent. And changing that patch will probably break the other ones... Maybe should I fix all the patches without merging? Please, do not take this as unwillingness to do the work. It's just I don't know how things are done around here.
Ok, no problem, let's push the patches already reworked by Adrian then. Thank you both.
Pushed.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.