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

PIL/Pillow image class #960

Merged
merged 27 commits into from
Jul 21, 2021
Merged

PIL/Pillow image class #960

merged 27 commits into from
Jul 21, 2021

Conversation

corranwebster
Copy link
Contributor

@corranwebster corranwebster commented Jun 19, 2021

This builds on #959 to provide an image class for PIL/Pillow Image instances. This is part of #909 but not essential to it - we could decide to defer PIL support.

This adds an optional dependency on pillow - this is already a dependency of WxPython, so this is only potentially a problem for Qt users.

@corranwebster corranwebster marked this pull request as draft June 19, 2021 09:07
@corranwebster corranwebster marked this pull request as ready for review June 21, 2021 08:57
@rahulporuri rahulporuri requested review from rahulporuri and removed request for rahulporuri July 1, 2021 10:17
@rahulporuri
Copy link
Contributor

rahulporuri commented Jul 6, 2021

i'm removing myself as a reviewer for now - i'll wait until after #959 is merged.

pyface/ui_traits.py Outdated Show resolved Hide resolved
@corranwebster
Copy link
Contributor Author

This makes pillow an optional dependency:

  • there is now an optional dependency for pillow
  • PILImage is only used if imported directly - it isn't in pyface.api

@rahulporuri rahulporuri self-requested a review July 9, 2021 05:11
Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

LGTM with a few nitpicky comments.

pyface/i_pil_image.py Outdated Show resolved Hide resolved
pyface/i_pil_image.py Outdated Show resolved Hide resolved
pyface/i_pil_image.py Outdated Show resolved Hide resolved
pyface/i_pil_image.py Outdated Show resolved Hide resolved
pyface/pil_image.py Outdated Show resolved Hide resolved
#
# Thanks for using Enthought open source!

from pyface.qt.QtGui import QIcon, QPixmap
Copy link
Contributor

Choose a reason for hiding this comment

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

Tangent : I always wondered why these pyface.qt imports preceeded other imports. Why is that? Never mind. I think it's because the pyface.qt imports are technically Qt-imports which are third party packages and so they should exist before Enthought library imports.

I wish this was codified somewhere because the answer isn't obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not done universally. In this case it was likely just copy-pasted from image_resource.py. In other cases as you note it's likely from taking Wx code and translating it and so the Wx imports get replaced by Qt imports in the same place.

It would be nice to standardize, but that would require going through and actually standardizing

pyface/ui/qt4/pil_image.py Outdated Show resolved Hide resolved
pyface/ui/wx/pil_image.py Outdated Show resolved Hide resolved
@@ -33,7 +33,6 @@ class AspectRatio(IntEnum):

def image_to_bitmap(image):
""" Convert a wx.Image to a wx.Bitmap.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we normally have a new line in the function docstring between the description and the parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Not sure what happened here. It might be a bad merge.

@corranwebster
Copy link
Contributor Author

Right, I think this is good. If everything passes I will merge.

@corranwebster corranwebster merged commit c6616a3 into master Jul 21, 2021
@corranwebster corranwebster deleted the feat/pil-image-class branch July 21, 2021 07:05
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.

2 participants