define pathSeparator in ext host for terminal suggest
#239540
Conversation
|
failing tests |
| /** | ||
| * Whether to normalize the path by replacing / with | ||
| * \\. This should be true when the OS is Windows. | ||
| */ | ||
| shouldNormalizePath?: boolean; |
There was a problem hiding this comment.
Should we just derive this from pathSeparator and always normalize in TerminalCompletionService when pathSeparator is \? The path separator could also be optional and default to \ when the ext host is Windows.
I'm also thinking of how we would apply this for the special cases like git bash and wsl (not on remote). For those we'd actually need some mapping function or call to correctly convert the paths so just setting pathSeparator to / won't be sufficient. Perhaps we should just not include this or pathSeparator this on the API at all for now and pass it through from exthost -> renderer, just not in the API. Then when think more about git bash/wsl support we can come up with a more robust solution, we may need something like this:
pathStyle: 'os' | 'wsl' | 'gitbash'`
meganrogge
force-pushed
the
merogge/normalize
branch
from
a61401f to
a58d48f
Compare
meganrogge
changed the title
shouldNormalizePath to terminal suggest resource request configpathSeparator in ext host for terminal suggest
| @@ -208,7 +206,8 @@ export class TerminalCompletionService extends Disposable implements ITerminalCo | |||
| } | |||
|
|
|||
| async resolveResources(resourceRequestConfig: TerminalResourceRequestConfig, promptValue: string, cursorPosition: number, provider: string, capabilities: ITerminalCapabilityStore): Promise<ITerminalCompletion[] | undefined> { | |||
| if (resourceRequestConfig.shouldNormalizePrefix) { | |||
| const useBackslash = resourceRequestConfig.pathSeparator === '\\'; | |||
There was a problem hiding this comment.
Little more obvious? useWindowsStylePath
| @@ -65,7 +64,6 @@ export interface TerminalResourceRequestConfig { | |||
| foldersRequested?: boolean; | |||
| cwd?: URI; | |||
| pathSeparator: string; | |||
There was a problem hiding this comment.
I think this type should live in extHost.protocol.ts as that defines types used across exthost and main procs?
| if (Array.isArray(completions)) { | ||
| return completions; | ||
| } else { | ||
| return new TerminalCompletionList(completions.items, { ...completions.resourceRequestConfig, pathSeparator: isWindows ? '\\' : '/' }); |
There was a problem hiding this comment.
I think we want to convert this to a DTO in extHost.protocol.ts so we're not passing an object over IPC which could serialize incorrectly
| @@ -779,7 +780,7 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I | |||
| }); | |||
| } | |||
|
|
|||
| public async $provideTerminalCompletions(id: string, options: ITerminalCompletionContextDto): Promise<vscode.TerminalCompletionItem[] | vscode.TerminalCompletionList | undefined> { | |||
| public async $provideTerminalCompletions(id: string, options: ITerminalCompletionContextDto): Promise<vscode.TerminalCompletionItem[] | TerminalCompletionList | undefined> { | |||
There was a problem hiding this comment.
It was extHost.protocol.ts I was thinking of here, that defines types that go across IPC. We shouldn't be returning objects or API interfaces here.
See $provideTerminalQuickFixes as an example:
There was a problem hiding this comment.
Ah that makes more sense - I had recalled you said extHostTypes, but was confused bc that is still in browser so would be wrong for remote.
fix #239411