Skip to content

Conversation

@radarhere
Copy link
Member

This code in Image.py implies that _show always uses xv.

Pillow/src/PIL/Image.py

Lines 3143 to 3151 in 41a7521

def _show(image, **options):
# override me, as necessary
_showxv(image, **options)
def _showxv(image, title=None, **options):
from . import ImageShow
ImageShow.show(image, title, **options)

This PR rearranges the code into the following to avoid that implication.

def _show(image, **options):
    # override me, as necessary
    from . import ImageShow

    ImageShow.show(image, **options)


_showxv = _show

title still has a default value of None because

def show(image, title=None, **options):

@radarhere radarhere changed the title Avoid implying that _show always using xv Avoid implying that _show always uses xv Jun 14, 2020
@nulano
Copy link
Contributor

nulano commented Jun 14, 2020

This change would mean that users replacing Image._showxv with custom code would no longer have an effect. I'm not sure how common that is, just wanted to note this change.

@radarhere
Copy link
Member Author

That's true. As you seem to be aware though. I really don't know why you would do that. If you were going to customise this functionality, _show is the more likely target than _showxv. If someone is replacing Pillow functions with their own code though, unless it is common, my instinct would be that it's on them to keep their code working.

This is a minor cleanup, so it can be dropped if there is debate over it.

@nulano
Copy link
Contributor

nulano commented Jun 14, 2020

I'm just commenting on the comment above the function: # Simple display support. User code may override this.

I think this change is fine, perhaps it should just be noted somewhere. Also note that this makes it into an unused internal function, although it seems to be used quite often from other projects, so I wonder whether it should also be deprecated.

@radarhere
Copy link
Member Author

Oh. I didn't actually notice that comment. I only saw # override me, as necessary.

@hugovk
Copy link
Member

hugovk commented Jun 21, 2020

Both _show and _showxv begin with underscores indicating they're internal and private, but # Simple display support. User code may override this. suggests otherwise!

A quick search has "9 available code results" for Image._showxv:

Compared with 16,074 for Image._show:

Idea: shall we put a deprecation warning on _showxv to remove it in the future, and remove that comment?

@radarhere radarhere closed this Jun 21, 2020
@radarhere radarhere deleted the showxv branch June 21, 2020 11:57
@radarhere radarhere mentioned this pull request Jun 21, 2020
@radarhere
Copy link
Member Author

Sure. See #4713 and #4714

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.

3 participants