From 57b320b57934113871d119fd3af953296f4aae1a Mon Sep 17 00:00:00 2001 From: Tim Mok Date: Mon, 16 Sep 2024 12:06:35 -0400 Subject: [PATCH 01/13] Plots service unit tests (#4628) Add some unit tests for the plots service Fix disposing and ensuring plot is removed Signed-off-by: Tim Mok Co-authored-by: Wasim Lorgat --- .../browser/positronPlotsService.ts | 8 +- .../test/browser/positronPlotsService.test.ts | 230 ++++++++++++++++++ .../common/languageRuntimePlotClient.ts | 16 +- .../common/positronPlotCommProxy.ts | 16 +- .../test/common/testRuntimeSessionService.ts | 14 +- 5 files changed, 264 insertions(+), 20 deletions(-) create mode 100644 src/vs/workbench/contrib/positronPlots/test/browser/positronPlotsService.test.ts diff --git a/src/vs/workbench/contrib/positronPlots/browser/positronPlotsService.ts b/src/vs/workbench/contrib/positronPlots/browser/positronPlotsService.ts index d5b7ef8dce0..ea9f2853efb 100644 --- a/src/vs/workbench/contrib/positronPlots/browser/positronPlotsService.ts +++ b/src/vs/workbench/contrib/positronPlots/browser/positronPlotsService.ts @@ -191,7 +191,7 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe // When the storage service is about to save state, store the current history policy // and storage policy in the workspace storage. - this._storageService.onWillSaveState(() => { + this._register(this._storageService.onWillSaveState(() => { this._storageService.store( HistoryPolicyStorageKey, @@ -220,7 +220,7 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe StorageScope.WORKSPACE, StorageTarget.MACHINE); } - }); + })); // When the extension service is about to stop, remove any HTML plots // from the plots list. These plots are backed by a proxy that runs in @@ -770,10 +770,10 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe */ removePlot(id: string): void { // Find the plot with the given ID and dispose it - // It will be automatically removed from the list during onDidClose this._plots.forEach((plot, index) => { if (plot.id === id) { this.unregisterPlotClient(plot); + this._plots.splice(index, 1); } }); @@ -1095,6 +1095,8 @@ export class PositronPlotsService extends Disposable implements IPositronPlotsSe this._plotClientsByComm.delete(metadata.id); })); + this._register(commProxy); + return commProxy; } diff --git a/src/vs/workbench/contrib/positronPlots/test/browser/positronPlotsService.test.ts b/src/vs/workbench/contrib/positronPlots/test/browser/positronPlotsService.test.ts new file mode 100644 index 00000000000..9bd93d8e56f --- /dev/null +++ b/src/vs/workbench/contrib/positronPlots/test/browser/positronPlotsService.test.ts @@ -0,0 +1,230 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (C) 2024 Posit Software, PBC. All rights reserved. + * Licensed under the Elastic License 2.0. See LICENSE.txt for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { raceTimeout, timeout } from 'vs/base/common/async'; +import { ensureNoDisposablesAreLeakedInTestSuite } from 'vs/base/test/common/utils'; +import { PositronIPyWidgetsService } from 'vs/workbench/contrib/positronIPyWidgets/browser/positronIPyWidgetsService'; +import { PositronPlotsService } from 'vs/workbench/contrib/positronPlots/browser/positronPlotsService'; +import { PositronWebviewPreloadService } from 'vs/workbench/contrib/positronWebviewPreloads/browser/positronWebviewPreloadsService'; +import { IPositronPlotMetadata } from 'vs/workbench/services/languageRuntime/common/languageRuntimePlotClient'; +import { LanguageRuntimeSessionMode } from 'vs/workbench/services/languageRuntime/common/languageRuntimeService'; +import { IPositronIPyWidgetsService } from 'vs/workbench/services/positronIPyWidgets/common/positronIPyWidgetsService'; +import { HistoryPolicy, IPositronPlotClient } from 'vs/workbench/services/positronPlots/common/positronPlots'; +import { IPositronWebviewPreloadService } from 'vs/workbench/services/positronWebviewPreloads/common/positronWebviewPreloadService'; +import { IRuntimeSessionService, RuntimeClientType } from 'vs/workbench/services/runtimeSession/common/runtimeSessionService'; +import { TestLanguageRuntimeSession } from 'vs/workbench/services/runtimeSession/test/common/testLanguageRuntimeSession'; +import { TestRuntimeSessionService } from 'vs/workbench/services/runtimeSession/test/common/testRuntimeSessionService'; +import { IViewsService } from 'vs/workbench/services/views/common/viewsService'; +import { TestViewsService, workbenchInstantiationService } from 'vs/workbench/test/browser/workbenchTestServices'; + +suite('Positron - Plots Service', () => { + + const disposables = ensureNoDisposablesAreLeakedInTestSuite(); + let plotsService: PositronPlotsService; + let runtimeSessionService: TestRuntimeSessionService; + + setup(() => { + const instantiationService = workbenchInstantiationService(undefined, disposables); + runtimeSessionService = disposables.add(instantiationService.createInstance(TestRuntimeSessionService)); + instantiationService.stub(IRuntimeSessionService, runtimeSessionService); + instantiationService.stub(IPositronWebviewPreloadService, disposables.add(instantiationService.createInstance(PositronWebviewPreloadService))); + instantiationService.stub(IPositronIPyWidgetsService, disposables.add(instantiationService.createInstance(PositronIPyWidgetsService))); + instantiationService.stub(IViewsService, new TestViewsService()); + + plotsService = disposables.add(instantiationService.createInstance(PositronPlotsService)); + }); + + async function createSession() { + const session = disposables.add(new TestLanguageRuntimeSession(LanguageRuntimeSessionMode.Console)); + runtimeSessionService.startSession(session); + + await timeout(0); + + const out: { + session: TestLanguageRuntimeSession; + plotClient: IPositronPlotClient | undefined; + } = { + session, plotClient: undefined, + }; + + disposables.add(session.onDidCreateClientInstance(client => out.plotClient = { + id: client.client.getClientId(), + metadata: {} as IPositronPlotMetadata, + } as IPositronPlotClient)); + + return out; + } + + test('history policy: change history policy', () => { + plotsService.selectHistoryPolicy(HistoryPolicy.AlwaysVisible); + assert.strictEqual(plotsService.historyPolicy, HistoryPolicy.AlwaysVisible); + + plotsService.selectHistoryPolicy(HistoryPolicy.Automatic); + assert.strictEqual(plotsService.historyPolicy, HistoryPolicy.Automatic); + + plotsService.selectHistoryPolicy(HistoryPolicy.NeverVisible); + assert.strictEqual(plotsService.historyPolicy, HistoryPolicy.NeverVisible); + }); + + test('history policy: change event', async () => { + let historyPolicyChanged = 0; + + const didChangeHistoryPolicy = new Promise((resolve) => { + const disposable = plotsService.onDidChangeHistoryPolicy((e) => { + historyPolicyChanged++; + resolve(); + }); + disposables.add(disposable); + }); + + // no event since 'Automatic' is the default + plotsService.selectHistoryPolicy(HistoryPolicy.Automatic); + + // event occurs when changing to 'AlwaysVisible' + plotsService.selectHistoryPolicy(HistoryPolicy.AlwaysVisible); + + await raceTimeout(didChangeHistoryPolicy, 100, () => assert.fail('onDidChangeHistoryPolicy event did not fire')); + assert.strictEqual(historyPolicyChanged, 1, 'onDidChangeHistoryPolicy event should fire once'); + }); + + test('sizing policy: check options and change size', () => { + assert.throws(() => plotsService.selectSizingPolicy('non-existant sizing policy')); + + assert.strictEqual(plotsService.sizingPolicies.length, 6); + + plotsService.selectSizingPolicy('auto'); + assert.strictEqual(plotsService.selectedSizingPolicy.id, 'auto'); + + plotsService.selectSizingPolicy('fill'); + assert.strictEqual(plotsService.selectedSizingPolicy.id, 'fill'); + + plotsService.selectSizingPolicy('landscape'); + assert.strictEqual(plotsService.selectedSizingPolicy.id, 'landscape'); + + plotsService.selectSizingPolicy('portrait'); + assert.strictEqual(plotsService.selectedSizingPolicy.id, 'portrait'); + + plotsService.selectSizingPolicy('square'); + assert.strictEqual(plotsService.selectedSizingPolicy.id, 'square'); + + plotsService.setCustomPlotSize({ width: 100, height: 100 }); + assert.strictEqual(plotsService.selectedSizingPolicy.id, 'custom'); + assert.strictEqual(plotsService.sizingPolicies.length, 7); + + plotsService.clearCustomPlotSize(); + assert.strictEqual(plotsService.selectedSizingPolicy.id, 'auto'); + assert.strictEqual(plotsService.sizingPolicies.length, 6); + }); + + test('sizing policy: change event', async () => { + let sizingPolicyChanged = 0; + + const didChangeSizingPolicy = new Promise((resolve) => { + const disposable = plotsService.onDidChangeSizingPolicy((e) => { + sizingPolicyChanged++; + resolve(); + }); + disposables.add(disposable); + }); + + // no event since 'auto' is the default + plotsService.selectSizingPolicy('auto'); + assert.strictEqual(plotsService.selectedSizingPolicy.id, 'auto'); + + // event occurs when changing to 'fill' + plotsService.selectSizingPolicy('fill'); + assert.strictEqual(plotsService.selectedSizingPolicy.id, 'fill'); + + await raceTimeout(didChangeSizingPolicy, 100, () => assert.fail('onDidChangeSizingPolicy event did not fire')); + assert.strictEqual(sizingPolicyChanged, 1, 'onDidChangeSizingPolicy event should fire once for changing to "fill"'); + }); + + test('selection: select plot', async () => { + const session = await createSession(); + + session.session.createClient(RuntimeClientType.Plot, {}, {}, 'plot1'); + session.session.createClient(RuntimeClientType.Plot, {}, {}, 'plot2'); + + assert.strictEqual(plotsService.selectedPlotId, 'plot2'); + + let selectPlotCalled = false; + const didSelectPlot = new Promise((resolve) => { + const disposable = plotsService.onDidSelectPlot((e) => { + selectPlotCalled = true; + resolve(); + }); + disposables.add(disposable); + }); + plotsService.selectPlot('plot1'); + + await raceTimeout(didSelectPlot, 100, () => assert.fail('onDidSelectPlot event did not fire')); + + assert.ok(selectPlotCalled, 'onDidSelectPlot event should fire'); + assert.strictEqual(plotsService.selectedPlotId, 'plot1'); + }); + + test('selection: remove selected plot', async () => { + const session = await createSession(); + + session.session.createClient(RuntimeClientType.Plot, {}, {}, 'plot1'); + + let removePlotCalled = false; + + const didRemovePlot = new Promise((resolve) => { + const disposable = plotsService.onDidRemovePlot((e) => { + removePlotCalled = true; + resolve(); + }); + disposables.add(disposable); + }); + + assert.strictEqual(plotsService.selectedPlotId, 'plot1'); + + plotsService.removeSelectedPlot(); + + await raceTimeout(didRemovePlot, 100, () => assert.fail('onDidRemovePlot event did not fire')); + + assert.ok(removePlotCalled, 'onDidRemovePlot event should fire'); + assert.strictEqual(plotsService.positronPlotInstances.length, 0); + assert.strictEqual(plotsService.selectedPlotId, undefined); + }); + + test('selection: expect error removing plot when no plot selected', () => { + assert.throws(() => plotsService.removeSelectedPlot(), { message: 'No plot is selected' }); + }); + + test('selection: select previous/next plot', async () => { + const session = await createSession(); + + session.session.createClient(RuntimeClientType.Plot, {}, {}, 'plot1'); + session.session.createClient(RuntimeClientType.Plot, {}, {}, 'plot2'); + session.session.createClient(RuntimeClientType.Plot, {}, {}, 'plot3'); + + assert.strictEqual(plotsService.selectedPlotId, 'plot3'); + + plotsService.selectPreviousPlot(); + assert.strictEqual(plotsService.selectedPlotId, 'plot2'); + + plotsService.selectPreviousPlot(); + assert.strictEqual(plotsService.selectedPlotId, 'plot1'); + + plotsService.selectNextPlot(); + assert.strictEqual(plotsService.selectedPlotId, 'plot2'); + + plotsService.selectNextPlot(); + assert.strictEqual(plotsService.selectedPlotId, 'plot3'); + }); + + test('plot client: create client event', async () => { + const session = await createSession(); + + assert.strictEqual(plotsService.positronPlotInstances.length, 0); + session.session.createClient(RuntimeClientType.Plot, {}, {}, 'plot1'); + + assert.strictEqual(plotsService.selectedPlotId, 'plot1'); + assert.strictEqual(plotsService.positronPlotInstances.length, 1); + }); +}); diff --git a/src/vs/workbench/services/languageRuntime/common/languageRuntimePlotClient.ts b/src/vs/workbench/services/languageRuntime/common/languageRuntimePlotClient.ts index fbb749cef0a..535e3a8ad95 100644 --- a/src/vs/workbench/services/languageRuntime/common/languageRuntimePlotClient.ts +++ b/src/vs/workbench/services/languageRuntime/common/languageRuntimePlotClient.ts @@ -143,13 +143,13 @@ export class PlotClientInstance extends Disposable implements IPositronPlotClien // Connect close emitter event this.onDidClose = this._closeEmitter.event; - this._commProxy.onDidClose((state) => { + this._register(this._commProxy.onDidClose((state) => { this._closeEmitter.fire(); // Silently cancel any pending render requests this._currentRender?.cancel(); this._stateEmitter.fire(PlotClientState.Closed); - }); + })); // Connect the state emitter event this.onDidChangeState = this._stateEmitter.event; @@ -167,20 +167,20 @@ export class PlotClientInstance extends Disposable implements IPositronPlotClien this.onDidSetIntrinsicSize = this._didSetIntrinsicSizeEmitter.event; // Listen to our own state changes - this.onDidChangeState((state) => { + this._register(this.onDidChangeState((state) => { this._state = state; - }); + })); // Listen for plot updates - this._commProxy.onDidRenderUpdate(async () => { + this._register(this._commProxy.onDidRenderUpdate(async () => { const rendered = await this.queuePlotUpdateRequest(); this._renderUpdateEmitter.fire(rendered); - }); + })); // Listn for plot show events - this._commProxy.onDidShowPlot(async (_evt) => { + this._register(this._commProxy.onDidShowPlot(async (_evt) => { this._didShowPlotEmitter.fire(); - }); + })); } /** diff --git a/src/vs/workbench/services/languageRuntime/common/positronPlotCommProxy.ts b/src/vs/workbench/services/languageRuntime/common/positronPlotCommProxy.ts index 90d22157f3e..88fdcefbde7 100644 --- a/src/vs/workbench/services/languageRuntime/common/positronPlotCommProxy.ts +++ b/src/vs/workbench/services/languageRuntime/common/positronPlotCommProxy.ts @@ -160,7 +160,7 @@ export class PositronPlotCommProxy extends Disposable { // Connect close emitter event this.onDidClose = this._closeEmitter.event; - clientStateEvent((state) => { + this._register(clientStateEvent((state) => { if (state === RuntimeClientState.Closed) { this._closeEmitter.fire(); @@ -168,7 +168,7 @@ export class PositronPlotCommProxy extends Disposable { this._currentRender?.cancel(); this._renderQueue.forEach((render) => render.cancel()); } - }); + })); // Connect the render update emitter event this.onDidRenderUpdate = this._renderUpdateEmitter.event; @@ -179,17 +179,17 @@ export class PositronPlotCommProxy extends Disposable { // Connect the intrinsic size emitter event this.onDidSetIntrinsicSize = this._didSetIntrinsicSizeEmitter.event; - this._comm.onDidClose(() => { + this._register(this._comm.onDidClose(() => { this._closeEmitter.fire(); - }); + })); - this._comm.onDidShow(() => { + this._register(this._comm.onDidShow(() => { this._didShowPlotEmitter.fire(); - }); + })); - this._comm.onDidUpdate((_evt) => { + this._register(this._comm.onDidUpdate((_evt) => { this._renderUpdateEmitter.fire(); - }); + })); this._register(this._comm); } diff --git a/src/vs/workbench/services/runtimeSession/test/common/testRuntimeSessionService.ts b/src/vs/workbench/services/runtimeSession/test/common/testRuntimeSessionService.ts index 0f6db7efba5..18e19582246 100644 --- a/src/vs/workbench/services/runtimeSession/test/common/testRuntimeSessionService.ts +++ b/src/vs/workbench/services/runtimeSession/test/common/testRuntimeSessionService.ts @@ -4,20 +4,32 @@ *--------------------------------------------------------------------------------------------*/ import { Emitter } from 'vs/base/common/event'; import { Disposable } from 'vs/base/common/lifecycle'; -import { ILanguageRuntimeSession, IRuntimeSessionService, IRuntimeSessionWillStartEvent } from 'vs/workbench/services/runtimeSession/common/runtimeSessionService'; +import { ILanguageRuntimeClientCreatedEvent } from 'vs/workbench/services/languageRuntime/common/languageRuntimeService'; +import { ILanguageRuntimeGlobalEvent, ILanguageRuntimeSession, IRuntimeSessionService, IRuntimeSessionWillStartEvent } from 'vs/workbench/services/runtimeSession/common/runtimeSessionService'; export class TestRuntimeSessionService extends Disposable implements Partial { private readonly _willStartEmitter = this._register(new Emitter()); + private readonly _didStartRuntime = this._register(new Emitter()); + private readonly _didReceiveRuntimeEvent = this._register(new Emitter()); + private readonly _didCreateClientInstance = this._register(new Emitter()); readonly activeSessions = new Array(); readonly onWillStartSession = this._willStartEmitter.event; + readonly onDidStartRuntime = this._didStartRuntime.event; + + readonly onDidReceiveRuntimeEvent = this._didReceiveRuntimeEvent.event; + + readonly onDidCreateClientInstance = this._didCreateClientInstance.event; + // Test helpers. startSession(session: ILanguageRuntimeSession): void { this.activeSessions.push(session); + this._register(session.onDidCreateClientInstance(e => this._didCreateClientInstance.fire(e))); this._willStartEmitter.fire({ session, isNew: true }); + this._didStartRuntime.fire(session); } } From 10f5bb396d151d586f5d9ecc23a345970eb7ac1b Mon Sep 17 00:00:00 2001 From: Brian Lambert Date: Mon, 16 Sep 2024 09:59:49 -0700 Subject: [PATCH 02/13] If it's OK to do so, drive focus into the newly-created CodeEditorWidget (#4685) --- .../browser/components/consoleInput.tsx | 61 +++++++++++++------ 1 file changed, 44 insertions(+), 17 deletions(-) diff --git a/src/vs/workbench/contrib/positronConsole/browser/components/consoleInput.tsx b/src/vs/workbench/contrib/positronConsole/browser/components/consoleInput.tsx index 12ccfeb41b5..dd3258848f5 100644 --- a/src/vs/workbench/contrib/positronConsole/browser/components/consoleInput.tsx +++ b/src/vs/workbench/contrib/positronConsole/browser/components/consoleInput.tsx @@ -8,7 +8,7 @@ import 'vs/css!./consoleInput'; // React. import * as React from 'react'; -import { FocusEvent, useEffect, useRef } from 'react'; // eslint-disable-line no-duplicate-imports +import { FocusEvent, useEffect, useLayoutEffect, useRef } from 'react'; // eslint-disable-line no-duplicate-imports // Other dependencies. import * as DOM from 'vs/base/browser/dom'; @@ -90,6 +90,40 @@ export const ConsoleInput = (props: ConsoleInputProps) => { const [, setCurrentCodeFragment, currentCodeFragmentRef] = useStateRef(undefined); + /** + * Determines whether it is OK to take focus. + * @returns true if it is OK to take focus; otherwise, false. + */ + const okToTakeFocus = () => { + // https://github.com/posit-dev/positron/issues/2802 + // It's only OK to take focus if there is no focused editor. This avoids stealing focus when + // the user could be actively working in an editor. + + // Get the context key service context. + const contextKeyContext = positronConsoleContext.contextKeyService.getContext( + DOM.getActiveElement() + ); + + // Sensitive to all editor contexts, simple (e.g. git commit textbox) or not (e.g. code + // editor). + if (contextKeyContext.getValue(EditorContextKeys.textInputFocus.key)) { + return false; + } + + // Sensitive to all quick pick contexts, e.g. the commande palette or the file picker. + if (contextKeyContext.getValue(InQuickPickContextKey.key)) { + return false; + } + + // Sensitive to terminal focus. + if (contextKeyContext.getValue(TerminalContextKeys.focus.key)) { + return false; + } + + // It's OK to take focus. + return true; + }; + /** * Updates the code editor widget position. * @param linePosition The line position. @@ -556,8 +590,8 @@ export const ConsoleInput = (props: ConsoleInputProps) => { } }; - // Main useEffect hook. - useEffect(() => { + // Main useLayoutEffect hook. + useLayoutEffect(() => { // Create the disposable store for cleanup. const disposableStore = new DisposableStore(); @@ -758,20 +792,8 @@ export const ConsoleInput = (props: ConsoleInputProps) => { positronConsoleContext.positronConsoleService.onDidChangeActivePositronConsoleInstance( positronConsoleInstance => { if (positronConsoleInstance === props.positronConsoleInstance) { - // https://github.com/posit-dev/positron/issues/2802 - // Only take focus if there is no focused editor to avoid stealing - // focus when the user could be actively working in an editor - - const ctxt = positronConsoleContext.contextKeyService.getContext(DOM.getActiveElement()); - - // Sensitive to all editor contexts, simple (e.g. git commit textbox) or not (e.g. code editor) - const inTextInput = ctxt.getValue(EditorContextKeys.textInputFocus.key); - // Sensitive to all quick pick contexts, e.g. the commande palette or the file picker const inQuickPick = positronConsoleContext.contextKeyService.getContextKeyValue(InQuickPickContextKey.key); - const inQuickPick = ctxt.getValue(InQuickPickContextKey.key); - // Sensitive to terminal focus - const inTerminal = ctxt.getValue(TerminalContextKeys.focus.key); - - if (!inTextInput && !inQuickPick && !inTerminal) { + // If it's OK to take focus, drive focus into the code editor widget. + if (okToTakeFocus()) { codeEditorWidget.focus(); } } @@ -880,6 +902,11 @@ export const ConsoleInput = (props: ConsoleInputProps) => { }) ); + // If it's OK to take focus, drive focus into the code editor widget. + if (okToTakeFocus()) { + codeEditorWidget.focus(); + } + // Return the cleanup function that will dispose of the disposables. return () => disposableStore.dispose(); // eslint-disable-next-line react-hooks/exhaustive-deps From a026b2390b78a67093a5f08dbfe55ab469c95da5 Mon Sep 17 00:00:00 2001 From: Brian Lambert Date: Mon, 16 Sep 2024 15:14:49 -0700 Subject: [PATCH 03/13] Improve how last focused element is determined in PositronModalReactRenderer (#4693) --- .../positronModalReactRenderer.tsx | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/vs/workbench/browser/positronModalReactRenderer/positronModalReactRenderer.tsx b/src/vs/workbench/browser/positronModalReactRenderer/positronModalReactRenderer.tsx index 2342627c856..c6866b429b7 100644 --- a/src/vs/workbench/browser/positronModalReactRenderer/positronModalReactRenderer.tsx +++ b/src/vs/workbench/browser/positronModalReactRenderer/positronModalReactRenderer.tsx @@ -121,9 +121,17 @@ export class PositronModalReactRenderer extends Disposable { // Call the base class's constructor. super(); - // Set the last focused element. - const activeElement = DOM.getWindow(options.parent).document.activeElement; - if (activeElement instanceof HTMLElement) { + // Get the active element. + let activeElement: Element | null = null; + if (options.parent) { + activeElement = DOM.getWindow(options.parent).document.activeElement; + } + if (!activeElement) { + activeElement = DOM.getActiveWindow().document.activeElement; + } + + // If the active element is an HTML element, set it as the last focused element. + if (DOM.isHTMLElement(activeElement)) { this._lastFocusedElement = activeElement; } From 7393e66f86b5cfcb73e125b165c246eaaa373f02 Mon Sep 17 00:00:00 2001 From: Brian Lambert Date: Mon, 16 Sep 2024 19:51:35 -0700 Subject: [PATCH 04/13] Data explorer UI/fix hover, cursor, and focus indicators in table summary (#4696) --- .../browser/components/columnSummaryCell.css | 18 ++++++++++++--- .../browser/components/columnSummaryCell.tsx | 22 +++++++------------ 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/src/vs/workbench/services/positronDataExplorer/browser/components/columnSummaryCell.css b/src/vs/workbench/services/positronDataExplorer/browser/components/columnSummaryCell.css index 55a5827ca8b..58a5fcddb2f 100644 --- a/src/vs/workbench/services/positronDataExplorer/browser/components/columnSummaryCell.css +++ b/src/vs/workbench/services/positronDataExplorer/browser/components/columnSummaryCell.css @@ -15,19 +15,28 @@ grid-template-rows: [basic-info] 34px [profile-info] 1fr [end-rows]; } -.data-grid-row-cell .content .column-summary .cursor { +.data-grid-row-cell .content .column-summary .cursor-indicator { top: 2px; right: 2px; bottom: 2px; left: 2px; z-index: -1; + opacity: 50%; position: absolute; border-radius: 4px; +} + +.data-grid-row-cell .content .column-summary:hover .cursor-indicator { background-color: var(--vscode-positronDataGrid-selectionBackground); } -.data-grid-row-cell .content .column-summary .cursor:not(.focused) { - opacity: 50%; +.data-grid-row-cell .content .column-summary .cursor-indicator.cursor { + background-color: var(--vscode-positronDataGrid-selectionBackground); +} + +.data-grid-row-cell .content .column-summary .cursor-indicator.cursor.focused { + opacity: 100%; + border: 1px solid var(--vscode-positronDataGrid-selectionBorder); } /* basic-info */ @@ -85,6 +94,7 @@ /* column-sparkline */ .data-grid-row-cell .content .column-summary .basic-info .column-sparkline { + pointer-events: none; grid-column: sparkline / missing-values; } @@ -94,6 +104,7 @@ display: grid; grid-gap: 5px; align-items: center; + pointer-events: none; grid-column: missing-values / right-gutter; grid-template-columns: [percent] 35px [graph] 25px [end]; } @@ -132,6 +143,7 @@ .data-grid-row-cell .content .column-summary .column-profile-info { display: grid; margin: 0 auto; + pointer-events: none; grid-row: profile-info / end-rows; grid-template-rows: [sparkline] min-content [tabular-info] min-content [end-rows]; grid-template-columns: [left-gutter] 55px [sparkline-tabular-info] 1fr [right-gutter] 30px [end-column]; diff --git a/src/vs/workbench/services/positronDataExplorer/browser/components/columnSummaryCell.tsx b/src/vs/workbench/services/positronDataExplorer/browser/components/columnSummaryCell.tsx index 586f95db2de..546708217f5 100644 --- a/src/vs/workbench/services/positronDataExplorer/browser/components/columnSummaryCell.tsx +++ b/src/vs/workbench/services/positronDataExplorer/browser/components/columnSummaryCell.tsx @@ -8,7 +8,7 @@ import 'vs/css!./columnSummaryCell'; // React. import * as React from 'react'; -import { useRef, useState } from 'react'; // eslint-disable-line no-duplicate-imports +import { useRef } from 'react'; // eslint-disable-line no-duplicate-imports // Other dependencies. import { IHoverService } from 'vs/platform/hover/browser/hover'; @@ -56,9 +56,6 @@ export const ColumnSummaryCell = (props: ColumnSummaryCellProps) => { // Reference hooks. const dataTypeRef = useRef(undefined!); - // State hooks. - const [mouseInside, setMouseInside] = useState(false); - /** * Determines whether summary stats is supported. * @returns true, if summary stats is supported; otherwise, false. @@ -366,21 +363,18 @@ export const ColumnSummaryCell = (props: ColumnSummaryCellProps) => {
setMouseInside(true)} - onMouseLeave={() => setMouseInside(false)} onMouseDown={() => { props.instance.scrollToRow(props.columnIndex); props.instance.setCursorRow(props.columnIndex); }} > - {(mouseInside || cursor) && -
- } +
Date: Tue, 17 Sep 2024 11:24:19 +0200 Subject: [PATCH 05/13] Bump ark to 0.1.136 (#4699) - https://github.com/posit-dev/ark/pull/500 - https://github.com/posit-dev/ark/pull/524 - https://github.com/posit-dev/ark/pull/525 - https://github.com/posit-dev/ark/pull/526 --- extensions/positron-r/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/positron-r/package.json b/extensions/positron-r/package.json index 0066f4431c9..f84e374c87e 100644 --- a/extensions/positron-r/package.json +++ b/extensions/positron-r/package.json @@ -634,7 +634,7 @@ }, "positron": { "binaryDependencies": { - "ark": "0.1.135" + "ark": "0.1.136" }, "minimumRVersion": "4.2.0", "minimumRenvVersion": "1.0.7" From 094c82c0de13d758f64b8db3f2a271671bde8459 Mon Sep 17 00:00:00 2001 From: sharon Date: Tue, 17 Sep 2024 12:44:56 -0400 Subject: [PATCH 06/13] update reh-web README with windows line endings note (#4680) Updates the reh-web README on how to deal with `remote/reh-web/package.json` showing up as unstaged with invisible changes on Windows as per the discussion here https://github.com/posit-dev/positron/commit/be3b7468394d6101f600c6b4e84f51c315ecf4e6. I also moved around some of the text in the README. Preview the updated README here: https://github.com/posit-dev/positron/blob/reh-web-package-json-windows-readme/remote/reh-web/README.md ### QA Notes Just a read through for typos and clarity :) Co-authored-by: Jennifer (Jenny) Bryan --- remote/reh-web/README.md | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/remote/reh-web/README.md b/remote/reh-web/README.md index 068eda5e746..e6e274c54e4 100644 --- a/remote/reh-web/README.md +++ b/remote/reh-web/README.md @@ -3,13 +3,15 @@ > [!IMPORTANT] > **Please don't edit files in this directory directly.** -The [package.json](./package.json) file in this directory is a merge of the [remote/package.json](../package.json) file and the [remote/web/package.json](../web/package.json) file, as the packages needed to build reh-web are a combination of the remote and web packages. +> [!NOTE] +> Please commit the created/updated `package.json` and `yarn.lock` files in this directory when they are updated. -The package.json file in this directory is created/updated via the [build/npm/postinstall.js](../../build/npm/postinstall.js) script. That script is automatically run after `yarn` is executed in the top-level directory. The script includes some handling to update the package.json file and run `yarn` to update yarn.lock and node_modules in this directory. +The [package.json](./package.json) file in this directory is a merge of the [remote/package.json](../package.json) file and the [remote/web/package.json](../web/package.json) file, as the packages needed to build reh-web are a combination of the remote and web packages. -Since the files in this directory are auto-generated, please don't edit them directly. Running `yarn` at the top-level of the project will kick off updates to these files. +The package.json file in this directory is created/updated via the [build/npm/postinstall.js](../../build/npm/postinstall.js) script. That script is automatically run after `yarn` is executed in the top-level directory and updates the package.json, yarn.lock and node_modules in this directory. -Git line ending normalization is set to LF for package.json via [.gitattributes](./.gitattributes) to avoid changing the line endings when the file is generated on different platforms. +Since the files in this directory are auto-generated via the `postinstall.js` script, please don't edit them directly. Running `yarn` at the top-level of the project will kick off updates to these files. -> [!NOTE] -> Please commit the created/updated package.json and yarn.lock files in this directory when they are updated. +If you're building on Windows, it's possible you might see 👻 invisible 👻 unstaged changes to the [package.json](./package.json) file. +- **The tl;dr**: if this happens, stage the file and it should disappear and you shouldn't have to think about this again. +- **The details**: this should only happen the first time you run `yarn` (and maybe also if you clear the Git index/cache) on Windows. The "invisible" changes occur after regenerating package.json on Windows, where CRLF is used for line endings instead of LF. Git line ending normalization has been set to LF for package.json via [.gitattributes](./.gitattributes) to avoid changing the line endings when the file is generated on different platforms. So, when you stage the file, the conversion to LF happens and it will no longer appear as modified. From 7513ae9cc8903ff9dc2b20003b803a928b4491fa Mon Sep 17 00:00:00 2001 From: Christopher Mead Date: Tue, 17 Sep 2024 14:54:06 -0600 Subject: [PATCH 07/13] Use automattic slack reporter for tests (#4678) Using the automattic slack reporter in place our the previous custom one. Benefits: * unit test failures will also be reported * same reporter python tests are using * more concise format that also links to commit Also bumping smoke test workflows to use ubuntu-latest-4x ### QA Notes Test failures properly reported --- .github/workflows/positron-full-test.yml | 23 +++++++++++------- .github/workflows/positron-merge-to-main.yml | 25 +++++++++++++------- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/.github/workflows/positron-full-test.yml b/.github/workflows/positron-full-test.yml index 2d597522574..4297b749b99 100644 --- a/.github/workflows/positron-full-test.yml +++ b/.github/workflows/positron-full-test.yml @@ -13,7 +13,7 @@ permissions: jobs: linux: name: Tests on Linux - runs-on: ubuntu-latest + runs-on: ubuntu-latest-4x timeout-minutes: 60 env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} @@ -231,10 +231,17 @@ jobs: name: run-artifacts path: .build/logs/smoke-tests-electron/ - - name: slack-smoke-test-report - if: failure() - uses: testlabauto/xunit-slack-reporter@v2.0.1 - env: - SLACK_CHANNEL: C07FR1JNZNJ #positron-test-results channel - SLACK_TOKEN: ${{ secrets.SMOKE_TESTS_SLACK_TOKEN }} - XUNIT_PATH: .build/logs/smoke-tests-electron/test-results/results.xml + slack-notification: + name: 'Send Slack notification' + runs-on: ubuntu-latest + needs: linux + if: ${{ failure() }} + steps: + - name: 'Send Slack notification' + uses: automattic/action-test-results-to-slack@v0.3.1 + with: + github_token: ${{ secrets.POSITRON_GITHUB_PAT }} + slack_token: ${{ secrets.SMOKE_TESTS_SLACK_TOKEN }} + slack_channel: C07FR1JNZNJ #positron-test-results channel + suite_name: Positron Full Test Suite + diff --git a/.github/workflows/positron-merge-to-main.yml b/.github/workflows/positron-merge-to-main.yml index 18ab9d52871..bd4e22d7d24 100644 --- a/.github/workflows/positron-merge-to-main.yml +++ b/.github/workflows/positron-merge-to-main.yml @@ -29,7 +29,7 @@ permissions: jobs: linux: name: Tests on Linux - runs-on: ubuntu-latest + runs-on: ubuntu-latest-4x timeout-minutes: 45 env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} @@ -207,11 +207,20 @@ jobs: with: name: run-artifacts path: .build/logs/smoke-tests-electron/ + outputs: + target: ${{ env.SMOKETEST_TARGET }} + + slack-notification: + name: 'Send Slack notification' + runs-on: ubuntu-latest + needs: linux + if: ${{ failure() && needs.linux.outputs.target == 'smoketest-merge-to-main' }} + steps: + - name: 'Send Slack notification' + uses: automattic/action-test-results-to-slack@v0.3.1 + with: + github_token: ${{ secrets.POSITRON_GITHUB_PAT }} + slack_token: ${{ secrets.SMOKE_TESTS_SLACK_TOKEN }} + slack_channel: C07FR1JNZNJ #positron-test-results channel + suite_name: Positron Merge to Main Test Suite - - name: slack-smoke-test-report - if: ${{ failure() && env.SMOKETEST_TARGET == 'smoketest-merge-to-main' }} - uses: testlabauto/xunit-slack-reporter@v2.0.1 - env: - SLACK_CHANNEL: C07FR1JNZNJ #positron-test-results channel - SLACK_TOKEN: ${{ secrets.SMOKE_TESTS_SLACK_TOKEN }} - XUNIT_PATH: .build/logs/smoke-tests-electron/test-results/results.xml From cc03931626dcff6aafe1fd1e3aa07c9acfd5b86b Mon Sep 17 00:00:00 2001 From: Jonathan Date: Tue, 17 Sep 2024 15:09:23 -0700 Subject: [PATCH 08/13] Modernize yarn invocations (#4704) This change addresses CI failures of the form: ``` yarn install v1.22.22 warning ../package.json: No license field error `install` has been replaced with `add` to add new dependencies. Run "yarn add install" instead. info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command. Error: Process completed with exit code 1. ``` Example run: https://github.com/posit-dev/positron/actions/runs/10896318741/job/30235932351 The fix is twofold: - Install `yarn` with `corepack` (the recommended mechanism as of Node 20) - Use `yarn add` instead of `yarn install` as recommended in the message ### QA Notes No product change; changes to CI files only --- .github/workflows/positron-full-test.yml | 8 ++++---- .github/workflows/positron-merge-to-main.yml | 9 +++++---- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/.github/workflows/positron-full-test.yml b/.github/workflows/positron-full-test.yml index 4297b749b99..8af898902b6 100644 --- a/.github/workflows/positron-full-test.yml +++ b/.github/workflows/positron-full-test.yml @@ -101,8 +101,8 @@ jobs: ELECTRON_SKIP_BINARY_DOWNLOAD: 1 POSITRON_GITHUB_PAT: ${{ github.token }} run: | - # Install Yarn - npm install -g yarn + # Enable corepack (for yarn) + corepack enable # Install node-gyp; this is required by some packages, and yarn # sometimes fails to automatically install it. @@ -111,8 +111,8 @@ jobs: # Perform the main yarn command; this installs all Node packages and # dependencies yarn --immutable --network-timeout 120000 - yarn --cwd test/automation install install --frozen-lockfile - yarn --cwd test/smoke install install --frozen-lockfile + yarn --cwd test/automation add install --frozen-lockfile + yarn --cwd test/smoke add install --frozen-lockfile # Note cache-hit will be set to true only when cache hit occurs for the exact key match. # For a partial key match it will be set to false diff --git a/.github/workflows/positron-merge-to-main.yml b/.github/workflows/positron-merge-to-main.yml index bd4e22d7d24..d8954036503 100644 --- a/.github/workflows/positron-merge-to-main.yml +++ b/.github/workflows/positron-merge-to-main.yml @@ -103,8 +103,8 @@ jobs: ELECTRON_SKIP_BINARY_DOWNLOAD: 1 POSITRON_GITHUB_PAT: ${{ github.token }} run: | - # Install Yarn - npm install -g yarn + # Enable corepack (for yarn) + corepack enable # Install node-gyp; this is required by some packages, and yarn # sometimes fails to automatically install it. @@ -113,8 +113,9 @@ jobs: # Perform the main yarn command; this installs all Node packages and # dependencies yarn --immutable --network-timeout 120000 - yarn --cwd test/automation install --frozen-lockfile - yarn --cwd test/smoke install --frozen-lockfile + yarn --cwd test/automation add install --frozen-lockfile + yarn --cwd test/smoke add install --frozen-lockfile + # Note cache-hit will be set to true only when cache hit occurs for the exact key match. # For a partial key match it will be set to false From 4a49fdd5594308ddf1697011c7c680d653cad860 Mon Sep 17 00:00:00 2001 From: Jonathan Date: Tue, 17 Sep 2024 15:40:50 -0700 Subject: [PATCH 09/13] Use arm64 builds of Ark on Linux (#4711) As a prerequisite to making arm64 builds of Positron, we need Positron to bundle arm64 builds of ark when building on arm64 Linux. This change does just that. Ark is already producing arm64 builds of Linux, e.g. https://github.com/posit-dev/ark/releases/tag/0.1.136. ### QA Notes N/A; this does not currently impact any shipping releases since none are made on arm64. --- extensions/positron-r/scripts/install-kernel.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/positron-r/scripts/install-kernel.ts b/extensions/positron-r/scripts/install-kernel.ts index f5290960f0e..8a5933ec517 100644 --- a/extensions/positron-r/scripts/install-kernel.ts +++ b/extensions/positron-r/scripts/install-kernel.ts @@ -7,7 +7,7 @@ import * as decompress from 'decompress'; import * as fs from 'fs'; import { IncomingMessage } from 'http'; import * as https from 'https'; -import { platform } from 'os'; +import { platform, arch } from 'os'; import * as path from 'path'; import { promisify } from 'util'; @@ -188,7 +188,7 @@ async function downloadAndReplaceArk(version: string, switch (platform()) { case 'win32': os = 'windows-x64'; break; case 'darwin': os = 'darwin-universal'; break; - case 'linux': os = 'linux-x64'; break; + case 'linux': os = (arch() === 'arm64' ? 'linux-arm64' : 'linux-x64'); break; default: { console.error(`Unsupported platform ${platform()}.`); return; From ddf9fd4a114b457b4d791f6689ba981b9f4c60f1 Mon Sep 17 00:00:00 2001 From: Jon Vanausdeln Date: Wed, 18 Sep 2024 08:46:20 -0700 Subject: [PATCH 10/13] Fix timing in R development Smoke Test (#4710) ### Intent It appears to be a slight timing issue that causes these test to fail in CI ## Approach * Moved the `runCommand` calls out of the retry loops to avoid overloading the terminal * Added additional line to check when command finished. This also needed a "clear terminal" to ensure it was checking for the correct command output ### QA Notes All tests should pass in CI --- .../positron/r-pkg-development/r-pkg-development.test.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/smoke/src/areas/positron/r-pkg-development/r-pkg-development.test.ts b/test/smoke/src/areas/positron/r-pkg-development/r-pkg-development.test.ts index 84af5aa760f..cb8b024771a 100644 --- a/test/smoke/src/areas/positron/r-pkg-development/r-pkg-development.test.ts +++ b/test/smoke/src/areas/positron/r-pkg-development/r-pkg-development.test.ts @@ -54,20 +54,23 @@ export function setup(logger: Logger) { }).toPass({ timeout: 50000 }); logger.log('Test R Package'); + await app.workbench.quickaccess.runCommand('r.packageTest'); await expect(async () => { - await app.workbench.quickaccess.runCommand('r.packageTest'); await app.workbench.terminal.waitForTerminalText(buffer => buffer.some(line => line.startsWith('[ FAIL 1 | WARN 0 | SKIP 0 | PASS 16 ]'))); + await app.workbench.terminal.waitForTerminalText(buffer => buffer.some(line => line.includes('Terminal will be reused by tasks'))); }).toPass({ timeout: 50000 }); logger.log('Check R Package'); + await app.workbench.quickaccess.runCommand('workbench.action.terminal.clear'); + await app.workbench.quickaccess.runCommand('r.packageCheck'); await expect(async () => { - await app.workbench.quickaccess.runCommand('r.packageCheck'); await app.workbench.terminal.waitForTerminalText(buffer => buffer.some(line => line.startsWith('Error: R CMD check found ERRORs'))); + await app.workbench.terminal.waitForTerminalText(buffer => buffer.some(line => line.includes('Terminal will be reused by tasks'))); }).toPass({ timeout: 50000 }); logger.log('Install R Package and Restart R'); + await app.workbench.quickaccess.runCommand('r.packageInstall'); await expect(async () => { - await app.workbench.quickaccess.runCommand('r.packageInstall'); await app.workbench.terminal.waitForTerminalText(buffer => buffer.some(line => line.startsWith('✔ Installed testfun 0.0.0.9000'))); await app.workbench.positronConsole.waitForReady('>'); await app.workbench.positronConsole.waitForConsoleContents((contents) => contents.some((line) => line.includes('restarted'))); From 3daab596f342babeb40bf69054cf62d73279402a Mon Sep 17 00:00:00 2001 From: sharon Date: Wed, 18 Sep 2024 12:07:18 -0400 Subject: [PATCH 11/13] remove `add` from `yarn install` CI command (#4722) Following https://github.com/posit-dev/positron/pull/4704, I think we want `yarn install` instead of `yarn add install` in these CI commands, as the latter will add the package `install` to the corresponding package.json. I think we just want to install dependencies from the frozen lockfiles on these lines and don't intend to install the `install` package. It seems like an extra `install` got introduced to those lines at some point, which is why we were getting an error about `install install` not being the right way to install the `install` package. ### QA Notes We can test this by running the yarn commands locally. 1. Delete `test/smoke/node_modules` 2. Run `yarn --cwd test/smoke add install --frozen-lockfile` 3. Notice that dependencies are installed, but we also added a new dependency "install" 4. Revert the changes to package.json and yarn.lock and delete `test/smoke/node_modules` 5. Run `yarn --cwd test/smoke install --frozen-lockfile` 6. Dependencies are still installed, but we don't install the `install` package --- .github/workflows/positron-full-test.yml | 4 ++-- .github/workflows/positron-merge-to-main.yml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/positron-full-test.yml b/.github/workflows/positron-full-test.yml index 8af898902b6..21a95f85aad 100644 --- a/.github/workflows/positron-full-test.yml +++ b/.github/workflows/positron-full-test.yml @@ -111,8 +111,8 @@ jobs: # Perform the main yarn command; this installs all Node packages and # dependencies yarn --immutable --network-timeout 120000 - yarn --cwd test/automation add install --frozen-lockfile - yarn --cwd test/smoke add install --frozen-lockfile + yarn --cwd test/automation install --frozen-lockfile + yarn --cwd test/smoke install --frozen-lockfile # Note cache-hit will be set to true only when cache hit occurs for the exact key match. # For a partial key match it will be set to false diff --git a/.github/workflows/positron-merge-to-main.yml b/.github/workflows/positron-merge-to-main.yml index d8954036503..8f345c44b06 100644 --- a/.github/workflows/positron-merge-to-main.yml +++ b/.github/workflows/positron-merge-to-main.yml @@ -113,8 +113,8 @@ jobs: # Perform the main yarn command; this installs all Node packages and # dependencies yarn --immutable --network-timeout 120000 - yarn --cwd test/automation add install --frozen-lockfile - yarn --cwd test/smoke add install --frozen-lockfile + yarn --cwd test/automation install --frozen-lockfile + yarn --cwd test/smoke install --frozen-lockfile # Note cache-hit will be set to true only when cache hit occurs for the exact key match. From 73a9b7850332c67db0e81a2a731418cfd2bed07e Mon Sep 17 00:00:00 2001 From: Isabel Zimmerman <54685329+isabelizimm@users.noreply.github.com> Date: Wed, 18 Sep 2024 13:05:15 -0400 Subject: [PATCH 12/13] python: play button runs file in console by default (#4705) part of #4675 ### QA Notes pressing play button should run the entire file in the console, not just a single line --- extensions/positron-python/package.json | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/extensions/positron-python/package.json b/extensions/positron-python/package.json index 2559594aad7..119a090e8b2 100644 --- a/extensions/positron-python/package.json +++ b/extensions/positron-python/package.json @@ -1474,21 +1474,21 @@ ], "editor/title/run": [ { - "command": "python.execSelectionInConsole", + "command": "python.execInConsole", "group": "navigation@0", - "title": "%python.command.python.execSelectionInConsole.title%", + "title": "%python.command.python.execInConsole.title%", "when": "resourceLangId == python && !isInDiffEditor && !virtualWorkspace && shellExecutionSupported" }, { - "command": "python.execInConsole", + "command": "python.execInTerminal-icon", "group": "navigation@1", - "title": "%python.command.python.execInConsole.title%", + "title": "%python.command.python.execInTerminalIcon.title%", "when": "resourceLangId == python && !isInDiffEditor && !virtualWorkspace && shellExecutionSupported" }, { - "command": "python.execInTerminal-icon", + "command": "python.execSelectionInConsole", "group": "navigation@2", - "title": "%python.command.python.execInTerminalIcon.title%", + "title": "%python.command.python.execSelectionInConsole.title%", "when": "resourceLangId == python && !isInDiffEditor && !virtualWorkspace && shellExecutionSupported" }, { From 6af852f4b407037270f7878f9243079e22cb1094 Mon Sep 17 00:00:00 2001 From: Isabel Zimmerman <54685329+isabelizimm@users.noreply.github.com> Date: Wed, 18 Sep 2024 14:01:58 -0400 Subject: [PATCH 13/13] python: detect app framework in .py files (#4625) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - creates context key for is/is not app - "Run App in Terminal" will show up when in an app We might want to think about how this interacts with the Shiny extension. I guess at this point, it will just populate a LOT of options in the Play button, which is ugly but benign. ### QA Notes create a `.py` file ``` import streamlit as st x = st.slider('x') # 👈 this is a widget st.write(x, 'squared is', x * x) ``` open and click on play button, you should see ![Screenshot 2024-09-12 at 4 01 23 PM](https://github.com/user-attachments/assets/afacee31-e6cc-48d8-b185-1adcfd3bf95d) (clicking on this file will only save the file and have no other affect) --------- Signed-off-by: Isabel Zimmerman <54685329+isabelizimm@users.noreply.github.com> Co-authored-by: Wasim Lorgat --- extensions/positron-python/package.json | 84 +++++++++++++++++ extensions/positron-python/package.nls.json | 7 ++ .../src/client/common/application/commands.ts | 6 ++ .../src/client/common/constants.ts | 6 ++ .../positron-python/src/client/extension.ts | 2 +- .../src/client/positron/extension.ts | 10 +- .../src/client/positron/webAppContexts.ts | 91 +++++++++++++++++++ .../codeExecution/codeExecutionManager.ts | 36 ++++++++ .../test/positron/webAppContexts.unit.test.ts | 62 +++++++++++++ .../codeExecutionManager.unit.test.ts | 6 ++ 10 files changed, 308 insertions(+), 2 deletions(-) create mode 100644 extensions/positron-python/src/client/positron/webAppContexts.ts create mode 100644 extensions/positron-python/src/test/positron/webAppContexts.unit.test.ts diff --git a/extensions/positron-python/package.json b/extensions/positron-python/package.json index 119a090e8b2..5007a718c2e 100644 --- a/extensions/positron-python/package.json +++ b/extensions/positron-python/package.json @@ -300,6 +300,48 @@ "icon": "$(play)", "title": "%python.command.python.execInTerminalIcon.title%" }, + { + "category": "Python", + "command": "python.execDashInTerminal", + "icon": "$(play)", + "title": "%python.command.python.execDashInTerminal.title%", + "enablement": "pythonAppFramework == dash && config.python.enableWebAppSupport" + }, + { + "category": "Python", + "command": "python.execFastAPIInTerminal", + "icon": "$(play)", + "title": "%python.command.python.execFastAPIInTerminal.title%", + "enablement": "pythonAppFramework == fastapi && config.python.enableWebAppSupport" + }, + { + "category": "Python", + "command": "python.execFlaskInTerminal", + "icon": "$(play)", + "title": "%python.command.python.execFlaskInTerminal.title%", + "enablement": "pythonAppFramework == flask && config.python.enableWebAppSupport" + }, + { + "category": "Python", + "command": "python.execGradioInTerminal", + "icon": "$(play)", + "title": "%python.command.python.execGradioInTerminal.title%", + "enablement": "pythonAppFramework == gradio && config.python.enableWebAppSupport" + }, + { + "category": "Python", + "command": "python.execShinyInTerminal", + "icon": "$(play)", + "title": "%python.command.python.execShinyInTerminal.title%", + "enablement": "pythonAppFramework == shiny && config.python.enableWebAppSupport" + }, + { + "category": "Python", + "command": "python.execStreamlitInTerminal", + "icon": "$(play)", + "title": "%python.command.python.execStreamlitInTerminal.title%", + "enablement": "pythonAppFramework == streamlit && config.python.enableWebAppSupport" + }, { "category": "Python", "command": "python.execInConsole", @@ -440,6 +482,12 @@ "scope": "application", "type": "boolean" }, + "python.enableWebAppSupport": { + "defaut": false, + "description": "%python.enableWebAppSupport.description%", + "scope": "application", + "type": "boolean" + }, "python.envFile": { "default": "${workspaceFolder}/.env", "description": "%python.envFile.description%", @@ -1479,6 +1527,42 @@ "title": "%python.command.python.execInConsole.title%", "when": "resourceLangId == python && !isInDiffEditor && !virtualWorkspace && shellExecutionSupported" }, + { + "command": "python.execDashInTerminal", + "group": "navigation@1", + "title": "%python.command.python.execDashInTerminal.title%", + "when": "pythonAppFramework == dash && config.python.enableWebAppSupport" + }, + { + "command": "python.execGradioInTerminal", + "group": "navigation@1", + "title": "%python.command.python.execGradioInTerminal.title%", + "when": "pythonAppFramework == gradio && config.python.enableWebAppSupport" + }, + { + "command": "python.execFastAPIInTerminal", + "group": "navigation@1", + "title": "%python.command.python.execFastAPIInTerminal.title%", + "when": "pythonAppFramework == fastapi && config.python.enableWebAppSupport" + }, + { + "command": "python.execFlaskInTerminal", + "group": "navigation@1", + "title": "%python.command.python.execFlaskInTerminal.title%", + "when": "pythonAppFramework == flask && config.python.enableWebAppSupport" + }, + { + "command": "python.execShinyInTerminal", + "group": "navigation@1", + "title": "%python.command.python.execShinyInTerminal.title%", + "when": "pythonAppFramework == shiny && config.python.enableWebAppSupport" + }, + { + "command": "python.execStreamlitInTerminal", + "group": "navigation@1", + "title": "%python.command.python.execStreamlitInTerminal.title%", + "when": "pythonAppFramework == streamlit && config.python.enableWebAppSupport" + }, { "command": "python.execInTerminal-icon", "group": "navigation@1", diff --git a/extensions/positron-python/package.nls.json b/extensions/positron-python/package.nls.json index 0e04ca2e541..f8982c9ac48 100644 --- a/extensions/positron-python/package.nls.json +++ b/extensions/positron-python/package.nls.json @@ -6,6 +6,12 @@ "python.command.python.createNewFile.title": "New Python File", "python.command.python.createTerminal.title": "Create Terminal", "python.command.python.execInTerminal.title": "Run Python File in Terminal", + "python.command.python.execDashInTerminal.title": "Run Dash App in Terminal", + "python.command.python.execFastAPIInTerminal.title": "Run FastAPI in Terminal", + "python.command.python.execFlaskInTerminal.title": "Run Flask App in Terminal", + "python.command.python.execGradioInTerminal.title": "Run Gradio App in Terminal", + "python.command.python.execShinyInTerminal.title": "Run Shiny App in Terminal", + "python.command.python.execStreamlitInTerminal.title": "Run Streamlit App in Terminal", "python.command.python.execInConsole.title": "Run Python File in Console", "python.command.python.debugInTerminal.title": "Debug Python File in Terminal", "python.command.python.execInTerminalIcon.title": "Run Python File in Terminal", @@ -38,6 +44,7 @@ "python.debugger.deprecatedMessage": "This configuration will be deprecated soon. Please replace `python` with `debugpy` to use the new Python Debugger extension.", "python.defaultInterpreterPath.description": "Path to default Python to use when extension loads up for the first time, no longer used once an interpreter is selected for the workspace. See [here](https://aka.ms/AAfekmf) to understand when this is used", "python.diagnostics.sourceMapsEnabled.description": "Enable source map support for meaningful stack traces in error logs.", + "python.enableWebAppSupport.description": "Enable experimental support for Python applications", "python.envFile.description": "Absolute path to a file containing environment variable definitions.", "python.experiments.enabled.description": "Enables A/B tests experiments in the Python extension. If enabled, you may get included in proposed enhancements and/or features.", "python.experiments.optInto.description": "List of experiment to opt into. If empty, user is assigned the default experiment groups. See [here](https://github.com/microsoft/vscode-python/wiki/AB-Experiments) for more details.", diff --git a/extensions/positron-python/src/client/common/application/commands.ts b/extensions/positron-python/src/client/common/application/commands.ts index 10525b4bead..babc0c5e743 100644 --- a/extensions/positron-python/src/client/common/application/commands.ts +++ b/extensions/positron-python/src/client/common/application/commands.ts @@ -102,6 +102,12 @@ export interface ICommandNameArgumentTypeMapping extends ICommandNameWithoutArgu [Commands.Exec_In_Terminal_Icon]: [undefined, Uri]; [Commands.Debug_In_Terminal]: [Uri]; // --- Start Positron --- + [Commands.Exec_Dash_In_Terminal]: [undefined, Uri]; + [Commands.Exec_FastAPI_In_Terminal]: [undefined, Uri]; + [Commands.Exec_Flask_In_Terminal]: [undefined, Uri]; + [Commands.Exec_Gradio_In_Terminal]: [undefined, Uri]; + [Commands.Exec_Shiny_In_Terminal]: [undefined, Uri]; + [Commands.Exec_Streamlit_In_Terminal]: [undefined, Uri]; [Commands.Exec_In_Console]: []; [Commands.Focus_Positron_Console]: []; // --- End Positron --- diff --git a/extensions/positron-python/src/client/common/constants.ts b/extensions/positron-python/src/client/common/constants.ts index 718890ef612..b45e73575a5 100644 --- a/extensions/positron-python/src/client/common/constants.ts +++ b/extensions/positron-python/src/client/common/constants.ts @@ -52,6 +52,12 @@ export namespace Commands { export const Exec_In_Terminal_Icon = 'python.execInTerminal-icon'; export const Exec_In_Separate_Terminal = 'python.execInDedicatedTerminal'; // --- Start Positron --- + export const Exec_Dash_In_Terminal = 'python.execDashInTerminal'; + export const Exec_FastAPI_In_Terminal = 'python.execFastAPIInTerminal'; + export const Exec_Flask_In_Terminal = 'python.execFlaskInTerminal'; + export const Exec_Gradio_In_Terminal = 'python.execGradioInTerminal'; + export const Exec_Shiny_In_Terminal = 'python.execShinyInTerminal'; + export const Exec_Streamlit_In_Terminal = 'python.execStreamlitInTerminal'; export const Exec_In_Console = 'python.execInConsole'; export const Exec_Selection_In_Console = 'python.execSelectionInConsole'; // --- End Positron --- diff --git a/extensions/positron-python/src/client/extension.ts b/extensions/positron-python/src/client/extension.ts index 3736954fbf0..05754662e74 100644 --- a/extensions/positron-python/src/client/extension.ts +++ b/extensions/positron-python/src/client/extension.ts @@ -91,7 +91,7 @@ export async function activate(context: IExtensionContext): Promise { +export async function activatePositron( + serviceContainer: IServiceContainer, + context: vscode.ExtensionContext, +): Promise { try { const disposables = serviceContainer.get(IDisposableRegistry); // Register a command to check if ipykernel is installed for a given interpreter. @@ -71,6 +75,10 @@ export async function activatePositron(serviceContainer: IServiceContainer): Pro disposables.push( vscode.commands.registerCommand('python.getMinimumPythonVersion', (): string => MINIMUM_PYTHON_VERSION.raw), ); + + // Activate detection for web applications + activateAppDetection(context.subscriptions); + traceInfo('activatePositron: done!'); } catch (ex) { traceError('activatePositron() failed.', ex); diff --git a/extensions/positron-python/src/client/positron/webAppContexts.ts b/extensions/positron-python/src/client/positron/webAppContexts.ts new file mode 100644 index 00000000000..faa0280a11b --- /dev/null +++ b/extensions/positron-python/src/client/positron/webAppContexts.ts @@ -0,0 +1,91 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (C) 2024 Posit Software, PBC. All rights reserved. + * Licensed under the Elastic License 2.0. See LICENSE.txt for license information. + *--------------------------------------------------------------------------------------------*/ + +import * as vscode from 'vscode'; +import { executeCommand } from '../common/vscodeApis/commandApis'; + +const libraries: string[] = ['streamlit', 'shiny', 'dash', 'gradio', 'flask', 'fastapi']; + +export function detectWebApp(document: vscode.TextDocument): void { + const text = document.getText(); + const framework = getFramework(text); + executeCommand('setContext', 'pythonAppFramework', framework); +} + +export function getFramework(text: string): string | undefined { + const importPattern = new RegExp(`import\\s+(${libraries.join('|')})`, 'g'); + const fromImportPattern = new RegExp(`from\\s+(${libraries.join('|')})\\S*\\simport`, 'g'); + const importMatch = importPattern.exec(text); + + if (importMatch) { + return importMatch[1]; + } + + const fromImportMatch = fromImportPattern.exec(text); + if (fromImportMatch) { + return fromImportMatch[1]; + } + + return undefined; +} + +export function activateAppDetection(disposables: vscode.Disposable[]): void { + let timeout: NodeJS.Timeout | undefined; + let activeEditor = vscode.window.activeTextEditor; + + function updateWebApp() { + if (!activeEditor) { + return; + } + detectWebApp(activeEditor.document); + } + + // Throttle updates if needed + function triggerUpdateApp(throttle = false) { + if (!activeEditor) { + return; + } + if (timeout) { + clearTimeout(timeout); + timeout = undefined; + } + if (throttle) { + timeout = setTimeout(updateWebApp, 500); + } else { + detectWebApp(activeEditor.document); + } + } + + // Trigger for the current active editor. + if (activeEditor) { + triggerUpdateApp(); + } + + disposables.push( + // Trigger when the active editor changes + vscode.window.onDidChangeActiveTextEditor((editor) => { + if (editor && editor.document.languageId === 'python') { + activeEditor = editor; + triggerUpdateApp(); + } + }), + + // Trigger when the active editor's content changes + vscode.workspace.onDidChangeTextDocument((event) => { + if (activeEditor && event.document === activeEditor.document) { + triggerUpdateApp(true); + } + }), + + // Trigger when new text document is opened + vscode.workspace.onDidOpenTextDocument((document) => { + if (document.languageId === 'python') { + // update to opened text document + activeEditor = vscode.window.activeTextEditor; + triggerUpdateApp(); + } + }), + ); +} diff --git a/extensions/positron-python/src/client/terminals/codeExecution/codeExecutionManager.ts b/extensions/positron-python/src/client/terminals/codeExecution/codeExecutionManager.ts index 8abd73565bb..13dafd54612 100644 --- a/extensions/positron-python/src/client/terminals/codeExecution/codeExecutionManager.ts +++ b/extensions/positron-python/src/client/terminals/codeExecution/codeExecutionManager.ts @@ -26,6 +26,9 @@ import { CreateEnvironmentCheckKind, triggerCreateEnvironmentCheckNonBlocking, } from '../../pythonEnvironments/creation/createEnvironmentTrigger'; +// --- Start Positron --- +//import { getAppFramework } from '../../positron/webAppContexts' +// --- End Positron --- @injectable() export class CodeExecutionManager implements ICodeExecutionManager { @@ -75,6 +78,39 @@ export class CodeExecutionManager implements ICodeExecutionManager { }, ); // --- Start Positron --- + [ + Commands.Exec_Streamlit_In_Terminal, + Commands.Exec_Dash_In_Terminal, + Commands.Exec_FastAPI_In_Terminal, + Commands.Exec_Flask_In_Terminal, + Commands.Exec_Gradio_In_Terminal, + Commands.Exec_Shiny_In_Terminal, + ].forEach((cmd) => { + this.disposableRegistry.push( + this.commandManager.registerCommand(cmd as any, async () => { + // use editor to get contents of file + const editor = vscode.window.activeTextEditor; + if (!editor) { + // No editor; nothing to do + return; + } + + const filePath = editor.document.uri.fsPath; + if (!filePath) { + // File is unsaved; show a warning + vscode.window.showWarningMessage('Cannot run unsaved file.'); + return; + } + + // Save the file before sourcing it to ensure that the contents are + // up to date with editor buffer. + await vscode.commands.executeCommand('workbench.action.files.save'); + + // TODO: connect appFramework to commands to run script + //const appFramework = getAppFramework(editor.document.getText()) + }), + ); + }); this.disposableRegistry.push( this.commandManager.registerCommand(Commands.Exec_In_Console as any, async () => { // Get the active text editor. diff --git a/extensions/positron-python/src/test/positron/webAppContexts.unit.test.ts b/extensions/positron-python/src/test/positron/webAppContexts.unit.test.ts new file mode 100644 index 00000000000..18dd7caccf3 --- /dev/null +++ b/extensions/positron-python/src/test/positron/webAppContexts.unit.test.ts @@ -0,0 +1,62 @@ +import * as vscode from 'vscode'; +import * as sinon from 'sinon'; +import { assert } from 'chai'; +import * as cmdApis from '../../client/common/vscodeApis/commandApis'; +import { detectWebApp, getFramework } from '../../client/positron/webAppContexts'; +import { IDisposableRegistry } from '../../client/common/types'; + +suite('Discover webapp frameworks', () => { + let document: vscode.TextDocument; + let executeCommandStub: sinon.SinonStub; + const disposables: IDisposableRegistry = []; + + setup(() => { + executeCommandStub = sinon.stub(cmdApis, 'executeCommand'); + document = { + getText: () => '', + } as vscode.TextDocument; + }); + + teardown(() => { + sinon.restore(); + disposables.forEach((d) => d.dispose()); + }); + + const texts = { + 'import streamlit': 'streamlit', + 'from shiny.ui import page_navbar': 'shiny', + 'import numpy': 'numpy', + }; + Object.entries(texts).forEach(([text, framework]) => { + const expected = text.includes('numpy') ? undefined : framework; + test('should set context pythonAppFramework if application is found', () => { + document.getText = () => text; + detectWebApp(document); + + assert.ok(executeCommandStub.calledOnceWith('setContext', 'pythonAppFramework', expected)); + }); + }); + + const frameworks = ['streamlit', 'shiny', 'gradio', 'flask', 'fastapi', 'numpy']; + frameworks.forEach((framework) => { + const expected = framework === 'numpy' ? undefined : framework; + test(`should detect ${expected}: import framework`, () => { + const text = `import ${framework}`; + const actual = getFramework(text); + + assert.strictEqual(actual, expected); + }); + test(`should detect ${expected}: from framework.test import XYZ`, () => { + const text = `from ${framework}.test import XYZ`; + const actual = getFramework(text); + + assert.strictEqual(actual, expected); + }); + test(`should detect ${expected}: from framework import XYZ`, () => { + const text = `from ${framework} import XYZ`; + const actual = getFramework(text); + + assert.strictEqual(actual, expected); + }); + }); +}); diff --git a/extensions/positron-python/src/test/terminals/codeExecution/codeExecutionManager.unit.test.ts b/extensions/positron-python/src/test/terminals/codeExecution/codeExecutionManager.unit.test.ts index 55eb51d1c31..103be1fd61c 100644 --- a/extensions/positron-python/src/test/terminals/codeExecution/codeExecutionManager.unit.test.ts +++ b/extensions/positron-python/src/test/terminals/codeExecution/codeExecutionManager.unit.test.ts @@ -90,6 +90,12 @@ suite('Terminal - Code Execution Manager', () => { [ // --- Start Positron --- // Add the Positron execute in console command and execute selection in console command. + Commands.Exec_Dash_In_Terminal, + Commands.Exec_FastAPI_In_Terminal, + Commands.Exec_Flask_In_Terminal, + Commands.Exec_Gradio_In_Terminal, + Commands.Exec_Shiny_In_Terminal, + Commands.Exec_Streamlit_In_Terminal, Commands.Exec_In_Console, Commands.Exec_Selection_In_Console, // --- End Positron ---