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.

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

Show keyboard shortcuts inside integrated terminal in Screencast Mode #81537

Open
mbaljeetsingh opened this issue Sep 27, 2019 · 12 comments

Comments

@mbaljeetsingh
Copy link

@mbaljeetsingh mbaljeetsingh commented Sep 27, 2019

I really like the option of showing Only Keyboard Shortcuts in Screencast Mode.

image

But then, Inside the terminal it ignores the shortcuts (Screencast Mode). The shortcuts are showing inside terminal only when Screencat Mode: Only Keyboard Shortcuts is unchecked.

Add the ability to show keyboard shortcuts terminal when the Screencat Mode: Only Keyboard Shortcuts are checked.

@vscodebot

This comment has been minimized.

Copy link

@vscodebot vscodebot bot commented Sep 27, 2019

(Experimental duplicate detection)
Thanks for submitting this issue. Please also check if it is already covered by an existing one, like:

@ArvinH

This comment has been minimized.

Copy link

@ArvinH ArvinH commented Oct 1, 2019

Hi I'm looking for some good first issues now and would like to try this one, however, I'm not quite sure whether I understand what you want to do.
On my side, it'll show keyboard shortcuts when I'm in terminal window while Screencat Mode: Only Keyboard Shortcuts is checked.

demo

@SSJ9899TDylan

This comment was marked as spam.

Copy link

@SSJ9899TDylan SSJ9899TDylan commented Oct 1, 2019

@Tyriar

This comment has been minimized.

Copy link
Member

@Tyriar Tyriar commented Oct 1, 2019

@ArvinH when you enable that setting only those shortcuts that go through VS Code's keybinding system will be printed. The terminal works a little specially though, any keybindings that do not match commands listed in the "terminal.integrated.commandsToSkipShell" setting will be handled purely by the terminal.

// Skip processing by xterm.js of keyboard events that resolve to commands described
// within commandsToSkipShell
const standardKeyboardEvent = new StandardKeyboardEvent(event);
const resolveResult = this._keybindingService.softDispatch(standardKeyboardEvent, standardKeyboardEvent.target);
if (resolveResult && this._skipTerminalCommands.some(k => k === resolveResult.commandId)) {
event.preventDefault();
return false;
}

I haven't looked at the screencast code but I imagine what we want to do is show it when a modifier (alt/shift/ctrl) is used.

The cmd+left/right ones are special whitelisted commands, an example of one that's not is ctrl+c or ctrl+e.

@ArvinH

This comment has been minimized.

Copy link

@ArvinH ArvinH commented Oct 2, 2019

Thank you @Tyriar I understood.
I'd like to give it a try! Will dig into it.

@ArvinH

This comment has been minimized.

Copy link

@ArvinH ArvinH commented Oct 3, 2019

I looked into this a bit more today, before it goes down to _skipTerminalCommands check, looks like it'll go through keybindingResolver first 👀 and it couldn't find the match keybinding https://github.com/microsoft/vscode/blob/master/src/vs/platform/keybinding/common/keybindingResolver.ts#L270-L282
so the shortcut detection here failed:
https://github.com/microsoft/vscode/blob/master/src/vs/workbench/browser/actions/developerActions.ts#L163
and I'm a little bit lost from here, could you give me some hint that where I should track down to it?

@Tyriar

This comment has been minimized.

Copy link
Member

@Tyriar Tyriar commented Oct 3, 2019

@ArvinH looks like you found the relevant code.

@joaomoreno any advice on how we should surface these "terminal shortcuts" that don't end up making it to this listener?

const onKeyDown = domEvent(container, 'keydown', true);

Should the terminal expose some event for when a shortcut is triggered that ToggleScreencastModeAction listens to?

@joaomoreno

This comment has been minimized.

Copy link
Member

@joaomoreno joaomoreno commented Oct 10, 2019

Unfortunately that seems to be the fix here.

@ArvinH

This comment has been minimized.

Copy link

@ArvinH ArvinH commented Oct 10, 2019

Hi @joaomoreno, would you mind giving me more context?

for instance, why it couldn't find the match keybinding here?
https://github.com/microsoft/vscode/blob/master/src/vs/platform/keybinding/common/keybindingResolver.ts#L270-L282

If I can figure this out, perhaps I'll know better how to expose events from terminal

@Tyriar

This comment has been minimized.

Copy link
Member

@Tyriar Tyriar commented Oct 10, 2019

@ArvinH the KeyboardEvent never makes it there as the terminal prevents it from propagating upwards as it's already been handled by sending it to the terminal process.

As for how to expose the event on the terminal, you likely want to expose it on ITerminalService:

onActiveTabChanged: Event<void>;
onTabDisposed: Event<ITerminalTab>;
onInstanceCreated: Event<ITerminalInstance>;
onInstanceDisposed: Event<ITerminalInstance>;
onInstanceProcessIdReady: Event<ITerminalInstance>;
onInstanceDimensionsChanged: Event<ITerminalInstance>;
onInstanceMaximumDimensionsChanged: Event<ITerminalInstance>;
onInstanceRequestSpawnExtHostProcess: Event<ISpawnExtHostProcessRequest>;
onInstanceRequestStartExtensionTerminal: Event<IStartExtensionTerminalRequest>;
onInstancesChanged: Event<void>;
onInstanceTitleChanged: Event<ITerminalInstance>;
onActiveInstanceChanged: Event<ITerminalInstance | undefined>;
onRequestAvailableShells: Event<IAvailableShellsRequest>;

And then implement is here:

protected readonly _onActiveTabChanged = new Emitter<void>();
public get onActiveTabChanged(): Event<void> { return this._onActiveTabChanged.event; }

Then pull in ITerminalService here:

const onMouseUp = domEvent(container, 'mouseup', true);

And use it.

@Tyriar Tyriar added this to the Backlog milestone Oct 26, 2019
@MikolajTwarog

This comment has been minimized.

Copy link

@MikolajTwarog MikolajTwarog commented Oct 29, 2019

@ArvinH Are you still working on this issue? If not, I would like trying to solve this myself.

@ArvinH

This comment has been minimized.

Copy link

@ArvinH ArvinH commented Oct 30, 2019

@MikolajTwarog hi, Sorry for late reply, I was busy on other stuff but I do want to continue fixing this, is that ok for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.