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

Do not automatically remove temporary ImageShow files on Unix #6045

Merged
merged 6 commits into from Feb 19, 2022

Conversation

radarhere
Copy link
Member

@radarhere radarhere commented Feb 11, 2022

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. This PR updates the return values to match 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. Given that there have been requests for multiple images to be shown at once and to be viewed after 20s, a possible solution to these requests is to not manually remove the temporary ImageShow files on Unix.

Copy link

@alv2017 alv2017 left a comment

Choose a reason for hiding this comment

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

@radarhere

  1. I have tested the functionality (Ubuntu 20.04): show() function worked well for all the viewers except GmDisplayViewer. In case of single file the image was sometimes shown and sometimes not shown. For multiple images, in my case 3 images, only two images showed up. I do not have any good explanation for such behavior of GmDisplayViewer...

  2. Good news: the temporary image files get deleted on reboot (Ubuntu 20.04)

  3. Code:
    (a) I think there is a backwards compatibility issue with viewers interface, I'm talking here about get_command() function. This function is redundant for Mac and Unix viewers. There is also a consistency issue if one compares Windows viewer implementation vs Mac and Unix.
    (b) I would say that DRY principle is violated for Unix viewers image_show() method.
    (c) Unix viewers get_command_ex() method became redundant, and it doesn't reflect the actually implemented functionality.
    (d) get_command() method does not reflect actually implemented functionality.
    Summary: the key thing here is to make a decision about viewers interface: either implement the new approach to all the viewers, or stick with the old interface, and apply it to all the viewers.

@radarhere
Copy link
Member Author

  1. a) When you say there is a backwards compatibility issue with get_command(), the only change I've made to get_command() is swapping "title" for "name" in DisplayViewer. Do you mean that get_command() should not remove the file on Unix? I've added a commit for that.
    Why is the function redundant for Mac and Unix viewers?
    What is the consistency issue?
    b) Did you mean show_image() on Viewer?
    c) Could you elaborate on this?
    d) Does my latest commit address this?

@alv2017
Copy link

alv2017 commented Feb 14, 2022

@radarhere

In the Viewer class viewers interface is defined. As per this definition the get_command() is expected to return specific viewer's display command, the return value of the get_command() function is expected to be used by show_file() method.
In my opinion, current PR violates this logic.

Question: What is the purpose of get_command() and get_command_ex() in this PR?

@radarhere
Copy link
Member Author

I am confused by your comment.

I have proposed minimal changes to get_command and get_command_ex

  • I swapped "title" for "name" in DisplayViewer as per your suggestion
  • I removed the rm command from get_command in UnixViewer

Are you just having a conversation about ImageShow in general, rather than reviewing my changes here? It's ok if you are, I just don't feel like that's clear.

If you're asking why get_command() and get_command_ex() exist at all, they have been present since PIL, so we did not add them in. I personally find it hard to think of a use case for them, but as they have some functionality and don't cause problems, I'm more inclined to leave them as is. If you think they should be removed, please create a separate PR that adds a deprecation warning to those methods (like #5959). If that change is accepted, then they can be removed in a future Pillow release.

@alvtech2017
Copy link

@radarhere

  1. I just expressed concerns about the part of the code that I have some doubts. (namely repetitive show_file methods, and seemingly redundant get_command() method.

  2. Please note, that I overlooked the changes you did on Feb 2. This might be one of the reasons my comment seems confusing to you. :)

  3. In general I'm Ok with the changes you made, and yes you are right, it will be easier to explain what I mean by submitting a pull request.

@alv2017
Copy link

alv2017 commented Feb 14, 2022

Sorry, I forgot to switch to my regular account 😄

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

  1. ImageShow.show() does not return True and False like the docstring states, it returns 0 and 1.
>>> True == 1
True
>>> False == 0
True
>>>

;)

Returning True or False is nicer. 👍


Let's include in the release notes too the fact they're no longer automatically* removed, so people know it's their job to clear it out if for example they show lots of images and don't reboot often.

(* Shall we rename "manually" -> "automatically" in the PR title?)

@radarhere radarhere changed the title Do not manually remove temporary ImageShow files on Unix Do not automatically remove temporary ImageShow files on Unix Feb 18, 2022
docs/releasenotes/9.1.0.rst Outdated Show resolved Hide resolved
@hugovk hugovk merged commit bfa6da6 into python-pillow:main Feb 19, 2022
@hugovk
Copy link
Member

hugovk commented Feb 19, 2022

Thanks!

@radarhere radarhere deleted the imageshow branch February 19, 2022 11:35
@alv2017
Copy link

alv2017 commented Feb 19, 2022

I suggest to close those issues as well: #5976, #5945

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

5 participants