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

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

Closed
wants to merge 4 commits into from

Conversation

radarhere
Copy link
Member

Helps #5945 and #5976. Alternative to #6021

Three changes here.

  1. As pointed out in Unix Viewers - 2nd attempt  #6021, ImageShow.show() does not return True and False like the docstring states, it returns 0 and 1. So this PR updates the docstring.
  2. As suggested out in Unix viewers #6005 (comment), display -title looks preferable to display -name.

Screen Shot 2022-02-09 at 4 17 44 pm
Screen Shot 2022-02-09 at 4 18 04 pm

  1. Rather than waiting an arbitrary 20 seconds on Unix, wait until the application has finished showing the file to remove it. Unix Viewers - 2nd attempt  #6021 also does this, but in that version, Python script hangs until the application is closed. This version does not.
    Since xdg-open does not wait until the application is closed, this PR instead uses xdg-mime to determine what application would be selected by xdg-open, and executes it directly.
    For the record - I'm also not opposed to the suggested alternative of just abandoning the idea of manually deleting temporary files and leaving it up to the operating system.

@nulano
Copy link
Contributor

nulano commented Feb 9, 2022

ImageShow.show() does not return True and False like the docstring states, it returns 0 and 1. So this PR updates the docstring.

Would it be a breaking change to instead change the returned value? I think it is quite unlikely that any code depends on the exact type of the returned value, since bool is a subclass of int:

>>> False == 0
True
>>> True == 1
True
>>> True + True
2
>>> type(True)
<class 'bool'>
>>> issubclass(bool, int)
True

@radarhere
Copy link
Member Author

You make a good argument. Ok, I've pushed that change.

@alv2017
Copy link

alv2017 commented Feb 9, 2022

@radarhere I'm upvoting the idea of leaving file deletion to OS. It seems to be a valid solution.

https://www.fosslinux.com/41739/linux-tmp-directory-everything-you-need-to-know.htm

@radarhere
Copy link
Member Author

@alv2017 while we may end up choosing that option, I'm still interested to know - do you have any objections to this PR as it currently stands?

@alv2017
Copy link

alv2017 commented Feb 9, 2022

@radarhere, I tested the changes manually on my machine, and

  • GmDisplayViewer fails on single and multiple images
  • XDGViewer fails when opening multiple images

@radarhere
Copy link
Member Author

Here is my recording of it working with GmDisplayViewer for me.

gm.mov

When you say that it fails, could you give some more detail? When you say that XDGViewer fails, what image viewer is it opening?

@alv2017
Copy link

alv2017 commented Feb 11, 2022

@radarhere

  1. In my case with GmDisplayViewer images get deleted before being displayed. This is happening for single and multiple images.

  2. In my case XDGViewer calls EolViewer. What is happening? -> only the first image shows up, the rest of the images get removed before being displayed. This is happening only for multiple images, for a single image everything works fine.

Script to test multiple images:

import os
from PIL import Image
from PIL.ImageShow import DisplayViewer, GmDisplayViewer, EogViewer, XDGViewer


if __name__ == "__main__":
    viewer = XDGViewer()
    img_dir = "/image/directory"
    img_files = ["img1.png", "img2.png", "img3.png"]

    for img_file in img_files:
        img_location = os.path.join(img_dir, img_file)
        with Image.open(img_location) as im:
            viewer.show(im)

@radarhere
Copy link
Member Author

radarhere commented Feb 11, 2022

Ok, the premise of my change - that commands like eog stay open until the image viewer closed - turns out to only be valid if the image viewer is being opened for the first time. Otherwise, the command finishes immediately. An -n argument could be added to ensure that a new instance is opened for eog, but adding that specifically, when any image viewer could have been selected by the user, seems fragile.

I've created PR #6045 instead.

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.

None yet

3 participants