DEP: Replace PdfWriter method add_js - #3979

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.13%. Comparing base (d26df36) to head (a459bc4).
⚠️ Report is 7 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3979      +/-   ##
==========================================
+ Coverage   98.11%   98.13%   +0.01%     
==========================================
  Files          59       59              
  Lines       11489    11513      +24     
  Branches     2150     2149       -1     
==========================================
+ Hits        11273    11298      +25     
  Misses        122      122              
+ Partials       94       93       -1     

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

This method would be better called open_action. add_action is used for the AA key entry in a page object. There is also an AA key entry in the catalog dictionary.

The value of OpenAction (2.0 specification):

A value specifying a destination that shall be displayed or an action that shall be performed when the document is opened. The value shall be either an array defining a destination or an action dictionary representing an action.

Thus this would be better, with the method having different paths on which type it is given (using isinstance):
open_action(self, action: Action | Destination)

@stefan6419846 as well as your code review, welcome guidance on the above changes.

This method would be better called open_action. add_action is used for the AA key entry in a page object. There is also an AA key entry in the catalog dictionary.

I am sorry, but I have some trouble following what you try to say here.

Thus this would be better, with the method having different paths on which type it is given (using isinstance):
open_action(self, action: Action | Destination)

open_action sounds like a property and not like a method to change/add something here. A Destination is no action, thus the name would be confusing.

How about add_open_action(self, action: Action)?

How about add_open_action(self, action: Action)?

So we would need add_close_action as well? This does not feel right.

How about add_open_action(self, action: Action)?

So we would need add_close_action as well? This does not feel right.

The will close action is not part of the open action; it is part of the additional-actions dictionary.

From the 2.0 specification:
WC dictionary: An ECMAScript action that shall be performed before closing a document. (The name WC stands for "will close.")

The add_action for PdfWriter will be for a future addition of the AA key, this matches with add_action for PageObject.

My confusion is still growing here: We have PageObject.add_action with corresponding triggers and the different Action class. This is what you previously implemented and which I assume would be the same here, except that it is not associated with a page, but the whole document?

If this is not the case, I guess we should go back to the design phase and properly document what the writer/whole document itself supports and how a corresponding API can look like, including where we could re-use existing code and where we need new one.

My confusion is still growing here: We have PageObject.add_action with corresponding triggers and the different Action class. This is what you previously implemented and which I assume would be the same here, except that it is not associated with a page, but the whole document?

No, this is associated with the OpenAction key of the Catalog dictionary. The analog of what has already been implemented in the PageObject is the AA key of the Catalog dictionary.

If this is not the case, I guess we should go back to the design phase and properly document what the writer/whole document itself supports and how a corresponding API can look like, including where we could re-use existing code and where we need new one.

We can use API we have from what is already implemented; add_action is the method corresponding to the AA key of all the objects. Becuase the Catalog dictionary can have actions in two keys (OpenAction and AA) this is the only special case.

@stefan6419846 further discussion about this below.

The optional A entry in the outline item dictionary and the dictionaries of some annotation types specifies an action performed when the annotation or outline item is activated; in PDF 1.2, a variety of other circumstances may trigger an action as well.

These "other circumstances" are by an additional-actions dictionary in these object types:

• Annotation
• Document catalog
• Form field
• Page object

In addition, the optional OpenAction entry in a document’s catalog dictionary may specify an action that shall be performed when the document is opened.

The page object additional-actions dictionary we added:

def add_action(self, trigger: PageTrigger, action: Action) -> None:
    return Action._create_new(self, trigger, action)

Annotations and the document catalog can have actions attached in two ways, for the document catalog these are the OpenAction and AA keys. This means that for the document catalog we need two methods to attach to PdfWriter: add_open_action and add_action, where add_action is reserved for future use as the not implemented (yet) method for the AA key.

After some more back-and-forth and research, I now start to further understand the topic.

A document itself can apparently have "regular" actions and one open action, which either is a action like we know from pages or a destination to jump to.

The current PR focuses on the open action itself. As I can only do one thing, the current implementation with always appending seems wrong, although add_js did something similar. If only one value is allowed, add_open_action would be misleading, although set_open_action violates Python preferring properties over getters and setters - thus we would need to implement a corresponding property open_action, which I am not sure about either. If we want to support destinations there, we should probably move _create_open_action out of the Action class.

Are my remarks correct or did I misrepresent/miss something?

Yes this is my understanding also.

I think the way you steered the design of the Action class was excellent. Keeping the _create_open_action in the Action class fits with the idea of adding an action by simply calling a method of the Action class.

Having add_open_action take either an action or a destination I think is the easiest solution, and although the naming seems to preclude a destination, it matches the specification and has that benefit.

As I can only do one thing, the current implementation with always appending seems wrong, although add_js did something similar.

Agree. I copied the add_js as wanted this to get the deprecation done cleanly.

I will make add_open_action accept a destination or an action.

OpenAction Value

A value specifying a destination that shall be displayed or an action that shall be performed when the document is opened. The value shall be either an array defining a destination or an action dictionary representing an action. If this entry is absent, the document shall be opened to the top of the first page at the default magnification factor.

PDF 2.0 specification.

@stefan6419846, action is used in a broader sense like in the specification, meaning the action is either do an actual action or to jump to a specific destination:
def add_open_action(self, action: Action | Destination) -> None

I think we should do this because users wanting to add an action will easily find this, and those knowing the specification and wanting to add a destination using OpenAction can do so also. It works for either use case.