Bug 53586

Created attachment 65651 [details] [review]
Unifies the parsing of AA entries

Since PDF spec 1.7 (in opposite to 1.6) defines the 'AA' dictionary entries only for Screen and Widget annotations, I have modified the patch to at least parse the entries in a unified way.

Comment on attachment 65651 [details] [review]
Unifies the parsing of AA entries

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

Instead of returning the internal object, AA should be parsed and a list of action objects should be returned.

(In reply to comment #3)
Hej Carlos,

> Instead of returning the internal object, AA should be parsed and a list of
> action objects should be returned.
I tried to stay consistent with the Page::getActions() implementation, which does it the same way. Converting the dictionaries into action objects inside the Annot/Page implementation seems to be the wrong place.

Created attachment 65697 [details] [review]
Provide 'AA' dictionary entries as LinkAction objects

This new patch moves the parsing of the 'AA' dictionary from the Qt4 frontend to the Annot class, so that all frontends can benefit from it.
The getAdditionalAction() methods return LinkAction objects now.

Comment on attachment 65697 [details] [review]
Provide 'AA' dictionary entries as LinkAction objects

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

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.

Created attachment 65765 [details] [review]
Updated patch

Changed the '0' to NULL. Didn't integrated the other changes, they made the code more complex and difficult to read afaics.

Can somebody commit the patch, please? My account seems not to work anymore.

Created attachment 65817 [details] [review]
Unify parsing of additional actions entries

Integrate missing review comments

Comment on attachment 65817 [details] [review]
Unify parsing of additional actions entries

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

Patch looks good to me, if Albert and Pino have no objections, I'll commit it.

Pushed to git master, thanks!