-
Notifications
You must be signed in to change notification settings - Fork 28.6k
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.
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
#28617 - implemented according to comments in PR #28952
Conversation
@cristianhosu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jrieken and @bpasero to be potential reviewers. |
Why should that be a command and not proper API? |
@bpasero gave an example on how this should be done on the previous pull request. I have deleted that one since my fork got messed up and had to clean/pull from upstream. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some feedback provided.
As for the name of the command, not sure if @jrieken has objections with vscode.quickOpen
. For a user I think the difference between the picker (e.g. showQuickPick
) and our quick open thing is not so obvious. For a user it will always be the picker.
Alternatively we could introduce real API like showQuickPick(prefix: string)
that will bring up quick open with the prefix. That might be easier to understand.
@@ -239,6 +240,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, showOptions: IShowOptions) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about exposing IShowOptions
to extensions, I would not do this until we hear otherwise.
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); | ||
public run(prefix?: string, showOptions?: IShowOptions): TPromise<any> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove prefix
and showOptions
from this action right? We are not using these arguments anywhere. The extension is using the command.
@@ -422,4 +423,13 @@ export function registerCommands(): void { | |||
return void 0; | |||
}); | |||
}); | |||
|
|||
CommandsRegistry.registerCommand('_workbench.quickOpen', function (accessor: ServicesAccessor, args: [string, IShowOptions]) { | |||
const quickOpenService = accessor.get(IQuickOpenService); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove IShowOptions
Unsure, about overloading What I don't understand is why another command ( |
@jrieken yes I like that. is every command we register on the workbench side automatically available from extensions? @cristianhosu can we introduce a command with the same ID as the action and use that from the action? |
Yes, every command is everywhere. By convention commands starting with an underscore are internal and shouldn't be used. |
I don't think so. i believe that the shortcut will look into both actions and commands and search for one with the given name, thus it might be error prone to have the same name. We could however remove the action altogether and leave only the action so to reduce the ambiguity.
Initially that's how it was made, but maybe a command is easier to migrate to an actual API if needed in the future (I'm just guessing). |
Yes, replacing the action with a command sounds good to me. As long as the action still shows up in the command palette. |
Now, the command executes the show method from the service. The same one
that gets executed from the action.
The command has _ because it is executed by an API command vscode.quickOpen.
I would change all the current bindings of the action to execute the command instead and leave the action as is so that if anyone has a keybinding to this action to not disrupt it.
If you meant something else by your comment, please give more details as I did not quite understand.
Also i've made the requested changes.
|
Thanks, I will have a look. |
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'); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@jrieken I've pushed another commit. |
@@ -423,3 +424,16 @@ export function registerCommands(): void { | |||
}); | |||
}); | |||
} | |||
|
|||
namespace GlobalQuickOpenCommand { | |||
export const ID = 'vscode.quickOpen'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cristianhosu shouldn't this be the same ID as the action we already have and not introduce a new command ID? And the action we had so far should use this command too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a previous comment, @jrieken commented on the exact line that i've modified in the action to call the command instead of the service.
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)
I'll try using the same ID and see if it works, but it could just execute both the command and the action, or maybe have a conflict between them, or randomly execute either the action or the command. I'll investigate this and come back with an actual response instead of an assumption.
Also the ID of the action is workbench.action.quickOpen and now it is implemented as a command instead of an action. Wouldn't this name induce confusion?
Edit: with the name changed the command is executed instead of the action. So maybe we could remove the action altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I meant. Nuke the action, the ID is now slight misleading but we don't wanna break existing keybinding customisations.
Merge upstream master to local master
@jrieken : So much win! Thank you
…On Mon, Jun 26, 2017 at 9:18 AM, Johannes Rieken ***@***.***> wrote:
***@***.**** approved this pull request.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#28952 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AQ3E7vCu-CRICTcxpGtp7NZ7CI838yRDks5sH01RgaJpZM4N9bUu>
.
--
Cristian Hosu
+40740.058.711
|
@cristianhosu what is the motivation of the change in watermark.ts? |
@@ -18,7 +18,6 @@ import { IWorkspaceContextService } from 'vs/platform/workspace/common/workspace | |||
import { IWorkbenchContribution, IWorkbenchContributionsRegistry, Extensions as WorkbenchExtensions } from 'vs/workbench/common/contributions'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not change watermark.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It had a reference to the Action. As I removed the action, this had to be removed as well
@@ -12,7 +12,6 @@ import * as nls from 'vs/nls'; | |||
import * as panel from 'vs/workbench/browser/panel'; | |||
import * as platform from 'vs/base/common/platform'; | |||
import { Extensions, IConfigurationRegistry } from 'vs/platform/configuration/common/configurationRegistry'; | |||
import { GlobalQuickOpenAction } from 'vs/workbench/browser/parts/quickopen/quickopen'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It had a reference to the Action. As I removed the action, this had to be removed as well
@@ -423,3 +424,24 @@ export function registerCommands(): void { | |||
}); | |||
}); | |||
} | |||
|
|||
namespace GlobalQuickOpenCommand { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cristianhosu while this adds the command, it no longer shows up in the F1 command palette. @jrieken didn't we add support to contribute commands to the F1 list recently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is support for this, i'll add it to F1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cristianhosu looks like currently we are not using this explicitly but this test shows how to add a command to the F1 list: https://github.com/Microsoft/vscode/blob/master/src/vs/platform/actions/test/common/menuService.test.ts#L192
Can you add this?
@@ -423,3 +424,24 @@ export function registerCommands(): void { | |||
}); | |||
}); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove the namespace GlobalQuickOpenCommand
here, not sure why we would want it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bpasero that makes this change smaller because GlobalQuickOpenCommand.ID
is used here and there
Of course. Are there any other changes that should be made? For the
watermark file for example? Maybe somehow using the command instead of the
action?
…On Jun 28, 2017 5:59 PM, "Benjamin Pasero" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/vs/workbench/electron-browser/commands.ts
<#28952 (comment)>:
> @@ -423,3 +424,24 @@ export function registerCommands(): void {
});
});
}
+
+namespace GlobalQuickOpenCommand {
@cristianhosu <https://github.com/cristianhosu> looks like currently we
are not using this explicitly but this test shows how to add a command to
the F1 list: https://github.com/Microsoft/vscode/blob/master/src/vs/
platform/actions/test/common/menuService.test.ts#L192
Can you add this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#28952 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AQ3E7t7FpvcbK8V50WgS3gcGYthx-jaGks5sImpIgaJpZM4N9bUu>
.
|
@cristianhosu yes, instead of removing the references to the now deleted action, you should export the ID of your new command and use that instead. |
- added command to watermark
I don't know why the build is failing... can you guys help me out? |
@cristianhosu looks like an unrelated test failure that also happens on our master. let me know when you are ready for another review. |
Since the build failure is unrelated to my commit, then I think I am ready
:)
…On Jul 7, 2017 07:51, "Benjamin Pasero" ***@***.***> wrote:
@cristianhosu <https://github.com/cristianhosu> looks like an unrelated
test failure that also happens on our master. let me know when you are
ready for another review.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#28952 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AQ3E7hM1YR30cT070n1HqleQzDYygeUnks5sLblOgaJpZM4N9bUu>
.
|
@cristianhosu thanks, I was not able to use your PR directly because of some changes still needed. I went ahead and pushed 0095617 on top of your work. Will make sure to mention you and this PR in our July release notes. |
No description provided.