diff --git a/extensions/positron-r/src/hyperlink.ts b/extensions/positron-r/src/hyperlink.ts index c060bc4fb504..658cedf9b46e 100644 --- a/extensions/positron-r/src/hyperlink.ts +++ b/extensions/positron-r/src/hyperlink.ts @@ -47,8 +47,8 @@ function handleNotRunnable(code: string) { )); } -async function handleManuallyRunnable(_runtime: RSession, code: string) { - const console = await positron.window.getConsoleForLanguage('r'); +async function handleManuallyRunnable(runtime: RSession, code: string) { + const console = await positron.window.getConsoleForSessionId(runtime.metadata.sessionId); if (!console) { // Not an expected path, but technically possible, diff --git a/src/positron-dts/positron.d.ts b/src/positron-dts/positron.d.ts index 66a6705087a3..f5e837f27d20 100644 --- a/src/positron-dts/positron.d.ts +++ b/src/positron-dts/positron.d.ts @@ -1529,14 +1529,14 @@ declare module 'positron' { okButtonTitle?: string): Thenable; /** - * Get the `Console` for a runtime `languageId` + * Get the `positron.Console` that is tied to this `sessionId` * - * @param languageId The runtime language id to retrieve a `Console` for, i.e. 'r' or 'python'. + * @param sessionId The session id to retrieve a `positron.Console` for. * * @returns A Thenable that resolves to a `Console` or `undefined` if no `Console` for - * that `languageId` exists. + * that `sessionId` exists. */ - export function getConsoleForLanguage(languageId: string): Thenable; + export function getConsoleForSessionId(sessionId: string): Thenable; /** * Fires when the width of the console input changes. The new width is passed as diff --git a/src/vs/workbench/api/browser/positron/mainThreadConsole.ts b/src/vs/workbench/api/browser/positron/mainThreadConsole.ts deleted file mode 100644 index fc75eedd7725..000000000000 --- a/src/vs/workbench/api/browser/positron/mainThreadConsole.ts +++ /dev/null @@ -1,29 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (C) 2024-2025 Posit Software, PBC. All rights reserved. - * Licensed under the Elastic License 2.0. See LICENSE.txt for license information. - *--------------------------------------------------------------------------------------------*/ - -import { IPositronConsoleInstance } from '../../../services/positronConsole/browser/interfaces/positronConsoleService.js'; - -/** - * The main thread's view of a console instance - * - * Cousin to `ExtHostConsole` - * - * When the extension host requests console behavior from the main thread, it - * typically ends up here. - */ -export class MainThreadConsole { - constructor( - private readonly _console: IPositronConsoleInstance - ) { - } - - getLanguageId(): string { - return this._console.runtimeMetadata.languageId; - } - - pasteText(text: string): void { - this._console.pasteText(text); - } -} diff --git a/src/vs/workbench/api/browser/positron/mainThreadConsoleService.ts b/src/vs/workbench/api/browser/positron/mainThreadConsoleService.ts index 2f474c59ae9a..13c87de998be 100644 --- a/src/vs/workbench/api/browser/positron/mainThreadConsoleService.ts +++ b/src/vs/workbench/api/browser/positron/mainThreadConsoleService.ts @@ -7,24 +7,12 @@ import { DisposableStore } from '../../../../base/common/lifecycle.js'; import { ExtHostConsoleServiceShape, ExtHostPositronContext, MainPositronContext, MainThreadConsoleServiceShape } from '../../common/positron/extHost.positron.protocol.js'; import { extHostNamedCustomer, IExtHostContext } from '../../../services/extensions/common/extHostCustomers.js'; import { IPositronConsoleInstance, IPositronConsoleService } from '../../../services/positronConsole/browser/interfaces/positronConsoleService.js'; -import { MainThreadConsole } from './mainThreadConsole.js'; @extHostNamedCustomer(MainPositronContext.MainThreadConsoleService) export class MainThreadConsoleService implements MainThreadConsoleServiceShape { private readonly _disposables = new DisposableStore(); - /** - * A Map of session ids to the respective console. - * Each session id maps to a single console. - * Multiple sessions could map to the same console, this happens - * when a user power-cycles the session for a console instance - * (i.e. shutdown session for console instance, then start a session for console instance) - * - * Kept in sync with consoles in `ExtHostConsoleService` - */ - private readonly _mainThreadConsolesBySessionId = new Map(); - private readonly _proxy: ExtHostConsoleServiceShape; constructor( @@ -42,95 +30,42 @@ export class MainThreadConsoleService implements MainThreadConsoleServiceShape { this._proxy.$onDidChangeConsoleWidth(newWidth); })); - // Forward new positron console session id to the extension host, and then register it - // in the main thread too + // Forward new positron console session id to the extension host this._disposables.add( this._positronConsoleService.onDidStartPositronConsoleInstance((console) => { - const sessionId = console.sessionMetadata.sessionId; - - // First update ext host - this._proxy.$addConsole(sessionId); - - // Then update main thread - this.addConsole(sessionId, console); + this._proxy.$onDidStartPositronConsoleInstance(console.sessionMetadata.sessionId); }) ); - // TODO: - // As of right now, we never delete console instances from the maps in - // `MainThreadConsoleService` and `ExtHostConsoleService` because we don't have a hook to - // know when a console is stopped. In particular, we should really call the `ExtHostConsole` - // `dispose()` method, which will ensure that any API callers who use the corresponding - // `Console` object will get a warning / error when calling the API of a closed console. - // - // this._disposables.add( - // this._positronConsoleService.onDidRemovePositronConsoleInstance((console) => { - // const sessionId = console.session.sessionId; - // - // // First update ext host - // this._proxy.$removeConsole(sessionId); - // - // // Then update main thread - // this.removeConsole(sessionId); - // }) - // ) + // Forward deleted positron console session id to the extension host + this._disposables.add( + this._positronConsoleService.onDidDeletePositronConsoleInstance((console) => { + this._proxy.$onDidDeletePositronConsoleInstance(console.sessionMetadata.sessionId); + }) + ) } dispose(): void { this._disposables.dispose(); } - private addConsole(sessionId: string, console: IPositronConsoleInstance) { - const mainThreadConsole = new MainThreadConsole(console); - this._mainThreadConsolesBySessionId.set(sessionId, mainThreadConsole); + private getConsoleForSessionId(sessionId: string): IPositronConsoleInstance | undefined { + return this._positronConsoleService.positronConsoleInstances.find((console) => console.sessionMetadata.sessionId === sessionId); } - // TODO: - // See comment in constructor - // - // private removeConsole(id: string) { - // // No dispose() method to call - // this._mainThreadConsolesByLanguageId.delete(id); - // } - // --- from extension host process $getConsoleWidth(): Promise { return Promise.resolve(this._positronConsoleService.getConsoleWidth()); } - /** - * Get the session id of the active console for a particular language id - * - * @param languageId The language id to find a session id for. - */ - $getSessionIdForLanguage(languageId: string): Promise { - // TODO: This is wrong in a multi-session world. It finds the - // first matching `languageId` in the map, but we likely want the "most - // recently activated and still alive" one. Reprex to prove it is wrong, - // which should eventually become a test: - // - Start R console 1 - // - Start R console 2 - // - Run `cli::cli_alert("{.run revdepcheck::cloud_summary()}")` in R - // console 2 and click the hyperlink. - // - The pasted code will incorrectly end up in R console 1. - - for (let [sessionId, console] of this._mainThreadConsolesBySessionId.entries()) { - if (console.getLanguageId() === languageId) { - return Promise.resolve(sessionId); - } - } - - return Promise.resolve(undefined); - } - - $tryPasteText(sessionId: string, text: string): void { - const mainThreadConsole = this._mainThreadConsolesBySessionId.get(sessionId); + $pasteText(sessionId: string, text: string): void { + const console = this.getConsoleForSessionId(sessionId); - if (!mainThreadConsole) { + if (!console) { return; } - mainThreadConsole.pasteText(text); + console.pasteText(text); } } diff --git a/src/vs/workbench/api/common/positron/extHost.positron.api.impl.ts b/src/vs/workbench/api/common/positron/extHost.positron.api.impl.ts index d80e59925159..4814144c1780 100644 --- a/src/vs/workbench/api/common/positron/extHost.positron.api.impl.ts +++ b/src/vs/workbench/api/common/positron/extHost.positron.api.impl.ts @@ -183,8 +183,8 @@ export function createPositronApiFactoryAndRegisterActors(accessor: ServicesAcce showSimpleModalDialogMessage(title: string, message: string, okButtonTitle?: string): Thenable { return extHostModalDialogs.showSimpleModalDialogMessage(title, message, okButtonTitle); }, - getConsoleForLanguage(languageId: string) { - return extHostConsoleService.getConsoleForLanguage(languageId); + getConsoleForSessionId(sessionId: string) { + return extHostConsoleService.getConsoleForSessionId(sessionId); }, get onDidChangeConsoleWidth() { return extHostConsoleService.onDidChangeConsoleWidth; diff --git a/src/vs/workbench/api/common/positron/extHost.positron.protocol.ts b/src/vs/workbench/api/common/positron/extHost.positron.protocol.ts index ebcc7e4cb831..e27664d8d29c 100644 --- a/src/vs/workbench/api/common/positron/extHost.positron.protocol.ts +++ b/src/vs/workbench/api/common/positron/extHost.positron.protocol.ts @@ -108,14 +108,13 @@ export interface ExtHostContextKeyServiceShape { } export interface MainThreadConsoleServiceShape { $getConsoleWidth(): Promise; - $getSessionIdForLanguage(languageId: string): Promise; - $tryPasteText(sessionId: string, text: string): void; + $pasteText(sessionId: string, text: string): void; } export interface ExtHostConsoleServiceShape { $onDidChangeConsoleWidth(newWidth: number): void; - $addConsole(sessionId: string): void; - $removeConsole(sessionId: string): void; + $onDidStartPositronConsoleInstance(sessionId: string): void; + $onDidDeletePositronConsoleInstance(sessionId: string): void; } export interface MainThreadMethodsShape { } diff --git a/src/vs/workbench/api/common/positron/extHostConsole.ts b/src/vs/workbench/api/common/positron/extHostConsole.ts index 8123bf21353e..6f036f3403db 100644 --- a/src/vs/workbench/api/common/positron/extHostConsole.ts +++ b/src/vs/workbench/api/common/positron/extHostConsole.ts @@ -4,22 +4,22 @@ *--------------------------------------------------------------------------------------------*/ import * as positron from 'positron'; +import { Disposable } from 'vscode'; import { MainThreadConsoleServiceShape } from './extHost.positron.protocol.js'; import { ILogService } from '../../../../platform/log/common/log.js'; /** * The extension host's view of a console instance * - * Cousin to `MainThreadConsole` - * * Do not add more methods to this class directly. Instead, add them to the * `positron.Console` API and implement them in `Object.freeze()` below by * calling out to the main thread. * - * `positron.Console` is modeled after the design of `vscode.TextEditor`, which - * similarly has both `ExtHostTextEditor` and `MainThreadTextEditor`. + * `positron.Console` is roughly modeled after the design of + * `vscode.TextEditor`, which has both `ExtHostTextEditor` and + * `MainThreadTextEditor`. We currently only have `ExtHostConsole`. */ -export class ExtHostConsole { +export class ExtHostConsole implements Disposable { private _disposed: boolean = false; @@ -41,7 +41,7 @@ export class ExtHostConsole { logService.warn('Console is closed/disposed.'); return; } - proxy.$tryPasteText(sessionId, text); + proxy.$pasteText(sessionId, text); } }); } diff --git a/src/vs/workbench/api/common/positron/extHostConsoleService.ts b/src/vs/workbench/api/common/positron/extHostConsoleService.ts index 830beb60db76..f56fc154c7ea 100644 --- a/src/vs/workbench/api/common/positron/extHostConsoleService.ts +++ b/src/vs/workbench/api/common/positron/extHostConsoleService.ts @@ -18,9 +18,16 @@ export class ExtHostConsoleService implements extHostProtocol.ExtHostConsoleServ * Multiple sessions could map to the same console, this happens * when a user power-cycles the session for a console instance * (i.e. shutdown session for console instance, then start a session for console instance) - * - * Kept in sync with consoles in `MainThreadConsoleService` */ + // TODO!: This doesn't survive a browser reload, and we don't really have a good way + // to revive it. We want to make sure of two things: + // - If a console is deleted, we update any existing handles to a `positron.Console` to + // ensure that it looks disposed and warns on any API usage + // - If the window is reloaded, we need to be able to still look up the console for + // a valid sessionId. The positron console service seems to maintain an up to date + // set of consoles, and is probably our source of truth. + // Is it even possible to hand out a `positron.Console` that survives a browser reload? + // Or should this be more of an `executeCode()`-like API that works off a `sessionId`? private readonly _extHostConsolesBySessionId = new Map(); private readonly _onDidChangeConsoleWidth = new Emitter(); @@ -46,28 +53,17 @@ export class ExtHostConsoleService implements extHostProtocol.ExtHostConsoleServ } /** - * Queries the main thread for the console that aligns with this - * `languageId`. + * Get the `positron.Console` that is tied to this `sessionId` * - * @param languageId The language id to find a console for. + * @param sessionId The session id to retrieve a `positron.Console` for. * @returns A promise that resolves to a `positron.Console` or `undefined` * if no console can be found. */ - async getConsoleForLanguage(languageId: string): Promise { - const sessionId = await this._proxy.$getSessionIdForLanguage(languageId); - - if (!sessionId) { - // Main thread says there is no `sessionId` for this `languageId` - return undefined; - } - - // Now find the console on the extension host side + async getConsoleForSessionId(sessionId: string): Promise { const extHostConsole = this._extHostConsolesBySessionId.get(sessionId); if (!extHostConsole) { // Extension host says there is no console for this `sessionId` - // (Should be extremely rare, if not impossible, for main thread and extension host to - // be out of sync here) return undefined; } @@ -83,13 +79,13 @@ export class ExtHostConsoleService implements extHostProtocol.ExtHostConsoleServ } // Called when a new console instance is started - $addConsole(sessionId: string): void { + $onDidStartPositronConsoleInstance(sessionId: string): void { const extHostConsole = new ExtHostConsole(sessionId, this._proxy, this._logService); this._extHostConsolesBySessionId.set(sessionId, extHostConsole); } - // Called when a console instance is removed - $removeConsole(sessionId: string): void { + // Called when a console instance is deleted + $onDidDeletePositronConsoleInstance(sessionId: string): void { const extHostConsole = this._extHostConsolesBySessionId.get(sessionId); this._extHostConsolesBySessionId.delete(sessionId); // "Dispose" of an `ExtHostConsole`, ensuring that future API calls warn / error