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
Unix Viewers - 2nd attempt #6021
Conversation
…y files changed: - Viewer class format property changed to 'PNG', format property removed from child classes. - Viewer class options property changed to {"compress_level": 1}, format property removed from child classes. - IPythonViewer format option set to None, IPythonViewer options set to {}. - Corresponding test Tests/test_imageshow.py::test_viewer changed to meet the changes mentioned above.
- In the current implementation of UnixViewers threads were used in order to control opening/closing of images by external Linux/Unix viewers. - The Python program remains opened till the user closes the image in the viewer. - I used viewer thread and monitoring thread, the monitoring thread is responsible for removal of the temporary images after user closed the image in the viewer. - Corresponding documentation is updated. - Tests are updated: Tests/test_imageshow.py, Tests/helper.py
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Please let me know if this PR has a chance of being accepted. Thanks! |
Test skip reason have been corrected.
@radarhere I corrected the test skip message. |
src/PIL/ImageShow.py
Outdated
} | ||
|
||
th = threading.Thread( | ||
target=subprocess.run, args=(command.split(),), kwargs=kwargs, name=path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've recently had to address the possibility of the file path having a space in it. This would split()
incorrectly in such a situation, yes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1) I do not think that there is a a split problem, because the file(path) is quoted, and the path always be represented as a single entity.
2) I wouldn't be concerned about users having a temporary path "foo bar baz", I think that they will be able to sort this out.
3) If you know how to break this, then I would be curious to see an example :)
@radarhere
Now I see what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution was to run the viewer command as a shell command. User input has been sanitized in get_command_ex stage. As a result (a) path problem solved; (b) possible security problem solved;
kwargs = {"shell": True, "stdout": subprocess.PIPE, "stderr": subprocess.PIPE}
th = threading.Thread(
target=subprocess.run, args=(command,), kwargs=kwargs, name=path
)
|
||
|
||
class GmDisplayViewer(UnixViewer): | ||
"""The GraphicsMagick ``gm display`` command.""" | ||
|
||
def get_command_ex(self, file, **options): | ||
executable = "gm" | ||
command = "gm display" | ||
command = f"gm display {quote(file)}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I've said before, I consider this to be a backwards incompatible change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it is more convenient to keep a viewer opening shell command at one place. The user input is sanitized and the command is ready for execution, with no further manipulations needed. The commands are readable and easy to understand .
As excuse I will add, that get_command_ex() is a helper function, and the interface of the function was preserved.
@@ -27,8 +27,7 @@ | |||
class test_image_results: | |||
@staticmethod | |||
def upload(a, b): | |||
a.show() | |||
b.show() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is hit, the tests will hang infinitely. I do not see any good reason for a.show(), b.show(): we are interested in return value of the upload function, and return value here is None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh? This is only used if there is an environment variale SHOW_ERRORS
set, which means the user is asking to be shown failed image comparisons. Removing this breaks that functionality. Also, the result of the upload function is irrelevant as there is a check in place to see if it is defined. If it isn't defined, it is never called anyway, so if this were to be removed, you would remove the whole if block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
As I mentioned above, I implemented the viewers as a UI part, and it was not an option to use them in tests.
-
To be honest, I find this part of code quite confusing:
Tests/helper.py
if os.environ.get("SHOW_ERRORS", None):
# local img.show for errors.
HAS_UPLOADER = True
class test_image_results:
@staticmethod
def upload(a, b):
return None
elif "GITHUB_ACTIONS" in os.environ:
HAS_UPLOADER = True
class test_image_results:
@staticmethod
def upload(a, b):
dir_errors = os.path.join(os.path.dirname(__file__), "errors")
os.makedirs(dir_errors, exist_ok=True)
tmpdir = tempfile.mkdtemp(dir=dir_errors)
a.save(os.path.join(tmpdir, "a.png"))
b.save(os.path.join(tmpdir, "b.png"))
return tmpdir
else:
try:
import test_image_results
HAS_UPLOADER = True
except ImportError:
pass
I've created #6039 as an alternate version of this. The primary difference is that it doesn't cause Python to hang while waiting for the image viewer to close. See what you think. |
#6045 has been merged instead. Thank you for a way for the suggestion! |
Fixes #5945, #5976
Changes proposed in this pull request: