Created attachment 65652 [details] [review]
Makes the additional actions available in Qt4 frontend

The new patch makes the additional actions available to the Qt4 frontend as well, however since the patch in #53586 has been adapted to allow additional actions only in screen and widget annotations, this patch adds the new class Poppler::WidgetAnnotation as well.

Comment on attachment 65652 [details] [review]
Makes the additional actions available in Qt4 frontend

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

::: qt4/src/poppler-annotation.cc
@@ +519,5 @@
> +
> +    Object actionObject;
> +    dict->lookup( key, &actionObject );
> +    ::LinkAction *linkAction = ::LinkAction::parseAction( &actionObject, parentDoc->doc->getCatalog()->getBaseURI() );
> +    actionObject.free();

I think the AA parsing should be common and done in the core, not in the frontends.

Created attachment 65698 [details] [review]
Provide additional actions in Qt4 frontend API

Adapt the patch to the new API provided by the patch of https://bugs.freedesktop.org/show_bug.cgi?id=53586

Comment on attachment 65698 [details] [review]
Provide additional actions in Qt4 frontend API

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

::: qt4/src/poppler-annotation.cc
@@ +518,5 @@
> +    ::LinkAction *linkAction = 0;
> +    if ( pdfAnnot->getType() == Annot::typeScreen )
> +        linkAction = static_cast<AnnotScreen*>( pdfAnnot )->getAdditionalAction( actionType );
> +    else if ( pdfAnnot->getType() == Annot::typeWidget )
> +        linkAction = static_cast<AnnotWidget*>( pdfAnnot )->getAdditionalAction( actionType );

Not sure if i prefer this else if to be just an else (because of the if at the beginning of the question). What do you think?

::: qt4/src/poppler-annotation.h
@@ +881,5 @@
>  
> +/**
> + * \short Widget annotation.
> + *
> + * The widget annotation represents a widget (form field) on a page.

How are apps going to match the Poppler::WidgetAnnotation with the Poppler::FormField so they can actually do something with the action?

(In reply to comment #4)
Hej Albert,

> ::: qt4/src/poppler-annotation.cc
> @@ +518,5 @@
> > +    ::LinkAction *linkAction = 0;
> > +    if ( pdfAnnot->getType() == Annot::typeScreen )
> > +        linkAction = static_cast<AnnotScreen*>( pdfAnnot )->getAdditionalAction( actionType );
> > +    else if ( pdfAnnot->getType() == Annot::typeWidget )
> > +        linkAction = static_cast<AnnotWidget*>( pdfAnnot )->getAdditionalAction( actionType );
> 
> Not sure if i prefer this else if to be just an else (because of the if at the
> beginning of the question). What do you think?
I have no preferences either... so shall I change it to a simple 'else'?

> > +/**
> > + * \short Widget annotation.
> > + *
> > + * The widget annotation represents a widget (form field) on a page.
> 
> How are apps going to match the Poppler::WidgetAnnotation with the
> Poppler::FormField so they can actually do something with the action?
I have not thought about that yet, since it would be out of scope of this patch and can be added later on if somebody is implementing support for that.

(In reply to comment #5)
> (In reply to comment #4)
> Hej Albert,
> 
> > ::: qt4/src/poppler-annotation.cc
> > @@ +518,5 @@
> > > +    ::LinkAction *linkAction = 0;
> > > +    if ( pdfAnnot->getType() == Annot::typeScreen )
> > > +        linkAction = static_cast<AnnotScreen*>( pdfAnnot )->getAdditionalAction( actionType );
> > > +    else if ( pdfAnnot->getType() == Annot::typeWidget )
> > > +        linkAction = static_cast<AnnotWidget*>( pdfAnnot )->getAdditionalAction( actionType );
> > 
> > Not sure if i prefer this else if to be just an else (because of the if at the
> > beginning of the question). What do you think?
> I have no preferences either... so shall I change it to a simple 'else'?

Yes, please

> 
> > > +/**
> > > + * \short Widget annotation.
> > > + *
> > > + * The widget annotation represents a widget (form field) on a page.
> > 
> > How are apps going to match the Poppler::WidgetAnnotation with the
> > Poppler::FormField so they can actually do something with the action?
> I have not thought about that yet, since it would be out of scope of this patch
> and can be added later on if somebody is implementing support for that.

I disagree with this, this is core of this patch.

You are adding an API that will give me the Link that has to be executed as FocusOutAction of a Form. If there is no way to link that action to the other way we return Forms, what's the point of it all?

Created attachment 66662 [details] [review]
Provide additional actions in Qt4 frontend API

Integrate Albert's feedback on previous patch. Use a simple 'else' statement now and extended the FormField API to provide access to the additional actions as well.

Now the big question is, if we can get the additionalAction from the Poppler::FormField do we really need that new Poppler::WidgetAnnotation class?

(In reply to comment #8)
> Now the big question is, if we can get the additionalAction from the
> Poppler::FormField do we really need that new Poppler::WidgetAnnotation class?
Yes we do, because poppler filters out all widget annotations that do not belong to a form (see poppler/Form.cc:1493), so there is would be way to access them otherwise. Certain PDF documents however use out-of-form widget annotations to trigger their additional actions (e.g the multimedia LaTeX packages uses it for auto-start of videos: https://bugs.kde.org/show_bug.cgi?id=300051).

Do you think it'd be possible to add a method in FormField that returns the widgetannotation instead of just the associated additional actions? And then add a operator== or something like that in the Annotation class (relying on the object id), so that they can be matched. What do you think?

If you do not have time for this, i think we can just remove the new API you added (sorry) and wait for someone to implement the "better" solution.

(In reply to comment #10)
Hej Albert,

> Do you think it'd be possible to add a method in FormField that returns the
> widgetannotation instead of just the associated additional actions?
I can't easily create a valid Poppler::WidgetAnnotation from inside the FormField class, because the annotation creation code is private to the Poppler::Annotation class.

Should the code for creating annotations be refactored to be used by the FormField as well? Or just using the previous patch that doesn't change the FormField code at all?

Ok, commited the initial part. If someone needs the second one it can be commited later.