[Poppler-bugs] [Bug 79936] Add support for printing to a Windows printer from pdftocairo

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=79936

--- Comment #32 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 103703
  --> https://bugs.freedesktop.org/attachment.cgi?id=103703
New patch version with many improvements

Review of attachment 103703:
-----------------------------------------------------------------

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/poppler-bugs/attachments/20140927/97b3b35e/attachment-0001.html>