Add back support for unencoded folder/workspace query params #15

code-asher · 2021-11-10T22:46:41Z

jsjoeio

LGTM! Left a couple comments but all none-blocking

src/vs/code/browser/workbench/workbench.ts

jsjoeio · 2021-11-10T23:27:00Z

src/vs/code/browser/workbench/workbench.ts

@@ -319,13 +329,21 @@ class WorkspaceProvider implements IWorkspaceProvider {
 		}

 		// Folder
+		// NOTE@coder: Modified to print as a human-readable string for file paths.


Nice comment 👏

Suggested change

// NOTE@coder: Modified to print as a human-readable string for file paths.

/** @coder Modified to print as a human-readable string for file paths. */

I can standardize on that if we want. I lean toward TODO, NOTE, etc because my editor highlights it so it stands out.

Maybe I need to add highlighting for jsdoc tags.

nvm I do have highlighting but my editor is not recognizing @coder as a valid tag name.

Maybe I can do multi-line and use @author coder.

I'm down for @author and also standardizing!

jsjoeio · 2021-11-10T23:28:33Z

src/vs/code/browser/workbench/workbench.ts

+	// convert it to use the vscode-remote scheme.
+	const toRemote = (value: string): string => {
+		if (value.startsWith("/")) {
+			return "vscode-remote://" + value;


nit: modernize 😛

Suggested change

return "vscode-remote://" + value;

return `vscode-remote://${value};

Something like?

import { URI } from 'vs/base/common/uri'; import { Schemas } from 'vs/base/common/network'; return URI.from({ scheme: Schemas.vscodeRemote, authority: '/foo/bar/baz’ }).toString(true /* skip encoding */) // => vscode-remote:///foo/bar/baz

Ahh template strings, I like to avoid them if there is just one variable at the start or end hahaha, I guess my old-school brain finds it more readable.

Good call on Schemas.vscodeRemote. I completely forgot to replace the hard-coded string with that.

The change feels semantically odd, right? Since what we have is a path and not an authority. I messed around with adding it to the path but it seems to lose the double slash. It does get passed into URI.parse so we should still get all the same validation that way.

src/vs/code/browser/workbench/workbench.ts

jsjoeio · 2021-11-10T23:31:11Z

src/vs/code/browser/workbench/workbench.ts

+			const target = workspace.workspaceUri.scheme === Schemas.vscodeRemote
+				? encodePath(workspace.workspaceUri.path)
+				: encodeURIComponent(workspace.workspaceUri.toString());
+			targetHref = `${document.location.origin}${document.location.pathname}?${WorkspaceProvider.QUERY_PARAM_WORKSPACE}=${target}`;


🤔 Just thinking out loud to make sure I understand this change. In these two if blocks, we're modifying the targetHref based on the workspace.folderUri.scheme. If the scheme matches the vscodeRemote (which I'm guessing would be localhost with code-server?), then we use our encodePath helper function which maintains the / in the URI/URL so that it's easier for users to edit these by hand.

Is that right?

vscodeRemote is vscode-remote. It is the scheme portion (file, http, https, etc).

In VS Code the vscode-remote scheme means the same thing as file, meaning it is on disk, but on the remote side.

Yup, you got it right!

Ahh just means it's on the remote side, got it. Thank you!

GirlBossRush · 2021-11-11T22:32:34Z

src/vs/code/browser/workbench/workbench.ts

@@ -319,13 +329,21 @@ class WorkspaceProvider implements IWorkspaceProvider {
 		}

 		// Folder
+		// NOTE@coder: Modified to print as a human-readable string for file paths.


Suggested change

// NOTE@coder: Modified to print as a human-readable string for file paths.

/** @coder Modified to print as a human-readable string for file paths. */

GirlBossRush · 2021-11-11T22:42:58Z

src/vs/code/browser/workbench/workbench.ts

+			const target = workspace.folderUri.scheme === Schemas.vscodeRemote
+				? encodePath(workspace.folderUri.path)
+				: encodeURIComponent(workspace.folderUri.toString());
+			targetHref = `${document.location.origin}${document.location.pathname}?${WorkspaceProvider.QUERY_PARAM_FOLDER}=${target}`;


Can this interpolation be replaced with a URL constructor? Something like:

const url = new URL(window.location.pathname, window.location.origin); url.searchParams.set( WorkspaceProvider.QUERY_PARAM_FOLDER, workspace.folderUri.scheme === Schemas.vscodeRemote ? encodePath(workspace.folderUri.path) : encodeURIComponent(workspace.folderUri.toString()));

This looks like it would work but I wanted to avoid modifying the original logic as much as possible.

GirlBossRush · 2021-11-11T22:50:21Z

src/vs/code/browser/workbench/workbench.ts

+export const encodePath = (path: string): string => {
+	return path.split('/').map((p) => encodeURIComponent(p)).join('/');
+};


nit

Suggested change

export const encodePath = (path: string): string => {

return path.split('/').map((p) => encodeURIComponent(p)).join('/');

};

export const encodeURIComponentHumanReadable = (path: string): string => {

// Encode everything...

return encodeURIComponent(path)

// Decode forward slashes to retain a human-readable result

.replaceAll(encodeURIComponent('/'), '/')

What is the advantage to doing it this way?

Add back support for unencoded folder/workspace query params

99d8d3e

Fixes coder/code-server#4484.

code-asher requested a review from jsjoeio November 10, 2021 22:46

code-asher force-pushed the code-server-v2 branch from a253181 to ef8f1c6 Compare November 10, 2021 22:59

code-asher mentioned this pull request Nov 10, 2021

Update VS Code coder/code-server#4494

Closed

jsjoeio approved these changes Nov 10, 2021

View reviewed changes

GirlBossRush suggested changes Nov 11, 2021

View reviewed changes

Standardize on @author for marking our changes

d933129

GirlBossRush approved these changes Nov 18, 2021

View reviewed changes

GirlBossRush merged commit 0e9d248 into coder:code-server-v2 Nov 18, 2021

jsjoeio mentioned this pull request Nov 18, 2021

Add back support for non-encoded syntax for folder and workspace query params coder/code-server#4484

Closed

code-asher deleted the decoded-query-params branch December 10, 2021 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add back support for unencoded folder/workspace query params #15

Add back support for unencoded folder/workspace query params #15

code-asher commented Nov 10, 2021

jsjoeio left a comment

jsjoeio Nov 10, 2021

GirlBossRush Nov 11, 2021

code-asher Nov 12, 2021

code-asher Nov 12, 2021 •

edited

Loading

code-asher Nov 12, 2021 •

edited

Loading

code-asher Nov 12, 2021

jsjoeio Nov 12, 2021

jsjoeio Nov 10, 2021

GirlBossRush Nov 11, 2021

code-asher Nov 12, 2021

jsjoeio Nov 10, 2021

code-asher Nov 12, 2021 •

edited

Loading

jsjoeio Nov 12, 2021

GirlBossRush Nov 11, 2021

GirlBossRush Nov 11, 2021

code-asher Nov 12, 2021

GirlBossRush Nov 11, 2021

code-asher Nov 12, 2021

	// NOTE@coder: Modified to print as a human-readable string for file paths.
	/** @coder Modified to print as a human-readable string for file paths. */

	return "vscode-remote://" + value;
	return `vscode-remote://${value};

-export const encodePath = (path: string): string => {
-	return path.split('/').map((p) => encodeURIComponent(p)).join('/');
-};
+export const encodeURIComponentHumanReadable = (path: string): string => {
+       // Encode everything...
+       return encodeURIComponent(path)
+         // Decode forward slashes to retain a human-readable result
+	  .replaceAll(encodeURIComponent('/'), '/')

Add back support for unencoded folder/workspace query params #15

Add back support for unencoded folder/workspace query params #15

Conversation

code-asher commented Nov 10, 2021

jsjoeio left a 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

code-asher Nov 12, 2021 • edited Loading

Choose a reason for hiding this comment

code-asher Nov 12, 2021 • edited Loading

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

code-asher Nov 12, 2021 • edited Loading

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

code-asher Nov 12, 2021 •

edited

Loading

code-asher Nov 12, 2021 •

edited

Loading

code-asher Nov 12, 2021 •

edited

Loading