I'm not totally convinced, but shouldn't we also add

Annot::actionPageOpening
Annot::actionPageClosing
Annot::actionPageVisible
Annot::actionPageInvisible

?

I didn't add them because we don't need them in Okular. The difference for those actions is that we don't need the link between formfield <-> annotation, so they can still be handled without problems through the Page / Annotation API.

But I don't think it would hurt. Should I update the patch?

I think we should really go for

     Link* additionalAction(AdditionalActionType type) const;
     Link* additionalAction(Annotation::AdditionalActionType type) const;

It most probably makes the implementation code simpler too. What do you think?

I thought that combining both in a single enumeration might be a good idea because I saw:

 * The widget annotation represents a widget (form field) on a page.
 *
 * \note This class is just provided for consistency of the annotation API,
 *       use the FormField classes to get all the form-related information.

So I thought that it makes sense API wise to have everything related to FormFields declared in poppler-form / the FormField class. Currently there is also no dependency from poppler-form.h to poppler-annotation.h.

From an implementation standpoint, yes it would make the type differentiation in popler-form.cc FormField::additionalAction simpler by splitting both branches into two functions.
But as an API user I find it simpler to only have to care about a single AdditionalAction enum.

At least in Okular reducing both enum's to a single AdditionalActionType enum helps so that we don't need duplicated API to carry / access them there. I found it more elegant to directly "abstract" the difference away in the poppler-qt api.

(In reply to Andre Heinecke from comment #4)
> I thought that combining both in a single enumeration might be a good idea
> because I saw:
> 
>  * The widget annotation represents a widget (form field) on a page.
>  *
>  * \note This class is just provided for consistency of the annotation API,
>  *       use the FormField classes to get all the form-related information.
> 
> So I thought that it makes sense API wise to have everything related to
> FormFields declared in poppler-form / the FormField class. Currently there
> is also no dependency from poppler-form.h to poppler-annotation.h.
> 
> From an implementation standpoint, yes it would make the type
> differentiation in popler-form.cc FormField::additionalAction simpler by
> splitting both branches into two functions.
> But as an API user I find it simpler to only have to care about a single
> AdditionalAction enum.
> 
> At least in Okular reducing both enum's to a single AdditionalActionType
> enum helps so that we don't need duplicated API to carry / access them
> there. I found it more elegant to directly "abstract" the difference away in
> the poppler-qt api.

I don't agree it's a good abstraction, you're duplicating API, by having "the same enum values" in two different places, which for people that are "not you or me" makes you wonder why that happens or if one is the new and the other the old, or what.

I think adding a new function accepting the annotation enums, is exactly fullfiling that "this class is just provided for consistency" comment.

You know you want the MousePressed action, but since there is that comment you go to the form field classes, and there you go, there's exactly the function accepting what you want.

Why is it any different for Okular?

(In reply to Albert Astals Cid from comment #5)
> I don't agree it's a good abstraction, you're duplicating API, by having
> "the same enum values" in two different places, which for people that are
> "not you or me" makes you wonder why that happens or if one is the new and
> the other the old, or what.

Ok.
 
> I think adding a new function accepting the annotation enums, is exactly
> fullfiling that "this class is just provided for consistency" comment.
> 
> You know you want the MousePressed action, but since there is that comment
> you go to the form field classes, and there you go, there's exactly the
> function accepting what you want.
> 
> Why is it any different for Okular?

In Okular I also didn't want to add a new additionalAction function / Enum to Okular::FormField with an additional underlying QHash for the new actions but found it more elegant to reuse the old structure and just extend it for the new actions.
But yes, it does not make much difference and you are right that the API is more direct if we split it.

I'll update the patches accordingly.

Created attachment 139422 [details] [review]
Support additional widget actions in PDF Forms - Updated 1

Updated to use a separate function for the Annotation additional actions.

I wonder if we should forward declare the Annotation::AdditionalActionType enum to avoid including poppler-annotation.h in poppler-form.h.
Which type should such a declaration use?

Pushed