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.

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

Overwrite editor options for vscode.open and vscode.diff #110398

Closed
wants to merge 2 commits into from
Closed

Overwrite editor options for vscode.open and vscode.diff #110398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd968fe
wip
bpasero Nov 11, 2020
27f7f71
allow certain commands to pass
bpasero Nov 11, 2020
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
25 changes: 21 additions & 4 deletions src/vs/workbench/api/common/apiCommands.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@
*--------------------------------------------------------------------------------------------*/

import type * as vscode from 'vscode';
import { URI } from 'vs/base/common/uri';
import { URI, UriComponents } from 'vs/base/common/uri';
import * as typeConverters from 'vs/workbench/api/common/extHostTypeConverters';
import { CommandsRegistry, ICommandService, ICommandHandler } from 'vs/platform/commands/common/commands';
import { ITextEditorOptions } from 'vs/platform/editor/common/editor';
import { IEditorOptions, ITextEditorOptions } from 'vs/platform/editor/common/editor';
import { EditorViewColumn } from 'vs/workbench/api/common/shared/editor';
import { EditorGroupLayout } from 'vs/workbench/services/editor/common/editorGroupsService';
import { ServicesAccessor } from 'vs/platform/instantiation/common/instantiation';
Expand All @@ -17,6 +17,7 @@ import { Schemas } from 'vs/base/common/network';
import { ILogService } from 'vs/platform/log/common/log';
import { IEnvironmentService } from 'vs/platform/environment/common/environment';
import { IViewDescriptorService, IViewsService, ViewVisibilityState } from 'vs/workbench/common/views';
import { SIDE_GROUP } from 'vs/workbench/services/editor/common/editorService';

// -----------------------------------------------------------------
// The following commands are registered on both sides separately.
Expand Down Expand Up @@ -112,7 +113,20 @@ CommandsRegistry.registerCommand(DiffAPICommand.ID, adjustHandler(DiffAPICommand

export class OpenAPICommand {
public static readonly ID = 'vscode.open';
public static execute(executor: ICommandsExecutor, resource: URI, columnOrOptions?: vscode.ViewColumn | typeConverters.TextEditorOpenOptions, label?: string): Promise<any> {
public static execute(executor: ICommandsExecutor, resource: UriComponents, columnOrOptions?: vscode.ViewColumn | typeConverters.TextEditorOpenOptions, label?: string): Promise<any> {
const internalTypes = OpenAPICommand._toInternalTypes(columnOrOptions);

return OpenAPICommand._doExecute(executor, resource, internalTypes.options, internalTypes.position, label);
}
public static executeWithContext(executor: ICommandsExecutor, context: { editorOptions: IEditorOptions; sideBySide: boolean }, resource: UriComponents, columnOrOptions?: vscode.ViewColumn | typeConverters.TextEditorOpenOptions, label?: string): Promise<any> {
const internalTypes = OpenAPICommand._toInternalTypes(columnOrOptions);

const options = { ...(internalTypes.options ?? Object.create(null)), ...context.editorOptions };
const position = context.sideBySide ? SIDE_GROUP : internalTypes.position;

return OpenAPICommand._doExecute(executor, resource, options, position, label);
}
private static _toInternalTypes(columnOrOptions?: vscode.ViewColumn | typeConverters.TextEditorOpenOptions): { position: number | undefined, options: ITextEditorOptions | undefined } {
let options: ITextEditorOptions | undefined;
let position: EditorViewColumn | undefined;

Expand All @@ -125,8 +139,11 @@ export class OpenAPICommand {
}
}

return { position, options };
}
private static _doExecute(executor: ICommandsExecutor, resource: UriComponents, options: ITextEditorOptions | undefined, position: number | undefined, label: string | undefined): Promise<any> {
return executor.executeCommand('_workbench.open', [
resource,
URI.revive(resource),
options,
position,
label
Expand Down
37 changes: 23 additions & 14 deletions src/vs/workbench/api/common/extHostCommands.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { URI } from 'vs/base/common/uri';
import { DisposableStore, toDisposable } from 'vs/base/common/lifecycle';
import { createDecorator } from 'vs/platform/instantiation/common/instantiation';
import { IExtHostRpcService } from 'vs/workbench/api/common/extHostRpcService';
import { DiffAPICommand, OpenAPICommand } from 'vs/workbench/api/common/apiCommands';

interface CommandHandler {
callback: Function;
Expand Down Expand Up @@ -247,20 +248,28 @@ export class CommandsConverter {

if (command.command && isNonEmptyArray(command.arguments)) {
// we have a contributed command with arguments. that
// means we don't want to send the arguments around

const id = ++this._cachIdPool;
this._cache.set(id, command);
disposables.add(toDisposable(() => {
this._cache.delete(id);
this._logService.trace('CommandsConverter#DISPOSE', id);
}));
result.$ident = id;

result.id = this._delegatingCommandId;
result.arguments = [id];

this._logService.trace('CommandsConverter#CREATE', command.command, id);
// means we don't want to send the arguments around unless
// we know exactly that the arguments are serializable

if (command.command === OpenAPICommand.ID || command.command === DiffAPICommand.ID) {
// this enables `vscode.open` and `vscode.diff` to benefit
// from the actual editor options to use based on the context
// (e.g. invocation from a custom tree view)
result.arguments = command.arguments;
Copy link
Member

Choose a reason for hiding this comment

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

I have tried the same thing right now and I can report that it doesn't work... This is what now happens

  • the arguments go through normal JSON-serialization (which for vscode.Range is special)
  • the command get send to the renderer
  • the renderer has vscode.open registered as well (need @alexdima to refresh my memory here)
  • when selecting an item in the tree the renderer-side vscode.open command is invoked and it fails to convert the selection/range argument

Copy link
Member

@jrieken jrieken Nov 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It seems have not coordinated well - sorry for the duplication. I have pushed some work here: https://github.com/microsoft/vscode/compare/joh/open

The idea is that CommandConverter#toInternal doesn't only create/feed the delegate command but that it actually converts some (API) commands to the internal variant. E.g a tree item will then be using _workbench.open and so on

Copy link
Member

Choose a reason for hiding this comment

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

@jrieken What can I help with?

Copy link
Member

Choose a reason for hiding this comment

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

There is src/vs/workbench/api/common/apiCommands.ts which registers API commands on both sides - renderer and extension host. Not sure why that was done. Also, it registers commands that accept arguments which are types of vsocde.d.ts which means they "don't survive" the JSON serialisation/deserialisation, like vscode.Selection.

Copy link
Member

@jrieken jrieken Nov 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I remember... We made API command registration extension host only so that we can have multiple extension host (the global flag) but then we hit bugs with API-commands that escaped the extension host without treatment, like in markdown and then we added some API-commands to the renderer as well. It seems that some have evolved into a state where they only partially work in the renderer

Copy link
Member Author

Choose a reason for hiding this comment

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

@jrieken I think your changes in https://github.com/microsoft/vscode/compare/joh/open are cleaner, so I am fine to drop my work and only keep the special massaging of the 2 commands on the main side. To clarify, this would mean we end up having _workbench.open as command in the tree and not vscode.open? I actually prefer that because now I need to depend on some internal API knowledge in the tree. I would rather depend on _workbench.open 👍

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it will be _workbench.open and it's friends. I have created #110455 which I am planning to merge despite some polish of where to revive uris and so

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool 👍

Copy link
Member

Choose a reason for hiding this comment

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

#110455 is merged

} else {
const id = ++this._cachIdPool;
this._cache.set(id, command);
disposables.add(toDisposable(() => {
this._cache.delete(id);
this._logService.trace('CommandsConverter#DISPOSE', id);
}));
result.$ident = id;

result.id = this._delegatingCommandId;
result.arguments = [id];

this._logService.trace('CommandsConverter#CREATE', command.command, id);
}
}

return result;
Expand Down
6 changes: 5 additions & 1 deletion src/vs/workbench/api/common/extHostTypeConverters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@ export namespace Range {
};
}

export function fromPositions(positions: PositionLike[]): editorRange.IRange {
return from({ start: positions[0], end: positions[1] });
}

export function to(range: undefined): types.Range;
export function to(range: editorRange.IRange): types.Range;
export function to(range: editorRange.IRange | undefined): types.Range | undefined;
Expand Down Expand Up @@ -1194,7 +1198,7 @@ export namespace TextEditorOpenOptions {
pinned: typeof options.preview === 'boolean' ? !options.preview : undefined,
inactive: options.background,
preserveFocus: options.preserveFocus,
selection: typeof options.selection === 'object' ? Range.from(options.selection) : undefined,
selection: Array.isArray(options.selection) ? Range.fromPositions(options.selection) : typeof options.selection === 'object' ? Range.from(options.selection) : undefined,
override: typeof options.override === 'boolean' ? false : undefined
};
}
Expand Down
10 changes: 8 additions & 2 deletions src/vs/workbench/contrib/views/browser/treeView.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import { IHoverDelegate, IHoverDelegateOptions } from 'vs/base/browser/ui/iconLa
import { IMarkdownString } from 'vs/base/common/htmlContent';
import { IIconLabelMarkdownString } from 'vs/base/browser/ui/iconLabel/iconLabel';
import { renderMarkdownAsPlaintext } from 'vs/base/browser/markdownRenderer';
import { OpenAPICommand } from 'vs/workbench/api/common/apiCommands';

class Root implements ITreeItem {
label = { label: 'root' };
Expand Down Expand Up @@ -462,8 +463,13 @@ export class TreeView extends Disposable implements ITreeView {
return;
}
const selection = this.tree!.getSelection();
if ((selection.length === 1) && selection[0].command) {
this.commandService.executeCommand(selection[0].command.id, ...(selection[0].command.arguments || []));
const command = selection[0].command;
if ((selection.length === 1) && command) {
if (command.id === OpenAPICommand.ID) {
OpenAPICommand.executeWithContext(this.commandService, e, command.arguments?.[0], command.arguments?.[1], command.arguments?.[2]);
} else {
this.commandService.executeCommand(command.id, ...(command.arguments || []));
}
}
}));

Expand Down