-
Notifications
You must be signed in to change notification settings - Fork 101
Update "Interpreter" command palette actions #7803
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
base: main
Are you sure you want to change the base?
Update "Interpreter" command palette actions #7803
Conversation
E2E Tests 🚀 |
@@ -39,11 +38,14 @@ interface RuntimeClientTypeQuickPickItem extends IQuickPickItem { runtimeClientT | |||
interface RuntimeClientInstanceQuickPickItem extends IQuickPickItem { runtimeClientInstance: IRuntimeClientInstance<any, any> } | |||
|
|||
// Action IDs | |||
export const LANGUAGE_RUNTIME_OPEN_ACTIVE_SESSIONS_ID = 'workbench.action.language.runtime.openActivePicker'; | |||
export const LANGUAGE_RUNTIME_START_SESSION_ID = 'workbench.action.language.runtime.openStartPicker'; | |||
export const LANGUAGE_RUNTIME_SELECT_SESSION_ID = 'workbench.action.language.runtime.selectSession'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used to be workbench.action.language.runtime.openActivePicker
but has been renamed so its more clear what this action is used for.
@@ -39,11 +38,14 @@ interface RuntimeClientTypeQuickPickItem extends IQuickPickItem { runtimeClientT | |||
interface RuntimeClientInstanceQuickPickItem extends IQuickPickItem { runtimeClientInstance: IRuntimeClientInstance<any, any> } | |||
|
|||
// Action IDs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to create a const for all the session specific actions since those are likely to be re-used in UI components as well.
export const LANGUAGE_RUNTIME_DUPLICATE_SESSION_ID = 'workbench.action.language.runtime.duplicateSession'; | ||
export const LANGUAGE_RUNTIME_DUPLICATE_ACTIVE_SESSION_ID = 'workbench.action.language.runtime.duplicateActiveSession'; | ||
|
||
export const LANGUAGE_RUNTIME_SELECT_RUNTIME_ID = 'workbench.action.languageRuntime.selectRuntime'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This action looks to only be used by the notebook services. We may want to split up session vs runtime commands but there's not a ton of non-session specific commands so I've left it for now.
allowStartSession?: boolean; | ||
title?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the placeholder
option which wasn't being used anywhere and added a title
option so other actions which use the helper function can provide their own specific title. You'll see this be set for the actions that can restart/stop/quit a session.
* @param placeholder The placeholder for the quick input. | ||
* @returns The language runtime the user selected, or undefined, if there are no running language runtimes or the user canceled the operation. | ||
*/ | ||
const selectRunningLanguageRuntime = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function has been removed because its replaced by selectLanguageRuntimeSession
which is setup to work with multi sessions. All use cases of this have been replaced with selectLanguageRuntimeSession
* @param accessor The service accessor. | ||
* @param sessionId The ID of the session to rename. | ||
*/ | ||
const renameLanguageRuntimeSession = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just moved this helper function to the top of the file along with the other helper functions.
registerLanguageRuntimeAction('workbench.action.languageRuntime.setActive', 'Set Active Interpreter', async accessor => { | ||
// Get the language runtime service. | ||
const runtimeSessionService = accessor.get(IRuntimeSessionService); | ||
|
||
// Have the user select the language runtime they wish to set as the active language runtime. | ||
const session = await selectRunningLanguageRuntime( | ||
accessor, | ||
localize('positron.lanuageRuntime.setActive', 'Set the active language runtime')); | ||
|
||
// If the user selected a language runtime, set it as the active language runtime. | ||
if (session) { | ||
runtimeSessionService.foregroundSession = session; | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This action was removed because we have a replacement for it (LANGUAGE_RUNTIME_SELECT_SESSION_ID
) that came out of the multi session work
@@ -579,148 +518,10 @@ export function registerLanguageRuntimeActions() { | |||
} | |||
}); | |||
|
|||
// Registers the start language runtime action. | |||
registerAction2(class SelectInterpreterAction extends Action2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an action that we don't show users in the command palette.
This action returns the most recently created session for the selected runtime if there is one, otherwise it creates a new one. The LANGUAGE_RUNTIME_SELECT_SESSION_ID
action sort of does the same thing (but better from a UX point of view).
I didn't find any references of it being used in our code so I'm removing it.
* | ||
* @returns The language runtime the user selected, or undefined, if the user canceled the operation. | ||
*/ | ||
const selectLanguageRuntime = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am removing this helper function because it is only used by the workbench.action.languageRuntime.select
which is being removed.
export const LANGUAGE_RUNTIME_DUPLICATE_SESSION_ID = 'workbench.action.language.runtime.duplicateSession'; | ||
export const LANGUAGE_RUNTIME_DUPLICATE_ACTIVE_SESSION_ID = 'workbench.action.language.runtime.duplicateActiveSession'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed this to specify its for the active session
id: 'workbench.action.languageRuntime.pick', | ||
title: nls.localize2('positron.command.pickInterpreter', "Pick Interpreter"), | ||
id: LANGUAGE_RUNTIME_SELECT_RUNTIME_ID, | ||
title: nls.localize2('positron.command.selectInterpreter', "Select Interpreter"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed this from Pick
to Select
for consistency. This action isn't exposed to users in the command palette - looks like its only used for notebooks
/** | ||
* Action that allows the user to interrupt an active session. | ||
*/ | ||
registerLanguageRuntimeAction('workbench.action.languageRuntime.interrupt', 'Interrupt Interpreter Session', async accessor => { | ||
(await selectLanguageRuntimeSession( | ||
accessor, | ||
{ title: 'Select Interpreter Session To Interrupt' }))?.interrupt(); | ||
}); | ||
|
||
/** | ||
* Action that allows the user to shutdown an active session. | ||
*/ | ||
registerLanguageRuntimeAction('workbench.action.languageRuntime.shutdown', 'Shutdown Interpreter Session', async accessor => { | ||
(await selectLanguageRuntimeSession( | ||
accessor, | ||
{ title: 'Select Interpreter Session To Shutdown' }))?.shutdown(); | ||
}); | ||
|
||
/** | ||
* Action that allows the user to force-quit an active session. | ||
*/ | ||
registerLanguageRuntimeAction('workbench.action.languageRuntime.forceQuit', 'Force Quit Interpreter Session', async accessor => { | ||
(await selectLanguageRuntimeSession( | ||
accessor, | ||
{ title: 'Select Interpreter Session To Force Quit' }))?.forceQuit(); | ||
}); | ||
|
||
/** | ||
* Action that allows the user to show the output channel for an active session. | ||
*/ | ||
registerLanguageRuntimeAction('workbench.action.languageRuntime.showOutput', 'Show Interpreter Session Output', async accessor => { | ||
(await selectLanguageRuntimeSession( | ||
accessor, | ||
{ title: 'Select Interpreter Session To Show Output' }))?.showOutput(); | ||
}); | ||
|
||
/** | ||
* Action that allows the user to show the profile report for an active session. | ||
*/ | ||
registerLanguageRuntimeAction('workbench.action.languageRuntime.showProfile', 'Show Interpreter Session Profile Report', async accessor => { | ||
(await selectLanguageRuntimeSession( | ||
accessor, | ||
{ title: 'Select Interpreter Session To Show Profile Report' }))?.showProfile(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were all updated to use the session selector picker we created for multi session
registerLanguageRuntimeAction('workbench.action.language.runtime.openClient', 'Create Runtime Client Widget', async accessor => { | ||
// Access services. | ||
const quickInputService = accessor.get(IQuickInputService); | ||
|
||
// Prompt the user to select a session | ||
const session = await selectLanguageRuntimeSession(accessor); | ||
if (!session) { | ||
return; | ||
} | ||
|
||
// Prompt the user to select the runtime client type. | ||
const selection = await quickInputService.pick<RuntimeClientTypeQuickPickItem>( | ||
[ | ||
{ | ||
id: generateUuid(), | ||
label: 'Environment Pane', | ||
runtimeClientType: RuntimeClientType.Variables, | ||
} | ||
], | ||
{ | ||
canPickMany: false, | ||
placeHolder: `Select runtime client for ${session.runtimeMetadata.runtimeName}` | ||
} | ||
return undefined; | ||
); | ||
|
||
// If the user selected a runtime client type, create the client for it. | ||
if (selection) { | ||
session.createClient(selection.runtimeClientType, null); | ||
} | ||
}); | ||
|
||
registerLanguageRuntimeAction('workbench.action.language.runtime.closeClient', 'Close Runtime Client Widget', async accessor => { | ||
const quickInputService = accessor.get(IQuickInputService); | ||
|
||
// Prompt the user to select a session | ||
const session = await selectLanguageRuntimeSession(accessor); | ||
if (!session) { | ||
return; | ||
} | ||
|
||
// Get the runtime client instances for the session. | ||
const runtimeClientInstances = await session.listClients(); | ||
if (!runtimeClientInstances.length) { | ||
alert('No clients are currently started.'); | ||
return; | ||
} | ||
|
||
// Create runtime client instance quick pick items. | ||
const runtimeClientInstanceQuickPickItems = runtimeClientInstances.map<RuntimeClientInstanceQuickPickItem>(runtimeClientInstance => ({ | ||
id: generateUuid(), | ||
label: runtimeClientInstance.getClientType(), | ||
runtimeClientInstance, | ||
} satisfies RuntimeClientInstanceQuickPickItem)); | ||
|
||
// Prompt the user to select a runtime client instance. | ||
const selection = await quickInputService.pick<RuntimeClientInstanceQuickPickItem>(runtimeClientInstanceQuickPickItems, { | ||
canPickMany: false, | ||
placeHolder: nls.localize('Client Close Selection Placeholder', 'Close Client for {0}', session.runtimeMetadata.runtimeName) | ||
}); | ||
|
||
// If the user selected a runtime client instance, dispose it. | ||
if (selection) { | ||
selection.runtimeClientInstance.dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two actions gets exposed in the command palette and I don't know what the use case is for these. Should we be showing these to users in the command palette? Alternatively, should these be moved into the “Developer” category?
// TODO: Should this be in a "Developer: " command? | ||
// TODO: Should this be a "Developer: " command? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the comments it looks like this command is mainly used by QA. Can this be moved into the “Developer” category? I don’t love providing this as a path for users to run code because it’s a pretty odd experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to expose it in the command palette at all, right? The tests execute the command using its ID?
}); | ||
|
||
// Registers the clear affiliated language runtime / clear saved interpreter action. | ||
registerLanguageRuntimeAction('workbench.action.languageRuntime.clearAffiliation', 'Clear Saved Interpreter', async accessor => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was renamed to workbench.action.languageRuntime.clearAffiliatedRuntime
export const LANGUAGE_RUNTIME_OPEN_ACTIVE_SESSIONS_ID = 'workbench.action.language.runtime.openActivePicker'; | ||
export const LANGUAGE_RUNTIME_START_SESSION_ID = 'workbench.action.language.runtime.openStartPicker'; | ||
export const LANGUAGE_RUNTIME_SELECT_SESSION_ID = 'workbench.action.language.runtime.selectSession'; | ||
export const LANGUAGE_RUNTIME_START_NEW_SESSION_ID = 'workbench.action.language.runtime.startNewSession'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used to be workbench.action.language.runtime.openStartPicker
but has been renamed so its more clear what this action is used for.
I kicked off the full e2e suite here: https://github.com/posit-dev/positron/actions/runs/15188737550 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking soooooo much more consistent! 🙌
I would advocate for neither the "Execute Code in Console/Execute Code Silently" commands nor those runtime client commands to be surfaced in the command palette.
For the changes in the Python extension that are causing the linting problem, try running npm run format-fix
from the Python extension directory. I think I typically can avoid problems of this nature by working from the multi-root workspace i.e. code positron.code-workspace
, but maybe something has gone awry with that.
src/vs/workbench/contrib/languageRuntime/browser/languageRuntimeActions.ts
Outdated
Show resolved
Hide resolved
registerLanguageRuntimeAction('workbench.action.languageRuntime.interrupt', 'Interrupt Interpreter Session', async accessor => { | ||
(await selectLanguageRuntimeSession( | ||
accessor, | ||
{ title: 'Select Interpreter Session To Interrupt' }))?.interrupt(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you only renamed this and did not change this behavior, but I think we might want to get another person to comment on this (should we continue to expose this in the command palette?). I feel fuzzy on how "Interrupt Console Session" is supposed to behave, and it seems like a strange thing for a user to want to do. Do we have a use case in mind?
This same question applies maybe a little bit to "Shutdown Interpreter Session" but less so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, now that you mention it - these block of commands make more sense if they only act on the session for the active console instance - almost all of these actions have a button equivalent in the console action bar.
We do have an interrupt button in the console action bar that shows up when code is executing so I think this is something that we could leave in the command palette (mainly for users who prefer doing everything via keyboard):
Screen.Recording.2025-05-22.at.10.55.26.AM.mov
The basic use case feels like its an escape hatch for users for long running code that should be stopped.
I think the ability to shutdown a session could be removed from the command palette (since we removed the accompanying button from the console action bar). This would mean the only way a user could invoke a shutdown would be via the q()/exit()
commands.
Co-authored-by: Julia Silge <[email protected]> Signed-off-by: Dhruvi Sompura <[email protected]>
I chatted with @softwarenerd about the client widget commands about these actions and it seems like we can hide all of these. |
Addresses #7189
This PR goes through all the interpreter commands we expose to users in the command palette and updates them so the naming scheme is consistent and works better with multiple interpreter sessions. This also updates a couple other UI element labels that trigger the command palette actions so labels match across the board.
Note: The diff is a bit messy looking because I reordered a bunch of the actions, so hiding whitespace is recommended. I am also commenting the actions that were renamed/removed so its easier to find the destructive changes.
For most of the actions, a user is now prompted to select an interpreter session they want to take the action on. There are a couple actions that specifically act on the foreground session/active console, and those now specify that by using the term "Active Interpreter Session/ Active Console".
There’s a few actions in this list that I wasn’t sure what to do with and would love feedback on:
Here's the new list of interpreter actions after the cleanup:
The plus button in the console pane was also updated so the hover label matches the action being triggered. Prior to the fix, the button always. showed the "Duplicate Session" label - even when the action being triggered was the create new session action.
Screen.Recording.2025-05-23.at.4.33.05.PM.mov
Release Notes
New Features
Bug Fixes
QA Notes
@:sessions @:web @:win @:console