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

Remove value from the Image repr #2111

Merged
merged 4 commits into from
Jul 16, 2018

Conversation

mwcraig
Copy link
Contributor

@mwcraig mwcraig commented Jul 2, 2018

Right now Image.__repr__ includes the value of the image...which bring my browser to a crashing halt for large images.

This is likely not the ideal way to handle this; I'd be happy to change widget.__repr__ to truncate all large values (over maybe 10k characters?) in the repr instead.

@vidartf
Copy link
Member

vidartf commented Jul 2, 2018

One thing we did e.g. in nbdime and nbval was to say something like <base64 string, hash=ea628...>. You could do something similar (but maybe with a cut-off value of only around a 100 characters).

@SylvainCorlay
Copy link
Member

This looks good to me.

This is based on @vidartf's suggestion, and includes a bit of copy+paste
from nbdime
@mwcraig
Copy link
Contributor Author

mwcraig commented Jul 5, 2018

I modified this to always present the value (truncated along the lines that @vidartf suggested). A more general solution in widgets._repr_from_keys would probably be ideal, but for now this presents a value for Image but won't kill your browser.

# Truncate the value in the repr, since it will
# typically be very, very large.

def hash_string(s):
Copy link
Member

Choose a reason for hiding this comment

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

Let's not define a new function every time we do a repr.

signature = []
sig_value = str(self.value)
if len(str(self.value)) > 100:
sig_value = '<base64, hash={}...>'.format(hash_string(sig_value)[:16])
Copy link
Member

Choose a reason for hiding this comment

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

We don't know that it is base64, do we? It might be a very long URL.

I like your previous approach of just truncating. Or perhaps we should just enhance the existing repr keys mechanism to let us return a repr form for any key, rather than just whether to include the key or not.

@mwcraig
Copy link
Contributor Author

mwcraig commented Jul 7, 2018

This just truncates long Image.value (added a couple tests too).

@jasongrout jasongrout added this to the 7.3 milestone Jul 16, 2018
@jasongrout
Copy link
Member

Looks great to me, and works well when I test it too. Thanks!

@jasongrout jasongrout merged commit 884e171 into jupyter-widgets:master Jul 16, 2018
@lock lock bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label May 21, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants