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

Pattern feature implementation #1041

Closed

Conversation

boushrabnd
Copy link

@boushrabnd boushrabnd commented Dec 5, 2023

This PR attempts to implement the Pattern Feature described by Issue #790.

Our current approach is to follow the same pipeline as set_font(). We have broken down the task into the following subtasks:

  • Creating PDFpattern class

  • Adding the set_fill_pattern method

  • Creating PDFpattern objects and adding them to the PDF Pattern dictionary

  • Serializing the call to a pattern

  • Documentation

  • Tests

Checklist:

  • The GitHub pipeline is OK (green),
    meaning that both pylint (static code analyzer) and black (code formatter) are happy with the changes of this PR.

  • A unit test is covering the code added / modified by this PR

  • This PR is ready to be merged

  • In case of a new feature, docstrings have been added, with also some documentation in the docs/ folder

  • A mention of the change is present in CHANGELOG.md

By submitting this pull request, I confirm that my contribution is made under the terms of the GNU LGPL 3.0 license.

@boushrabnd boushrabnd requested a review from gmischler as a code owner December 5, 2023 09:05
fpdf/drawing.py Outdated
Comment on lines 1269 to 1278
@property
def fill_pattern(self):
"""The desired fill pattern for this path/group."""
pass

@fill_pattern.setter
def fill_pattern(self, new):
"""The desired fill pattern for this path/group."""
pass

Copy link
Member

@Lucas-C Lucas-C Dec 5, 2023

Choose a reason for hiding this comment

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

Those getter / setter do nothing, and are in fact not really needed if the property you introduced is public (no leading underscore)

@Lucas-C Lucas-C marked this pull request as draft December 5, 2023 09:18
@Lucas-C
Copy link
Member

Lucas-C commented Dec 5, 2023

Thank you for initiating a PR 👍

I converted it into a draft for now, as this seems still incomplete.
I'll perform a code review once some unit tests have been added to this PR 😊

@boushrabnd
Copy link
Author

Thank you for initiating a PR 👍

I converted it into a draft for now, as this seems still incomplete. I'll perform a code review once some unit tests have been added to this PR 😊

Sorry! I should have made it a draft thank you!

@Lucas-C
Copy link
Member

Lucas-C commented May 24, 2024

Hi @boushrabnd!

I see that you have added extra commits following my last comment, but I haven't been back to you since last december...

Are you still working on this feature?
Do you need help on how to add unit tests?

@bbendou
Copy link

bbendou commented May 24, 2024

Hi @boushrabnd!

I see that you have added extra commits following my last comment, but I haven't been back to you since last december...

Are you still working on this feature? Do you need help on how to add unit tests?

Hello Lucas! I apologies, unfortunately I haven't had the chance to work on it more since december as my university workload got a bit hectic. If you don't mind I can continue to work on it this summer? I understand if you would like someone else to take it on, but I would love to give it a second try if possible.

Thank you for your patience

@boushrabnd
Copy link
Author

Hi @boushrabnd!
I see that you have added extra commits following my last comment, but I haven't been back to you since last december...
Are you still working on this feature? Do you need help on how to add unit tests?

Hello Lucas! I apologies, unfortunately I haven't had the chance to work on it more since december as my university workload got a bit hectic. If you don't mind I can continue to work on it this summer? I understand if you would like someone else to take it on, but I would love to give it a second try if possible.

Thank you for your patience

I just realized I replied with my other account... this is to confirm that I am the same person lol

@Lucas-C
Copy link
Member

Lucas-C commented May 24, 2024

Hello Lucas! I apologies, unfortunately I haven't had the chance to work on it more since december as my university workload got a bit hectic. If you don't mind I can continue to work on it this summer? I understand if you would like someone else to take it on, but I would love to give it a second try if possible.

That is 100% totally fine 🙂
This is a very interesting feature, and I would myself love to play with patterns!
But there is really no rush, you can take all the time you need.
And if at the end of the summer you realize you won't have the time required to finish this, that's again totally fine!
Please just keep us informed 😄

@Lucas-C
Copy link
Member

Lucas-C commented Oct 14, 2024

Hi @boushrabnd 🙂

I think the summer is over now.
I wish it would have lasted longer, in France where I live it's starting to get cold 😅

I just wanted to know if you are still planning to work on this? 🙂

@boushrabnd
Copy link
Author

Hi @boushrabnd 🙂

I think the summer is over now. I wish it would have lasted longer, in France where I live it's starting to get cold 😅

I just wanted to know if you are still planning to work on this? 🙂

Apologies for the empty promise, this summer was more hectic than expected with a family situation. I think it'll be better to assign this task to someone else. Apologies for not letting you know earlier and thank you for your patience!

@Lucas-C
Copy link
Member

Lucas-C commented Oct 14, 2024

Apologies for the empty promise, this summer was more hectic than expected with a family situation. I think it'll be better to assign this task to someone else. Apologies for not letting you know earlier and thank you for your patience!

No problem @boushrabnd 🙂
That's perfectly fine.
Thank you for your quick answer!

I'm closing this PR now.

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

Successfully merging this pull request may close these issues.

5 participants