[Poppler-bugs] [Bug 71213] Add new PDFWriter class for writing PDF with printing options

[bugzilla-daemon at freedesktop.org]

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

--- Comment #10 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 88957
  --> https://bugs.freedesktop.org/attachment.cgi?id=88957
Add PDFWriter

Review of attachment 88957:
-----------------------------------------------------------------

This looks great to me. There are several memory leaks and some other things
that could be improved a bit.

::: poppler/PDFWriter.cc
@@ +62,5 @@
> +  pages.push_back(page);
> +}
> +
> +struct PageScale {
> +  int paper;

This can be unsigned

@@ +104,5 @@
> +    std::swap(width, height);
> +
> +  // find smallest paper size that will fit this page
> +  std::priority_queue<PageScale> queue;
> +  for (int i = 0; i < (int)paperSizes.size(); i++) {

Why not using unsigned for i instead of casting?

@@ +186,5 @@
> +      mediaSize->y2 = paperSizes[0].width;
> +      margins->x1 = paperSizes[0].bottom;
> +      margins->x2 = mediaSize->y2 - paperSizes[0].top;
> +      margins->y1 = paperSizes[0].left;
> +      margins->y2 = mediaSize->x2 - paperSizes[0].right;

This block is repeated a lot, I wonder if we could add a helper function,
something like setSizeAndMargins(PDFRectangle *mediaSize, PDFRectangle
*margins, double with, double height, double top, double bottom, double left,
double right); I could be static inline

@@ +376,5 @@
> +void PDFWriter::writePageObject(int pageNum, int copy, PDFRectangle *mediaSize)
> +{
> +  PDFRectangle margins;
> +  Matrix ctm;
> +  Object ctmObj;

This looks unused.

@@ +432,5 @@
> +      obj2.free();
> +    }
> +  } else {
> +    writeObject(&contentsObj);
> +  }

contentsObj.free();

@@ +445,5 @@
> +	outputKey = gFalse;
> +	break;
> +      }
> +      excludeKey++;
> +    }

I think this could be easier to read if you use a helper function to check
whether the entry should be excluded:

if (excludePageEntry(keyName))
  continue;

@@ +453,5 @@
> +      writeObject(pageDict->getValNF(i, &obj1));
> +      outputStr->printf("\n");
> +      obj1.free();
> +    }
> +  }

pageObj.free();

@@ +490,5 @@
> +  outputStr->printf("/Subtype /Form\n");
> +
> +  pageDict->lookup("MediaBox", &obj);
> +  outputStr->printf("/BBox ");
> +  writeObject(&obj);

obj.free();

@@ +494,5 @@
> +  writeObject(&obj);
> +  outputStr->printf("\n");
> +
> +  pageDict->lookup("Resources", &obj);
> +  if (!obj.isNull()) {

This could be a single line:

if (!pageDict->lookup("Resources", &obj)->isNull())
...

@@ +498,5 @@
> +  if (!obj.isNull()) {
> +    outputStr->printf("/Resources ");
> +    writeObject(&obj);
> +    outputStr->printf("\n");
> +  }

obj.free();

@@ +501,5 @@
> +    outputStr->printf("\n");
> +  }
> +
> +  pageDict->lookup("Group", &obj);
> +  if (!obj.isNull()) {

This could be a single line too.

@@ +505,5 @@
> +  if (!obj.isNull()) {
> +    outputStr->printf("/Group ");
> +    writeObject(&obj);
> +    outputStr->printf("\n");
> +  }

obj.free();

@@ +552,5 @@
> +    Stream *str = obj.getStream();
> +    writeStream(str);
> +  } else {
> +    error(errSyntaxError, -1, "Weird page contents");
> +  }

obj.free();

@@ +586,5 @@
> +  outputStr->printf("endobj\n");
> +
> +  beginIndirectObject(&resourcesRef);
> +  outputStr->printf("<< /XObject <<\n");
> +  for (int i = 0; i < (int)xobjectRefs.size(); i++)

You could use unsigned for i here too.

