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

Make image as part of the widget model - alternate implementation #376

Merged
merged 1 commit into from
Oct 19, 2021

Conversation

martinRenou
Copy link
Member

cc. @ianhi

@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2021

Binder 👈 Launch a binder notebook on branch martinRenou/ipympl/image_state_alternate

@martinRenou martinRenou force-pushed the image_state_alternate branch 2 times, most recently from 234f484 to 76eabcd Compare October 8, 2021 13:39
Copy link
Collaborator

@ianhi ianhi left a comment

Choose a reason for hiding this comment

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

This is great! I this this is better than #369.

Maybe the only eventual improvement (with changes in ipywidgets) would be to de-duplicate the data by being able to pull the image from the png in the mimebundle.

If you think this is done then all good to merge from me

Comment on lines 115 to +127
this.send_message('refresh');
this.send_message('send_image_mode');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this ordering matter? Why is it necessary to change it?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't matter, I didn't mean to change it I will revert

@martinRenou
Copy link
Member Author

martinRenou commented Oct 9, 2021

Though I believe this doesn't work properly (yet?). It does not work if there is no front-end. For example it doesn't work if you execute the Notebook with nbconvert, or if you generate documentation with jupyter sphinx.

@martinRenou
Copy link
Member Author

I would be in favor of #369 actually. Ideally, we would have it as a synced trait, but instead of sending the entire image every single time, we would sync the backend/frontend traits sending only image diffs.

This is an optimization that I would like to see being implemented more generally in ipywidgets. For example, when syncing a List trait it would be better to send diffs of this list instead of the entire list.

@martinRenou
Copy link
Member Author

I will explore implementing this with special serialization/deserialization. But I am not confident it can work without changing ipywidgets's core.

@martinRenou martinRenou marked this pull request as draft October 11, 2021 11:24
@ianhi
Copy link
Collaborator

ianhi commented Oct 12, 2021

I would be in favor of #369 actually. Ideally, we would have it as a synced trait, but instead of sending the entire image every single time, we would sync the backend/frontend traits sending only image diffs.

That sounds good to me. The big reason I preferred this approach was the effect on performance and not painting ourselves into a corner w.r.t true blitting.

This is an optimization that I would like to see being implemented more generally in ipywidgets. For example, when syncing a List trait it would be better to send diffs of this list instead of the entire list.

Definitely agree!

@martinRenou martinRenou force-pushed the image_state_alternate branch 3 times, most recently from 30f7dba to 52cfd53 Compare October 12, 2021 14:07
@martinRenou
Copy link
Member Author

I am not sure we can make it work with the custom serialization...

I updated this PR to save the state Python side as well, it should work with nbconvert now.

@martinRenou martinRenou marked this pull request as ready for review October 18, 2021 14:38
@martinRenou
Copy link
Member Author

The toolbar and resizable feature are now disabled if the widget is closed.

ipympl_disabled

@martinRenou
Copy link
Member Author

I wonder if the toolbar should even be visible

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.

2 participants