[Poppler-bugs] [Bug 103912] support tags, links, page labels, outline, metadata in cairo backend

[bugzilla-daemon at freedesktop.org]

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

--- Comment #10 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 135718
  --> https://bugs.freedesktop.org/attachment.cgi?id=135718
write document logical structure

Review of attachment 135718:
-----------------------------------------------------------------

LGTM, but I have a few comments

::: poppler/CairoOutputDev.cc
@@ +170,5 @@
>    actualText = NULL;
> +  logicalStruct = false;
> +  pdfPageNum = 0;
> +  cairoPageNum = 0;
> +  forwardLinkCount = 0;

Initialize firstPage to gFalse here too.

@@ +179,4 @@
>    align_stroke_coords = gFalse;
>    adjusted_stroke_width = gFalse;
>    xref = NULL;
> +  annotations = nullptr;

linkAnnot should be initialized to nullptr here too I guess.

@@ +201,5 @@
>      text->decRefCnt();
>    if (actualText)
> +    delete actualText;
> +  if (annotations)
> +    delete annotations;

Now that we have C++11 support I think it's a good idea to use unique_ptr in
newly added code at least. This could be a unique_ptr<Annots> and we don't need
this explicit delete.

@@ +227,4 @@
>    }
>  }
>  
> +GBool CairoOutputDev::isPDF()

this could be const

@@ +322,5 @@
> +  if (logicalStruct && isPDF()) {
> +    int numDests = doc->getCatalog()->numDestNameTree();
> +    for (int i = 0; i < numDests; i++) {
> +      GooString *name = doc->getCatalog()->getDestNameTreeName(i);
> +      LinkDest *dest = doc->getCatalog()->getDestNameTreeDest(i);

We could use unique_ptr here too...

@@ +325,5 @@
> +      GooString *name = doc->getCatalog()->getDestNameTreeName(i);
> +      LinkDest *dest = doc->getCatalog()->getDestNameTreeDest(i);
> +      if (dest->isPageRef()) {
> +	destsMap[dest->getPageRef()].insert(
> +	  std::make_pair(std::unique_ptr<GooString>(name->copy()), std::unique_ptr<LinkDest>(dest)));

then we just std::move() the dest here

@@ +327,5 @@
> +      if (dest->isPageRef()) {
> +	destsMap[dest->getPageRef()].insert(
> +	  std::make_pair(std::unique_ptr<GooString>(name->copy()), std::unique_ptr<LinkDest>(dest)));
> +      } else {
> +	delete dest;

And we don't need this.

@@ +336,5 @@
> +    for (int i = 0; i < numDests; i++) {
> +      const char *name = doc->getCatalog()->getDestsName(i);
> +      LinkDest *dest = doc->getCatalog()->getDestsDest(i);
> +      if (dest->isPageRef()) {
> +	destsMap[dest->getPageRef()].insert(

Note that we would need to get the ref here first if we use std::move(), using
a local var.

@@ +339,5 @@
> +      if (dest->isPageRef()) {
> +	destsMap[dest->getPageRef()].insert(
> +	  std::make_pair(std::unique_ptr<GooString>(new GooString(name)),
> +			 std::unique_ptr<LinkDest>(dest)));
> +      }

And same here, so we don't leak the dest in case it's not page ref.

@@ +366,5 @@
> +  cairoPageNum++;
> +
> +  if (logicalStruct && isPDF()) {
> +    if (annotations)
> +      delete annotations;

We wouldn't need this

@@ +368,5 @@
> +  if (logicalStruct && isPDF()) {
> +    if (annotations)
> +      delete annotations;
> +    Object obj = doc->getPage(pageNum)->getAnnotsObject(xref);
> +    annotations = new Annots(doc, pageNum, &obj);

and here we would just std::make_unique<Annots>. I don't know if I pushed the
patch that adds make_unique impl in the end, though.

@@ +3474,5 @@
> +  } else {
> +    // Link page ref is to a page that has not been emitted.
> +    // Create a named destination and add the name to destsMap so destination will
> +    // be create if/when the page is emitted.
> +    GooString *name = new GooString();

here I would use make_unique and then move below, but it's ok this way too.

::: poppler/CairoOutputDev.h
@@ +263,5 @@
>    void type3D1(GfxState *state, double wx, double wy,
>  	       double llx, double lly, double urx, double ury) override;
>  
> +  virtual void beginMarkedContent(GfxState *state, char *name, Dict *properties) override;
> +  virtual void endMarkedContent(GfxState *state) override;

virtual is not needed here, override is enough

::: poppler/OutputDev.h
@@ +148,4 @@
>    virtual void startPage(int pageNum, GfxState *state, XRef *xref) {}
>  
>    // End a page.
> +  virtual void endPage(GfxState *state) {}

Could we make state default to nullptr? This way we avoid having to modify all
the callers to pass nullptr when unused.

::: poppler/UTF.h
@@ +84,5 @@
> +
> +// Convert a PDF Text String to UTF-8
> +//   textStr    - PDF text string
> +//   returns UTF-8 string.
> +char *TextStringToUtf8(GooString *textStr);

Changes to UTF could probably be split to a different patch.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/poppler-bugs/attachments/20171212/4d2389ac/attachment.html>