[Poppler-bugs] [Bug 53586] [PATCH] Unify parsing of 'AA' dictionary entries in screen and widget annotations

[bugzilla-daemon at freedesktop.org]

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

--- Comment #6 from Carlos Garcia Campos <carlosgc at gnome.org> 2012-08-18 16:58:42 UTC ---
Comment on attachment 65697
  --> https://bugs.freedesktop.org/attachment.cgi?id=65697
Provide 'AA' dictionary entries as LinkAction objects

Review of attachment 65697:
 --> (https://bugs.freedesktop.org/page.cgi?id=splinter.html&bug=53586&attachment=65697)
-----------------------------------------------------------------

Patch looks good to me, I've just a few minor comments.

::: poppler/Annot.cc
@@ +198,5 @@
> +static LinkAction* getAdditionalAction(Annot::AdditionalActionsType type, Object *additionalActions, PDFDoc *doc) {
> +
> +  Object additionalActionsObject;
> +
> +  additionalActions->fetch(doc->getXRef(), &additionalActionsObject);

in poppler we usually do something like:

if (additionalActions->fetch(doc->getXRef(),
&additionalActionsObject)->isDict()) {
 .....
}
additionalActionsObject.free();

@@ +202,5 @@
> +  additionalActions->fetch(doc->getXRef(), &additionalActionsObject);
> +
> +  if (!additionalActionsObject.isDict()) {
> +    additionalActionsObject.free();
> +    return 0;

Use NULL instead of 0

@@ +216,5 @@
> +                     type == Annot::actionFocusOut ?      "BI" :
> +                     type == Annot::actionPageOpening ?   "PO" :
> +                     type == Annot::actionPageClosing ?   "PC" :
> +                     type == Annot::actionPageVisible ?   "PV" :
> +                     type == Annot::actionPageInvisible ? "PI" : 0);

Ditto.

@@ +220,5 @@
> +                     type == Annot::actionPageInvisible ? "PI" : 0);
> +
> +  Object actionObject;
> +  dict->lookup(key, &actionObject);
> +  LinkAction *linkAction = LinkAction::parseAction(&actionObject, doc->getCatalog()->getBaseURI());

LinkAction::parseAction() expects a dict, you could do something like this:

LinkAction *linkAction = NULL;
if (additionalActionsObject.dictLookup(key, &actionObject)->isDict()) {
  linkAction = LinkAction::parseAction(&actionObject,
doc->getCatalog()->getBaseURI());
}

and you don't need the Dict variable.

@@ +5171,5 @@
> +
> +LinkAction* AnnotScreen::getAdditionalAction(AdditionalActionsType type)
> +{
> +  if (type == actionFocusIn || type == actionFocusOut) // not defined for screen annotation
> +    return 0;

Use NULL instead of 0.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.