@@ +592,5 @@
> +  outputStr->printf(">> >>\n");
> +  outputStr->printf("endobj\n");
> +
> +  GooString content;
> +  for (int i = 0; i < (int)xobjectRefs.size(); i++) {

Ditto.

@@ +632,5 @@
> +  outputStr->printf("/Kids [");
> +  if (collate) {
> +    for (int cp = 0; cp < copies; cp++) {
> +      if (reverse) {
> +        for (int pg = (int)pageRefs[0].size() - 1; pg >= 0; pg--)

Ditto.

@@ +635,5 @@
> +      if (reverse) {
> +        for (int pg = (int)pageRefs[0].size() - 1; pg >= 0; pg--)
> +          outputStr->printf(" %d %d R", pageRefs[cp][pg].num, pageRefs[cp][pg].gen);
> +      } else {
> +        for (int pg = 0; pg < (int)pageRefs[0].size(); pg++)

Ditto.

@@ +641,5 @@
> +      }
> +    }
> +  } else {
> +    if (reverse) {
> +      for (int pg = (int)pageRefs[0].size() - 1; pg >= 0; pg--) {

Ditto.

@@ +646,5 @@
> +        for (int cp = 0; cp < copies; cp++)
> +          outputStr->printf(" %d %d R", pageRefs[cp][pg].num, pageRefs[cp][pg].gen);
> +      }
> +    } else {
> +      for (int pg = 0; pg < (int)pageRefs[0].size(); pg++)

Ditto.

@@ +662,5 @@
> +  PDFRectangle mediaSize;
> +  int outputPageNum;
> +  int nupPageNum; // 0..numberUp-1
> +  int pageNum;
> +  Object pageObj;

This is unused.

@@ +677,5 @@
> +  // are not marked as they will be included in the XObjects created for
> +  // each page.
> +  outputPageNum = 1;
> +  nupPageNum = 0;
> +  for (int i = 0; i < (int)pages.size(); i++) {

Ditto.

@@ +686,5 @@
> +      Object pageObj;
> +      Ref *refPage = doc->getCatalog()->getPageRef(pageNum);
> +      doc->getXRef()->fetch(refPage->num, refPage->gen, &pageObj);
> +      Dict *pageDict = pageObj.getDict();
> +      doc->markPageObjects(pageDict, yRef, countRef, 0, numberUp > 1 ? gFalse : gTrue);

You don't need a variable for this, you can just pass the dict to the function:

doc->markPageObjects(pageObj.getDict(), ....);

and then

pageObj.free();

@@ +711,5 @@
> +    pageRefs.push_back(refs);
> +  }
> +  outputPageNum = 1;
> +  nupPageNum = 0;
> +  for (int i = 0; i < (int)pages.size(); i++) {

unsigned

@@ +756,5 @@
> +  delete trailerDict;
> +  delete yRef;
> +  delete countRef;
> +
> +  outputStr->close();

delete outputStr;

@@ +757,5 @@
> +  delete yRef;
> +  delete countRef;
> +
> +  outputStr->close();
> +}

I think we could make this function more generic by writing a stream, and then
add writeFile, that creates a FileOutpoutStream and calls the generic one
passing the stream. That way frontends can add a method to write to a stream
instead of creating a file in disk.

::: poppler/PDFWriter.h
@@ +27,5 @@
> +
> +  // order to layout multiple pages on a sheet.
> +  // LR_TB = Left to Right then Top to Bottom
> +  // LR_BT = Left to Right then Bottom to Top etc
> +  enum NumberUpOrder { LR_TB, LR_BT, RL_TB, RL_BT, TB_LR, TB_RL, BT_LR, BT_RL };

Even if it's a bit longer I think it's easier to read if you use full words
instead of abbreviations, and you don't need the comment explaining what the
abbreviations stand for.

::: poppler/Stream.cc
@@ +371,5 @@
> +void OutStream::format(const char *format, ...)
> +{
> +  va_list argptr;
> +  va_start (argptr, format);
> +  GooString *s = GooString::formatv(format, argptr);

Can we do something like this?

GooString s;
s.appendfv(format, argptr);

This way we avoid the alloc/dealloc of the GooString.

-- 
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/20131110/4aa2e133/attachment.html>