Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for callout arrow in FreeTextAnnotation #19380

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

edoardocavazza
Copy link

This PR adds callout information to the FreeTextAnnotation model.
This data are required to extend the editor support for this kind of annotations.

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Thanks for the patch, however given that this adds basically unused code it's not entirely clear (at least) to me that we should add this.[1]

Also, even if we'd accept this feature it'd definitely require unit-tests.


[1] The fact that we added lineEndings previously always felt like a mistake, since we've never used that code.

* Set the line ending; should only be used with FreeText annotations.
* @param {Name} lineEnding - The line ending name.
*/
setLineEnding(lineEnding) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't add an almost duplicate of an existing method, since both cases should use rather one common helper to reduce code-size.

Comment on lines 3924 to 3925
this.data.calloutLine = dict.getArray("CL");
this.data.rectDifference = dict.getArray("RD");
Copy link
Collaborator

Choose a reason for hiding this comment

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

In a corrupt PDF document those fields can contain anything and obviously need to be properly parsed and validated, rather than included as-is since that could lead to failures.

@edoardocavazza
Copy link
Author

Thanks @Snuffleupagus for the review! I'll try to reply to your concerns before going back to the code.

however given that this adds basically unused code it's not entirely clear (at least) to me that we should add this

At the moment, the core annotation editor of PDF.js allows to edit FreeText annotations, but this results in the loss of the arrow callout. This PR does not directly fix the editing, but opens the possibility for the core and custom editors to improve their handling of FreeText annotations.

I also think is valuable that annotation models are as accurate as possible, since they can be accessed and inspected using page.getAnnotations(). I would like to open a bunch of PR like this that adds missing relevant data to the annotation.


I haven't looked into PDF.js unit testing yet, but I'd be happy to extend the PR if there's a chance it'll get accepted.

@edoardocavazza edoardocavazza force-pushed the free-text-annot-line-ending branch from c2a52bc to e2bb200 Compare January 27, 2025 08:04
@edoardocavazza
Copy link
Author

Hello @Snuffleupagus, I added some validation when reading values from the dictionary and added a couple of unit tests.

@Snuffleupagus
Copy link
Collaborator

I added some validation when reading values from the dictionary and added a couple of unit tests.

  • That unfortunately doesn't change the fact that this is basically "dead" code from our perspective (as previously mentioned).
    Since every feature increases maintenance burden, and code-size, I think that we'd need agreement between maintainers before adding a bunch of "unused" features in this code. /cc @calixteman, @timvandermeij Any opinions about this?

  • The unit-tests don't seem to check the actual lineEnding-parsing, compare e.g. with these existing tests.
    Any tests using saveNewAnnotations/printNewAnnotations should be in addition to basic ones (rather than replacements).

@edoardocavazza
Copy link
Author

If it can help with the judgment, these are the other two PRs I would like to open (they lack review and unit tests, but I'll think about it after solving this one):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants