`terminalShellType` value is stuck at "gitbash"

[andreamah]

Testing #190104

This is sort of unrelated to the TPI, but when I inspect context keys on my terminal area, I notice that it has terminalShellType: "gitbash", even when the active terminal isn't using git bash. Here are my repro steps (not sure if this can be simplified):

1. create a terminal profile using an extension and launch the extension.
2. Open git bash, then close it and open the extension-provided terminal profile.
3. inspect context keys on the extension-provided terminal instance.
4. terminalShellType: "gitbash" :bug:

[Krytan]

I would like to solve this problem, if it is still an issue. Do you mind if I start? @Tyriar @andreamah

[Tyriar]

@Krytan sure!

[wdhongtw]

After scanning some source, there seems no good way to solve this. Because

1. there is no way for extension to specify terminal type when (AFAIK) extension register a terminal profile (TerminalProfileProvider ExtensionTerminalOptions Pseudoterminal), and
2. For Windows, shell type is determined by some heuristic logic, based on executable string, and default to return undefined. (For WSL, the shell type is always "undefined" in my quick experiment.)

When terminal service try to update terminalShellType context key, it fallback to NOP if the shell type is undefined, that's the behavior @andreamah noticed at first place.

See:

• https://code.visualstudio.com/api/references/vscode-api#ExtensionTerminalOptions
• https://github.com/microsoft/vscode/blob/main/src/vs/platform/terminal/node/windowsShellHelper.ts#L136
• https://github.com/microsoft/vscode/blob/main/src/vs/workbench/contrib/terminal/browser/terminalService.ts#L210

For this issue to be solved, probably there should be a way for builtin terminal profile to specify shell type (so it works for all bultin profile in all platform), and for extension provided terminal profiles, maybe we can add more optional fields to ExtensionTerminalOptions or method to Pseudoterminal.

As for the wierd behavior noticed currently, maybe we can set terminalShellType to "unknown", to make this state less confusing. But since that this behavior is already state in here, I wonder this can be changed.

[Tyriar]

@wdhongtw I think the problem is here:

https://github.com/microsoft/vscode/blob/973dc3065b154f31e6ca21b5fe80f0b23cf858a6/src/vs/workbench/contrib/terminal/browser/terminalService.ts#L209-L210

It should check if !instance first, then set it even if it's undefined if !!instance

[Tyriar]

@wdhongtw do you want to submit a PR since you investigated and found the problem?

[wdhongtw]

@Tyriar Yes, I can submit a PR for this.

But, before doing this, I need to confirm some detail.

The current behavior is

• From TerminalInstance.setShellType src\vs\workbench\contrib\terminal\browser\terminalInstance.ts
  • A. If new terminal instance is created, and shell type of the new terminal istance is defined, update terminalShellType
• From src\vs\workbench\contrib\terminal\browser\terminalService.ts
  • B. If active terminal instance change, and shell type of the new terminal istance is defined, update terminalShellType
  • C. If there is no terminal instance, reset terminalShellType
• D. otherwise, do nothing
  • so the editor keeps last known terminalShellType when shell type is undefined during new terminal creation or active terminal change.

It should check if !instance first, then set it even if it's undefined if !!instance

If so, what value should be set on this case? should we set it to "unknown"

--- a/src/vs/workbench/contrib/terminal/browser/terminalService.ts
+++ b/src/vs/workbench/contrib/terminal/browser/terminalService.ts
@@ -206,7 +206,9 @@ export class TerminalService extends Disposable implements ITerminalService {
                        if (!instance && !this._isShuttingDown) {
                                this._terminalGroupService.hidePanel();
                        }
-                       if (instance?.shellType) {
+                       if (!!instance && !instance.shellType) {
+                               this._terminalShellTypeContextKey.set('unknown');
+                       } else if (instance?.shellType) {
                                this._terminalShellTypeContextKey.set(instance.shellType.toString());
                        } else if (!instance) {
                                this._terminalShellTypeContextKey.reset();

or just call reset()? like

--- a/src/vs/workbench/contrib/terminal/browser/terminalService.ts
+++ b/src/vs/workbench/contrib/terminal/browser/terminalService.ts
@@ -208,7 +208,7 @@ export class TerminalService extends Disposable implements ITerminalService {
                        }
                        if (instance?.shellType) {
                                this._terminalShellTypeContextKey.set(instance.shellType.toString());
-                       } else if (!instance) {
+                       } else if (!instance || !(instance.shellType)) {
                                this._terminalShellTypeContextKey.reset();
                        }
                }));

---

@andreamah Can you provide the extension that open extension-provided terminal and trigger this issue?

[wdhongtw]

Some commits about the comment/design history of this context key

• 9e8496a6d9e02e5558fced1db5bd437e10c84515
• 1968633f31c511fb65665cdd5e40713a9b8ea645