[vscode] Support env.onDidChangeShell event #13097
Conversation
rschnekenbu
force-pushed
the
issues/13061
branch
from
a3a009c
to
3de96bb
Compare
| @@ -1021,6 +1027,7 @@ export class TerminalFrontendContribution implements FrontendApplicationContribu | |||
| } | |||
|
|
|||
| this.preferenceService.set(`terminal.integrated.defaultProfile.${OS.backend.type().toLowerCase()}`, result[0], PreferenceScope.User); | |||
| this.profileService.setDefaultProfile(result[0]); | |||
There was a problem hiding this comment.
This shouldn't be necessary, see line 275 in this file.
There was a problem hiding this comment.
Good point! I removed this line.
| @@ -220,6 +220,9 @@ export class TerminalFrontendContribution implements FrontendApplicationContribu | |||
| protected readonly onDidChangeCurrentTerminalEmitter = new Emitter<TerminalWidget | undefined>(); | |||
| readonly onDidChangeCurrentTerminal: Event<TerminalWidget | undefined> = this.onDidChangeCurrentTerminalEmitter.event; | |||
|
|
|||
| protected readonly onDidChangeShellEmitter = new Emitter<string>(); | |||
There was a problem hiding this comment.
Why are we routing this event through the terminal service instead of just listening to the event on the terminal profile service?
There was a problem hiding this comment.
I did remove this indirection, the terminalProfileService is now directly accessed.
| @@ -38,3 +41,9 @@ export class ShellTerminalProfile implements TerminalProfile { | |||
| return new ShellTerminalProfile(this.terminalService, { ...this.options, ...options }); | |||
| } | |||
| } | |||
|
|
|||
| export namespace ShellTerminalProfile { | |||
There was a problem hiding this comment.
ShellTerminalProfile is a class, we can do an instanceof check.
There was a problem hiding this comment.
Removed namespace addition, and use instanceof when required.
| import { TerminalService } from './base/terminal-service'; | ||
| import { TerminalWidget, TerminalWidgetOptions } from './base/terminal-widget'; | ||
| import { TerminalProfile } from './terminal-profile-service'; | ||
|
|
||
| export class ShellTerminalProfile implements TerminalProfile { | ||
| constructor(protected readonly terminalService: TerminalService, protected readonly options: TerminalWidgetOptions) { } | ||
|
|
||
| readonly shellPath: string; |
There was a problem hiding this comment.
If the shell path is optional in the options, it can't be mandatory in this variable. Something clashes here.
There was a problem hiding this comment.
This is now potentially undefined
|
|
||
| readonly shellPath: string; | ||
|
|
||
| constructor(protected readonly terminalService: TerminalService, protected readonly options: TerminalWidgetOptions) { this.shellPath = options.shellPath!; } |
There was a problem hiding this comment.
We keep the options anyway, why not do a getter?
There was a problem hiding this comment.
Good point, switched to a getter returning string | undefined.
contributed on behalf of STMicroelectronics Signed-off-by: Remi Schnekenburger <rschnekenburger@eclipsesource.com> Address review comments - remove indirection from TerminalService - change shellPath as getter rather than property - use instanceof instead of is()
rschnekenbu
force-pushed
the
issues/13061
branch
from
31725fb
to
1f32a87
Compare
What it does
Add env.onDidChangeShell event made public on vscode 1.83.
The documentation of this one was not obvious, and the doc was updated after testing on vscode side (see microsoft/vscode#194229). This event is triggered when the default profile change, it sends the shell used by this new terminal profile.
I added a sample extension to test this new API. The behavior is now similar to the one present on vscode. It provides a notification message as soon as the event is sent.
Fixes #13061
Contributed on behalf of ST Microelectronics
How to test
Terminal: Choose the default terminal profile. A popup with the shell path for the new default profile shall popup.Follow-ups
Review checklist
Reminder for reviewers