Unix viewers #6005

alv2017 · 2022-01-31T22:42:32Z

Changes proposed in this pull request:

The major changes affected UnixViewer class. I used Python threads in order to control image opening by external Linux/Unix viewers. I used a viewer thread and a viewer monitoring thread that monitors the image viewing process and it is also responsible for removal of the temporary images.
I also changed how the way **get_command_ex(self, file, options): method works: the command returned from the method is the exact command opening the image file provided as an argument. This is in order to keep the CL stuff as simple as possible.
I added additional method **get_executable(self, file, options): that returns an executable of the specific Unix Viewer.
Set Viewer class property format to "PNG", and remove this property from the Viewer child classes.
Set Viewer class property options to {"compress_level": 1}, and remove this property from the Viewer child classes.
Move XDGViewer to the bottom of the viewers list. This viewer will be utilized only in case if all the previous viewers failed.
Remove, skip or timeout tests that are attempting to open actual image.

radarhere · 2022-02-01T04:34:45Z

src/PIL/ImageShow.py

        # note: xv is pretty outdated.  most modern systems have
        # imagemagick's display command instead.
-        command = executable = "xv"
-        if title:
-            command += f" -name {quote(title)}"


Could you explain why you think xv doesn't need a title argument?

The -name option: sets string displayed in image window's title bar. I thought that this option is not particularly useful and removed it. Now I realize that this had an effect on documentation, including the ImageShow.show() method.

radarhere · 2022-02-01T04:51:53Z

src/PIL/ImageShow.py

-    def get_command_ex(self, file, title=None, **options):
-        command = executable = "display"
-        if title:
-            command += f" -name {quote(title)}"


Could you explain why you think display doesn't need a title argument?

Same thing as for xv, on top of it I think that the option is used incorrectly here. Please see the doc for X Windows: https://linux.die.net/man/7/x. I think that the -title option would be more relevant here.

radarhere · 2022-02-01T04:54:45Z

The major changes affected UnixViewer class. I used Python threads in order to control image opening by external Linux/Unix viewers. I used a viewer thread and a viewer monitoring thread that monitors the image viewing process and it is also responsible for removal of the temporary images.

For clarity - this is motivated by wanting to show multiple images at a time, and to navigate between them after 20 seconds.

Minor note - you understand that our CI jobs are now running indefinitely? :p

I also changed how the way **get_command_ex(self, file, options): method works: the command returned from the method is the exact command opening the image file provided as an argument. This is in order to keep the CL stuff as simple as possible.

Pillow values backwards compatibility, and this would be a backwards incompatible change. Could you provide some more detail about why this is needed?

alv2017 · 2022-02-01T09:30:38Z

Minor note: This is motivated by the idea that the Python program doesn't terminate till the user closes the opened image in the viewer. In short the termination of the thread requires a manual user action. Unfortunately, what works for the user, doesn't work for CI. :) I would be curious to see failing CI jobs, because all the unit tests that I run on my machine execute successfully.

I also think that the user has to have an explicit control of the processes created by the program.

get_command_ex() method: I do not agree that there is a backward compatibility issue here: the interface is preserved:
we take object, file and options as input and we return command and executable as output. I think that this is a cleaner and more consistent approach.

alv2017 · 2022-02-01T09:37:59Z

Hi, Andrew, thanks for the feedback, and please let me know

Do you agree/disagree with the taken approach: viewers have to be managed by the Python program that started them.
What would be your solution?

Segfaults are annoying to debug.

This reverts commit 6565d13.

This patch enables ARM64 as a new platform for Windows. Platform query and documentation is updated accordingly.

In order to enable win-arm64, VS2019 should be used, while other platforms should work with newer version as well. Tested on x64-win10.

Co-authored-by: Hugo van Kemenade <[email protected]>

- Image.show() method is no longer supporting **title** option, corresponding docs were updated; - XDGViewer is moved to the end of the viewers queue, this viewer will be utilized when all the previous viewers failed to open the image; - Documentation of xvViewer has been updated; - The tests that were opening images in viewers were disabled. Reason: due to implementation a program or a test will not terminate till the image in the viewer is explicitly closed.

radarhere · 2022-02-03T02:22:29Z

Yes, #6010 conflicted with this.

You asked for my thoughts, so...

