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

Interrupt kernel button on interactive window toolbar should be disabled when kernel is not busy #7488

Merged
merged 2 commits into from
Sep 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions news/2 Fixes/7269.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Interrupt kernel button on interactive window toolbar should be disabled when kernel is not busy.
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,7 @@
"light": "resources/light/interrupt.svg",
"dark": "resources/dark/interrupt.svg"
},
"enablement": "isWorkspaceTrusted"
"enablement": "isWorkspaceTrusted && jupyter.interactive.canInterruptNotebookKernel"
},
{
"command": "jupyter.restartkernel",
Expand All @@ -559,7 +559,7 @@
"light": "resources/light/restart-kernel.svg",
"dark": "resources/dark/restart-kernel.svg"
},
"enablement": "isWorkspaceTrusted"
"enablement": "isWorkspaceTrusted && jupyter.interactive.canRestartNotebookKernel"
},
{
"command": "jupyter.notebookeditor.interruptkernel",
Expand Down
75 changes: 63 additions & 12 deletions src/client/datascience/commands/activeEditorContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ import { traceError } from '../../common/logger';
import { IDisposable, IDisposableRegistry } from '../../common/types';
import { isNotebookCell } from '../../common/utils/misc';
import { EditorContexts } from '../constants';
import { getNotebookMetadata, isJupyterNotebook, isPythonNotebook } from '../notebook/helpers/helpers';
import { getActiveInteractiveWindow } from '../interactive-window/helpers';
import { JupyterNotebookView } from '../notebook/constants';
import { getNotebookMetadata, isPythonNotebook } from '../notebook/helpers/helpers';
import { IInteractiveWindow, IInteractiveWindowProvider, INotebook, INotebookProvider } from '../types';

@injectable()
Expand All @@ -28,6 +30,8 @@ export class ActiveEditorContextService implements IExtensionSingleActivationSer
private pythonOrInteractiveOrNativeContext: ContextKey;
private canRestartNotebookKernelContext: ContextKey;
private canInterruptNotebookKernelContext: ContextKey;
private canRestartInteractiveWindowKernelContext: ContextKey;
private canInterruptInteractiveWindowKernelContext: ContextKey;
private hasNativeNotebookCells: ContextKey;
private isPythonFileActive: boolean = false;
private isPythonNotebook: ContextKey;
Expand All @@ -50,6 +54,14 @@ export class ActiveEditorContextService implements IExtensionSingleActivationSer
EditorContexts.CanInterruptNotebookKernel,
this.commandManager
);
this.canRestartInteractiveWindowKernelContext = new ContextKey(
EditorContexts.CanRestartInteractiveWindowKernel,
this.commandManager
);
this.canInterruptInteractiveWindowKernelContext = new ContextKey(
EditorContexts.CanInterruptInteractiveWindowKernel,
this.commandManager
);
this.interactiveContext = new ContextKey(EditorContexts.IsInteractiveActive, this.commandManager);
this.interactiveOrNativeContext = new ContextKey(
EditorContexts.IsInteractiveOrNativeActive,
Expand Down Expand Up @@ -82,6 +94,9 @@ export class ActiveEditorContextService implements IExtensionSingleActivationSer
if (this.vscNotebook.activeNotebookEditor) {
this.onDidChangeActiveNotebookEditor(this.vscNotebook.activeNotebookEditor);
}
if (this.interactiveProvider.activeWindow) {
this.onDidChangeActiveInteractiveWindow();
}
this.vscNotebook.onDidChangeActiveNotebookEditor(this.onDidChangeActiveNotebookEditor, this, this.disposables);

// Do we already have python file opened.
Expand All @@ -100,24 +115,28 @@ export class ActiveEditorContextService implements IExtensionSingleActivationSer
private onDidChangeActiveInteractiveWindow(e?: IInteractiveWindow) {
this.interactiveContext.set(!!e).ignoreErrors();
this.updateMergedContexts();
this.updateContextOfActiveInteractiveWindowKernel();
}
Copy link
Contributor

@DonJayamanne DonJayamanne Sep 13, 2021

Choose a reason for hiding this comment

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

You can use vscode built in context variable notebookCellExecuting
That'll be easier (less code) and more responsive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we adopt that for the notebook editor as well then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried it out, that context key only works when you set focus to the notebook cell that is executing. I don't think that will work for our use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Try this notebookHasRunningCell

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That wouldn't work well too, e.g. if a notebook and IW are open side by side, running a cell in a notebook will cause the interrupt button in the IW toolbar to light up, and vice versa. Also in the IW case, since the IW may not actually be active (focus may be in the Python file), that would cause issues as well. The current implementation still suffers from this limitation #7487 for SxS IWs, but at least allows us to ensure that the IW toolbar is enabled correctly based on the active #%% file or active IW, and to ensure that IWs' and notebooks' run state doesn't affect the toolbar button enablement for each other.

Copy link
Contributor

Choose a reason for hiding this comment

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

yuck, agreed.

private onDidChangeActiveNotebookEditor(e?: NotebookEditor) {
const isJupyterNotebookDoc = e ? isJupyterNotebook(e.document) : false;
const isJupyterNotebookDoc = e ? e.document.notebookType === JupyterNotebookView : false;
this.nativeContext.set(isJupyterNotebookDoc).ignoreErrors();

this.isPythonNotebook
.set(e && isJupyterNotebookDoc ? isPythonNotebook(getNotebookMetadata(e.document)) : false)
.ignoreErrors();
this.updateContextOfActiveNotebookKernel(e);
this.updateContextOfActiveInteractiveWindowKernel();
this.updateNativeNotebookContext();
this.updateNativeNotebookCellContext();
this.updateMergedContexts();
}
private updateNativeNotebookContext() {
this.hasNativeNotebookOpen.set(this.vscNotebook.notebookDocuments.some(isJupyterNotebook)).ignoreErrors();
this.hasNativeNotebookOpen
.set(this.vscNotebook.notebookDocuments.some((nb) => nb.notebookType === JupyterNotebookView))
.ignoreErrors();
}
private updateContextOfActiveNotebookKernel(activeEditor?: NotebookEditor) {
if (activeEditor && isJupyterNotebook(activeEditor.document)) {
if (activeEditor && activeEditor.document.notebookType === JupyterNotebookView) {
this.notebookProvider
.getOrCreateNotebook({
identity: activeEditor.document.uri,
Expand All @@ -140,23 +159,55 @@ export class ActiveEditorContextService implements IExtensionSingleActivationSer
this.canInterruptNotebookKernelContext.set(false).ignoreErrors();
}
}
private updateContextOfActiveInteractiveWindowKernel() {
const interactiveWindow = getActiveInteractiveWindow(this.interactiveProvider);
if (interactiveWindow?.notebookUri) {
this.notebookProvider
.getOrCreateNotebook({
identity: interactiveWindow.notebookUri,
resource: interactiveWindow.owner,
getOnly: true
})
.then(async (nb) => {
if (
getActiveInteractiveWindow(this.interactiveProvider) === interactiveWindow &&
nb !== undefined
) {
const canStart = nb && nb.status !== ServerStatus.NotStarted;
this.canRestartInteractiveWindowKernelContext.set(!!canStart).ignoreErrors();
const canInterrupt = nb && nb.status === ServerStatus.Busy;
this.canInterruptInteractiveWindowKernelContext.set(!!canInterrupt).ignoreErrors();
}
})
.catch(
traceError.bind(
undefined,
'Failed to determine if a kernel is active for the current interactive window'
)
);
} else {
this.canRestartInteractiveWindowKernelContext.set(false).ignoreErrors();
this.canInterruptInteractiveWindowKernelContext.set(false).ignoreErrors();
}
}
private onDidKernelStatusChange({ notebook }: { status: ServerStatus; notebook: INotebook }) {
// Ok, kernel status has changed.
const activeEditor = this.vscNotebook.activeNotebookEditor;
if (!activeEditor) {
return;
}
if (activeEditor.document.uri.toString() !== notebook.identity.toString()) {
// Status of a notebook thats not related to active editor has changed.
// We can ignore that.
return;
const activeInteractiveWindow = getActiveInteractiveWindow(this.interactiveProvider);
if (
activeInteractiveWindow &&
activeInteractiveWindow.notebookUri?.toString() === notebook.identity.toString()
) {
this.updateContextOfActiveInteractiveWindowKernel();
} else if (activeEditor && activeEditor.document.uri.toString() === notebook.identity.toString()) {
this.updateContextOfActiveNotebookKernel(activeEditor);
}
this.updateContextOfActiveNotebookKernel(activeEditor);
}
private onDidChangeActiveTextEditor(e?: TextEditor) {
this.isPythonFileActive = e?.document.languageId === PYTHON_LANGUAGE && !isNotebookCell(e.document.uri);
this.updateNativeNotebookCellContext();
this.updateMergedContexts();
this.updateContextOfActiveInteractiveWindowKernel();
}
private updateMergedContexts() {
this.interactiveOrNativeContext
Expand Down
2 changes: 2 additions & 0 deletions src/client/datascience/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@ export namespace EditorContexts {
export const HaveCellSelected = 'jupyter.havecellselected';
export const CanRestartNotebookKernel = 'jupyter.notebookeditor.canrestartNotebookkernel';
export const CanInterruptNotebookKernel = 'jupyter.notebookeditor.canInterruptNotebookKernel';
export const CanRestartInteractiveWindowKernel = 'jupyter.interactive.canRestartNotebookKernel';
export const CanInterruptInteractiveWindowKernel = 'jupyter.interactive.canInterruptNotebookKernel';
export const DebuggingInProgress = 'jupyter.notebookeditor.debuggingInProgress';
export const RunByLineInProgress = 'jupyter.notebookeditor.runByLineInProgress';
export const IsPythonNotebook = 'jupyter.ispythonnotebook';
Expand Down
21 changes: 21 additions & 0 deletions src/client/datascience/interactive-window/helpers.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import { IInteractiveWindow, IInteractiveWindowProvider } from '../types';
import { window } from 'vscode';
import { NotebookCellScheme } from '../../common/constants';

export function getActiveInteractiveWindow(
interactiveWindowProvider: IInteractiveWindowProvider
): IInteractiveWindow | undefined {
if (interactiveWindowProvider.activeWindow) {
return interactiveWindowProvider.activeWindow;
}
if (window.activeTextEditor === undefined) {
return;
}
const textDocumentUri = window.activeTextEditor.document.uri;
if (textDocumentUri.scheme !== NotebookCellScheme) {
return interactiveWindowProvider.get(textDocumentUri);
}
}
24 changes: 5 additions & 19 deletions src/client/datascience/variablesView/notebookWatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,16 @@ import {
NotebookCellExecutionState,
NotebookCellExecutionStateChangeEvent,
Uri,
window,
workspace
} from 'vscode';
import '../../common/extensions';
import { IFileSystem } from '../../common/platform/types';
import { IDisposableRegistry } from '../../common/types';
import { getActiveInteractiveWindow } from '../interactive-window/helpers';
import { IKernelProvider } from '../jupyter/kernels/types';
import { isJupyterNotebook } from '../notebook/helpers/helpers';
import { KernelState, KernelStateEventArgs } from '../notebookExtensibility';
import {
IInteractiveWindow,
IInteractiveWindowProvider,
INotebook,
INotebookEditor,
INotebookEditorProvider
} from '../types';
import { IInteractiveWindowProvider, INotebook, INotebookEditor, INotebookEditorProvider } from '../types';
import { IActiveNotebookChangedEvent, INotebookWatcher } from './types';

interface IExecutionCountEntry {
Expand All @@ -49,7 +43,7 @@ export class NotebookWatcher implements INotebookWatcher {
return this.notebookEditorProvider.activeEditor?.notebook || this.getActiveInteractiveWindowNotebook();
}
public get activeNotebookExecutionCount(): number | undefined {
const activeInteractiveWindow = this.getActiveInteractiveWindow();
const activeInteractiveWindow = getActiveInteractiveWindow(this.interactiveWindowProvider);
const activeNotebookOrInteractiveWindow =
this.notebookEditorProvider.activeEditor?.file || activeInteractiveWindow?.notebookUri;
if (activeNotebookOrInteractiveWindow) {
Expand Down Expand Up @@ -187,7 +181,7 @@ export class NotebookWatcher implements INotebookWatcher {
) {
return true;
}
const activeInteractiveWindow = this.getActiveInteractiveWindow();
const activeInteractiveWindow = getActiveInteractiveWindow(this.interactiveWindowProvider);
if (
activeInteractiveWindow?.notebookUri !== undefined &&
this.fileSystem.arePathsSame(activeInteractiveWindow.notebookUri, kernelStateEvent.resource)
Expand All @@ -197,16 +191,8 @@ export class NotebookWatcher implements INotebookWatcher {
return false;
}

private getActiveInteractiveWindow(): IInteractiveWindow | undefined {
if (window.activeTextEditor === undefined) {
return;
}
const textDocumentUri = window.activeTextEditor.document.uri;
return this.interactiveWindowProvider.get(textDocumentUri);
}

private getActiveInteractiveWindowNotebook(): INotebook | undefined {
const interactiveWindow = this.getActiveInteractiveWindow();
const interactiveWindow = getActiveInteractiveWindow(this.interactiveWindowProvider);
const notebookDocument = workspace.notebookDocuments.find(
(notebookDocument) => notebookDocument.uri.toString() === interactiveWindow?.notebookUri?.toString()
);
Expand Down