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

Added IPythonViewer #5289

Merged
merged 5 commits into from Mar 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 17 additions & 1 deletion Tests/test_imageshow.py
Expand Up @@ -62,4 +62,20 @@ def test_viewer():

def test_viewers():
for viewer in ImageShow._viewers:
viewer.get_command("test.jpg")
try:
viewer.get_command("test.jpg")
except NotImplementedError:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pass
assert viewer.show(hopper()) == 1

Would remove the need for a separate test.

Copy link
Member Author

Choose a reason for hiding this comment

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

The separate test

  • confirms that the IPythonViewer is present when IPython is available
  • runs IPythonViewer even if another Viewer instance has priority

Also, IPython seems to be a special case, where running its Viewer doesn't disrupt the test process - thinking generically for the sake of the future, just because a Viewer isn't based on a command line instruction doesn't mean that we want it to run in the middle of the test suite?



def test_ipythonviewer():
pytest.importorskip("IPython", reason="IPython not installed")
for viewer in ImageShow._viewers:
if isinstance(viewer, ImageShow.IPythonViewer):
test_viewer = viewer
break
else:
assert False

im = hopper()
assert test_viewer.show(im) == 1
1 change: 1 addition & 0 deletions docs/reference/ImageShow.rst
Expand Up @@ -9,6 +9,7 @@ All default viewers convert the image to be shown to PNG format.

.. autofunction:: PIL.ImageShow.show

.. autoclass:: IPythonViewer
.. autoclass:: WindowsViewer
.. autoclass:: MacViewer

Expand Down
13 changes: 10 additions & 3 deletions docs/releasenotes/8.2.0.rst
Expand Up @@ -21,10 +21,17 @@ TODO
API Additions
=============

TODO
^^^^
ImageShow.IPythonViewer
^^^^^^^^^^^^^^^^^^^^^^^

TODO
If IPython is present, this new ``ImageShow.Viewer`` subclass will be
registered. It displays images on all IPython frontends. This will be helpful
to users of Google Colab, allowing ``im.show()`` to display images.

It is lower in priority than the other default Viewer instances, so it will
Copy link
Contributor

Choose a reason for hiding this comment

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

It is lower in priority than the other default Viewer instances

Please correct me if I'm wrong, but this is false. The new viewer has the highest priority AFAICT

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right! Thanks for catching that. However, the documentation correctly states my intention, so I've updated the code to match it.

only be used by ``im.show()`` or ``ImageShow.show()`` if none of the other
viewers are available. This means that the behaviour of ``ImageShow`` will stay
the same for most Pillow users.

Security
========
Expand Down
17 changes: 17 additions & 0 deletions src/PIL/ImageShow.py
Expand Up @@ -225,6 +225,23 @@ def get_command_ex(self, file, title=None, **options):
if shutil.which("xv"):
register(XVViewer)


class IPythonViewer(Viewer):
"""The viewer for IPython frontends."""

def show_image(self, image, **options):
ipython_display(image)
return 1


try:
from IPython.display import display as ipython_display
except ImportError:
pass
else:
register(IPythonViewer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to put IPythonViewer before the unix viewers but after the other Windows/macOS ones? I think it would be best to put this at the top (or use register(..., order=0)) to help consistency in IPython (e.g. when combined with tools that offer PDF exports, etc.).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, no reason. I've moved it up.



if __name__ == "__main__":

if len(sys.argv) < 2:
Expand Down