fix #8903 Encode workspacePath before being used as URL hash value.

[shuyaqian]

What it does

fixes #8903
Encode workspacePath before being used as URL hash value when openNewWindow.

How to test

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[vince-fugnitto]

Thank you for your contribution @shuyaqian 👍
Before it can be accepted, please be sure to:

• include a proper sign-off for your commit.
• be sure to sing the Eclipse Contributor Agreement (ECA) with the same email as your sign-off

This is the reason for the failing ECA check.

[shuyaqian]

Thank you for your contribution @shuyaqian 👍
Before it can be accepted, please be sure to:

• include a proper sign-off for your commit.
• be sure to sing the Eclipse Contributor Agreement (ECA) with the same email as your sign-off

This is the reason for the failing ECA check.

I've added it. Thank you.

[vince-fugnitto]

@shuyaqian the changes work for me when starting the example browser application, but not the electron (desktop) application.

To test you can do the following:

• start the example-electron app: yarn start:electron
• attempt to open a folder with %2 in the name

[kittaakos]

What about this location?

theia/packages/workspace/src/browser/workspace-service.ts

Line 153 in f1d4690

window.location.hash = workspacePath;

[kittaakos]

but not the electron (desktop) application.

I guess here is the missing part:

theia/packages/core/src/electron-main/electron-main-application.ts

Line 275 in f1d4690

workspacePath = await fs.realpath(path.resolve(params.cwd, options.file));

This is quite a breaking, isn't it? I might be overlooking something, but if this is a breaking change, please include it in the changelog. Thanks!

[shuyaqian]

but not the electron (desktop) application.

I guess here is the missing part:

theia/packages/core/src/electron-main/electron-main-application.ts

Line 275 in f1d4690

workspacePath = await fs.realpath(path.resolve(params.cwd, options.file));

This is quite a breaking, isn't it? I might be overlooking something, but if this is a breaking change, please include it in the changelog. Thanks!

Thank you for your suggestion. I've revised it.

[vince-fugnitto]

@shuyaqian sorry for taking a while to get back to the pull-request, would you mind rebasing the pull-request to the latest master, and also including an breaking change note in the changelog regarding the changes (it might be better to be transparent that urls are now encoded when opening the application from now on).

[kittaakos]

What about this location?

theia/packages/workspace/src/browser/workspace-service.ts

Line 153 in f1d4690

window.location.hash = workspacePath;

Please also check if we need to change it here. Thanks!

[shuyaqian]

What about this location?

theia/packages/workspace/src/browser/workspace-service.ts

Line 153 in f1d4690

window.location.hash = workspacePath;

Please also check if we need to change it here. Thanks!

I've revised it, thank you very much!

[vince-fugnitto]

@shuyaqian thank you for the changes! 👍
I confirmed that opening a workspace named a%20b can now be successfully opened in both the browser and electron targets.

[paul-marechal]

Encoding should be done when setting withFragment, not here.

[paul-marechal]

@shuyaqian are this changes not useful anymore? Note that I had an unresolved comment.