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 INotebookTracker optional in the manager plugin #3033

Merged
merged 1 commit into from
Feb 17, 2021

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented Dec 9, 2020

This should help make the plugin more flexible.

For example when it is loaded as a federated extension on a page that doesn't provide the @jupyterlab/notebook-extension:tracker.

@jasongrout
Copy link
Member

Just curious, how are you registering the widget manager in the case where you do not have a tracker?

@vidartf
Copy link
Member

vidartf commented Jan 4, 2021

@jtpio The change in itself is ok, but as Jason points out, there probably needs to be additional changes to allow for providing the context. To make it more reusable for other things, it would probably also make sense to make changes to the manager to separate:

  • the code that only need to know about the kernel/session connection
  • the code that needs to know about the notebook model (for loading/saving state, marking the model as dirty on changes)

@jtpio
Copy link
Member Author

jtpio commented Jan 6, 2021

Just curious, how are you registering the widget manager in the case where you do not have a tracker?

For now the main motivation is when the jupyterlab_widgets extension is loaded as federated on the page, in a lab-based app that does not necessarily have the notebook tracker (for example the tree page in JupyterLab Classic). So having the token optional would make it more like a no-op instead of logging an error in the dev tools console.

there probably needs to be additional changes to allow for providing the context. To make it more reusable for other things, it would probably also make sense to make changes to the manager to separate

Yes absolutely. It sounds like it would also make it easier to provide widget support for code consoles: #3004
Or even arbitrary jupyterlab widgets backed by a kernel.

@jasongrout
Copy link
Member

Merging since this doesn't hurt anything, and helps slightly in the classic case (or other cases where there is not a notebook tracker).

@jasongrout jasongrout merged commit 08093b0 into jupyter-widgets:7.x Feb 17, 2021
@jasongrout jasongrout modified the milestones: 8.0, Minor release Feb 17, 2021
@jasongrout
Copy link
Member

I'll note that master (8.0) does not have this issue, I think courtesy of #2528

@jtpio jtpio deleted the optional-tracker branch February 17, 2021 08:27
@vidartf
Copy link
Member

vidartf commented Feb 19, 2021

Also note that I had completely forgotten that the work to split the kernel manager to not need document context was already done (by me, even!) in master:

export class KernelWidgetManager extends LabWidgetManager {

@jtpio
Copy link
Member Author

jtpio commented Feb 19, 2021

Ah nice!

We probably didn't realize since this PR was against the 7.x branch.

@jasongrout jasongrout modified the milestones: Minor release, 7.x Dec 22, 2021
@jasongrout jasongrout modified the milestones: 7.x, 7.7 Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants