-
Notifications
You must be signed in to change notification settings - Fork 10
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
Snapshot functionality #37
Conversation
I got this idea while musing about reviving the ipynb_static backend in vispy. But this change makes that backend type unnecessary. |
What is the series of events here? Does the sphinx notebook extension run the notebook or are you saving the examples with the output still there? |
In IPython an object can provide multiple representations. The "application/vnd.jupyter.widget-view+json" mime type sets up the connection with JS in a live session, but is not visible at all when the notebook is shown offline. This PR simply adds a "text/html" representation, which is ignored in a live session, but is shown in a static setting. |
Very cool. I had no idea. |
Hmm, it's slightly more complex. This idea does not work for nbconvert because it still prioritizes the "application/vnd.jupyter.widget-view+json" over the "text/html". Consequently it does work as I intended on Github's notebook renderer, but not for e.g. nbSphinx and nbviewer. Looking into another idea now. |
This is ready for review. This became a bit different from how it started. I hope everything is clear from the added docs and comments. Here's also a screenshot from what an example in our docs will look like. The snapshots are just images (not interactive of course). In the live notebook, the "initial snapshot" is not visible, but the interactive widget is. |
TBH that snapshot method was a bit of a whim, but I think it can be a really nice feature. E.g. create a notebook with 3D data, manually position the camera to show something specific, then snapshot it, save the notebook, and anyone who views it can see what you meant to show :) |
jupyter_rfb/_utils.py
Outdated
return True # declare that we handled the exception | ||
|
||
|
||
class Snapshot: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this differ from the Image
class that I'm 99% sure exists? Could this class be replaced or subclass from that?
Here it is: https://ipywidgets.readthedocs.io/en/latest/examples/Widget%20List.html#Image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it inherit from IPython.display.DisplayObject
instead. We need to add the css class
, and also have this little title node, so it felt too different from an Image
. Anyway, by inheriting from DisplayObject we can make the docs leaner and the API cleaner :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me. I did want to point out that my comment about making the output context optional (off by default?) and dashboards is that it may come as a surprise to people with Jupyter experience that errors are shown. Even worse, they may not want errors to be shown if they are building dashboards like with Voila or some of the other options which make live website versions of the widgets contained in a notebook. I honestly don't remember all the available options.
Thanks for clarifying. I do hope that everyone (including people with Jupyter experience) expect errors to be shown by default. Jupyter does show errors by default, but in our situation this was inconsistent; depending on certain conditions, the errors could be or could not be shown. We fixed that inconsistency with the capturing at the cost of always showing errors in the output. If we would make it optional to apply the outputcontext, we'd be back to inconsistent behavior. So if we'd go forward with a way to hide errors for cases like dashboards, we should silence all errors. Probably by capturing them like we do now, but then simply not showing them. |
Sounds good. Feel free to merge. |
f = io.StringIO() | ||
kwargs.pop("file", None) | ||
_original_print(*args, file=f, flush=True, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this should capture from any print
, regardless of file
argument? Unless it's sys.stdout
or sys.stderr
, it's going to break things in pretty weird ways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good point! It should probably check the file argument and only consume if it looks like standard output ... I will make an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes that static renderings of the notebook (by nbsphinx, Github, etc.) include a snapshot of the canvas, which makes all examples quite a bit nicer to read.
The cost is that an intitial draw is requested when a canvas is first shown. Well worth it, I think.
Todo: