Bug 105758

Summary:	[Patch] Add support for hide action in PDF Forms
Product:	poppler	Reporter:	Andre Heinecke <aheinecke>
Component:	general	Assignee:	poppler-bugs <poppler-bugs>
Status:	RESOLVED FIXED	QA Contact:
Severity:	enhancement
Priority:	medium	CC:	nate
Version:	unspecified
Hardware:	All
OS:	All
URL:	https://phabricator.kde.org/T8274
Whiteboard:
i915 platform:		i915 features:
Attachments:	Patch to add support for the hide action Patch to add support for the hide action - Updated 1

Description Andre Heinecke 2018-03-27 07:15:59 UTC

Created attachment 138372 [details] [review]
Patch to add support for the hide action

This adds the action "Hide" as specified in Adobe PFD Reference Version 1.7 Section 8.5.3 (page 665f.)

Hide Actions can be used to show or hide form fields.

It is part of a changeset for Okular to support such actions. See https://phabricator.kde.org/T8274 for details. This is why the qt5 binding is also extended as I used this for testing. The task also contains a test document.

The Okular task contains an autotest for this feature.

----

A Problem I had when implementing this patch was that according to Adobe's reference the "T" value can also be "An indirect reference to an annotation dictionary"  or "An array of such dictionaries".
I was not 100% sure what this meant for parsing or how the API should look like for that. With Adobe Acrobat DC I was not able to create an example containing such a thing.

So I've only implemented API for a Named target and commented in the Action's declaration. I think this is the best in such a situation as in the future the API can be extended, if necessary, without breaking something.

Comment 1 Albert Astals Cid 2018-04-07 09:23:59 UTC

Thanks and sorry for the delay on reviewing!

I was wondering if to future proof the Qt5 API and its users (the core API can be changed as much as we want), it would make sense to change
  QString targetName() const;
be
  QVector<QString> targets() const;

And for now we just return one value. This way when we implement cases b/c the application that uses poppler-qt5 will hopefully magically just work?

Also can you please change the Qt5::LinkHide constructor to be like the LinkOCGState one, i.e. so that it gets the private passed in, this way it's clear "normal users" should not create one, and if we need to pass more stuff to the constructor we don't break ABI or anything since it's still a pointer.

Comment 2 Andre Heinecke 2018-04-09 13:11:07 UTC

Created attachment 138702 [details] [review]
Patch to add support for the hide action - Updated 1

(In reply to Albert Astals Cid from comment #1)
> Thanks and sorry for the delay on reviewing!

No problem. Thanks for the review :-)

> I was wondering if to future proof the Qt5 API and its users (the core API
> can be changed as much as we want), it would make sense to change
>   QString targetName() const;
> be
>   QVector<QString> targets() const;
> 
> And for now we just return one value. This way when we implement cases b/c
> the application that uses poppler-qt5 will hopefully magically just work?

Good idea. I'll change the okular patch accordingly.

> Also can you please change the Qt5::LinkHide constructor to be like the
> LinkOCGState one, i.e. so that it gets the private passed in, this way it's
> clear "normal users" should not create one, and if we need to pass more
> stuff to the constructor we don't break ABI or anything since it's still a
> pointer.

Changed. I didn't understand why the OCGState used this pattern and oriented myself on the others ;-)

Comment 3 Albert Astals Cid 2018-04-16 16:11:19 UTC

Pushed (with some improvements)

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.