-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
MAINT: Cleanup of annotations #1745
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1745 +/- ##
==========================================
- Coverage 94.15% 94.15% -0.01%
==========================================
Files 38 41 +3
Lines 7189 7284 +95
Branches 1427 1428 +1
==========================================
+ Hits 6769 6858 +89
- Misses 262 266 +4
- Partials 158 160 +2
☔ View full report in Codecov by Sentry. |
ac7d8bd
to
b9c0bf2
Compare
* Annotation module: pypdf/generic/_annotations.py ➔ pypdf/annotation.py * Create abstract base class AnnotationDictionary * Create annotation classes: Text, FreeText * DOC: Remove AnnotationBuilder from the docs, use AnnotationDictionary classes directly See #107 See #1741 The annotationBuilder design pattern discussion
b9c0bf2
to
8b3ced1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first comments
pypdf/annotations.py
Outdated
p1: Vertex, | ||
p2: Vertex, | ||
rect: Union[RectangleObject, Tuple[float, float, float, float]], | ||
text: str = "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we consider this parameter only through property same for title_bar ? You have not added it in polyline despite it exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very uncertain what we should do with this parameter. Depending on the context, it has different semantics:
Text that shall be displayed for the annotation or, if this type of
annotation does not display text, an alternate description of the
annotation’s contents in human-readable form. In either case, this text is
useful when extracting the document’s contents in support of
accessibility to users with disabilities or for other purposes (see 14.9.3,
“Alternate Descriptions”). See 12.5.6, “Annotation Types” for more
details on the meaning of this entry for each annotation type.
I don't want to make user have to know this.
Instead, I would prefer one parameter name that represents the semantics. That means although it might be called "text" for two different annotation types, we might have two different parameters for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there will be two:
text
for Free text annotations and markup annotations: Although the semantics is slightly different 🤔alternative_descriptions
for sound annotations (which we don't support so far
Hm. I'm really not sure. Maybe just text
everywhere? Or contents
?
Thank you pubpub-zz I'm dumb 🤦
dfec95b
to
6fcd6a5
Compare
@pubpub-zz I've added a couple of updates. Open questions I see are:
Am I missing anything? |
For (1) I tend to make everything keyword-only. There is no particular order that makes more sense than others + we have lots of parameters. It's just too easy to get something wrong. For (2) I'm really undecided. For (3): I tend rather to not do that. I vaguely remember matplotlib having similar behavior and that it felt rather weird to me as a user to have such global state when I call the module in different parts of the code. |
(3) would have been clearer with the annotation builder class. Then we could have assigned / set the default author to an instanciated AnnotationBuilder + make the current static methods non-static. |
Personnaly I would leave the "mandatory" parameters as positional (eg: rect+text for TextAnnotation, rect+destination for LinkAnnotation,...
I would propose all properties to be accepted within the constructor as keyed parameters.
Will review your PR. |
Co-authored-by: pubpub-zz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a couple minor typos I noticed (documentation)
Co-authored-by: Andrew <[email protected]>
## What's new ### New Features (ENH) - Accelerate image list keys generation (#2014) - Use `cryptography` for encryption/decryption as a fallback for PyCryptodome (#2000) - Extract LaTeX characters (#2016) - ASCIIHexDecode.decode now returns bytes instead of str (#1994) ### Bug Fixes (BUG) - Add RunLengthDecode filter (#2012) - Process /Separation ColorSpace (#2007) - Handle single element ColorSpace list (#2026) - Process lookup decoded as TextStringObjects (#2008) ### Robustness (ROB) - Cope with garbage collector during cloning (#1841) ### Maintenance (MAINT) - Cleanup of annotations (#1745) [Full Changelog](3.13.0...3.14.0)
The goal of this PR is to create a more intuitive interface for creating annotations. The AnnotationBuild gets deprecated in favor of several annotation classes, e.g.
pypdf/generic/_annotations.py
➔pypdf/annotations/
Closes #107
Hints for reviewers
See AnnotationBuilder design discussion