Skip to content

Display commands when necessary #9986

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

Merged
merged 1 commit into from
Feb 7, 2020
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/3 Code Health/8869.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Display `Commands` related to `Interactive Window` and `Notebooks` only when necessary.
97 changes: 66 additions & 31 deletions package.json

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions src/client/common/contextKey.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { ICommandManager } from './application/types';

export class ContextKey {
public get value(): boolean | undefined {
return this.lastValue;
}
private lastValue?: boolean;

constructor(private name: string, private commandManager: ICommandManager) {}
Expand Down
8 changes: 6 additions & 2 deletions src/client/datascience/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,16 @@ export namespace EditorContexts {
export const HaveInteractiveCells = 'python.datascience.haveinteractivecells';
export const HaveRedoableCells = 'python.datascience.haveredoablecells';
export const HaveInteractive = 'python.datascience.haveinteractive';
export const IsInteractive = 'python.datascience.isinteractive';
export const IsInteractiveActive = 'python.datascience.isinteractiveactive';
export const OwnsSelection = 'python.datascience.ownsSelection';
export const HaveNativeCells = 'python.datascience.havenativecells';
export const HaveNativeRedoableCells = 'python.datascience.havenativeredoablecells';
export const HaveNative = 'python.datascience.havenative';
export const IsNative = 'python.datascience.isnative';
export const IsNativeActive = 'python.datascience.isnativeactive';
export const IsInteractiveOrNativeActive = 'python.datascience.isinteractiveornativeactive';
export const IsPythonOrNativeActive = 'python.datascience.ispythonornativeactive';
export const IsPythonOrInteractiveActive = 'python.datascience.ispythonorinteractiveeactive';
Copy link
Author

Choose a reason for hiding this comment

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

Added some or commands as its not possible to have or conditions in the commands.

export const IsPythonOrInteractiveOrNativeActive = 'python.datascience.ispythonorinteractiveornativeeactive';
export const HaveCellSelected = 'python.datascience.havecellselected';
}

Expand Down
39 changes: 34 additions & 5 deletions src/client/datascience/context/activeEditorContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
'use strict';

import { inject, injectable } from 'inversify';
import { TextEditor } from 'vscode';
import { IExtensionSingleActivationService } from '../../activation/types';
import { ICommandManager } from '../../common/application/types';
import { ICommandManager, IDocumentManager } from '../../common/application/types';
import { PYTHON_LANGUAGE } from '../../common/constants';
import { ContextKey } from '../../common/contextKey';
import { IDisposable, IDisposableRegistry } from '../../common/types';
import { EditorContexts } from '../constants';
Expand All @@ -14,28 +16,55 @@ import { IInteractiveWindow, IInteractiveWindowProvider, INotebookEditor, INoteb
@injectable()
export class ActiveEditorContextService implements IExtensionSingleActivationService, IDisposable {
private readonly disposables: IDisposable[] = [];
private nativeContext: ContextKey;
private interactiveContext: ContextKey;
private interactiveOrNativeContext: ContextKey;
private pythonOrInteractiveContext: ContextKey;
private pythonOrNativeContext: ContextKey;
private pythonOrInteractiveOrNativeContext: ContextKey;
private isPythonFileActive: boolean = false;
constructor(
@inject(IInteractiveWindowProvider) private readonly interactiveProvider: IInteractiveWindowProvider,
@inject(INotebookEditorProvider) private readonly notebookProvider: INotebookEditorProvider,
@inject(IDocumentManager) private readonly docManager: IDocumentManager,
@inject(ICommandManager) private readonly commandManager: ICommandManager,
@inject(IDisposableRegistry) disposables: IDisposableRegistry
) {
disposables.push(this);
this.nativeContext = new ContextKey(EditorContexts.IsNativeActive, this.commandManager);
this.interactiveContext = new ContextKey(EditorContexts.IsInteractiveActive, this.commandManager);
this.interactiveOrNativeContext = new ContextKey(EditorContexts.IsInteractiveOrNativeActive, this.commandManager);
this.pythonOrNativeContext = new ContextKey(EditorContexts.IsPythonOrNativeActive, this.commandManager);
this.pythonOrInteractiveContext = new ContextKey(EditorContexts.IsPythonOrInteractiveActive, this.commandManager);
this.pythonOrInteractiveOrNativeContext = new ContextKey(EditorContexts.IsPythonOrInteractiveOrNativeActive, this.commandManager);
}
public dispose() {
this.disposables.forEach(item => item.dispose());
}
public async activate(): Promise<void> {
this.docManager.onDidChangeActiveTextEditor(this.onDidChangeActiveTextEditor, this, this.disposables);
this.interactiveProvider.onDidChangeActiveInteractiveWindow(this.onDidChangeActiveInteractiveWindow, this, this.disposables);
this.notebookProvider.onDidChangeActiveNotebookEditor(this.onDidChangeActiveNotebookEditor, this, this.disposables);
}

private onDidChangeActiveInteractiveWindow(e?: IInteractiveWindow) {
const interactiveContext = new ContextKey(EditorContexts.IsInteractive, this.commandManager);
interactiveContext.set(!!e).ignoreErrors();
this.interactiveContext.set(!!e).ignoreErrors();
this.updateMergedContexts();
}
private onDidChangeActiveNotebookEditor(e?: INotebookEditor) {
const interactiveContext = new ContextKey(EditorContexts.IsNative, this.commandManager);
interactiveContext.set(!!e).ignoreErrors();
this.nativeContext.set(!!e).ignoreErrors();
this.updateMergedContexts();
}
private onDidChangeActiveTextEditor(e?: TextEditor) {
this.isPythonFileActive = e?.document.languageId === PYTHON_LANGUAGE;
this.updateMergedContexts();
}
private updateMergedContexts() {
this.interactiveOrNativeContext.set(this.nativeContext.value === true && this.interactiveContext.value === true).ignoreErrors();
this.pythonOrNativeContext.set(this.nativeContext.value === true || this.isPythonFileActive === true).ignoreErrors();
this.pythonOrInteractiveContext.set(this.interactiveContext.value === true || this.isPythonFileActive === true).ignoreErrors();
this.pythonOrInteractiveOrNativeContext
.set(this.nativeContext.value === true || (this.interactiveContext.value === true && this.isPythonFileActive === true))
.ignoreErrors();
}
}
7 changes: 4 additions & 3 deletions src/client/datascience/interactive-common/interactiveBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ import {
INotebookServerOptions,
InterruptResult,
IStatusProvider,
IThemeFinder
IThemeFinder,
WebViewViewChangeEventArgs
} from '../types';
import { WebViewHost } from '../webViewHost';
import { InteractiveWindowMessageListener } from './interactiveWindowMessageListener';
Expand Down Expand Up @@ -389,7 +390,7 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
});
}

protected onViewStateChanged(_visible: boolean, active: boolean) {
protected onViewStateChanged(args: WebViewViewChangeEventArgs) {
// Only activate if the active editor is empty. This means that
// vscode thinks we are actually supposed to have focus. It would be
// nice if they would more accurrately tell us this, but this works for now.
Expand All @@ -398,7 +399,7 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
// it's been activated. However if there's no active text editor and we're active, we
// can safely attempt to give ourselves focus. This won't actually give us focus if we aren't
// allowed to have it.
if (active && !this.viewState.active) {
if (args.current.active && !args.previous.active) {
Copy link
Author

Choose a reason for hiding this comment

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

Changed this so that the state is update immediately.
The fact that something is active and it isn't reflected in its own state results in a bug in other parts of the code.
Modified to ensure we pass previous state for places that require it (this was the only place)

this.activating().ignoreErrors();
}
}
Expand Down
9 changes: 5 additions & 4 deletions src/client/datascience/interactive-ipynb/nativeEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ import {
INotebookImporter,
INotebookServerOptions,
IStatusProvider,
IThemeFinder
IThemeFinder,
WebViewViewChangeEventArgs
} from '../types';

// tslint:disable-next-line:no-require-imports no-var-requires
Expand Down Expand Up @@ -495,12 +496,12 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
}
}

protected async onViewStateChanged(visible: boolean, active: boolean) {
super.onViewStateChanged(visible, active);
protected async onViewStateChanged(args: WebViewViewChangeEventArgs) {
super.onViewStateChanged(args);

// Update our contexts
const interactiveContext = new ContextKey(EditorContexts.HaveNative, this.commandManager);
Copy link

Choose a reason for hiding this comment

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

interactiveContext [](start = 14, length = 18)

should probably rename this. It's not the 'interactiveContext', but the 'nativeContext'

Copy link
Author

Choose a reason for hiding this comment

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

Because CI sucks big time, I'll do it in the next PR.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in another PR that I'll be submitting shortly

interactiveContext.set(visible && active).catch();
interactiveContext.set(args.current.visible && args.current.active).catch();
this._onDidChangeViewState.fire();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ import {
INotebookExporter,
INotebookServerOptions,
IStatusProvider,
IThemeFinder
IThemeFinder,
WebViewViewChangeEventArgs
} from '../types';

const historyReactDir = path.join(EXTENSION_ROOT_DIR, 'out', 'datascience-ui', 'notebook');
Expand Down Expand Up @@ -241,8 +242,8 @@ export class InteractiveWindow extends InteractiveBase implements IInteractiveWi
public scrollToCell(id: string): void {
this.postMessage(InteractiveWindowMessages.ScrollToCell, { id }).ignoreErrors();
}
protected async onViewStateChanged(visible: boolean, active: boolean) {
super.onViewStateChanged(visible, active);
protected async onViewStateChanged(args: WebViewViewChangeEventArgs) {
super.onViewStateChanged(args);
this._onDidChangeViewState.fire();
}

Expand Down
6 changes: 6 additions & 0 deletions src/client/datascience/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -725,3 +725,9 @@ export interface IJupyterInterpreterDependencyManager {
*/
installMissingDependencies(err?: JupyterInstallError): Promise<void>;
}

type WebViewViewState = {
readonly visible: boolean;
readonly active: boolean;
};
export type WebViewViewChangeEventArgs = { current: WebViewViewState; previous: WebViewViewState };
16 changes: 9 additions & 7 deletions src/client/datascience/webViewHost.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { StopWatch } from '../common/utils/stopWatch';
import { captureTelemetry, sendTelemetryEvent } from '../telemetry';
import { DefaultTheme, Telemetry } from './constants';
import { CssMessages, IGetCssRequest, IGetMonacoThemeRequest, SharedMessages } from './messages';
import { ICodeCssGenerator, IDataScienceExtraSettings, IThemeFinder } from './types';
import { ICodeCssGenerator, IDataScienceExtraSettings, IThemeFinder, WebViewViewChangeEventArgs } from './types';

@injectable() // For some reason this is necessary to get the class hierarchy to work.
export class WebViewHost<IMapping> implements IDisposable {
Expand Down Expand Up @@ -144,7 +144,7 @@ export class WebViewHost<IMapping> implements IDisposable {
this.messageListener.onMessage(type.toString(), payload);
}

protected onViewStateChanged(_visible: boolean, _active: boolean) {
protected onViewStateChanged(_args: WebViewViewChangeEventArgs) {
noop();
}

Expand Down Expand Up @@ -263,11 +263,13 @@ export class WebViewHost<IMapping> implements IDisposable {
}

private webPanelViewStateChanged = (webPanel: IWebPanel) => {
const isVisible = webPanel.isVisible();
const isActive = webPanel.isActive();
this.onViewStateChanged(isVisible, isActive);
this.viewState.visible = isVisible;
this.viewState.active = isActive;
const visible = webPanel.isVisible();
const active = webPanel.isActive();
const current = { visible, active };
const previous = { visible: this.viewState.visible, active: this.viewState.active };
this.viewState.visible = visible;
this.viewState.active = active;
this.onViewStateChanged({ current, previous });
};

@captureTelemetry(Telemetry.WebviewStyleUpdate)
Expand Down