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

Jump to bottom

#28617 - implemented according to comments in PR #28952

Closed
wants to merge 17 commits into from
Closed

#28617 - implemented according to comments in PR #28952

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
77751c9
#28617 - implemented according to comments in PR
cristianhosu Jun 18, 2017
c2134a2
#28617 - changed according to PR 28952
cristianhosu Jun 19, 2017
2cd6f12
#28617 - modified according to PR comments
cristianhosu Jun 20, 2017
7c5078e
#28617 - removed references to quick open action
cristianhosu Jun 23, 2017
ebd7bb0
merge monaco.d.ts
cristianhosu Jun 23, 2017
10ff9cb
Merge remote-tracking branch 'upstream/master'
cristianhosu Jun 23, 2017
cd6c611
Merge from upstream/master
cristianhosu Jun 23, 2017
b2d9137
merge from upstream
cristianhosu Jun 23, 2017
1e98f71
Merge branch 'master' into #28617
cristianhosu Jun 23, 2017
c1626a7
fixed merge duplication
cristianhosu Jun 23, 2017
be27e6b
Merge remote-tracking branch 'upstream/master'
cristianhosu Jun 24, 2017
0f20e2e
merge
cristianhosu Jun 24, 2017
f16860f
Merge branch '#28617'
cristianhosu Jun 24, 2017
02d5a9d
Merge branch 'master' into #28617
cristianhosu Jun 24, 2017
ee650e6
#28617 - added command to menu registry
cristianhosu Jul 7, 2017
ce7b823
#28617
cristianhosu Jul 7, 2017
244df43
Merge branch 'master' into #28617
cristianhosu Jul 7, 2017
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
10 changes: 10 additions & 0 deletions src/vs/workbench/api/node/extHostApiCommands.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,16 @@ export class ExtHostApiCommands {
{ name: 'column', description: '(optional) Column in which to open', constraint: v => v === void 0 || typeof v === 'number' }
]
});

this._register('vscode.quickOpen', (prefix: string) => {
return this._commands.executeCommand('_workbench.quickOpen', [prefix]);
}, {
description: 'Shows the quick open widget',
args: [
{ name: 'prefix', description: '(optional) Open the quick open widget with a default prefix' },
{ name: 'showOptions', description: '(optional) Show options' }
]
});
}

// --- command impl
Expand Down
6 changes: 3 additions & 3 deletions src/vs/workbench/browser/parts/quickopen/quickopen.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { Action } from 'vs/base/common/actions';
import { IQuickOpenService } from 'vs/platform/quickOpen/common/quickOpen';
import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding';
import { ContextKeyExpr } from "vs/platform/contextkey/common/contextkey";
import { ICommandHandler } from "vs/platform/commands/common/commands";
import { ICommandHandler, ICommandService } from "vs/platform/commands/common/commands";

export const inQuickOpenContext = ContextKeyExpr.has('inQuickOpen');
export const defaultQuickOpenContextKey = 'inFilesPicker';
Expand All @@ -22,15 +22,15 @@ export class GlobalQuickOpenAction extends Action {
public static ID = 'workbench.action.quickOpen';
public static LABEL = nls.localize('quickOpen', "Go to File...");

constructor(id: string, label: string, @IQuickOpenService private quickOpenService: IQuickOpenService) {
constructor(id: string, label: string, @IQuickOpenService private quickOpenService: IQuickOpenService, @ICommandService private commandService: ICommandService) {
super(id, label);

this.order = 100; // Allow other actions to position before or after
this.class = 'quickopen';
}

public run(): TPromise<any> {
this.quickOpenService.show(null);
this.commandService.executeCommand('vscode.quickOpen');
Copy link
Member

Choose a reason for hiding this comment

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

This is madness. Don't add a new command, replace the action with a command having the same id, register the command to the command palette. Don't add an API command

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really think this is madness per se, but okay. I see what you mean. It's not okay to have both a command and an action that do the same thing. But for my piece of mind and since I've just started working on VS Code, can you please detail what;s the difference between an API command, a command and an action? what are they each used for?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for being harsh. There is a world of debt and basically an action is a command with extras. A command is basically just a function with an identifier. Then we use that identifier to bind command execution to a user gesture, e.g. bind a keybinding to an identifier or bind a UI-botton to an identifier. These UI gestures basically all call executeCommand with their assigned id. That is why running exec-command from within an action is a double loop (the run is already the result of calling exec-command).

A simple sample of registering a command (id to function) and a menu item (button to id) can be found here: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/html/browser/webview.ts#L28. Not shown in that sample, but commands have access to all services via the first argument passed to them.

Copy link
Member

@jrieken jrieken Jun 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

So, my recommendation is to turn the GlobalQuickOpenCommand into a namespace which export (as const) the ID. That is because others use the ID and you wanna keep things simple. Then inside the namespace you can just do the registration and things should just work. Let me know if there something I can help out with


return TPromise.as(true);
}
Expand Down
10 changes: 10 additions & 0 deletions src/vs/workbench/electron-browser/commands.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { CommandsRegistry } from 'vs/platform/commands/common/commands';
import { IWorkbenchEditorService } from 'vs/workbench/services/editor/common/editorService';
import URI from 'vs/base/common/uri';
import { IEditorOptions, Position as EditorPosition } from 'vs/platform/editor/common/editor';
import { IQuickOpenService } from 'vs/platform/quickOpen/common/quickOpen';

// --- List Commands

Expand Down Expand Up @@ -422,4 +423,13 @@ export function registerCommands(): void {
return void 0;
});
});

CommandsRegistry.registerCommand('_workbench.quickOpen', function (accessor: ServicesAccessor, args: [string]) {
const quickOpenService = accessor.get(IQuickOpenService);
Copy link
Member

Choose a reason for hiding this comment

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

Remove IShowOptions

const [prefix] = args;

return quickOpenService.show(prefix).then(() => {
return void 0;
});
});
}