Bug 103912

Created attachment 135717 [details] [review]
Fix some bugs in StructTreeRoot

Created attachment 135718 [details] [review]
write document logical structure

Created attachment 135719 [details] [review]
copy pdf page labels

Created attachment 135720 [details] [review]
pdftocairo: add -fl, -ll options for specify page range using page labels

Created attachment 135721 [details] [review]
only output link annotations when setInteractive(true)

Created attachment 135722 [details] [review]
thumbnails

Created attachment 135723 [details] [review]
outline

Created attachment 135724 [details] [review]
metadata

Comment on attachment 135717 [details] [review]
Fix some bugs in StructTreeRoot

Review of attachment 135717 [details] [review]:
-----------------------------------------------------------------

LGTM

::: poppler/Object.h
@@ +90,5 @@
> +  bool operator() (const Ref& lhs, const Ref& rhs) const {
> +    if (lhs.num != rhs.num)
> +      return lhs.num < rhs.num;
> +    else
> +      return lhs.gen < rhs.gen;

We don't need the else after a return

Comment on attachment 135718 [details] [review]
write document logical structure

Review of attachment 135718 [details] [review]:
-----------------------------------------------------------------

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.

Comment on attachment 135719 [details] [review]
copy pdf page labels

Review of attachment 135719 [details] [review]:
-----------------------------------------------------------------

OK!

Comment on attachment 135720 [details] [review]
pdftocairo: add -fl, -ll options for specify page range using page labels

Review of attachment 135720 [details] [review]:
-----------------------------------------------------------------

OK

Comment on attachment 135721 [details] [review]
only output link annotations when setInteractive(true)

Review of attachment 135721 [details] [review]:
-----------------------------------------------------------------

OK

Comment on attachment 135722 [details] [review]
thumbnails

Review of attachment 135722 [details] [review]:
-----------------------------------------------------------------

OK

*** Bug 104203 has been marked as a duplicate of this bug. ***

(In reply to Carlos Garcia Campos from comment #9)
> Comment on attachment 135717 [details] [review] [review]
> Fix some bugs in StructTreeRoot
> 
> Review of attachment 135717 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> LGTM
> 
> ::: poppler/Object.h
> @@ +90,5 @@
> > +  bool operator() (const Ref& lhs, const Ref& rhs) const {
> > +    if (lhs.num != rhs.num)
> > +      return lhs.num < rhs.num;
> > +    else
> > +      return lhs.gen < rhs.gen;
> 
> We don't need the else after a return

I've pushed this patch with the above change as it fixes a couple of other bug reports. I'll get onto the other patches soon.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/poppler/poppler/issues/207.