Skip to content

Conversation

j-t-1
Copy link
Contributor

@j-t-1 j-t-1 commented Jul 16, 2025

Sets entries in a page object’s additional-actions dictionary.

@j-t-1
Copy link
Contributor Author

j-t-1 commented Jul 16, 2025

Ideally want to be able to add open and close actions. To avoid a future deprecation, can we add this as _add_js for now?

@stefan6419846
Copy link
Collaborator

To avoid a future deprecation, can we add this as _add_js for now?

I do not understand what you are referring to. Could you please elaborate?

@j-t-1
Copy link
Contributor Author

j-t-1 commented Jul 16, 2025

I could have described better...

def add_js(self, javascript: str, /, *, open_action: bool = True) -> None:

Currently adds either open or close page action. To be able to add both will require modifying the function signature. Or maybe change it so that if given an open action keeps any close action and vice versa.

@stefan6419846
Copy link
Collaborator

We shouldn't have public API that is internal as it might break in the future.

Do you have an actual use case for implementing this? Or is this just because the implementation allows this? I have to be honest that I personally never actively used Javascript in PDF files.

@j-t-1
Copy link
Contributor Author

j-t-1 commented Jul 16, 2025

Let me know if you have a good way to modify this, or the functionality requirements needed.

Currently use the catalog OpenAction but the JavaScript at page-level is more flexible to only run the JavaScript once opening or closing a page.

Will also do a new PR for these tests, as could be in same function?

def test_add_js(pdf_file_writer):
pdf_file_writer.add_js("this.print({bUI:true,bSilent:false,bShrinkToFit:true});")
assert (
"/Names" in pdf_file_writer._root_object
), "add_js should add a name catalog in the root object."
assert (
"/JavaScript" in pdf_file_writer._root_object["/Names"]
), "add_js should add a JavaScript name tree under the name catalog."
def test_added_js(pdf_file_writer):
def get_javascript_name() -> Any:
assert "/Names" in pdf_file_writer._root_object
assert "/JavaScript" in pdf_file_writer._root_object["/Names"]
assert "/Names" in pdf_file_writer._root_object["/Names"]["/JavaScript"]
return pdf_file_writer._root_object["/Names"]["/JavaScript"]["/Names"][
-2
] # return -2 in order to get the latest javascript
pdf_file_writer.add_js("this.print({bUI:true,bSilent:false,bShrinkToFit:true});")
first_js = get_javascript_name()
pdf_file_writer.add_js("this.print({bUI:true,bSilent:false,bShrinkToFit:true});")
second_js = get_javascript_name()
assert (
first_js != second_js
), "add_js should add to the previous script in the catalog."

@stefan6419846
Copy link
Collaborator

Let me know if you have a good way to modify this, or the functionality requirements needed.

I am not completely sure if I can follow you here, but I would just let the user pass the NameObject and document the basics.

Will also do a new PR for these tests, as could be in same function?

Sorry, but I am rather confused what you want to say/ask with this.

@j-t-1
Copy link
Contributor Author

j-t-1 commented Jul 16, 2025

Modified. Is easier to pass a str.

Ignore the comment on tests.

@stefan6419846 can this PR be merged?

Copy link

codecov bot commented Jul 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.99%. Comparing base (0ceb8a4) to head (925f68e).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3382      +/-   ##
==========================================
+ Coverage   96.97%   96.99%   +0.01%     
==========================================
  Files          54       56       +2     
  Lines        9337     9394      +57     
  Branches     1711     1716       +5     
==========================================
+ Hits         9055     9112      +57     
  Misses        168      168              
  Partials      114      114              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

pypdf/_page.py Outdated
else:
self[NameObject("/Annots")] = value

def add_js(self, javascript: str, /, action_type: Literal["O", "C"] = "O") -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is always tied to an action, why use the generic add_js name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is at the page level, either opening or closing a page, so although generic is somewhat self-documenting with code completion on a page object.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but how does "self-documenting" and using a specific name correlate here? add_js sounds rather generic, but this only provides the option to execute JavaScript on opening the page (regardless of whether the PDF standard allows for more JavaScript-related functionality or not).

Copy link
Contributor Author

@j-t-1 j-t-1 Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A page object’s additional-actions dictionary only has two trigger events (when the page is opened or closed) but you are correct there are multiple action types (listed in Table 201 — Action types), and it would be better to have a generalized interface.

add_js is now add_action. Because there are many actions this PR will just add a JavaScript action, while providing future extensibility.

@stefan6419846 thanks for the reviews, the previous attempts were not using the correct terminology. Because this function will need to be extensible for future action types, very much welcome your feedback. I could also put this as a discussion.

>>> output.add_action("/C", "JavaScript", 'app.alert("This is page " + this.pageNum);')
# Example: This will display the page number when the page is closed.
Note that this will replace any existing open or close event on this page.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These limitations still sound like we should further refine the public API. add_action should add the new action in a defined manner, not overwrite existing ones - especially when the specification supports having both events defined on the page, but our API design only allows one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this needs to be robust; and extensible to the many action types. May be worth posting as a discussion to get ideas.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can of course open a discussion, but with JavaScript being ignored by many PDF readers and (at least from my experience) limited adoption, I guess there will not be much feedback.

This is one of the reasons why we usually ask to open issues first to discuss new APIs, although we might have missed this there as well and additional feedback would most likely have been sparse as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, feedback and demand may be limited. Although this is initially for JavaScript, it will be extensible and generic for other action types. The many and varied action types means this is a route to upgrading the interactive features that pypdf can provide.

@stefan6419846 stefan6419846 added needs-discussion The PR/issue needs more discussion before we can continue on-hold PR requests that need clarification before they can be merged.A comment must give details labels Sep 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion The PR/issue needs more discussion before we can continue on-hold PR requests that need clarification before they can be merged.A comment must give details
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants