Skip to content
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/7367.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test scaffolding for notebook editor.
1 change: 1 addition & 0 deletions news/3 Code Health/7371.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Tests for the notebook editor for different mime types.
26 changes: 18 additions & 8 deletions src/client/datascience/interactive-common/interactiveBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
private variableRequestStopWatch: StopWatch | undefined;
private variableRequestPendingCount: number = 0;
private loadPromise: Promise<void> | undefined;
private setDark: boolean = false;

constructor(
@unmanaged() private readonly listeners: IInteractiveWindowListener[],
Expand Down Expand Up @@ -466,6 +467,9 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
// Make sure we're loaded first.
await this.ensureServerActive();

// Make sure we set the dark setting
await this.ensureDarkSet();

// Then show our webpanel
await this.show();

Expand Down Expand Up @@ -1025,12 +1029,23 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
}
}

private async ensureDarkSet(): Promise<void> {
if (!this.setDark) {
this.setDark = true;

// Wait for the web panel to get the isDark setting
const knownDark = await this.isDark();

// Before we run any cells, update the dark setting
if (this.notebook) {
await this.notebook.setMatplotLibStyle(knownDark);
}
}
}

private async createNotebook(): Promise<void> {
traceInfo('Getting jupyter server options ...');

// Wait for the webpanel to pass back our current theme darkness
const knownDark = await this.isDark();

// Extract our options
const options = await this.getNotebookOptions();

Expand All @@ -1044,11 +1059,6 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
this.notebook = await server.createNotebook(await this.getNotebookIdentity());
}

// Before we run any cells, update the dark setting
if (this.notebook) {
await this.notebook.setMatplotLibStyle(knownDark);
}

traceInfo('Connected to jupyter server.');
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,19 @@ interface ISyncData {
@injectable()
export class InteractiveWindowProvider implements IInteractiveWindowProvider, IAsyncDisposable {

private activeInteractiveWindow : IInteractiveWindow | undefined;
private postOffice : PostOffice;
private activeInteractiveWindow: IInteractiveWindow | undefined;
private postOffice: PostOffice;
private id: string;
private pendingSyncs : Map<string, ISyncData> = new Map<string, ISyncData>();
private pendingSyncs: Map<string, ISyncData> = new Map<string, ISyncData>();
private executedCode: EventEmitter<string> = new EventEmitter<string>();
private activeInteractiveWindowExecuteHandler: Disposable | undefined;
constructor(
@inject(ILiveShareApi) liveShare: ILiveShareApi,
@inject(IServiceContainer) private serviceContainer: IServiceContainer,
@inject(IAsyncDisposableRegistry) asyncRegistry : IAsyncDisposableRegistry,
@inject(IAsyncDisposableRegistry) asyncRegistry: IAsyncDisposableRegistry,
@inject(IDisposableRegistry) private disposables: IDisposableRegistry,
@inject(IConfigurationService) private configService: IConfigurationService
) {
) {
asyncRegistry.push(this);

// Create a post office so we can make sure interactive windows are created at the same time
Expand All @@ -53,15 +53,15 @@ export class InteractiveWindowProvider implements IInteractiveWindowProvider, IA
this.id = uuid();
}

public getActive() : IInteractiveWindow | undefined {
public getActive(): IInteractiveWindow | undefined {
return this.activeInteractiveWindow;
}

public get onExecutedCode() : Event<string> {
public get onExecutedCode(): Event<string> {
return this.executedCode.event;
}

public async getOrCreateActive() : Promise<IInteractiveWindow> {
public async getOrCreateActive(): Promise<IInteractiveWindow> {
if (!this.activeInteractiveWindow) {
await this.create();
}
Expand All @@ -77,7 +77,7 @@ export class InteractiveWindowProvider implements IInteractiveWindowProvider, IA
throw new Error(localize.DataScience.pythonInteractiveCreateFailed());
}

public async getNotebookOptions() : Promise<INotebookServerOptions> {
public async getNotebookOptions(): Promise<INotebookServerOptions> {
// Find the settings that we are going to launch our server with
const settings = this.configService.getSettings();
let serverURI: string | undefined = settings.datascience.jupyterServerURI;
Expand All @@ -96,11 +96,11 @@ export class InteractiveWindowProvider implements IInteractiveWindowProvider, IA
};
}

public dispose() : Promise<void> {
public dispose(): Promise<void> {
return this.postOffice.dispose();
}

private async create() : Promise<void> {
private async create(): Promise<IInteractiveWindow> {
// Set it as soon as we create it. The .ctor for the interactive window
// may cause a subclass to talk to the IInteractiveWindowProvider to get the active interactive window.
this.activeInteractiveWindow = this.serviceContainer.get<IInteractiveWindow>(IInteractiveWindow);
Expand All @@ -110,6 +110,7 @@ export class InteractiveWindowProvider implements IInteractiveWindowProvider, IA
this.activeInteractiveWindowExecuteHandler = this.activeInteractiveWindow.onExecutedCode(this.onInteractiveWindowExecute);
this.disposables.push(this.activeInteractiveWindowExecuteHandler);
await this.activeInteractiveWindow.ready;
return this.activeInteractiveWindow;
}

private onPeerCountChanged(newCount: number) {
Expand Down Expand Up @@ -162,7 +163,7 @@ export class InteractiveWindowProvider implements IInteractiveWindowProvider, IA
}
}

private async synchronizeCreate() : Promise<void> {
private async synchronizeCreate(): Promise<void> {
// Create a new pending wait if necessary
if (this.postOffice.peerCount > 0 || this.postOffice.role === vsls.Role.Guest) {
const key = uuid();
Expand Down
14 changes: 11 additions & 3 deletions src/client/datascience/jupyter/jupyterNotebook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ export class JupyterNotebookBase implements INotebook {
private pendingCellSubscriptions: CellSubscriber[] = [];
private ranInitialSetup = false;
private _resource: Uri;
private _disposed: boolean = false;

constructor(
_liveShare: ILiveShareApi, // This is so the liveshare mixin works
Expand All @@ -160,8 +161,12 @@ export class JupyterNotebookBase implements INotebook {

public dispose(): Promise<void> {
traceInfo(`Shutting down session ${this.resource.toString()}`);
const dispose = this.session ? this.session.dispose() : undefined;
return dispose ? dispose : Promise.resolve();
if (!this._disposed) {
this._disposed = true;
const dispose = this.session ? this.session.dispose() : undefined;
return dispose ? dispose : Promise.resolve();
}
return Promise.resolve();
}

public get resource(): Uri {
Expand Down Expand Up @@ -615,7 +620,10 @@ export class JupyterNotebookBase implements INotebook {
// If the server crashes, cancel the current observable
exitHandlerDisposable = this.launchInfo.connectionInfo.disconnected((c) => {
const str = c ? c.toString() : '';
subscriber.error(this.sessionStartTime, new Error(localize.DataScience.jupyterServerCrashed().format(str)));
// Only do an error if we're not disposed. If we're disposed we already shutdown.
if (!this._disposed) {
subscriber.error(this.sessionStartTime, new Error(localize.DataScience.jupyterServerCrashed().format(str)));
}
subscriber.complete(this.sessionStartTime);
});
}
Expand Down
23 changes: 11 additions & 12 deletions src/client/datascience/webViewHost.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,15 @@ export class WebViewHost<IMapping> implements IDisposable {
}
}

public setTheme(isDark: boolean) {
if (this.themeIsDarkPromise && !this.themeIsDarkPromise.resolved) {
this.themeIsDarkPromise.resolve(isDark);
} else {
this.themeIsDarkPromise = createDeferred<boolean>();
this.themeIsDarkPromise.resolve(isDark);
}
}

protected reload() {
// Make not disposed anymore
this.disposed = false;
Expand Down Expand Up @@ -200,12 +209,7 @@ export class WebViewHost<IMapping> implements IDisposable {

@captureTelemetry(Telemetry.WebviewStyleUpdate)
private async handleCssRequest(request: IGetCssRequest): Promise<void> {
if (this.themeIsDarkPromise && !this.themeIsDarkPromise.resolved) {
this.themeIsDarkPromise.resolve(request.isDark);
} else {
this.themeIsDarkPromise = createDeferred<boolean>();
this.themeIsDarkPromise.resolve(request.isDark);
}
this.setTheme(request.isDark);
const settings = this.generateDataScienceExtraSettings();
const isDark = await this.themeFinder.isThemeDark(settings.extraSettings.theme);
const css = await this.cssGenerator.generateThemeCss(request.isDark, settings.extraSettings.theme);
Expand All @@ -214,12 +218,7 @@ export class WebViewHost<IMapping> implements IDisposable {

@captureTelemetry(Telemetry.WebviewMonacoStyleUpdate)
private async handleMonacoThemeRequest(request: IGetMonacoThemeRequest): Promise<void> {
if (this.themeIsDarkPromise && !this.themeIsDarkPromise.resolved) {
this.themeIsDarkPromise.resolve(request.isDark);
} else {
this.themeIsDarkPromise = createDeferred<boolean>();
this.themeIsDarkPromise.resolve(request.isDark);
}
this.setTheme(request.isDark);
const settings = this.generateDataScienceExtraSettings();
const monacoTheme = await this.cssGenerator.generateMonacoTheme(request.isDark, settings.extraSettings.theme);
return this.postMessageInternal(CssMessages.GetMonacoThemeResponse, { theme: monacoTheme });
Expand Down
26 changes: 22 additions & 4 deletions src/datascience-ui/interactive-common/cellOutput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
'use strict';
import '../../client/common/extensions';

// tslint:disable-next-line: no-var-requires no-require-imports
const ansiToHtml = require('ansi-to-html');

import { nbformat } from '@jupyterlab/coreutils';
import { JSONObject } from '@phosphor/coreutils';
import ansiRegex from 'ansi-regex';
import ansiToHtml from 'ansi-to-html';
// tslint:disable-next-line: no-require-imports
import cloneDeep = require('lodash/cloneDeep');
import * as monacoEditor from 'monaco-editor/esm/vs/editor/editor.api';
Expand All @@ -15,6 +17,7 @@ import * as React from 'react';
import { concatMultilineString } from '../../client/datascience/common';
import { Identifiers } from '../../client/datascience/constants';
import { CellState } from '../../client/datascience/types';
import { ClassType } from '../../client/ioc/types';
import { noop } from '../../test/core';
import { Image, ImageName } from '../react-common/image';
import { ImageButton } from '../react-common/imageButton';
Expand Down Expand Up @@ -42,11 +45,26 @@ interface ICellOutput {
}
// tslint:disable: react-this-binding-issue
export class CellOutput extends React.Component<ICellOutputProps> {

// tslint:disable-next-line: no-any
private static ansiToHtmlClass_ctor: ClassType<any> | undefined;
constructor(prop: ICellOutputProps) {
super(prop);
}

// tslint:disable-next-line: no-any
private static get ansiToHtmlClass(): ClassType<any> {
if (!CellOutput.ansiToHtmlClass_ctor) {
// ansiToHtml is different between the tests running and webpack. figure out which one
// tslint:disable-next-line: no-any
if (ansiToHtml instanceof Function) {
CellOutput.ansiToHtmlClass_ctor = ansiToHtml;
Copy link
Member

Choose a reason for hiding this comment

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

Is this the commonjs vs ES6 default import issue?

Copy link
Author

Choose a reason for hiding this comment

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

Yep. Had the same problem with memoize in the variable explorer.
Tests use one format and webpack uses the other.


In reply to: 326731645 [](ancestors = 326731645)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I ran into this with node-fetch as well. I think this solution seems fine to me. But I did see one site that I was looking into with node-fetch that was mentioning that webpack could be configured to pick main config over module config. Not sure if that is really worth looking into, but it might make this code unneeded. node-fetch/node-fetch#450


In reply to: 326733133 [](ancestors = 326733133,326731645)

} else {
CellOutput.ansiToHtmlClass_ctor = ansiToHtml.default;
}
}
return CellOutput.ansiToHtmlClass_ctor!;
}

private static getAnsiToHtmlOptions() : { fg: string; bg: string; colors: string [] } {
// Here's the default colors for ansiToHtml. We need to use the
// colors from our current theme.
Expand Down Expand Up @@ -192,7 +210,7 @@ export class CellOutput extends React.Component<ICellOutputProps> {
// colorizing if we don't have html that needs <xmp> around it (ex. <type ='string'>)
try {
if (ansiRegex().test(formatted)) {
const converter = new ansiToHtml(CellOutput.getAnsiToHtmlOptions());
const converter = new CellOutput.ansiToHtmlClass(CellOutput.getAnsiToHtmlOptions());
const html = converter.toHtml(formatted);
copy.data = {
'text/html': html
Expand All @@ -208,7 +226,7 @@ export class CellOutput extends React.Component<ICellOutputProps> {
renderWithScrollbars = true;
const error = copy as nbformat.IError;
try {
const converter = new ansiToHtml(CellOutput.getAnsiToHtmlOptions());
const converter = new CellOutput.ansiToHtmlClass(CellOutput.getAnsiToHtmlOptions());
const trace = converter.toHtml(error.traceback.join('\n'));
copy.data = {
'text/html': trace
Expand Down
8 changes: 5 additions & 3 deletions src/datascience-ui/interactive-common/mainStateController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,11 @@ export class MainStateController implements IMessageHandler {
// Tell the interactive window code we have started.
this.postOffice.sendMessage<IInteractiveWindowMapping, 'started'>(InteractiveWindowMessages.Started);

// Get our monaco theme and css
this.postOffice.sendUnsafeMessage(CssMessages.GetCssRequest, { isDark: this.props.expectingDark });
this.postOffice.sendUnsafeMessage(CssMessages.GetMonacoThemeRequest, { isDark: this.props.expectingDark });
// Get our monaco theme and css if not running a test, because these make everything async too
if (!this.props.testMode) {
this.postOffice.sendUnsafeMessage(CssMessages.GetCssRequest, { isDark: this.props.expectingDark });
this.postOffice.sendUnsafeMessage(CssMessages.GetMonacoThemeRequest, { isDark: this.props.expectingDark });
}
}

public dispose() {
Expand Down
9 changes: 8 additions & 1 deletion src/datascience-ui/native-editor/nativeEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,12 @@ interface INativeEditorProps {
}

export class NativeEditor extends React.Component<INativeEditorProps, IMainState> {
// Public so can access it from test code
public stateController: NativeEditorStateController;
private renderCount: number = 0;
private mainPanelRef: React.RefObject<HTMLDivElement> = React.createRef<HTMLDivElement>();
private contentPanelScrollRef: React.RefObject<HTMLElement> = React.createRef<HTMLElement>();
private contentPanelRef: React.RefObject<ContentPanel> = React.createRef<ContentPanel>();
private stateController: NativeEditorStateController;
private debounceUpdateVisibleCells = debounce(this.updateVisibleCells.bind(this), 100);
private cellRefs: Map<string, React.RefObject<NativeCell>> = new Map<string, React.RefObject<NativeCell>>();
private cellContainerRefs: Map<string, React.RefObject<HTMLDivElement>> = new Map<string, React.RefObject<HTMLDivElement>>();
Expand Down Expand Up @@ -82,6 +84,11 @@ export class NativeEditor extends React.Component<INativeEditorProps, IMainState
}

public render() {
// If in test mode, update our count. Use this to determine how many renders a normal update takes.
if (this.props.testMode) {
this.renderCount = this.renderCount + 1;
}

// Update the state controller with our new state
this.stateController.renderUpdate(this.state);
const progressBar = this.state.busy && !this.props.testMode ? <Progress /> : undefined;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ export class NativeEditorStateController extends MainStateController {
this.resumeUpdates();
}

public addNewCell = () => {
public addNewCell = (): ICellViewModel | undefined => {
const cells = this.getState().cellVMs;
this.suspendUpdates();
const id = uuid();
Expand All @@ -95,6 +95,7 @@ export class NativeEditorStateController extends MainStateController {
vm.useQuickEdit = false;
}
this.resumeUpdates();
return vm;
}

public runAbove = (cellId?: string) => {
Expand Down
8 changes: 4 additions & 4 deletions src/test/datascience/dataScienceIocContainer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,10 @@ import {
} from '../../client/datascience/interactive-common/intellisense/dotNetIntellisenseProvider';
import { NativeEditor } from '../../client/datascience/interactive-ipynb/nativeEditor';
import { NativeEditorCommandListener } from '../../client/datascience/interactive-ipynb/nativeEditorCommandListener';
import { NativeEditorProvider } from '../../client/datascience/interactive-ipynb/nativeEditorProvider';
import { InteractiveWindow } from '../../client/datascience/interactive-window/interactiveWindow';
import {
InteractiveWindowCommandListener
} from '../../client/datascience/interactive-window/interactiveWindowCommandListener';
import { InteractiveWindowProvider } from '../../client/datascience/interactive-window/interactiveWindowProvider';
import { JupyterCommandFactory } from '../../client/datascience/jupyter/jupyterCommand';
import { JupyterDebugger } from '../../client/datascience/jupyter/jupyterDebugger';
import { JupyterExecutionFactory } from '../../client/datascience/jupyter/jupyterExecutionFactory';
Expand Down Expand Up @@ -254,6 +252,8 @@ import { MockLanguageServer } from './mockLanguageServer';
import { MockLanguageServerAnalysisOptions } from './mockLanguageServerAnalysisOptions';
import { MockLiveShareApi } from './mockLiveShare';
import { blurWindow, createMessageEvent } from './reactHelpers';
import { TestInteractiveWindowProvider } from './testInteractiveWindowProvider';
import { TestNativeEditorProvider } from './testNativeEditorProvider';

export class DataScienceIocContainer extends UnitTestIocContainer {

Expand Down Expand Up @@ -337,7 +337,7 @@ export class DataScienceIocContainer extends UnitTestIocContainer {
public registerDataScienceTypes() {
this.registerFileSystemTypes();
this.serviceManager.addSingleton<IJupyterExecution>(IJupyterExecution, JupyterExecutionFactory);
this.serviceManager.addSingleton<IInteractiveWindowProvider>(IInteractiveWindowProvider, InteractiveWindowProvider);
this.serviceManager.addSingleton<IInteractiveWindowProvider>(IInteractiveWindowProvider, TestInteractiveWindowProvider);
this.serviceManager.addSingleton<IDataViewerProvider>(IDataViewerProvider, DataViewerProvider);
this.serviceManager.addSingleton<IPlotViewerProvider>(IPlotViewerProvider, PlotViewerProvider);
this.serviceManager.addSingleton<ILogger>(ILogger, Logger);
Expand Down Expand Up @@ -367,7 +367,7 @@ export class DataScienceIocContainer extends UnitTestIocContainer {
this.serviceManager.addSingleton<IJupyterDebugger>(IJupyterDebugger, JupyterDebugger);
this.serviceManager.addSingleton<IDebugLocationTracker>(IDebugLocationTracker, DebugLocationTracker);
this.serviceManager.addSingleton<IDebugLocationTrackerFactory>(IDebugLocationTrackerFactory, DebugLocationTrackerFactory);
this.serviceManager.addSingleton<INotebookEditorProvider>(INotebookEditorProvider, NativeEditorProvider);
this.serviceManager.addSingleton<INotebookEditorProvider>(INotebookEditorProvider, TestNativeEditorProvider);
this.serviceManager.add<INotebookEditor>(INotebookEditor, NativeEditor);
this.serviceManager.addSingleton<IDataScienceCommandListener>(IDataScienceCommandListener, NativeEditorCommandListener);

Expand Down
Loading