Skip to content
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

Workspace: add url encoding #9850

Merged

Conversation

OmarSdt-EC
Copy link
Contributor

@OmarSdt-EC OmarSdt-EC commented Aug 5, 2021

The commit adds the encoding of URIs. After this update, we can now
open workspaces with reserved symbols like '%' in their name.

What it does

FIxes #8909
The commit introduces the use of encoding when we are loading/reloading a workspace URL.
Previously, the application would fail to open a workspace URL with reserved characters such as %.

How to test

  1. Try to open a workspace with '%' in the name
  2. Check if it opens correctly

Review checklist

Reminder for reviewers

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed some unexpected behavior on browser reload:

  1. Open a workspace with %20 in its directory name
  2. Observe that the fragment in the url does not contain the escaped sequence for %20 but instead a space character.
  3. Reloading the browser window closes the workspace. (No workspace is selected upon reload)

@OmarSdt-EC
Copy link
Contributor Author

I noticed some unexpected behavior on browser reload:

  1. Open a workspace with %20 in its directory name
  2. Observe that the fragment in the url does not contain the escaped sequence for %20 but instead a space character.
  3. Reloading the browser window closes the workspace. (No workspace is selected upon reload)

Thank you @msujew , I'll take a look at that

@vince-fugnitto vince-fugnitto added the workspace issues related to the workspace label Aug 9, 2021
@OmarSdt-EC OmarSdt-EC force-pushed the os/fix-encodeWorkspace branch 3 times, most recently from dc323cd to d718690 Compare August 11, 2021 19:03
@OmarSdt-EC OmarSdt-EC changed the title URI:folders with '%20' in their name can be opened Workspace: add url encoding Aug 11, 2021
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that the issue exists on master and is addressed by this change nicely. I tested this on both browser and Electron targets (Windows):

  • Opening a directory with %20 in its name works correctly ✔️
  • The encoded URL appears in the browser bar ✔️
  • Reloading the application correctly brings one back to the previously opened project ✔️

@OmarSdt-EC OmarSdt-EC force-pushed the os/fix-encodeWorkspace branch 2 times, most recently from b0f0896 to eed2909 Compare August 26, 2021 17:19
The commit introduces the use of `encoding` when we are loading/reloading a workspace URL.
Previously, the application would fail to open a workspace URL with reserved characters such as `%`.
@OmarSdt-EC OmarSdt-EC force-pushed the os/fix-encodeWorkspace branch from eed2909 to 6468e6b Compare August 26, 2021 17:58
@paul-marechal paul-marechal merged commit 237894d into eclipse-theia:master Aug 26, 2021
@paul-marechal paul-marechal added this to the 1.17.0 milestone Aug 26, 2021
RomanNikitenko pushed a commit that referenced this pull request Sep 16, 2021
The commit introduces the use of `encoding` when we are loading/reloading a workspace URL.
Previously, the application would fail to open a workspace URL with reserved characters such as `%`.
RomanNikitenko pushed a commit that referenced this pull request Sep 16, 2021
The commit introduces the use of `encoding` when we are loading/reloading a workspace URL.
Previously, the application would fail to open a workspace URL with reserved characters such as `%`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
workspace issues related to the workspace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants