add terminal completions for other shell types #233776

meganrogge · 2024-11-13T17:30:02Z

fixes #233805
fixes #226562

Done:

add a setting, off by default and experimental
update suggest enabled setting description to indicate not just for pwsh?
when the cursor is at position 0 IE no prefix and I request completions, I should get all from the extension, but do not get any
pass cancellation token to provider call
inspect autocomplete src packages to delete ones we don't care about, re-run npm i to reduce size of package-lock.json
document TODOs with issues
Support fish completions #233799
allow providers to have unique replacement indices #233800
Add support for file/folder completions #233801
Provide arguments/options/subcommands for terminal completions #233802
create issue to add tests
add terminal extension completion tests #233803

Co-authored-by: Daniel Imms <[email protected]>

src/vs/workbench/api/browser/mainThreadTerminalService.ts

src/vs/workbench/contrib/terminalContrib/suggest/browser/pwshCompletionProviderAddon.ts

…ompletionProviderAddon.ts

meganrogge · 2024-11-14T17:11:33Z

Think I finally figured out the bundling issue

It did not like this

vscode/extensions/terminal-suggest/src/terminalSuggestMain.ts

Line 94 in 499475f

const module = await import(file);

But changing the tsconfig fixed it however, now running OSS, I see Activating extension 'vscode.terminal-suggest' failed: Cannot use import statement outside a module. 😢

meganrogge · 2024-11-14T17:17:55Z

looks like some tests are failing / leaking disposables now... looking into it

meganrogge · 2024-11-14T17:23:43Z

going to try to reproduce

Tyriar · 2024-11-14T17:38:14Z

But changing the tsconfig fixed it however, now running OSS, I see Activating extension 'vscode.terminal-suggest' failed: Cannot use import statement outside a module. 😢

You might need to hard code all the imports for now? I'm not sure how dynamic imports work with extensions.

Tyriar · 2024-11-14T17:41:27Z

extensions/terminal-suggest/README.md

+# Node npm
+
+**Notice:** This extension is bundled with Visual Studio Code. It can be disabled but not uninstalled.
+
+## Features
+
+### Terminal suggest
+
+Provides terminal suggestions for zsh and bash


We'll want to publish this up some before merging:

Tyriar · 2024-11-14T17:55:54Z

extensions/terminal-suggest/package.json

+    "@fig/autocomplete-generators": "^2.4.0",
+    "find-up": "^5.0.0",
+    "find-yarn-workspace-root": "^2.0.0",
+    "jsonc-parser": "^3.2.0",
+    "minimatch": "^5.1.6",
+    "request-light": "^0.7.0",
+    "vscode-uri": "^3.0.8",
+    "which": "^4.0.0",
+    "which-pm": "^2.1.1"


Do we need all these?

Tyriar · 2024-11-14T17:56:20Z

src/vs/workbench/contrib/terminalContrib/suggest/browser/terminalSuggestAddon.ts

+		const enableExtensionCompletions = this._configurationService.getValue<ITerminalSuggestConfiguration>(terminalSuggestConfigSection).enableExtensionCompletions;
+		if (enableExtensionCompletions && !this._hasActivatedExtensions) {
+			await this._extensionService.activateByEvent('onTerminalCompletionsRequested');
+			this._hasActivatedExtensions = true;
+		}


Do we need to always activate this event? If the user requests completions, then installs an extension, it will remain deactivated currently I think?

I see a bunch of listeners in _registerExtensionListeners now, but can we just fire the activation event and do away with that?

no because if you restart the extension host for example, it won't get reset so it won't get activated without the listeners?

Even if you just call activate every time? I thought that's how it's done everywhere else, as opposed to trying to cover all the cases like you're doing here?

Yeah other places just activate every time

Tyriar · 2024-11-14T17:56:32Z

src/vs/workbench/contrib/terminalContrib/suggest/common/terminalSuggestConfiguration.ts

 }

 export const terminalSuggestConfiguration: IStringDictionary<IConfigurationPropertySchema> = {
 	[TerminalSuggestSettingId.Enabled]: {
 		restricted: true,
-		markdownDescription: localize('suggest.enabled', "Enables experimental terminal Intellisense suggestions for supported shells ({0}) when {1} is set to {2}.\n\nIf shell integration is installed manually, {3} needs to be set to {4} before calling the shell integration script.", 'PowerShell v7+', `\`#${TerminalSettingId.ShellIntegrationEnabled}#\``, '`true`', '`VSCODE_SUGGEST`', '`1`'),
+		markdownDescription: localize('suggest.enabled', "Enables experimental terminal Intellisense suggestions for supported shells ({0}) when {1} is set to {2}.\n\nIf shell integration is installed manually, {3} needs to be set to {4} before calling the shell integration script. \n\nFor zsh and bash completions, {5} will also need to be set.", 'PowerShell v7+, zsh, bash', `\`#${TerminalSettingId.ShellIntegrationEnabled}#\``, '`true`', '`VSCODE_SUGGEST`', '`1`', `\`#${TerminalSuggestSettingId.EnableExtensionCompletions}#\``),
 		type: 'boolean',
 		default: false,


We should add experimental to this too

Tyriar · 2024-11-14T17:56:42Z

src/vs/workbench/services/suggest/browser/simpleCompletionModel.ts

+			// scored. If word is undefined, then match against the empty string.
+			item.word = word || '';


If word could be undefined wouldn't it throw above when word.length is called?

Tyriar · 2024-11-14T18:01:57Z

src/vs/workbench/contrib/terminalContrib/suggest/browser/terminalCompletionService.ts

-		// TODO: Use Promise.all so all providers are called in parallel
-		for (const providerMap of this._providers.values()) {
-			for (const [extensionId, provider] of providerMap) {
+		const collectCompletions = async (providers: ITerminalCompletionProvider[]) => {


Pull into private member function?

Tyriar · 2024-11-14T18:03:22Z

src/vs/workbench/contrib/terminalContrib/suggest/browser/terminalCompletionService.ts

@@ -57,31 +63,46 @@ export class TerminalCompletionService extends Disposable implements ITerminalCo
 		});
 	}

-	async provideCompletions(promptValue: string, cursorPosition: number, shellType: TerminalShellType): Promise<ISimpleCompletion[] | undefined> {
+	async provideCompletions(promptValue: string, cursorPosition: number, shellType: TerminalShellType, token: CancellationToken, triggeredProviders?: ITerminalCompletionProvider[]): Promise<ISimpleCompletion[] | undefined> {


Passing in triggeredProviders here is a little confusing. Could we not allow the terminal completion service complete control over what providers are used?

Tyriar · 2024-11-14T18:06:20Z

src/vs/workbench/contrib/terminalContrib/suggest/browser/terminalCompletionService.ts

 }

 export interface ITerminalCompletionService {
 	_serviceBrand: undefined;
+	providers: ITerminalCompletionProvider[];


Suggested change

providers: ITerminalCompletionProvider[];

readonly providers: ITerminalCompletionProvider[];

Tyriar · 2024-11-14T18:07:44Z

src/vs/workbench/contrib/terminalContrib/suggest/browser/terminalCompletionService.ts

 export class TerminalCompletionService extends Disposable implements ITerminalCompletionService {
 	declare _serviceBrand: undefined;
 	private readonly _providers: Map</*ext id*/string, Map</*provider id*/string, ITerminalCompletionProvider>> = new Map();
+	get providers() { return [...this._providers.values()].flatMap(providerMap => [...providerMap.values()]); }


A generator would be preferable here

Tyriar · 2024-11-14T18:10:04Z

src/vs/workbench/services/suggest/browser/simpleCompletionModel.ts

@@ -165,13 +164,13 @@ export class SimpleCompletionModel {
 				} else {
 					// by default match `word` against the `label`
 					const match = scoreFn(word, wordLow, wordPos, item.completion.label, item.labelLow, 0, this._fuzzyScoreOptions);
-					if (!match) {
+					if (!match && word !== '') {


Not sure why this was needed?

match is undefined if the prompt is empty. this is part of the fix for #233805

This worked before in pwsh fine though 🤔

meganrogge · 2024-11-14T18:13:55Z

I reverted the tsconfig change for now so you can test in oss

meganrogge and others added 30 commits September 17, 2024 12:46

start work

dffafc6

pass in shellType, commandLine

927cbbb

handle in terminal suggest addon

dffd1cc

modify check

b50cbe3

Tsconfig

281c385

get it to work

6d3e7dd

get it started

724afd3

basic csae working

2a9cb9d

Revert change to launch.json

5fcb3b8

update names, read me

7d77a05

suggest -> completion

291098d

rename

54a6fa6

make kind and label mandatory

cf2fa12

fix issues

c358e0a

stop for the day

0e33b08

fix issue

339a970

different approach

3cd70ff

readd check

30c5d9c

simplify

e8d9770

get cd to show up

0f6736f

Merge branch 'main' into merogge/start-terminal-suggest

fb17cc4

rm submodule

dffb0da

modify gitignore

e9f9ccc

fix issue

f13c3e0

handle completion model -> show completions

c1c4553

Merge branch 'main' into merogge/start-terminal-suggest

45aec85

fix issue

1b27e5f

Merge branch 'main' into merogge/start-terminal-suggest

9b1da41

revert change to gitignore

e4b6614

add pwsh regions, TODOs

fa5ed22

Co-authored-by: Daniel Imms <[email protected]>

meganrogge added 11 commits November 13, 2024 14:37

add setting

853a5f0

Tweak setting description

f5aaab4

tweak description

024fffe

set to false by default

e5164ce

command line empty, return ''

5ce6817

on config change, reactivate

7b8cae6

move to a function

8dbbf35

get it to work on empty prompt

f918e3e

get it to work for empty prompt

d19ab7d

rm failing test

064299a

pass in cancellation token

499475f

meganrogge commented Nov 13, 2024

View reviewed changes

src/vs/workbench/api/browser/mainThreadTerminalService.ts Outdated Show resolved Hide resolved

Update src/vs/workbench/api/browser/mainThreadTerminalService.ts

bf19a56

meganrogge commented Nov 13, 2024

View reviewed changes

src/vs/workbench/contrib/terminalContrib/suggest/browser/pwshCompletionProviderAddon.ts Outdated Show resolved Hide resolved

meganrogge added 7 commits November 13, 2024 15:57

Update src/vs/workbench/contrib/terminalContrib/suggest/browser/pwshC…

c7c8d5f

…ompletionProviderAddon.ts

add plugins to try to fix issue

4d23527

Try something else with webpack

69131e6

Try something else with webpack

fd61c3d

revert change to webpack, try something else

9edc7ab

try something else with tsconfig

750a9cb

set noEmit to false

9acbe65

meganrogge added 2 commits November 14, 2024 11:19

simplify tsconfig

477751b

don't leak disposable

f8fddea

Tyriar requested changes Nov 14, 2024

View reviewed changes

move back to commonJS so oss runs again

7650be2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add terminal completions for other shell types #233776

add terminal completions for other shell types #233776

meganrogge commented Nov 13, 2024 •

edited

Loading

meganrogge commented Nov 14, 2024 •

edited

Loading

meganrogge commented Nov 14, 2024

meganrogge commented Nov 14, 2024

Tyriar commented Nov 14, 2024

Tyriar Nov 14, 2024

Tyriar Nov 14, 2024

Tyriar Nov 14, 2024

Tyriar Nov 14, 2024

meganrogge Nov 14, 2024

Tyriar Nov 14, 2024

meganrogge Nov 14, 2024

Tyriar Nov 14, 2024

Tyriar Nov 14, 2024

Tyriar Nov 14, 2024

Tyriar Nov 14, 2024

Tyriar Nov 14, 2024

Tyriar Nov 14, 2024

Tyriar Nov 14, 2024

meganrogge Nov 14, 2024

Tyriar Nov 15, 2024

meganrogge commented Nov 14, 2024

		// scored. If word is undefined, then match against the empty string.
		item.word = word \|\| '';

	providers: ITerminalCompletionProvider[];
	readonly providers: ITerminalCompletionProvider[];

add terminal completions for other shell types #233776

Are you sure you want to change the base?

add terminal completions for other shell types #233776

Conversation

meganrogge commented Nov 13, 2024 • edited Loading

meganrogge commented Nov 14, 2024 • edited Loading

meganrogge commented Nov 14, 2024

meganrogge commented Nov 14, 2024

Tyriar commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meganrogge commented Nov 14, 2024

meganrogge commented Nov 13, 2024 •

edited

Loading

meganrogge commented Nov 14, 2024 •

edited

Loading