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

Remove deprecations for Pillow 10.0.0 #7059

Merged
merged 18 commits into from Apr 8, 2023
Merged

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Apr 1, 2023

Remove deprecations scheduled for removal in Pillow 10.0.0:

TODO in followup:

@hugovk hugovk added the Removal Removal of a feature, usually done in major releases label Apr 1, 2023
@hugovk hugovk added this to the 10.0.0 milestone Apr 1, 2023
@hugovk hugovk changed the title Drop support for PyQt5 and PySide2, deprecated in 9.2.0 Remove deprecations for Pillow 10.0.0 Apr 2, 2023
@hugovk hugovk marked this pull request as draft April 2, 2023 16:37
@hugovk
Copy link
Member Author

hugovk commented Apr 2, 2023

Will remove the other deprecations here too.

@hugovk
Copy link
Member Author

hugovk commented Apr 2, 2023

Question about 6369c80 (#7059):

Should we also do this?

-    def show_file(self, path=None, **options):
+    def show_file(self, path, **options):
         """
         Display given file.
         """
-        if path is None:
-            msg = "Missing required argument: 'path'"
-            raise TypeError(msg)
         os.system(self.get_command(path, **options))  # nosec
         return 1

@hugovk
Copy link
Member Author

hugovk commented Apr 2, 2023

Question about 4c9f8b4 (#7059):

class _E has this comment:

# _E(scale, offset) represents the affine transformation scale * x + offset.
# The "data" field is named for compatibility with the old implementation,
# and should be renamed once coerce_e is removed.

What should it be renamed to?

@radarhere
Copy link
Member

Question about 6369c80 (#7059):

Should we also do this?

-    def show_file(self, path=None, **options):
+    def show_file(self, path, **options):
         """
         Display given file.
         """
-        if path is None:
-            msg = "Missing required argument: 'path'"
-            raise TypeError(msg)
         os.system(self.get_command(path, **options))  # nosec
         return 1

Yes, sounds like good to me. The default value of None was only added in #5959 - my intention would have been just to allow both path and file to work during the deprecation period.

@radarhere
Copy link
Member

Question about 4c9f8b4 (#7059):

class _E has this comment:

# _E(scale, offset) represents the affine transformation scale * x + offset.
# The "data" field is named for compatibility with the old implementation,
# and should be renamed once coerce_e is removed.

What should it be renamed to?

From my reading of the comment, the user's plan was for it to be renamed "offset"

@nulano
Copy link
Contributor

nulano commented Apr 2, 2023

My reading of the comment is the following change:

-# _E(scale, offset) represents the affine transformation scale * x + offset.
-# The "data" field is named for compatibility with the old implementation,
-# and should be renamed once coerce_e is removed.
 class _E:
-    def __init__(self, scale, data):
+    def __init__(self, scale, offset):
         self.scale = scale
-        self.data = data
+        self.offset = offset

(which I think is the same as @radarhere's comment, but I didn't understand it until I posted this comment)

Copy link
Contributor

@nulano nulano left a comment

Choose a reason for hiding this comment

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

I have made a few suggestions for textsize tests: hugovk#100

@@ -665,7 +474,6 @@ def getmask2(
self,
text,
mode="",
fill=_UNSPECIFIED,
Copy link
Contributor

Choose a reason for hiding this comment

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

In #6195 (comment) there was a suggestion to make this function, and possibly others, use keyword-only arguments. I see that I was planning to deprecate positional arguments for other text-related functions, but it seems I didn't get to it in time for removal in Pillow 10.

One option would be to remove support for positional arguments here by replacing fill with *, and to deprecate positional arguments in related functions, so that they will be consistent in Pillow 11.

Removing positional argument support here would simplify updating the API while preserving backwards compatibility, and allow the documentation to be in a logical or alphabetical order rather than version-added order.

However, this does not have to be decided in this PR. Perhaps it might be better to discuss in a separate issue/PR - perhaps I could make a separate deprecation PR once this is merged to discuss.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, let's discuss this in another issue or PR.

)[1]
+ spacing
)
return self.textbbox((0, 0), "A", font, stroke_width=stroke_width)[3] + spacing
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is slightly different when stroke_width != 0. The deprecated function FreeTypeFont.getsize seems to have an additional issue that I had not noticed before: it was adding the stroke width to the "height" twice even though it was returning the bottom coordinate instead of the actual height.

This has the effect of changing multiline spacing due to the change (see the changed image for an example). There are three options on how to proceed, and it is not clear to me whether any one of them is better than the others:

  • Do not consider stroke width for line spacing (i.e. remove the parameter). I believe this would be the best visually, I would not expect a varying stroking width to affect the text layout.
  • Consider 2x stroke width for line spacing (i.e. add stroke width to the returned value). This would preserve exact backwards compatibility. It would extend the line spacing by the size of stroke both on the top and bottom side.
  • Consider 1x stroke width for line spacing (i.e. the current proposed change). This is the simplest option, and avoids mixing ImageDraw and ImageFont behaviour.

An alternative suggestion could be to move line spacing calculations to the font class, which can have other (including non backwards compatible) benefits, but I'm not sure what that API would look like.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shall we drop these ImageFont removals from this PR and we can consider all this separately?

The other changes here fix a failure with the new 3.12.0a7 (#7072 (comment)), so it'd be good to get that merged.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, whatever you prefer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, made a copy of this branch and pushed to https://github.com/hugovk/Pillow/tree/rm-qt5a, then dropped the last two commits here (ac23f4f, 7712062), then rebased on main (to cleanup the fixups) and force-pushed.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nulano Please could you make a followup for ImageFont? Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

I've created PR #7080 to help this along - feel free to suggest changes.

@radarhere radarhere mentioned this pull request Apr 6, 2023
@hugovk hugovk marked this pull request as ready for review April 6, 2023 14:09
hugovk and others added 3 commits April 8, 2023 10:29
Remove deprecations for Pillow 10.0.0
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
@radarhere radarhere merged commit 75388cf into python-pillow:main Apr 8, 2023
62 checks passed
@radarhere
Copy link
Member

I've created PR #7080 as the followup PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Removal Removal of a feature, usually done in major releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants