Labmanager refactor #2532
Labmanager refactor #2532
Conversation
|
For reference, here is the diff of this PR onto #2528: vidartf/ipywidgets@lab-optionals...vidartf:labmanager-refactor |
|
Needs a rebase (and at least some of this is incorporated in other PRs like #2707) |
Extract parts of the widget manager into an abstract base class, and then export a new widget manager that does not rely on the document context. Instead, it takes a kernel as an argument to the constructor.
vidartf
force-pushed
the
labmanager-refactor
branch
from
729aaf9 to
9d999d7
Compare
|
Rebased, but will need a careful review, as I'm not sure I caught all the nuances with the new services API. |
…e deregistered easily.
…s to connected. In particular, there was a subtle piece of logic for not restoring notebook widgets when a kernel reconnected.
|
@vidartf, I took a look at this today. I pushed a few commits cleaning up a few things. If those look good to you, then I think we can merge this. |
Right now in JupyterLab, if you create and display a widget in a notebook, then refresh the page, the widget is replaced with a “could not find model” error message. This is because the initial kernel widget restore is called before the widget manager has a kernel, so the kernel restore silently fails and the widget manager thinks it has restored things. Then when we try to render the widgets, we give up too soon, thinking that we have already restored any existing widget state. This change makes it so that the widget manager recognizes that the initial restore attempt did happen (even if it didn’t actually get anything from the kernel because the kernel was not set), but the manager also realizes that the kernel restore did *not* happen. Then when the kernel is set, the kernel connected signal triggers another restore, which can then complete the widget manager restoration.
… natural Instead of just an initial state restore, this makes it so that we never ask for a kernel restore when another kernel restore is already in progress. A subcase of this is the initial restore race logic from the previous commit.
|
I also fixed a race condition that has been bothering us since updating to JLab 2.0. CC @wolfv. |
|
@vidartf, do you want to take a look at this again? |
| this._initialRestoredStatus = true; | ||
| this._restored.emit(); | ||
| } catch (err) { | ||
| // Do nothing |
There was a problem hiding this comment.
This shouldn't really happen for the KernelWidgetManager, but I agree with being defensive.
There was a problem hiding this comment.
Do you mean there won't be an error in loading from kernel in the widget manager? We aren't throwing one explicitly, but of course there could be one, and that would leave us in a weird state for the flag, so I think it's important to be defensive.
Probably even better semantically to have it in a finally statement, though.
There was a problem hiding this comment.
Or maybe even better to move the flag setting into _loadFromKernel...
| await this._loadFromKernel(); | ||
| } finally { | ||
| this._kernelRestoreInProgress = false; | ||
| } |
There was a problem hiding this comment.
Would it make more sense to put the setting/unsetting of this flag inside the method?
There was a problem hiding this comment.
Can you be more explicit about what you have in mind?
I assume you are talking about the last two commits that fix a serious bug. If I would have made them in a different PR, I would have had to redo them anyway, so it only made sense to make them in the context of this PR. Thanks for the seeing the expediency. |
lock
bot
added
the
resolved-locked
This PR builds on top of #2528.
What does this do?
Extract parts of the widget manager into an abstract base class, and then export a new widget manager that does not rely on the document context. Instead, it takes a kernel as an argument to the constructor.
What is the purpose?
The goal of this PR is to make it easier for others to reuse widgets in a setting outside of Jupyterlab, (but while still using Jupyterlab components as libraries).
Does this really belong in this package?
Ideally, the base and kernel-based manager could live in a new, separate package, but I don't want to increase the maintenance burden of the repo/releases any more, so I think it would probably be better to later on optimize the current package for tree-shaking.