-
Notifications
You must be signed in to change notification settings - Fork 949
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
Update widget menu items in classic Notebook #2012
Conversation
CC @ricklupton |
From in-person discussion: The widget manager should listen to the
|
The views should already be deleted when they are removed from the page. We don't want to close the widget (i.e., the model) since we may display it again. |
Clearing all output does not generate an event, so that would need to be added to the notebook first: |
What about for clearing all outputs and restarting the kernel? Is this a separate event? |
Restart and clear all also doesn't have an event: https://github.com/jupyter/notebook/blob/2aac713937c0487dd2193c85a69f02e9556421fe/notebook/static/notebook/js/notebook.js#L2290 |
So...moving forward with this - is this PR enough of an improvement to merge, or does it muddle things further and should be reconsidered before merging? |
Actually, let's take a step back and see if we also want to implement #1632 here too. It looks like there are appropriate hooks in the classic notebook: There is an event that the notebook triggers before saving - perhaps in the widgets menu we have a setting (i.e., menu item with toggle checkmark) that automatically saves the widget state when you save the notebook. |
(I'm not sure if the jlab notebook has such hooks, but we don't have a widget menu in jlab anyway) |
Okay everyone, how about this?
The three Save options make sure that one is selected between them. The setting is saved with the notebook. Saving all widgets saves all widget state with the notebook (including possibly widgets from previous kernels or state that was loaded from the notebook). Toggling this option would not also save the notebook immediately, but would just set up things so that when the notebook is saved, the state is saved. |
Hi Jason, thanks for keeping on with this! I like the idea of having a toggle to save widgets along with the notebook, but I'm not sure about the live/all distinction. Does "live" mean "model connected to a kernel" or "model connected to a visible view"? I'm assuming the first in what I write below. In my case I think I would have to frequently toggle between the two.
So in this case it would be best to always leave it as "save all". But maybe there are other uses when you do want to "save live" every time you open the notebook? |
I think if your typical workflow is to open a notebook from scratch, run the cells, and save, the "save live" version is what you would usually want - it erases the previous state, and you don't have to remember to run "clear all". |
When opening a notebook with stored widgets, it recreates the widgets (i.e. opens them in the kernel), so these will be live. One could possibly imagine a race condition here, where the widgets are slow to re-open, and save request happens quickly. We would have to make sure that this was handled correctly (wait for promises to resolve, then save?) or warned about. |
It sounds like we need to continue the discussion here, so bumping to 7.3. |
Since it hasn't been explicitly made clear here, I'd like to add the desire for a default preference setting for whether or not to save the workbooks. Motivation: I generate a new collection of workbooks with each simulation experiment I do, and it's very useful to be able to look up the the results of analysis within the workbook without having to rerun the code. Currently I am often forgetting to press the save widget state menu item. |
@jasongrout do you want to get this in for 7.5? |
No. |
Since this hasn't been worked on in 2 years, I'm closing this. If someone wants to push it forward, please add a note here and open a new PR. |
Fixes #1866
This changes the menu items to: