add remoteAuthority to IWorkspaceProvider.open #183414
Conversation
aeschli
changed the title
| private encodeWorkspacePath(uri: URI): string { | ||
| if (this.config.remoteAuthority && uri.scheme === Schemas.vscodeRemote) { | ||
| private encodeWorkspacePath(uri: URI, remoteAuthority: string | undefined): string { | ||
| if (uri.scheme === Schemas.vscodeRemote && isEqualAuthority(this.config.remoteAuthority, remoteAuthority)) { |
There was a problem hiding this comment.
This method is not clear to me anymore: what if the uri has a different remoteAuthority than what is passed in? Don't we have to rewrite the Uri then?
There was a problem hiding this comment.
If the uri defines a remoteAuthority, that on will be used. So remoteAuthority is really just for empty workspaces.
|
|
||
| // Empty | ||
| let targetHref: string | undefined = undefined; | ||
| if (!workspace) { | ||
| targetHref = `${document.location.origin}${document.location.pathname}?${WorkspaceProvider.QUERY_PARAM_EMPTY_WINDOW}=true`; | ||
| targetHref = `${document.location.origin}${document.location.pathname}?${WorkspaceProvider.QUERY_PARAM_EMPTY_WINDOW}=${remoteAuthority ?? 'true'}`; |
There was a problem hiding this comment.
Where are we reading out this remote authority on the other end?
|
I added the code that also reads out the authority from the query parameters. Thinking about this more, I want to suggest a different approach
The That should minimize the breakage and is better API. Let me know what you think and I can implement this. |
|
Yes that sounds reasonable! |
|
I implemented the suggestion with I think the cases where remotes are changed through open are quite rare. Only now this becomes a topic when vscode.dev supports locals and remotes and we have scenarios that switch from one remote to another (tunnels to 'wsl in tunnels') |
aeschli
force-pushed
the
aeschli/specified-cardinal-767
branch
from
0cb45d5 to
cef0796
Compare
| * The empty string as remote authority signals that a local window should be opened. | ||
| * IEmptyWorkspace is currently only used in the web embedder API. | ||
| */ | ||
| export interface IEmptyWorkspace extends IBaseWindowOpenable { |
There was a problem hiding this comment.
I wonder about terminology: should this rather be IEmptyWindow instead of empty workspace? In theory, a multi-root workspace could have 0 folders and also be called an empty workspace.
There was a problem hiding this comment.
IEmptyWindow is ok for me. But there can be files opened in it, so it's not always an empty window
I'm fine either way.
| } | ||
|
|
||
| export function isFileToOpen(uriToOpen: IWindowOpenable): uriToOpen is IFileToOpen { | ||
| return !!(uriToOpen as IFileToOpen).fileUri; | ||
| export function isEmptyWorkspace(ws: any): ws is IEmptyWorkspace { |
There was a problem hiding this comment.
Similar, empty window.
Btw I use similar terminology here:
vscode/src/vs/platform/backup/node/backup.ts
Lines 9 to 17 in 1ee7232
|
Test failures in the browser integration tests that I don't yet understand... |
|
Is this still relevant? |
This allows to specify a remote authority when opening an empty window through the IWorkspaceProvider.
This follows the same model as with the native host service:
The remoteAuthority can be undefined so it will be deducted from the currently open remote, or
nullthen a non-remote (local) window will be opened.