Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement executeCommand for rstudioapi as OpenRPC contract #2356

Merged
merged 7 commits into from
Mar 4, 2024
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
2 changes: 1 addition & 1 deletion extensions/positron-python
2 changes: 1 addition & 1 deletion extensions/positron-r/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@
},
"positron": {
"binaryDependencies": {
"ark": "0.1.60"
"ark": "0.1.61"
}
}
}
15 changes: 15 additions & 0 deletions positron/comms/ui-frontend-openrpc.json
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,21 @@
],
"result": {}
},
{
"name": "execute_command",
"summary": "Execute a Positron command",
"description": "Use this to execute a Positron command from the backend (like from a runtime)",
"params": [
{
"name": "command",
"description": "The command to execute",
"schema": {
"type": "string"
}
}
],
"result": {}
},
{
"name": "last_active_editor_context",
"summary": "Context metadata for the last editor",
Expand Down
8 changes: 8 additions & 0 deletions src/positron-dts/positron.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1117,5 +1117,13 @@ declare module 'positron' {
* Returns a `EditorContext` for the last active editor.
*/
export function lastActiveEditorContext(): Thenable<EditorContext | null>;

/**
* Executes a Positron command.
*
* @param command The Positron command name.
*/
export function executeCommand(commandId: string): Thenable<null>;

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { Disposable, DisposableStore } from 'vs/base/common/lifecycle';
import { Event, Emitter } from 'vs/base/common/event';
import { IPositronConsoleService } from 'vs/workbench/services/positronConsole/browser/interfaces/positronConsoleService';
import { IPositronVariablesService } from 'vs/workbench/services/positronVariables/common/interfaces/positronVariablesService';
import { INotificationService } from 'vs/platform/notification/common/notification';
import { ILogService } from 'vs/platform/log/common/log';
import { IRuntimeClientInstance, RuntimeClientState, RuntimeClientType } from 'vs/workbench/services/languageRuntime/common/languageRuntimeClientInstance';
import { DeferredPromise } from 'vs/base/common/async';
Expand All @@ -24,7 +25,7 @@ import { IPositronHelpService } from 'vs/workbench/contrib/positronHelp/browser/
import { INotebookService } from 'vs/workbench/contrib/notebook/common/notebookService';
import { IRuntimeClientEvent } from 'vs/workbench/services/languageRuntime/common/languageRuntimeUiClient';
import { URI } from 'vs/base/common/uri';
import { BusyEvent, UiFrontendEvent, OpenEditorEvent, PromptStateEvent, WorkingDirectoryEvent } from 'vs/workbench/services/languageRuntime/common/positronUiComm';
import { BusyEvent, UiFrontendEvent, OpenEditorEvent, PromptStateEvent, WorkingDirectoryEvent, ShowMessageEvent } from 'vs/workbench/services/languageRuntime/common/positronUiComm';
import { IEditorService } from 'vs/workbench/services/editor/common/editorService';
import { ITextResourceEditorInput } from 'vs/platform/editor/common/editor';
import { IPositronDataExplorerService } from 'vs/workbench/services/positronDataExplorer/browser/interfaces/positronDataExplorerService';
Expand Down Expand Up @@ -105,6 +106,7 @@ class ExtHostLanguageRuntimeAdapter implements ILanguageRuntime {
readonly metadata: ILanguageRuntimeMetadata,
readonly dynState: ILanguageRuntimeDynState,
private readonly _languageRuntimeService: ILanguageRuntimeService,
private readonly _notificationService: INotificationService,
private readonly _logService: ILogService,
private readonly _notebookService: INotebookService,
private readonly _editorService: IEditorService,
Expand Down Expand Up @@ -180,6 +182,10 @@ class ExtHostLanguageRuntimeAdapter implements ILanguageRuntime {
// Update current working directory
const dir = ev.data as WorkingDirectoryEvent;
this.dynState.currentWorkingDirectory = dir.directory;
} else if (ev.name === UiFrontendEvent.ShowMessage) {
// Show a message
const msg = ev.data as ShowMessageEvent;
this._notificationService.info(msg.message);
}

// Propagate event
Expand Down Expand Up @@ -950,6 +956,7 @@ export class MainThreadLanguageRuntime implements MainThreadLanguageRuntimeShape
@IPositronHelpService private readonly _positronHelpService: IPositronHelpService,
@IPositronPlotsService private readonly _positronPlotService: IPositronPlotsService,
@IPositronIPyWidgetsService private readonly _positronIPyWidgetsService: IPositronIPyWidgetsService,
@INotificationService private readonly _notificationService: INotificationService,
@ILogService private readonly _logService: ILogService,
@INotebookService private readonly _notebookService: INotebookService,
@IEditorService private readonly _editorService: IEditorService,
Expand Down Expand Up @@ -995,6 +1002,7 @@ export class MainThreadLanguageRuntime implements MainThreadLanguageRuntimeShape
metadata,
dynState,
this._languageRuntimeService,
this._notificationService,
this._logService,
this._notebookService,
this._editorService,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { ExtHostConsoleService } from 'vs/workbench/api/common/positron/extHostC
import { ExtHostMethods } from './extHostMethods';
import { ExtHostEditors } from '../extHostTextEditors';
import { UiFrontendRequest } from 'vs/workbench/services/languageRuntime/common/positronUiComm';
import { ExtHostCommands } from 'vs/workbench/api/common/extHostCommands';

/**
* Factory interface for creating an instance of the Positron API.
Expand Down Expand Up @@ -57,12 +58,13 @@ export function createPositronApiFactoryAndRegisterActors(accessor: ServicesAcce
const extHostLanguageFeatures: ExtHostLanguageFeatures =
rpcProtocol.getRaw(ExtHostContext.ExtHostLanguageFeatures);
const extHostEditors: ExtHostEditors = rpcProtocol.getRaw(ExtHostContext.ExtHostEditors);
const extHostCommands: ExtHostCommands = rpcProtocol.getRaw(ExtHostContext.ExtHostCommands);

const extHostLanguageRuntime = rpcProtocol.set(ExtHostPositronContext.ExtHostLanguageRuntime, new ExtHostLanguageRuntime(rpcProtocol));
const extHostPreviewPanels = rpcProtocol.set(ExtHostPositronContext.ExtHostPreviewPanel, new ExtHostPreviewPanels(rpcProtocol, extHostWebviews, extHostWorkspace));
const extHostModalDialogs = rpcProtocol.set(ExtHostPositronContext.ExtHostModalDialogs, new ExtHostModalDialogs(rpcProtocol));
const extHostConsoleService = rpcProtocol.set(ExtHostPositronContext.ExtHostConsoleService, new ExtHostConsoleService(rpcProtocol, extHostLogService));
const extHostMethods = rpcProtocol.set(ExtHostPositronContext.ExtHostMethods, new ExtHostMethods(rpcProtocol, extHostEditors));
const extHostMethods = rpcProtocol.set(ExtHostPositronContext.ExtHostMethods, new ExtHostMethods(rpcProtocol, extHostEditors, extHostCommands));

return function (extension: IExtensionDescription, extensionInfo: IExtensionRegistries, configProvider: ExtHostConfigProvider): typeof positron {

Expand Down Expand Up @@ -145,6 +147,9 @@ export function createPositronApiFactoryAndRegisterActors(accessor: ServicesAcce
lastActiveEditorContext(): Thenable<positron.EditorContext | null> {
return extHostMethods.lastActiveEditorContext();
},
executeCommand(commandId: string): Thenable<null> {
return extHostMethods.executeCommand(commandId);
},
};

// --- End Positron ---
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ export interface MainThreadMethodsShape { }

export interface ExtHostMethodsShape {
lastActiveEditorContext(): Promise<IEditorContext | null>;
executeCommand(commandId: string): Promise<null>;
}

/**
Expand Down
14 changes: 14 additions & 0 deletions src/vs/workbench/api/common/positron/extHostMethods.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import * as extHostProtocol from './extHost.positron.protocol';
import { ExtHostEditors } from '../extHostTextEditors';
import { ExtHostCommands } from '../extHostCommands';
import { UiFrontendRequest, EditorContext } from 'vs/workbench/services/languageRuntime/common/positronUiComm';
import { JsonRpcErrorCode } from 'vs/workbench/services/languageRuntime/common/positronBaseComm';
import { EndOfLine } from '../extHostTypeConverters';
Expand Down Expand Up @@ -32,6 +33,7 @@ export class ExtHostMethods implements extHostProtocol.ExtHostMethodsShape {
constructor(
_mainContext: extHostProtocol.IMainPositronContext,
private readonly editors: ExtHostEditors,
private readonly commands: ExtHostCommands
) {
}

Expand Down Expand Up @@ -67,6 +69,13 @@ export class ExtHostMethods implements extHostProtocol.ExtHostMethodsShape {
result = await this.debugSleep(params.ms as number);
break;
}
case UiFrontendRequest.ExecuteCommand: {
if (!params || !Object.keys(params).includes('command')) {
return newInvalidParamsError(method);
}
result = await this.executeCommand(params.command as string);
break;
}
}

return <JsonRpcResult>({ result });
Expand Down Expand Up @@ -144,6 +153,11 @@ export class ExtHostMethods implements extHostProtocol.ExtHostMethodsShape {
};
}

async executeCommand(commandId: string): Promise<null> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we'll quickly run into the need of passing arguments? These could be implemented as a Array<any> in the OpenRPC contract (IIRC we have support for this, or at least partial support) and then applied with the spread syntax.

This means a downside of executeCommand compared to a normal request is decreased type-checking of arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None of the commands I am using so far have any arguments, so I would prefer to leave this as is for now, until we need args here and can test it out better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In my view we should keep this executeCommand as the RStudio API emulator -- it accepts only RStudio command IDs and maps them to VS Code commands and/or emulates them in some other way.

If we want an API that accepts VS Code/Positron command IDs and semantics (can take args and return values), I think that'd be a wholly separate API.

Copy link
Contributor Author

@juliasilge juliasilge Mar 1, 2024

Choose a reason for hiding this comment

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

Ah OK, that is different from how this is implemented now; right now this accepts only VS Code commands. We had a discussion about that here in amalthea.

What do you think on this? Should we go with Davis' suggestion there and move the mapping from RStudio commands to VS Code commands over here in Positron?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also was thinking that the RStudio API emulation would remain fully contained in Ark. From that perspective, the fact that the emulation happens to use this new UI comm API for executing VS Code commands would be an implementation detail.

(In any case, regarding extending to supporting arguments I agree it's better left to another PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clearly reasonable people can disagree on this 😄 but I want to join Lionel in gently advocating for the current implementation, i.e. rstudioapi emulation belongs in Ark.

await this.commands.executeCommand(commandId);
return null;
}

async debugSleep(ms: number): Promise<null> {
await delay(ms);
return null;
Expand Down
15 changes: 15 additions & 0 deletions src/vs/workbench/services/languageRuntime/common/positronUiComm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,20 @@ export interface DebugSleepRequest {

}

/**
* Request: Execute a Positron command
*
* Use this to execute a Positron command from the backend (like from a
* runtime)
*/
export interface ExecuteCommandRequest {
/**
* The command to execute
*/
command: string;

}

/**
* Request: Context metadata for the last editor
*
Expand All @@ -247,6 +261,7 @@ export enum UiFrontendEvent {

export enum UiFrontendRequest {
DebugSleep = 'debug_sleep',
ExecuteCommand = 'execute_command',
LastActiveEditorContext = 'last_active_editor_context'
}

Expand Down
Loading