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

Use notebook URI as context for toolbar commands #13585

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

msujew
Copy link
Member

@msujew msujew commented Apr 10, 2024

What it does

Fixes the Restart Kernel command of the jupyter notebook extension. It expects a URI as its argument. We used to pass in the whole notebook model.

How to test

  1. Open a Jupyter notebook file
  2. Run the Restart Kernel command from the main toolbar
  3. The kernel should restart (see notification) and then work as expected

Review checklist

Reminder for reviewers

@msujew msujew added vscode issues related to VSCode compatibility notebook issues related to notebooks labels Apr 10, 2024
@msujew msujew requested a review from jonah-iden April 10, 2024 13:08
@jonah-iden
Copy link
Contributor

jonah-iden commented Apr 10, 2024

This is actually what the ArgumentProcessor in notebooks.ts/notebooks-main.ts was for. Wouldn't that be a nicer solution than or does that not work because it needs the uri and not the document

@msujew
Copy link
Member Author

msujew commented Apr 10, 2024

This is actually what the ArgumentProcessor in notebooks.ts/notebooks-main.ts was for. Wouldn't that be a nicer solution than or does that not work because it needs the uri and not the document

@jonah-iden Well, the ArgumentProcessor correctly converted the NotebookModel into a theia.NotebookDocument on the plugin host. However, this isn't the type that's expected by commands registered to the interactive/toolbar menu. The command here expects a vscode.Uri, not vscode.NotebookDocument.

Copy link
Contributor

@jonah-iden jonah-iden left a comment

Choose a reason for hiding this comment

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

ok then its as i thought. Approved

@msujew msujew merged commit e99e42b into master Apr 10, 2024
14 checks passed
@msujew msujew deleted the msujew/fix-kernel-restart branch April 10, 2024 19:13
@github-actions github-actions bot added this to the 1.49.0 milestone Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notebook issues related to notebooks vscode issues related to VSCode compatibility
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants