Make WorkbenchActionRegistry just a forwarder #35528

jrieken · 2017-10-03T14:41:00Z

The goal about this is to remove IWorkbenchActionRegistry because we have commands, keybindings, and menu registries which are more flexible. However, there is weird code in places which needs to be checked first. At the end duplications like https://github.com/Microsoft/vscode/pull/35528/files#diff-b90278b8812101a32d844114fa0dc752L501 shouldn't be in our code anymore.

…t a forwarder

jrieken · 2017-10-03T14:43:24Z

src/vs/workbench/browser/parts/statusbar/statusbarPart.ts

-					}
-				*/
-				this.telemetryService.publicLog('workbenchActionExecuted', { id: action.id, from: 'status bar' });
-				(action.run() || TPromise.as(null)).done(() => {


@bpasero What's up with this workbenchActionExecuted-event? Is its purpose really to only report workbench-actions? So, no editor commands, no extension commands, no other commands?

@jrieken I would think that this telemetry event is logged for a) workbench actions, b) editor actions and c) contributed actions. The name is probably old and we never changed it but the data should be that.

Great. In that case this was a bug.

jrieken · 2017-10-03T14:44:29Z

src/vs/workbench/browser/parts/statusbar/statusbarPart.ts

-
-			return;
-		}
-
 		// Maintain old behaviour of always focusing the editor here
 		const activeEditor = this.editorService.getActiveEditor();
 		const codeEditor = getCodeEditor(activeEditor);
 		if (codeEditor) {
 			codeEditor.focus();


Not sure why we have it... Maybe because the command palette does that and because enablement of commands often depends on editor focus...

@jrieken I think this would not be needed if all commands have a proper when clause and would simply not show up when not enabled. Related: #18517

Well, the problem is that selecting something from the status bar takes/steels focus and that this means you cannot have things like 'comment line' in there. Not sure if they should find and focus an editor when executed...

jrieken · 2017-10-03T14:45:15Z

src/vs/workbench/parts/quickopen/browser/commandsHandler.ts

-
-		return entries;
-	}
-


I wanna get rid of these things

jrieken · 2017-10-03T14:46:13Z

src/vs/workbench/common/actions.ts

+			return null;
+		} else {
+			return title.original;
+		}
 	}


I believe these two methods should be somewhere else

jrieken · 2017-10-03T14:46:38Z

src/vs/workbench/common/actions.ts

+		if (descriptor.label) {
+			// slightly weird if-check required because of
+			// https://github.com/Microsoft/vscode/blob/d28ace31aa147596e35adf101a27768a048c79ec/src/vs/workbench/parts/files/browser/fileActions.contribution.ts#L194
+			MenuRegistry.appendMenuItem(MenuId.CommandPalette, {


That one save-action needs clarification...

jrieken · 2017-10-04T10:07:55Z

@bpasero @sandy081 Please review the changes

bpasero

@jrieken overall looks good to me but it looks like we are now reporting the workbenchActionExecuted telemetry event 2 times (once from here and once from here) when I invoke a command from the command palette.

bpasero · 2017-10-04T13:20:06Z

src/vs/platform/actions/common/actions.ts

@@ -87,7 +87,7 @@ export interface IMenuRegistry {
 	getMenuItems(loc: MenuId): IMenuItem[];
 }

-export const MenuRegistry: IMenuRegistry = new class {
+export const MenuRegistry: IMenuRegistry = new class implements IMenuRegistry {


Cool, didn't know that is possible...

jrieken · 2017-10-04T14:22:59Z

Thanks @bpasero. I did push two more changes that move the generic error logging out of actions and into the keybindings service. Tho, I did left the weird argument that's being passed to action.run({ from }). Do we really call actions with the 'keybinding' argument?

bpasero · 2017-10-05T06:07:04Z

@jrieken with latest I am getting a NPE in keybindingService when running out of sources where the telemetry service is null.

I think we are changing a little bit the semantic of the workbenchActionExecuted telemetry event with these changes. Now we seem to always fire it for any command executed via keyboard and previously we would only fire it for workbench actions (not for editor actions and not for extension actions when triggered via keybinding).

Lastly, there are more uses of workbench actions that do not provide a label beyond the save action from me: here in search land.

jrieken · 2017-10-05T08:15:45Z

I think we are changing a little bit the semantic of the workbenchActionExecuted telemetry event with these changes.

Yes, but are we saying that todays behaviour is correct or is it in-correct? Today we only send the event for a (random?) sub-set of commands and I don't believe that's on purpose... cc @seanmcbreen Also, the fact that we default to "triggered by keyboard" is bogus. If an extension executes a (workbench) command (via execucteCommand, as code action, or as post completion command) we will report it as triggered by keyboard which isn't correct

Lastly, there are more uses of workbench actions that do not provide a label beyond t

@roblourens What does an action without label do? Also, same question to you @bpasero? Should this just be a command?

jrieken · 2017-10-05T08:22:20Z

@jrieken with latest I am getting a NPE in keybindingService when running out of sources where the telemetry service is null.

Ops, this was actually missing a commit...

bpasero · 2017-10-05T08:27:58Z

@jrieken yes I was not saying to not report telemetry on everything, just wanted to raise that this will mean more data is collected for every time a command is triggered via keybinding.

I have changed the action to be included in command palette under the name "Save All Files", this is good to use when someone wants to save all dirty files but not any untitled files.

jrieken · 2017-10-05T08:44:33Z

@sandy081 Please check the changes I made in the keybindings land

sandy081 · 2017-10-05T10:59:28Z

@jrieken LGTM.

roblourens · 2017-10-05T17:17:54Z

@roblourens What does an action without label do?

I guess this is so the user can set a keybinding, but it doesn't show up in the command palette. I don't think they are useful there but if this is a problem, I can add a label.

jrieken · 2017-10-06T06:57:26Z

There are better ways to achieve that. You can simply register a command (even with a label for better keybindings UI but without having it show in any menu).

wip - make IWorkbenchActionRegistry not hold any data but make it jus…

65d45fd

…t a forwarder

jrieken commented Oct 3, 2017

View reviewed changes

jrieken self-assigned this Oct 3, 2017

make the registry be just a forwarder

54a46c8

jrieken changed the title ~~WIP - ActionRegistry debt~~ Make WorkbenchActionRegistry just a forwarder Oct 4, 2017

jrieken requested a review from sandy081 October 4, 2017 10:07

jrieken added the debt Code quality issues label Oct 4, 2017

bpasero requested changes Oct 4, 2017

View reviewed changes

jrieken added 2 commits October 4, 2017 16:20

don't generically log telemetry from workspace commands

015698a

make keybinding service log workbenchAction-event

d7a466f

deliver missing changes

c25c048

bpasero approved these changes Oct 5, 2017

View reviewed changes

sandy081 approved these changes Oct 5, 2017

View reviewed changes

jrieken merged commit 6080c16 into master Oct 5, 2017

jrieken deleted the joh/action_debt branch October 5, 2017 11:02

roblourens mentioned this pull request Oct 6, 2017

Change label-less search actions to commands #35735

Closed

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make WorkbenchActionRegistry just a forwarder #35528

Make WorkbenchActionRegistry just a forwarder #35528

jrieken commented Oct 3, 2017 •

edited

jrieken Oct 3, 2017 •

edited

bpasero Oct 3, 2017

jrieken Oct 3, 2017

jrieken Oct 3, 2017

bpasero Oct 3, 2017 •

edited

jrieken Oct 3, 2017

jrieken Oct 3, 2017

jrieken Oct 3, 2017

jrieken Oct 3, 2017

jrieken commented Oct 4, 2017

bpasero left a comment

bpasero Oct 4, 2017

jrieken commented Oct 4, 2017

bpasero commented Oct 5, 2017 •

edited

jrieken commented Oct 5, 2017

jrieken commented Oct 5, 2017

bpasero commented Oct 5, 2017

jrieken commented Oct 5, 2017

sandy081 commented Oct 5, 2017

roblourens commented Oct 5, 2017

jrieken commented Oct 6, 2017

Make WorkbenchActionRegistry just a forwarder #35528

Make WorkbenchActionRegistry just a forwarder #35528

Conversation

jrieken commented Oct 3, 2017 • edited

jrieken Oct 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpasero Oct 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrieken commented Oct 4, 2017

bpasero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrieken commented Oct 4, 2017

bpasero commented Oct 5, 2017 • edited

jrieken commented Oct 5, 2017

jrieken commented Oct 5, 2017

bpasero commented Oct 5, 2017

jrieken commented Oct 5, 2017

sandy081 commented Oct 5, 2017

roblourens commented Oct 5, 2017

jrieken commented Oct 6, 2017

jrieken commented Oct 3, 2017 •

edited

jrieken Oct 3, 2017 •

edited

bpasero Oct 3, 2017 •

edited

bpasero commented Oct 5, 2017 •

edited