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.

Sign up for GitHub

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

Support diagnostics panel in Notebook v7+ #981

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Support diagnostics panel in Notebook v7+ #981

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 17 additions & 10 deletions packages/jupyterlab-lsp/src/features/diagnostics/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { INotebookShell } from '@jupyter-notebook/application';
import { INotebookShell, NotebookApp } from '@jupyter-notebook/application';
import {
ILayoutRestorer,
JupyterFrontEnd,
Expand Down Expand Up @@ -99,20 +99,27 @@ export const DIAGNOSTICS_PLUGIN: JupyterFrontEndPlugin<IDiagnosticsFeature> = {
const panelWidget = diagnosticsPanel.widget;
if (!panelWidget.isAttached) {
void tracker.add(panelWidget);
app.shell.add(panelWidget, 'main', {
ref: ref,
mode: 'split-bottom'
});
if (
typeof NotebookApp !== 'undefined' &&
app instanceof NotebookApp
) {
app.shell.add(panelWidget, 'right');
await app.commands.execute('application:toggle-panel', {
id: panelWidget.id,
side: 'right'
});
Comment on lines +106 to +110
Copy link
Member Author

Choose a reason for hiding this comment

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

This does not seem to be enough to add a Notebook panel from extension 😿. I am yet to investigate further, but in the meantime, @jtpio do you have thoughts on this one (given our other conversation on lab-notebook shell interfaces divergence)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm...

app instanceof NotebookApp

This is probably not going to work, long term, as it means the notebook packages will be bundled along with the already-large (and duplicated) jupyterlab-lsp assets, and could very well return a false negative if webpack is feeling grumpy.

Making it a sharedPackage might improve things, but would really need to be in an entirely separate plugin file with a naked (not type) import...

Further, this is often not enough information, as one still has to consult the PageConfig.getOption('notebookPage'), to determine whether a given notebook page should not be able to get a panel (e.g. tree, terminal)... it would actually be lovely if this list of documented string constants (not as a TypeScript enum, for the same reason above) were hoisted as a dedicate package.

Alas, we kinda missed the boat on changing the IFrontendShell interface to provide a dumb, string-capability-based model on the major version revs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thus far, this pattern has worked for stuff I've been tinkering with:

https://github.com/deathbeds/jupyterlab-fonts/blob/f3fa93bc2803e23139f6f131127e6e15ed71d1f6/packages/jupyterlab-fonts/src/plugin.ts#L40

} else {
app.shell.add(panelWidget, 'main', {
ref: ref,
mode: 'split-bottom'
});
}
}
app.shell.activateById(panelWidget.id);
void tracker.save(panelWidget);
},
label: trans.__('Show diagnostics panel'),
icon: diagnosticsIcon,
isEnabled: () => {
// TODO notebook
return app.name != 'JupyterLab Classic';
}
icon: diagnosticsIcon
});

// add to menus
Expand Down