ENH: Add color parameter to AnnotationBuilder.rectangle #1723

hirenpopat67 · 2023-03-17T09:18:16Z

…ectangle function

codecov · 2023-03-17T09:26:46Z

Codecov Report

Patch coverage has no change and project coverage change: -0.03 ⚠️

Comparison is base (1e4086e) 92.49% compared to head (fde732f) 92.47%.

❗ Current head fde732f differs from pull request most recent head 599b6bb. Consider uploading reports for the commit 599b6bb to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1723      +/-   ##
==========================================
- Coverage   92.49%   92.47%   -0.03%     
==========================================
  Files          34       34              
  Lines        6533     6514      -19     
  Branches     1292     1288       -4     
==========================================
- Hits         6043     6024      -19     
+ Misses        318      317       -1     
- Partials      172      173       +1

Impacted Files	Coverage Δ
pypdf/generic/_annotations.py	`93.47% <0.00%> (-2.61%)`	⬇️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

hirenpopat67 · 2023-03-17T10:03:29Z

I have added colour variable while draw rectangle on pdf change _annotations.py file rectangle function

hirenpopat67 · 2023-03-17T11:57:01Z

Codecov Report

Patch coverage has no change and project coverage change: -0.03 warning

Comparison is base (9878034) 92.50% compared to head (684e2ff) 92.47%.

Additional details and impacted files
umbrella View full report in Codecov by Sentry. loudspeaker Do you have feedback about the report comment? Let us know in this issue.

I have added colour variable while draw rectangle on pdf change _annotations.py file rectangle function

MartinThoma · 2023-03-17T17:43:35Z

Thank you for the PR 🙏

Should we rather call it border_color?

Also, new parameters need to be added to the end for backwards compatibility. And I think we should start using keyword-only parameters, e.g.:

    @staticmethod
    def rectangle(
        rect: Union[RectangleObject, Tuple[float, float, float, float]],
        interiour_color: Optional[str] = None,
        *,  # This makes it keyword-only, see https://peps.python.org/pep-3102/
        rectangle_color: Optional[str] = None,  # I guess it's the border_color, but I didn't check.
    ) -> DictionaryObject:

pubpub-zz · 2023-03-18T13:45:42Z

As an alternative, as "/C" is used on many annotations I propose to add a function applicable to all annotations. example :

annotation = AnnotationBuilder.rectangle(rect=(50, 550, 200, 650)).set_border_color([255,0,0])

it would be great if the color could be either an arrayobject, a standard array or tuple of ints, and maybe an string from PIL.ImageColor.

@MartinThoma / @hirenpopat67 your opinion ?

MartinThoma · 2023-03-18T13:51:05Z

@pubpub-zz What would be the advantage of a method set_border_color vs a keyword-parameter border_color?

MartinThoma · 2023-03-18T13:51:40Z

it would be great if the color could be either an arrayobject, a standard array or tuple of ints, and maybe an string from PIL.ImageColor

Sounds good - but we should do it consistently for all color parameters in pypdf

pubpub-zz · 2023-03-18T15:19:57Z

@pubpub-zz What would be the advantage of a method set_border_color vs a keyword-parameter border_color?

the advantage would be to be able to have this solution for all annotations (circle, line,...)

also it will be a good approach later to add other fields/properties

MartinThoma

Ensure backwards compatibility by making it a keyword-only argument
Use a name that is more telling than rectangle_color, e.g border_color

hirenpopat67 · 2023-03-18T16:20:07Z

*Dear @MartinThoma,* Thank you for taking the time to review my pull request. I appreciate your feedback and suggestions for improvement. I completely agree with your comments and will make the necessary changes to ensure backwards compatibility by making the argument a keyword-only one. Additionally, I will rename the argument to something more descriptive such as "border_color" to better reflect its purpose. These changes should help make the code more readable and user-friendly. Once again, thank you for your help and valuable feedback. I will work on these changes and submit a revised pull request as soon as possible. Best regards, *Hiren Popat* Software Engineer (Python Developer)

…

On Sat, 18 Mar 2023 at 21:21, Martin Thoma ***@***.***> wrote: ***@***.**** requested changes on this pull request. 1. Ensure backwards compatibility by making it a keyword-only argument 2. Use a name that is more telling than rectangle_color, e.g border_color — Reply to this email directly, view it on GitHub <#1723 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQ32DDZ7WZJK6A6OXUY6L7TW4XKWXANCNFSM6AAAAAAV6IXZAI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

MartinThoma · 2023-03-18T17:00:03Z

the advantage [of a method .set_border_color in contrast to a keyword-argument] would be to be able to have this solution for all annotations (circle, line,...)

You mean as a way to avoid code duplication?

The current annotation builder methods return DictionaryObjects. How would we change it so that this method can be added?

pubpub-zz · 2023-03-18T18:47:54Z

the advantage [of a method .set_border_color in contrast to a keyword-argument] would be to be able to have this solution for all annotations (circle, line,...)

You mean as a way to avoid code duplication?

The current annotation builder methods return DictionaryObjects. How would we change it so that this method can be added?

I would propose the AnnotationBuilder functions to create AnnotationDictionaryObject on which class the functions set_border_color() and later some other ones such as annotation_name("/NM"), border_type("/Border"),...

hirenpopat67 · 2023-03-20T06:09:59Z

I have created the set_border_color() function and would like to request your feedback before creating a new pull request to merge the code. Please review the code and let me know if you have any suggestions or concerns. Review:- https://github.com/hirenpopat67/pypdf/blob/main/pypdf/generic/_annotations.py Thank you for your attention to this matter. I look forward to hearing your feedback. Best regards, *Hiren Popat* Software Engineer (Python Developer)

…

On Sun, 19 Mar, 2023, 12:18 am pubpub-zz, ***@***.***> wrote: the advantage [of a method .set_border_color in contrast to a keyword-argument] would be to be able to have this solution for all annotations (circle, line,...) You mean as a way to avoid code duplication? The current annotation builder methods return DictionaryObjects. How would we change it so that this method can be added? I would propose the AnnotationBuilder functions to create AnnotationDictionaryObject on which class the functions set_border_color() and later some other ones such as annotation_name("/NM"), border_type("/Border"),... — Reply to this email directly, view it on GitHub <#1723 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQ32DD3JEUS7FSEI5WQ7BGLW4X7ONANCNFSM6AAAAAAV6IXZAI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

hirenpopat67 · 2023-03-20T06:18:45Z

As discussed i have created set_border_color function to avoid duplication

fixes py-pdf#1723

pubpub-zz · 2023-03-20T19:07:28Z

@hirenpopat67
I've pushed a PR (hirenpopat67#1) on your branch to show what I had in mind. What do you think of it ?

hirenpopat67 · 2023-03-21T11:17:12Z

@hirenpopat67 I've pushed a PR (hirenpopat67#1) on your branch to show what I had in mind. What do you think of it ?

Great ,I have reviewed code its working fine.

Thank you for your time and consideration..

hirenpopat67 · 2023-03-21T11:21:10Z

I hope this email finds you well. I am writing to express my gratitude for your proposal to merge the code. I have reviewed the code and can confirm that it is working flawlessly. I am glad to hear that my contribution has helped in achieving the project's goals. Thank you once again.. Best regards, *Hiren Popat* Software Engineer (Python Developer)

…

On Tue, 21 Mar, 2023, 4:03 pm Hiren Popat, ***@***.***> wrote: I hope this email finds you well. I am writing to express my gratitude for your proposal to merge the code. I have reviewed the code and can confirm that it is working flawlessly. I am glad to hear that my contribution has helped in achieving the project's goals. Thank you once again.. Best regards, *Hiren Popat* Software Engineer (Python Developer) On Tue, 21 Mar, 2023, 3:05 pm Hiren Popat, ***@***.***> wrote: > Thankyou for PR > > From my initial inspection, it looks like your changes are working as > intended. > > In addition to the changes that I made, I also created a new function > called set_border_colour which allows users to customize the color of the > annotations that they draw. I think this would be a valuable addition to > the project and would greatly enhance its functionality. > > I would greatly appreciate it if you could take a look at my code and > merge it into the branch if you agree that it would be a beneficial > addition. If there are any issues that need to be addressed before merging, > please let me know and I will be happy to make any necessary changes. > > Thank you for your time and consideration. > > Best regards, > > *Hiren Popat* > > Software Engineer (Python Developer) > > On Tue, 21 Mar, 2023, 12:37 am pubpub-zz, ***@***.***> > wrote: > >> @hirenpopat67 <https://github.com/hirenpopat67> >> I've pushed a PR on your branch to show what I had in mind. What do you >> think of it ? >> >> — >> Reply to this email directly, view it on GitHub >> <#1723 (comment)>, or >> unsubscribe >> <https://github.com/notifications/unsubscribe-auth/AQ32DD4PKGP3IMZERY6DNPDW5CTHZANCNFSM6AAAAAAV6IXZAI> >> . >> You are receiving this because you were mentioned.Message ID: >> ***@***.***> >> >

pubpub-zz · 2023-03-21T19:51:45Z

@hirenpopat67 , can you complete test_generic.py in order to cover the new function

MartinThoma · 2023-03-21T20:46:59Z

I'm still not sure if the AnnotationObject / set_border_color really is a good solution. Do all annotations have a border. I know that /Rectangle is required, but for border I'm less certain.

Are there any other attributes that all annotations have?

pubpub-zz · 2023-03-21T21:09:11Z

from pdf 1.7 spec,

but it applicable to circle (border) strike out color, ...

maybe set_color() could be better...

pubpub-zz

to fix mypy and also to cope with RGB on 0-255 ranges

pubpub-zz · 2023-03-21T21:19:38Z

pypdf/generic/_annotations.py

+try:
+    from PIL import ImageColor as ImageColorLoaded
+except ImportError:
+    ImageColorLoaded = None


to fix mypy

Suggested change

ImageColorLoaded = None

ImageColorLoaded = None # type: ignore

pubpub-zz · 2023-03-21T21:23:34Z

pypdf/generic/_annotations.py

+        if isinstance(border_color, str):
+            if ImageColorLoaded is None:
+                raise ImportError("PIL required but not installed")
+            border_color = ImageColorLoaded.getrgb(border_color)


Suggested change

border_color = ImageColorLoaded.getrgb(border_color)

border_color = (x/255.0 for x in ImageColorLoaded.getrgb(border_color)[:3])

for mypy

pubpub-zz · 2023-03-21T21:24:50Z

pypdf/generic/_annotations.py

+                raise ImportError("PIL required but not installed")
+            border_color = ImageColorLoaded.getrgb(border_color)
+        if isinstance(border_color, (list, tuple)):
+            border_color = ArrayObject([FloatObject(n) for n in border_color[:3]])


Suggested change

border_color = ArrayObject([FloatObject(n) for n in border_color[:3]])

border_color = ArrayObject([FloatObject(n if n<=1.0 else n/255.0) for n in border_color[:3]])

if the array provided is an array of RGB color

hirenpopat67 · 2023-03-22T07:16:51Z

I am writing to suggest a small improvement to the existing implementation of the rectangle function.

Currently, the function allows users to draw a rectangle on a PDF and specify the interior color of the rectangle using the interior_color argument. However, there is no option to specify the fill color of the rectangle.

Rather than create set_border_colour function we can create colour color keyword argument to the rectangle function
to make the function more user-friendly , I would like to suggest adding a "color" keyword argument to the rectangle function. This would allow users to specify both the border and fill color of the rectangle using a single function call.

Here is an example of the updated function signature:

@staticmethod
def rectangle(
rect: Union[RectangleObject, Tuple[float, float, float, float]],
interior_color: Optional[str] = None,
color: Optional[str] = None,
) -> AnnotationObject:

I believe that this change would make the function more intuitive and user-friendly. Please let me know if you have any questions or concerns regarding this suggestion.

Thank you for considering my suggestion.

Best regards,
Hiren Popat

Software Engineer (Python Developer)

pubpub-zz · 2023-03-22T21:19:32Z

I am writing to suggest a small improvement to the existing implementation of the rectangle function.

Currently, the function allows users to draw a rectangle on a PDF and specify the interior color of the rectangle using the interior_color argument. However, there is no option to specify the fill color of the rectangle.

Rather than create set_border_colour function we can create colour color keyword argument to the rectangle function to make the function more user-friendly , I would like to suggest adding a "color" keyword argument to the rectangle function. This would allow users to specify both the border and fill color of the rectangle using a single function call.

the approach I've proposed will be very efficient in this case adding the new function we will get:

AnnotationBuilder.rectangle((100,200,300,400),
     ).set_border_color("red",
     ).set_interior_color("yellow")

but without extra mods

AnnotationBuilder.ellipse((100,200,300,400),
     ).set_border_color("red",
     ).set_interior_color("yellow")

Do you agree ?

hirenpopat67 · 2023-03-23T06:31:34Z

@pubpub-zz Yes, I agree. Your proposed approach of using the AnnotationBuilder class to create annotations,while setting the border and interior colors, is efficient and should work well in this case. The method chaining syntax you've used makes the code concise and easy to read.

pubpub-zz · 2023-03-23T11:46:14Z

based on #1740, I recommend set_color()

MartinThoma · 2023-03-23T20:00:07Z

I think we are now having a way more complicated discussion within this PR. Let's move that to #1741 :-)

MartinThoma · 2023-03-26T09:52:26Z

Please wait with this PR until we come to a conclusion with #1741

It might be #1745 - let's wait another week for opinions on that :-) (please also share your opinion about the #1745

hirenpopat67 · 2023-03-26T10:48:02Z

Please wait with this PR until we come to a conclusion with #1741

It might be #1745 - let's wait another week for opinions on that :-) (please also share your opinion about the #1745

@MartinThoma
Thanks for asking my opinion i have shared #1741

…lor rectangle

hirenpopat67 closed this Mar 17, 2023

hirenpopat67 reopened this Mar 17, 2023

MartinThoma changed the title ~~add color functionality when draw a box in the pdf _annotation.py's r…~~ ENH: Add color parameter to AnnotationBuilder.rectangle Mar 17, 2023

MartinThoma self-requested a review March 18, 2023 15:49

MartinThoma requested changes Mar 18, 2023

View reviewed changes

pubpub-zz added a commit to pubpub-zz/pypdf that referenced this pull request Mar 20, 2023

BUG : fixes no attribute 'indirect_reference' error py-pdf#1723

439c1e0

fixes py-pdf#1723

pubpub-zz mentioned this pull request Mar 20, 2023

BUG: 'DictionaryObject' object has no attribute 'indirect_reference' #1729

Merged

hirenpopat67 requested a review from MartinThoma March 21, 2023 11:02

pubpub-zz requested changes Mar 21, 2023

View reviewed changes

MartinThoma closed this in 0331a7c Mar 23, 2023

pubpub-zz reopened this Mar 23, 2023

pubpub-zz mentioned this pull request Mar 23, 2023

ENH: Add Highlight text markup annotation #1740

Merged

MartinThoma marked this pull request as draft March 26, 2023 09:50

_annotations.py : add color variable in rectangle function to draw co…

02cdd27

…lor rectangle

hirenpopat67 closed this Apr 25, 2023

hirenpopat67 force-pushed the main branch from 599b6bb to 02cdd27 Compare April 25, 2023 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Add color parameter to AnnotationBuilder.rectangle #1723

ENH: Add color parameter to AnnotationBuilder.rectangle #1723

hirenpopat67 commented Mar 17, 2023

codecov bot commented Mar 17, 2023 •

edited

Loading

hirenpopat67 commented Mar 17, 2023

hirenpopat67 commented Mar 17, 2023

Codecov Report

MartinThoma commented Mar 17, 2023

pubpub-zz commented Mar 18, 2023

MartinThoma commented Mar 18, 2023 •

edited

Loading

MartinThoma commented Mar 18, 2023

pubpub-zz commented Mar 18, 2023 •

edited by MartinThoma

Loading

MartinThoma left a comment

hirenpopat67 commented Mar 18, 2023 via email

MartinThoma commented Mar 18, 2023

pubpub-zz commented Mar 18, 2023

hirenpopat67 commented Mar 20, 2023 via email

hirenpopat67 commented Mar 20, 2023

pubpub-zz commented Mar 20, 2023 •

edited

Loading

hirenpopat67 commented Mar 21, 2023

hirenpopat67 commented Mar 21, 2023 via email

pubpub-zz commented Mar 21, 2023

MartinThoma commented Mar 21, 2023

pubpub-zz commented Mar 21, 2023

pubpub-zz left a comment

pubpub-zz Mar 21, 2023

pubpub-zz Mar 21, 2023

pubpub-zz Mar 21, 2023

hirenpopat67 commented Mar 22, 2023

pubpub-zz commented Mar 22, 2023

hirenpopat67 commented Mar 23, 2023

pubpub-zz commented Mar 23, 2023

MartinThoma commented Mar 23, 2023

MartinThoma commented Mar 26, 2023

hirenpopat67 commented Mar 26, 2023 •

edited by MartinThoma

Loading

	ImageColorLoaded = None
	ImageColorLoaded = None # type: ignore

	border_color = ImageColorLoaded.getrgb(border_color)
	border_color = (x/255.0 for x in ImageColorLoaded.getrgb(border_color)[:3])

	border_color = ArrayObject([FloatObject(n) for n in border_color[:3]])
	border_color = ArrayObject([FloatObject(n if n<=1.0 else n/255.0) for n in border_color[:3]])

ENH: Add color parameter to AnnotationBuilder.rectangle #1723

ENH: Add color parameter to AnnotationBuilder.rectangle #1723

Conversation

hirenpopat67 commented Mar 17, 2023

codecov bot commented Mar 17, 2023 • edited Loading

Codecov Report

hirenpopat67 commented Mar 17, 2023

hirenpopat67 commented Mar 17, 2023

Codecov Report

MartinThoma commented Mar 17, 2023

pubpub-zz commented Mar 18, 2023

MartinThoma commented Mar 18, 2023 • edited Loading

MartinThoma commented Mar 18, 2023

pubpub-zz commented Mar 18, 2023 • edited by MartinThoma Loading

MartinThoma left a comment

Choose a reason for hiding this comment

hirenpopat67 commented Mar 18, 2023 via email

MartinThoma commented Mar 18, 2023

pubpub-zz commented Mar 18, 2023

hirenpopat67 commented Mar 20, 2023 via email

hirenpopat67 commented Mar 20, 2023

pubpub-zz commented Mar 20, 2023 • edited Loading

hirenpopat67 commented Mar 21, 2023

hirenpopat67 commented Mar 21, 2023 via email

pubpub-zz commented Mar 21, 2023

MartinThoma commented Mar 21, 2023

pubpub-zz commented Mar 21, 2023

pubpub-zz left a comment

Choose a reason for hiding this comment

pubpub-zz Mar 21, 2023

Choose a reason for hiding this comment

pubpub-zz Mar 21, 2023

Choose a reason for hiding this comment

pubpub-zz Mar 21, 2023

Choose a reason for hiding this comment

hirenpopat67 commented Mar 22, 2023

pubpub-zz commented Mar 22, 2023

hirenpopat67 commented Mar 23, 2023

pubpub-zz commented Mar 23, 2023

MartinThoma commented Mar 23, 2023

MartinThoma commented Mar 26, 2023

hirenpopat67 commented Mar 26, 2023 • edited by MartinThoma Loading

codecov bot commented Mar 17, 2023 •

edited

Loading

MartinThoma commented Mar 18, 2023 •

edited

Loading

pubpub-zz commented Mar 18, 2023 •

edited by MartinThoma

Loading

pubpub-zz commented Mar 20, 2023 •

edited

Loading

hirenpopat67 commented Mar 26, 2023 •

edited by MartinThoma

Loading