Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 18, 2021
Merged

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99d8d3e
Add back support for unencoded folder/workspace query params
code-asher Nov 10, 2021
d933129
Standardize on @author for marking our changes
code-asher Nov 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions src/vs/code/browser/workbench/workbench.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,16 @@ function doCreateUri(path: string, queryValues: Map<string, string>): URI {
return URI.parse(window.location.href).with({ path, query });
}

/**
* Encode a path for opening via the folder or workspace query parameter. This
* preserves slashes so it can be edited by hand more easily.
*
* @remark coder
*/
export const encodePath = (path: string): string => {
return path.split('/').map((p) => encodeURIComponent(p)).join('/');
};
Comment on lines +46 to +48
Copy link

Choose a reason for hiding this comment

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

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('/'), '/')

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the advantage to doing it this way?


interface ICredential {
service: string;
account: string;
Expand Down Expand Up @@ -319,13 +329,21 @@ class WorkspaceProvider implements IWorkspaceProvider {
}

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

Choose a reason for hiding this comment

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

Nice comment 👏

Copy link

Choose a reason for hiding this comment

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

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. */

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@code-asher code-asher Nov 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe I need to add highlighting for jsdoc tags.

Copy link
Member Author

@code-asher code-asher Nov 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

I'm down for @author and also standardizing!

else if (isFolderToOpen(workspace)) {
targetHref = `${document.location.origin}${document.location.pathname}?${WorkspaceProvider.QUERY_PARAM_FOLDER}=${encodeURIComponent(workspace.folderUri.toString())}`;
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}`;
Copy link

Choose a reason for hiding this comment

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

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()));

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}

// Workspace
// NOTE@coder: Modified to print as a human-readable string for file paths.
else if (isWorkspaceToOpen(workspace)) {
targetHref = `${document.location.origin}${document.location.pathname}?${WorkspaceProvider.QUERY_PARAM_WORKSPACE}=${encodeURIComponent(workspace.workspaceUri.toString())}`;
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}`;
Comment on lines +349 to +352
Copy link

Choose a reason for hiding this comment

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

🤔 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?

Copy link
Member Author

@code-asher code-asher Nov 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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!

Copy link

Choose a reason for hiding this comment

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

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

}

// Append payload if any
Expand Down Expand Up @@ -423,18 +441,28 @@ class WindowIndicator implements IWindowIndicator {
let payload = Object.create(null);
let logLevel: string | undefined = undefined;

// NOTE@coder: If the value begins with a slash assume it is a file path and
// convert it to use the vscode-remote scheme.
const toRemote = (value: string): string => {
if (value.startsWith("/")) {
return "vscode-remote://" + value;
Copy link

Choose a reason for hiding this comment

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

nit: modernize 😛

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}
return value
}

const query = new URL(document.location.href).searchParams;
query.forEach((value, key) => {
switch (key) {

// Folder
case WorkspaceProvider.QUERY_PARAM_FOLDER:
value = toRemote(value); // NOTE@coder: See above.
workspace = { folderUri: URI.parse(value) };
foundWorkspace = true;
break;

// Workspace
case WorkspaceProvider.QUERY_PARAM_WORKSPACE:
value = toRemote(value); // NOTE@coder: See above.
workspace = { workspaceUri: URI.parse(value) };
foundWorkspace = true;
break;
Expand Down