ENH: Add PdfWriter.open_destination property

[pubpub-zz]

(fix JS executed twice extracted in another PR)
add capability to define an opening destination for the PDF

[codecov]

Codecov Report

Base: 94.25% // Head: 94.31% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (ccf9346) compared to base (6f73cb1).
Patch coverage: 96.42% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1431      +/-   ##
==========================================
+ Coverage   94.25%   94.31%   +0.06%     
==========================================
  Files          28       28              
  Lines        5096     5171      +75     
  Branches      973      980       +7     
==========================================
+ Hits         4803     4877      +74     
  Misses        177      177              
- Partials      116      117       +1     

Impacted Files	Coverage Δ
PyPDF2/_writer.py	91.53% <96.42%> (+0.22%)	⬆️
PyPDF2/generic/_base.py	100.00% <0.00%> (ø)
PyPDF2/generic/_utils.py	100.00% <0.00%> (ø)
PyPDF2/filters.py	97.29% <0.00%> (+0.01%)	⬆️
PyPDF2/_reader.py	89.61% <0.00%> (+0.03%)	⬆️
PyPDF2/_encryption.py	92.66% <0.00%> (+0.04%)	⬆️
PyPDF2/generic/_data_structures.py	95.11% <0.00%> (+0.04%)	⬆️
PyPDF2/_page.py	92.22% <0.00%> (+0.05%)	⬆️
PyPDF2/xmp.py	92.10% <0.00%> (+0.97%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[MartinThoma]

@pubpub-zz Is the new opening property related in any way to the issues you mentioned?

[MartinThoma]

In general I prefer PRs which deal with a single issue. This makes it way easier for me to merge a PR as I only have to check one thing at a time.

[pubpub-zz]

In general I prefer PRs which deal with a single issue. This makes it way easier for me to merge a PR as I only have to check one thing at a time.

I've introduced the opening at the same time as the change was removing access to AutoOpen field
I can split if you prefer

[MartinThoma]

I think splitting would be nice 🙏

I'm uncertain about the naming opening, but the JS-fixes are very clearly something I want to merge

[pubpub-zz]

I think splitting would be nice 🙏

I'm uncertain about the naming opening, but the JS-fixes are very clearly something I want to merge

any ideas another name for opening ?

[MartinThoma]

If you want to make OpenAction accessible, I would just do that and call it open_action. But there is more going on in the code.

According to TABLE 3.25 Entries in the catalog dictionary the OpenAction entry is either an array or a dictionary:

(Optional; PDF 1.1) A value specifying a destination to be displayed or
an action to be performed when the document is opened. The value is
either an array defining a destination (see Section 8.2.1, “Destinations”)
or an action dictionary representing an action (Section 8.5, “Actions”). If
this entry is absent, the document should be opened to the top of the
first page at the default magnification factor.

Why is the isinstance(oa, (str, bytes)) helpful?

I'm sorry if I ask dumb questions; I know way less about PDF than you 😅

[pubpub-zz]

If you want to make OpenAction accessible, I would just do that and call it open_action. But there is more going on in the code.

I dislike the idea of calling it open_action as actually the JS actions are now executed from other Names/JavaScript.
OpenAction will now only deal with destinations to define the opening of the pdf

According to TABLE 3.25 Entries in the catalog dictionary the OpenAction entry is either an array or a dictionary:

(Optional; PDF 1.1) A value specifying a destination to be displayed or
an action to be performed when the document is opened. The value is
either an array defining a destination (see Section 8.2.1, “Destinations”)
or an action dictionary representing an action (Section 8.5, “Actions”). If
this entry is absent, the document should be opened to the top of the
first page at the default magnification factor.

Why is the isinstance(oa, (str, bytes)) helpful?

You can define named destinations. Even if not in the PDF standard it works (I was also doubtfull about it😉). For the getter I've prefered to keep the name instead of the actual destination for users to not believe that something has been modified

I've extracted the fix on in another PR

[pubpub-zz]

Waiting for #1439 to be merged before updating this one

[pubpub-zz]

@MartinThoma
looks ready

[MartinThoma]

@MasterOdin What do you think about newly added opening property?

[MasterOdin]

Personally, I feel like calling it open_action would be better as opening doesn't feel right as a noun saying "PDF Writer opening". I could see calling it maybe opening_action as in "PDF Writer opening action" vs open_action.

I do think it's fine to add this property, even if it's not really one that's often used given it is in the PDF spec (see Table 28 in 7.7.2).

[pubpub-zz]

Personally, I feel like calling it open_action would be better as opening doesn't feel right as a noun saying "PDF Writer opening". I could see calling it maybe opening_action as in "PDF Writer opening action" vs open_action.

I do think it's fine to add this property, even if it's not really one that's often used given it is in the PDF spec (see Table 28 in 7.7.2).

The point is that this property is not dealing with any action/script. What about open_destination ?

[MasterOdin]

open_destination makes sense for what it does. Really just want the open or opening applied as an adjective to a word that describes what it does.

[pubpub-zz]

@MartinThoma
it should be good now for you

[MartinThoma]

Thank you @pubpub-zz for putting that much work again in this PR ❤️

And thank you @MasterOdin for the thorough review 🤗