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

Set minimum version for Pillow (10.0.0) and Python (3.8) #388

Merged
merged 6 commits into from
Jan 17, 2024

Conversation

jburel
Copy link
Member

@jburel jburel commented Jan 10, 2024

Problem noticed while working on ome/omero-scripts#214

@jburel jburel requested a review from will-moore January 10, 2024 15:47
Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

Looks related to #376 as these APIs were removed in Pillow 10 - https://pillow.readthedocs.io/en/stable/releasenotes/10.0.0.html#font-size-and-offset-methods

Thinking about it, is it also a good occasion to update setup.py to ensure a minimal Pillow version? These functions have been deprecated since Pillow 9.2.0 - https://pillow.readthedocs.io/en/stable/releasenotes/9.2.0.html#font-size-and-offset-methods

@jburel
Copy link
Member Author

jburel commented Jan 10, 2024

since Pillow 10 has been around for over 6 months (01/07/2023). I think we should have to 10.0.0 as a minimum

@sbesson
Copy link
Member

sbesson commented Jan 11, 2024

Thanks @jburel . Requiring Pillow>10 effectively enforces Python 3.8 as the minimal supported version in upcoming OMERO.py versions - see https://pillow.readthedocs.io/en/latest/installation.html#python-support

Given the current timeline for Python 3.x support and the fact Python 3.7 and earlier is EOL, no objections with enforcing this from my side

@jburel
Copy link
Member Author

jburel commented Jan 11, 2024

@will-moore any comment from your side? It will be good to have that PR merged and do a release so we can move forward with the test-infra upgrade work

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

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

Looks good.
When looking up font.getbbox() I noticed that it returns x, y, x2, y2 not x, y, width, height. Width is the same as x2 if the x, y origin is 0, 0, but this isn't always true.
In this example, x is 0 but y isn't:

https://stackoverflow.com/questions/74327497/understanding-the-getbbox-method-of-a-pillow-font-object

However, that's only the case if stroke isn't zero, and even then x is still 0 so we're OK for width.
I tried to run image.createMovie() to test, but couldn't get that to work due to unrelated issues.
But I think the changes look good 👍

@jburel
Copy link
Member Author

jburel commented Jan 11, 2024

@will-moore do you suggest to do box = font.getbbox(...) then box[2]-box[0] for the width to be on the safe side.?
This is no the case in the changes you made for figure.
I will also need to adjust the change in omero-scripts

@jburel
Copy link
Member Author

jburel commented Jan 11, 2024

@will-moore I have made the changes to calculate the diff. I have also fixed the code in ome/omero-scripts#214

@will-moore
Copy link
Member

That looks great. Wasn't sure it was worth it given that we don't know it's needed, but nice to be sure - thx

@jburel
Copy link
Member Author

jburel commented Jan 15, 2024

Since the requirement has been modified i.e. minimum version for Pillow. Proposing to merge and tag as 5.18.0

@sbesson
Copy link
Member

sbesson commented Jan 15, 2024

Should

python_requires='>=3',
also be modified to capture the minimal Python requirement?

@jburel jburel merged commit 1ed6b94 into ome:master Jan 17, 2024
7 checks passed
@jburel jburel changed the title Fix issue with font Set minimum version for Pillow (10.0.0) and Python (3.8) Jan 17, 2024
@jburel jburel deleted the update branch April 29, 2024 13:39
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.

3 participants