Add option: include command in screencast overlay

[PEZ]

This PR fixes #126713

Adding a setting screencastMode.showShortcutCommandTitles for including the command title in the Screencast Mode overlay. The setting defaults to true. When it is active the keybindings are:

1. Prepended with the Command title.
2. Displaying the full shortcut chord.

Also adding a setting screencastMode.showShortcutCommandTitlesWithCategory which will prepend the command category when enabled.

NB: keybindingService.lookupKeybinding(commandId) is used to display the full shortcut chord. This method/function has a bug making it sometimes pick up the wrong keybinding, if different keybindings are configured for the shortcut for different operating systems. I think it might be the same root cause as for #126306.

[ghost]

All CLA requirements met.

[PEZ]

It seems very involved to figure out the keybinding for a given command on other platforms than the current. The keybindingRegistry doesn't keep the info around. I edited the PR description to not include that part of the functionality.

[joaomoreno]

@PEZ There seem to be extra commits on this PR that you did not authored. Can you clean the branch?

[PEZ]

Hmm, yeah, I rebased with main, interpreting some PR instruction that way... I now force pushed a reset from before the rebase. Is that what you need, @joaomoreno ?

[PEZ]

@alexdima , in this PR I am currently sending along the context service to lookUpKeybindings(). With 87d09c2 that isn't strictly necessary anymore. You think I should call it w/o passing on context?

Also, the context doesn't even help. With or without passing it, contextMatchesRules() fails. I also tested with your commit applied. Same. Any idea why the context would be different in the screencast situation than when summoning the command palette?

[PEZ]

Here's an example screencast using this feature:

https://www.youtube.com/watch?v=U-cys75qaCw

[alexdima]

You think I should call it w/o passing on context?

👍 to call without a context (it looks like you're passing in the global context anyways, so the behavior should be the same with or without passing it in).

[joaomoreno]

A few problems:

• Most commands don't show any label. Try copy text or comment line. Only the keybinding appears.
• The setting should probably be an enum instead of two settings, something like: screencastMode.keyboardShortcutsPrefix: 'none' | 'command' | 'commandAndGroup'
• The default should not change: it should be off/'none' by default

[PEZ]

@joaomoreno , thanks! I've fixed the settings.

I think I need some guidance for the issue with that many commands do not show a label. I'm currently using the MenuRegistry for this and as far as I have tracked things this registry is populated with addCommand() at line 232 of src/vs/platform/actions/common/actions.ts. The workbench.action.files.save command is never registered here. Should I be looking somewhere else?

[joaomoreno]

You should use the command registry instead.

[PEZ]

@joaomoreno , that's one of the things I've considered, but it doesn't seem to have what we need to populate the overlay:

export interface ICommand {
	id: string;
	handler: ICommandHandler;
	description?: ICommandHandlerDescription | null;
}

[PEZ]

I'm stuck on the issue with many built in commands not showing a label.

I can say this though, that for extension contributed commands it works, and that when I create screencasts I much rather use the PR build than the regular build. Meaning to say, we're adding value as it stands. 😄

[joaomoreno]

@joaomoreno , that's one of the things I've considered, but it doesn't seem to have what we need to populate the overlay:

Isn't description set for most commands?

[PEZ]

@joaomoreno:

Isn't description set for most commands?

The problem I am hitting is not a lack of description. Yet. The CommanRegisitry has ICommands, where I think I need ICommandActions. That said, I don't see much in the way of description either.

If I pick out one command from each registry like so:

const command = shortcut?.commandId ? MenuRegistry.getCommand(shortcut.commandId) : null;
const command2 = shortcut?.commandId ? CommandsRegistry.getCommand(shortcut.commandId) : null;

Logging them out shows this for some different commands:

MenuRegistry-command: undefined
CommandRegistry-command: {"id":"editor.action.smartSelect.expand"}

MenuRegistry-command: undefined
CommandRegistry-command: {"id":"editor.action.addSelectionToNextFindMatch"}

MenuRegistry-command: {"id":"paredit.forwardUpSexp","title":"Forward Up Sexp","category":"Calva Paredit","precondition":{"key":"editorLangId","value":"clojure","type":4}}
CommandRegistry-command: {"id":"paredit.forwardUpSexp"}

MenuRegistry-command: {"id":"calva.evaluateCurrentTopLevelForm","title":"Evaluate Top Level Form (defun)","category":"Calva","precondition":{"key":"calva:connected","type":2}}
CommandRegistry-command: {"id":"calva.evaluateCurrentTopLevelForm"}

MenuRegistry-command: {"id":"workbench.action.showCommands","title":{"value":"Show All Commands","original":"Show All Commands"}}
CommandRegistry-command: {"id":"workbench.action.showCommands"}

MenuRegistry-command: undefined
CommandRegistry-command: {"id":"workbench.action.closeQuickOpen","weight":200,"when":{"key":"inQuickOpen","type":2},"primary":9,"secondary":[1033]}

Maybe the real fix is to have more commands registered in the MenuRegistry? But seems a bit out of scope for this PR, WDYT?

[joaomoreno]

@alexdima Sorry to call you in once more, but what do you think?

[alexdima]

@joaomoreno See my answer to the question above ⬆️ #126742 (comment)

Is there any other question that I missed?

[PEZ]

@alexdima @joaomoreno , if I may try to summarize what I think are the considerations:

I've addressed all but one change request, which is: With the current state of the PR only commands registered in the MenuRegistry will get a command title in the screencast overlay. It seems all extension contributed commands are there, but amongst the built-in commands most are not registrered there. This raises two questions:

1. Is that good enough for merging?
2. Is there some way in reach where we can display titles for the commands not in the MenuRegistry?

As to the second question, I can't see there is. If there is, I need guidance to find it.

As to the first question, in my opinion the current PR brings a lot of the feature's value already, I use it often for my screencasts/demo sessions. To me it would make sense to ship this as the first take and then make separate issues for:

1. Getting more built-in commands registered in the MenuRegistry (or device another way for providing what is needed by the screencast overlay)
2. Add an option for displaying the default keyboard shortcuts (currently the PR displays the active bindings)
3. Add an option for displaying the keyboard shortcuts for all platforms, when they differ

EDIT: Of course it can be argued that 2 and 3 are not added as settings, but rather be there, non-optional. My main point is that with these additions, this feature will rock even more than it already does.

[alexdima]

Commands are basically a map from a string identifier to a function to execute. So commands do not inherently come with a title in our command registry. The title is something we add for actions, which is then shown in the Command Palette or in the menus. We have hit this problem in the past, for example in the keybindings editor. There, when a title is missing, we just show the command ids: e.g.

I haven't reviewed the code in this PR, but I imagine the keybindings editor needs to do something very similar (i.e. given a command id, figure out what a good title would be for it). Maybe there is potential for code reuse, I didn't dig deeper.

[PEZ]

The title is something we add for actions

This is probably why the MenuRegistry is a nice place to pick this up, since it contains ICommandActions.

I'll have a look at the shortcuts editor and see if I can reuse something. So far missing the title hasn't been the main problem, but I think it makes total sense to use the id then also in the screencast overlay.

[PEZ]

Progress. I just made a quick test. If i ”reuse” this function from the shortcuts editor, and use that map for grabbing labels instead of going via MenuRegistry.getCommand(shortcut.commandId) I get labels for all shortcuts I press:

	private getActionsLabels(): Map<string, string> {
		const actionsLabels: Map<string, string> = new Map<string, string>();
		EditorExtensionsRegistry.getEditorActions().forEach(editorAction => actionsLabels.set(editorAction.id, editorAction.label));
		for (const menuItem of MenuRegistry.getMenuItems(MenuId.CommandPalette)) {
			if (isIMenuItem(menuItem)) {
				const title = typeof menuItem.command.title === 'string' ? menuItem.command.title : menuItem.command.title.value;
				const category = menuItem.command.category ? typeof menuItem.command.category === 'string' ? menuItem.command.category : menuItem.command.category.value : undefined;
				actionsLabels.set(menuItem.command.id, category ? `${category}: ${title}` : title);
			}
		}
		return actionsLabels;
	}

Should I go this way? (I don't think we'll win so much by refactoring the function out of the shortcuts editor, because we have slightly different requirements in the screencast overlay as the user options are designed right now. I think using a copy and adapting it makes sense, but it is not my codebase. 😄)

[alexdima]

👍 I would leave it as is and suggest we wait for @joaomoreno 's feedback.

[joaomoreno]

If we're happy with the results, let's just merge it as is.

One nit though. Currently when a command description is found, this appears as such:

And now that I see it in action, I would actually prefer we generalize the setting. Instead of screencastMode.keyboardShortcutsPrefix, how about screencastMode.keyboardShortcutsFormat:

• keys (shows the keyboard combination only)
• command
• commandWithGroup
• commandAndKeys
• commandWithGroupAndKeys

What do you think?

[PEZ]

I love the suggested options. The only consideration here is that we currently do not catch enough commands. So the command and commandWithGroup options might often not show anything. Maybe that is totally fine in practice. I'll make the changes and we can test how it feels. Otherwise we can maybe have some fallbacks in place.

[PEZ]

@joaomoreno I have been running with the generalized settings a while now and I think they work great.

I choose to default to commandAndKeys and might be a bit blind/biased about it, but during my testing it has felt totally natural as a default. 😄

As a v1 I think this is fine now. I want to keep iterating on it, something in this order:

1. Display command titles for more commands. We now know how find more commands, but it doesn't feel right to loop through all EditorActions at every keypress, so I will need to figure out how to only do that if the source has changed.
2. Make it possible to choose between showing default shortcut bindings and the ones configured.
3. Add option to display shortcut bindings for different OS platforms when they differ.

[joaomoreno]

Thanks! 🍻

[ArturoDent]

Is this in the Insiders Build yet? I can't find the setting to test it.

Version: 1.62.0-insider (user setup)
Commit: 4bbec28
Date: 2021-11-02T08:19:40.220Z
Electron: 13.5.1
Chrome: 91.0.4472.164
Node.js: 14.16.0
V8: 9.1.269.39-electron.0
OS: Windows_NT x64 10.0.22000

[joaomoreno]

@ArturoDent, not yet. We're currently stabilizing so we can release stable. It will be in Insiders closer to the end of this week.