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

ENH: Add add_free_text_annotation to PdfWriter #981

Closed
wants to merge 8 commits into from

Conversation

MartinThoma
Copy link
Member

Full credit to the GitHub user snakemicro:
#107 (comment)

Co-authored-by: snakemicro

Full credit to the GitHub user snakemicro:
#107 (comment)

Co-authored-by: snakemicro
@codecov
Copy link

codecov bot commented Jun 12, 2022

Codecov Report

Merging #981 (0370aea) into main (dd2d69a) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #981      +/-   ##
==========================================
+ Coverage   91.97%   92.01%   +0.03%     
==========================================
  Files          24       24              
  Lines        4661     4683      +22     
  Branches      962      968       +6     
==========================================
+ Hits         4287     4309      +22     
  Misses        229      229              
  Partials      145      145              
Impacted Files Coverage Δ
PyPDF2/_utils.py 98.77% <100.00%> (+0.01%) ⬆️
PyPDF2/_writer.py 89.13% <100.00%> (+0.40%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd2d69a...0370aea. Read the comment docs.

@MartinThoma MartinThoma marked this pull request as ready for review June 12, 2022 14:12
@MartinThoma MartinThoma added is-feature A feature request PdfWriter The PdfWriter component is affected labels Jun 26, 2022
@MartinThoma
Copy link
Member Author

I'm uncertain if this function should be split into two:

  1. add_box
  2. add_text_annotation

Also, I was wondering about the italic/bold/font/font size parameters. Maybe that should also be a single FontStyle object?

I might also need to read up on "Rich Text Strings"

@MartinThoma
Copy link
Member Author

Note for myself "DS" is "default string"

@MartinThoma
Copy link
Member Author

MartinThoma commented Jul 16, 2022

I don't think splitting makes sense as this is one Annotation entry.

I thought about moving all of the style parameters to their own class, e.g.:

class TextStyle:
    """Specify the style in which a text is shown."""

    # This class would be a good example for a dataclass. However, I don't
    # want to add additional dependencies and dataclasses were introduced
    # with PEP 557 for Python 3.7+
    # Maybe a named tuple would be good?
    def __init__(
        self,
        bold: bool = False,
        italic: bool = False,
        font_size: str = "14pt",
        font_color: str = "ff0000",
        font: str = "Helvetica",
    ):
        self.bold = bold
        self.italic = italic
        self.font = font
        self.font_color = font_color
        self.font_size = font_size

    @property
    def css2_font(self):
        """See TABLE 8.74 CSS2 style attributes used in rich text strings"""
        font_str = "font: "
        if self.bold is True:
            font_str = font_str + "bold "
        if self.italic is True:
            font_str = font_str + "italic "
        font_str = f"{font_str}{self.font} {self.font_size}"
        font_str = f"{font_str};text-align:left;color:#{self.font_color}"
        return font_str

This would reduce the number of parameters for add_free_text_annotation. Adding a BorderStyle class could further remove one parameter from it.

I'm currently wondering if that makes it simpler for the user or more complicated.

Although using the TextStyle class would make the code look a bit cleaner, I feel like this makes things more complicated.

I will check how function signatures of other libraries look like and if there are other annotations that could use such a TextStyle class.

Other Libraries

  • pdfrw: Seems to rely on the user knowing how to do this (example)
  • pdf-annotate uses an Appearence class
  • reportlab seems not to support appearance changes.
  • PyMuPDF as an annotation class

@MartinThoma
Copy link
Member Author

I like having an Annotation class. So we could have something like PdfWriter.add_annotation(annotation: Annotation) and also PageObject.annotations returning a list of those annotation objects. So a symmetry between reader and writer.

Also, me might re-use the same interface for multiple types of annotations.

@MartinThoma
Copy link
Member Author

#1120 is better as it's more flexible / easier to extend. I close this one.

MartinThoma added a commit that referenced this pull request Jul 22, 2022
…ionBuilder (#1120)

* Add `page.annotations` (getter and setter)
* Add `writer.add_annotation(page_number, annotation_dictionary)`
* Add AnnotationBuilder to generate the `annotation_dictionary` for the different subtypes of annotations. Similarly, we could have an AnnotationsParser.

See #107

Closes #981
@MartinThoma MartinThoma deleted the add-free-text-annotation branch July 29, 2022 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is-feature A feature request PdfWriter The PdfWriter component is affected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant