Skip to content

Commit

Permalink
In show_file, use os.remove to remove temporary images
Browse files Browse the repository at this point in the history
  • Loading branch information
radarhere committed Feb 2, 2022
1 parent eccd853 commit 8da8013
Show file tree
Hide file tree
Showing 2 changed files with 119 additions and 32 deletions.
6 changes: 4 additions & 2 deletions Tests/test_imageshow.py
Expand Up @@ -85,11 +85,13 @@ def test_ipythonviewer():
not on_ci() or is_win32(),
reason="Only run on CIs; hangs on Windows CIs",
)
def test_file_deprecated():
def test_file_deprecated(tmp_path):
f = str(tmp_path / "temp.jpg")
for viewer in ImageShow._viewers:
hopper().save(f)
with pytest.warns(DeprecationWarning):
try:
viewer.show_file(file="test.jpg")
viewer.show_file(file=f)
except NotImplementedError:
pass
with pytest.raises(TypeError):
Expand Down
145 changes: 115 additions & 30 deletions src/PIL/ImageShow.py
Expand Up @@ -15,7 +15,6 @@
import shutil
import subprocess
import sys
import tempfile
import warnings
from shlex import quote

Expand Down Expand Up @@ -180,16 +179,15 @@ def show_file(self, path=None, **options):
path = options.pop("file")
else:
raise TypeError("Missing required argument: 'path'")
fd, temp_path = tempfile.mkstemp()
with os.fdopen(fd, "w") as f:
f.write(path)
with open(temp_path) as f:
subprocess.Popen(
["im=$(cat); open -a Preview.app $im; sleep 20; rm -f $im"],
shell=True,
stdin=f,
)
os.remove(temp_path)
subprocess.call(["open", "-a", "Preview.app", path])
subprocess.Popen(
[
sys.executable,
"-c",
"import os, sys, time;time.sleep(20);os.remove(sys.argv[1])",
path,
]
)
return 1


Expand All @@ -205,6 +203,16 @@ def get_command(self, file, **options):
command = self.get_command_ex(file, **options)[0]
return f"({command} {quote(file)}; rm -f {quote(file)})&"


class XDGViewer(UnixViewer):
"""
The freedesktop.org ``xdg-open`` command.
"""

def get_command_ex(self, file, **options):
command = executable = "xdg-open"
return command, executable

def show_file(self, path=None, **options):
"""
Display given file.
Expand All @@ -223,28 +231,11 @@ def show_file(self, path=None, **options):
path = options.pop("file")
else:
raise TypeError("Missing required argument: 'path'")
fd, temp_path = tempfile.mkstemp()
with os.fdopen(fd, "w") as f:
f.write(path)
with open(temp_path) as f:
command = self.get_command_ex(path, **options)[0]
subprocess.Popen(
["im=$(cat);" + command + " $im; rm -f $im"], shell=True, stdin=f
)
os.remove(temp_path)
subprocess.Popen(["xdg-open", path])
os.remove(path)
return 1


class XDGViewer(UnixViewer):
"""
The freedesktop.org ``xdg-open`` command.
"""

def get_command_ex(self, file, **options):
command = executable = "xdg-open"
return command, executable


class DisplayViewer(UnixViewer):
"""
The ImageMagick ``display`` command.
Expand All @@ -257,6 +248,32 @@ def get_command_ex(self, file, title=None, **options):
command += f" -name {quote(title)}"
return command, executable

def show_file(self, path=None, **options):
"""
Display given file.
Before Pillow 9.1.0, the first argument was ``file``. This is now deprecated,
and ``path`` should be used instead.
"""
if path is None:
if "file" in options:
warnings.warn(
"The 'file' argument is deprecated and will be removed in Pillow "
"10 (2023-07-01). Use 'path' instead.",
DeprecationWarning,
)
path = options.pop("file")
else:
raise TypeError("Missing required argument: 'path'")
args = ["display"]
if "title" in options:

This comment has been minimized.

Copy link
@kcs

kcs Mar 14, 2022

Hi @radarhere
this condition here is not enough because the explicit setting of title in
if viewer.show(image, title=title, **options): in the show function
the "title" key will be always present even if it has None value

Because of this if I try to show an image using DisplayViewer without specifying any title
it will throw an exception in Popen stating that one of the args is of NoneType

I would suggest using

if options.get("title", None):

This comment has been minimized.

Copy link
@radarhere

radarhere Mar 15, 2022

Author Member

You're right, I've created PR #6136 for this.

This comment has been minimized.

Copy link
@radarhere

radarhere Mar 24, 2022

Author Member

That PR has now been merged, so this fix should be part of Pillow 9.1.0, due out on April 1.

args += ["-name", options["title"]]
args.append(path)

subprocess.Popen(args)
os.remove(path)
return 1


class GmDisplayViewer(UnixViewer):
"""The GraphicsMagick ``gm display`` command."""
Expand All @@ -266,6 +283,27 @@ def get_command_ex(self, file, **options):
command = "gm display"
return command, executable

def show_file(self, path=None, **options):
"""
Display given file.
Before Pillow 9.1.0, the first argument was ``file``. This is now deprecated,
and ``path`` should be used instead.
"""
if path is None:
if "file" in options:
warnings.warn(
"The 'file' argument is deprecated and will be removed in Pillow "
"10 (2023-07-01). Use 'path' instead.",
DeprecationWarning,
)
path = options.pop("file")
else:
raise TypeError("Missing required argument: 'path'")
subprocess.Popen(["gm", "display", path])
os.remove(path)
return 1


class EogViewer(UnixViewer):
"""The GNOME Image Viewer ``eog`` command."""
Expand All @@ -275,6 +313,27 @@ def get_command_ex(self, file, **options):
command = "eog -n"
return command, executable

def show_file(self, path=None, **options):
"""
Display given file.
Before Pillow 9.1.0, the first argument was ``file``. This is now deprecated,
and ``path`` should be used instead.
"""
if path is None:
if "file" in options:
warnings.warn(
"The 'file' argument is deprecated and will be removed in Pillow "
"10 (2023-07-01). Use 'path' instead.",
DeprecationWarning,
)
path = options.pop("file")
else:
raise TypeError("Missing required argument: 'path'")
subprocess.Popen(["eog", "-n", path])
os.remove(path)
return 1


class XVViewer(UnixViewer):
"""
Expand All @@ -290,6 +349,32 @@ def get_command_ex(self, file, title=None, **options):
command += f" -name {quote(title)}"
return command, executable

def show_file(self, path=None, **options):
"""
Display given file.
Before Pillow 9.1.0, the first argument was ``file``. This is now deprecated,
and ``path`` should be used instead.
"""
if path is None:
if "file" in options:
warnings.warn(
"The 'file' argument is deprecated and will be removed in Pillow "
"10 (2023-07-01). Use 'path' instead.",
DeprecationWarning,
)
path = options.pop("file")
else:
raise TypeError("Missing required argument: 'path'")
args = ["xv"]
if "title" in options:
args += ["-name", options["title"]]
args.append(path)

subprocess.Popen(args)
os.remove(path)
return 1


if sys.platform not in ("win32", "darwin"): # unixoids
if shutil.which("xdg-open"):
Expand Down

0 comments on commit 8da8013

Please sign in to comment.