-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
plugin-ext: fix local-dir resolution #8385
Conversation
@@ -26,7 +26,7 @@ export class PluginVSCodeDeployerParticipant implements PluginDeployerParticipan | |||
|
|||
async onWillStart(context: PluginDeployerStartContext): Promise<void> { | |||
const extensionsDirUri = await this.environments.getExtensionsDirUri(); | |||
context.userEntries.push(extensionsDirUri.withScheme('local-dir').toString()); | |||
context.userEntries.push(extensionsDirUri.withScheme('local-dir').toString(true)); |
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.
Are you sure it is necessary?
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.
You are right, it works without. Will remove.
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.
Is there a reason why we prefer URI encoded strings? It made goofy-looking paths when debugging, and if someone doesn't parse it back into a URI object it will most likely cause issues. I could not find a reason why we want URI encoded strings here.
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.
We use encoded by default since we pass them over the wire and not encoded when we need to give to a user for navigation. Here it does not matter. What is important that one should use FileUri.parse
to get a system dependent path it can handle encoded and not encoded URIs.
packages/plugin-ext/src/main/node/resolvers/plugin-local-dir-resolver.ts
Show resolved
Hide resolved
@eclipse-theia/ecd-theia-committers Could someone with the window machine verify? |
a7bfef9
to
52d394a
Compare
Windows: Extensions are not picked up from the user's home. Node's FS API. Use `FileUri.fsPath` to get a valid system path. Signed-off-by: Paul Maréchal <[email protected]>
52d394a
to
a188c1b
Compare
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.
Verified. I installed the Go To Method
extension. Quit the app, start the app, the extension was in the view among the installed ones. It did not work from the master
.
Windows: Extensions are not picked up from the user's home.
Node's FS API.
Use
FileUri.fsPath
to get a valid system path.Signed-off-by: Paul Maréchal [email protected]
How to test
Windows:
Review checklist
Reminder for reviewers