I still think you're breaking backwards compatibility on get_command_ex, as you're changing the purpose of the first part of the tuple that is returned.
I don't mind get_executable, but I'm also not clear on the need for it.
My only weird feeling about moving format and options up to Viewer is that IPythonViewer doesn't use them, but that's minor. Fyi - before Changed WindowsViewer format to PNG #4080, the format for WindowsViewer was BMP.
I'm not inclined to rearrange XDGViewer. The user from Added ImageShow support for xdg-open #5897 thought it was a good idea to have it first, so that the preferred application is opened. A user from XDGViewer and GmDisplayViewer fail to show images #5976 thinks that the viewer should be lightweight instead. I don't personally favour reverting a change if there's not unity on what to do.
I haven't checked out title instead of name for display, but I don't think there's a compelling reason here to drop the title argument completely.
If there is a way to keep the temporary images around until the viewer is done with them, without pausing Python execution for the user, yes, that sounds interesting. You may have to adjust this in light of In show_file, use os.remove to remove temporary images #6010. I haven't experimented much, but do you need to jump to threading, or can the same effect be achieved with just subprocess?

As something of an aside, in the documentation, it is mentioned that show() is intended for debugging.

https://pillow.readthedocs.io/en/stable/handbook/overview.html#image-display

For debugging, there’s also a show() method which saves an image to disk, and calls an external display utility.

You're talking about opening multiple images at once, keeping them open longer than 20 seconds and swapping between them - sounds to me like you're trying to use this for more than just debugging? Nothing wrong with that, just a thought.

alv2017 · 2022-02-03T08:20:38Z

@radarhere thank you for your reply!
The conflict occurred because I rebased incorrectly.
I will take into consideration your concerns and submit a new PR for the viewers.

radarhere · 2022-02-07T02:17:06Z

The next version of this PR is #6021

radarhere · 2022-02-21T09:59:06Z

The final form of this was #6045

radarhere reviewed Feb 1, 2022

View reviewed changes

radarhere and others added 23 commits February 3, 2022 01:36

Updated raqm to 0.8.0

8baeee3

TST: Parametrize numpy roundtrip to find failing case

255b143

Segfaults are annoying to debug.

Updated lcms2 to 2.13

a5642f0

Switched from windows-2019 to windows-latest

a83e8d1

Updated raqm to 0.9.0

9b1857f

Revert "detect FreeType / HarfBuzz features"

7640e3b

This reverts commit 6565d13.

Fix lcms2 URL

16c0d48

Update harfbuzz to 3.3.0

eb05885

Update harfbuzz to 3.3.1

4d8d871

Remove EOL CentOS 8

0fa00f9

Windows: Enable ARM64 for MSVC

eada1c9

This patch enables ARM64 as a new platform for Windows. Platform query and documentation is updated accordingly.

lcms2: Update to VS2019

ba22622

In order to enable win-arm64, VS2019 should be used, while other platforms should work with newer version as well. Tested on x64-win10.

Update CHANGES.rst [ci skip]

8203621

Improved consistency of returning an image access object from load()

6c90ee1

Restrict builtins within lambdas for ImageMath.eval

9675673

In show_file, use os.remove to remove temporary images

63d3179

Updated formatting

0efd532

Co-authored-by: Hugo van Kemenade <[email protected]>

Added delay after opening image with xdg-open

b26313d

Update CHANGES.rst [ci skip]

1a60a32

src/PIL/ImageShow.py: issue with UnixViewers solved: #5976

94ef447

Code formatting applied

81d2bec

Test skip reason has been corrected.

92ccf42

alv2017 marked this pull request as draft February 2, 2022 23:55

alv2017 closed this Feb 3, 2022

alv2017 deleted the unix_viewers branch February 3, 2022 00:44

This was referenced Feb 9, 2022

Wait until executable has finished to remove file on Unix #6039

Closed

Do not automatically remove temporary ImageShow files on Unix #6045

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unix viewers #6005

Unix viewers #6005

alv2017 commented Jan 31, 2022 •

edited

Loading

radarhere Feb 1, 2022

alv2017 Feb 1, 2022

radarhere Feb 1, 2022

alv2017 Feb 1, 2022

radarhere commented Feb 1, 2022

alv2017 commented Feb 1, 2022

alv2017 commented Feb 1, 2022

radarhere commented Feb 3, 2022

alv2017 commented Feb 3, 2022

radarhere commented Feb 7, 2022

radarhere commented Feb 21, 2022

Unix viewers #6005

Unix viewers #6005

Conversation

alv2017 commented Jan 31, 2022 • edited Loading

radarhere Feb 1, 2022

Choose a reason for hiding this comment

alv2017 Feb 1, 2022

Choose a reason for hiding this comment

radarhere Feb 1, 2022

Choose a reason for hiding this comment

alv2017 Feb 1, 2022

Choose a reason for hiding this comment

radarhere commented Feb 1, 2022

alv2017 commented Feb 1, 2022

alv2017 commented Feb 1, 2022

radarhere commented Feb 3, 2022

alv2017 commented Feb 3, 2022

radarhere commented Feb 7, 2022

radarhere commented Feb 21, 2022

alv2017 commented Jan 31, 2022 •

edited

Loading