<html> <head> <base href="https://bugs.freedesktop.org/" /> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW --- - Add new PDFWriter class for writing PDF with printing options" href="https://bugs.freedesktop.org/show_bug.cgi?id=71213#c10">Comment # 10</a> on <a class="bz_bug_link bz_status_NEW " title="NEW --- - Add new PDFWriter class for writing PDF with printing options" href="https://bugs.freedesktop.org/show_bug.cgi?id=71213">bug 71213</a> from <a class="email" href="mailto:carlosgc@gnome.org" title="Carlos Garcia Campos <carlosgc@gnome.org>"> Carlos Garcia Campos</a> <pre>Comment on <a href="attachment.cgi?id=88957" name="attach_88957" title="Add PDFWriter">attachment 88957</a> <a href="attachment.cgi?id=88957&action=edit" title="Add PDFWriter">[details]</a> <a href='page.cgi?id=splinter.html&bug=71213&attachment=88957'>[review]</a> Add PDFWriter Review of <a href="attachment.cgi?id=88957" name="attach_88957" title="Add PDFWriter">attachment 88957</a> <a href="attachment.cgi?id=88957&action=edit" title="Add PDFWriter">[details]</a> <a href='page.cgi?id=splinter.html&bug=71213&attachment=88957'>[review]</a>: ----------------------------------------------------------------- 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.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>