Theia: Can't use Ctrl-c to kill currently running interactive process in terminal

[marcdumais-work]

I think the browser's key mapping for "copy" conflicts with the terminal's "kill current process".

Work-around: instead do Ctrl-Z to put the process in background, find the process with "ps" and kill it. Or kill it in another instance of terminal.

[hexa00]

An idea would be to let ctrl-c go though when there's no selection ?

[simark]

Here's what happens.

• Theia installs a "capture" phase keydown handler on the whole document.
• xterm.js installs a "bubble" phrase keydown handler on a textarea (that is in focus when the user clicks on the terminal).
• When typing ctrl-c while a terminal is focused, Theia's handler is invoked. It finds that ctrl-c is bound to the "core.copy" command, and runs that command. It then calls event.stopPropagation(), so the handler of xterm.js is never invoked.

What we would want instead is that when a terminal is in focus:

• The xterm.js handler should be invoked somehow.
• The "core.copy" command (or whatever command associated to ctrl-c) should not be invoked.

I don't know how to change the architecture to achieve this, so some guidance would be appreciated.

One path might be to have the terminal module register a keybinding for ctrl-c, valid for when in the terminal context/focused on a terminal, and for the keybinding system to support some kind of priority. Because the focus would be in the terminal, the terminal's keybinding handler would be invoked first, taking precedence over the "global" one.

The terminal's handler could either:

• Indicate to the keybinding system to not stop propagation of the event. The event would reach xterm.js's handler and do its thing.
• Manually call xterm.js's keydown handler. This seems a bit hackier because we need to know the internals of xterm.js, but it could still work.

[svenefftinge]

As described in #814 I think the terminal extension should register a keybinding for CTRL+C using a more specific keybinding context (i.e. 'terminalActive').

[hexa00]

Note this approach should be generalized such that all keybinding of xterm are registered.

[hexa00]

I think this needs more thinking... I'm not sure anymore that this is correct..

[hexa00]

Seems like it would be better rebind the Copy in case the terminal is active to like Ctrl-Shift-C and let Ctrl-C go passthough

[simark]

As described in #814 I think the terminal extension should register a keybinding for CTRL+C using a more specific keybinding context (i.e. 'terminalActive').

What in the keybinding code ensures that the keybinding of the terminal would take precedence over the copy keybinding? Last time I check the code (specifically keybinding.ts, method getKeybindingForKeyCode), I found that it would just pick the first handler and run that. I didn't see anything about priority, and didn't really understand how contexts fit into that.

[svenefftinge]

Yes, the code is broken. It should do the context check from line https://github.com/theia-ide/theia/blob/master/packages/core/src/common/keybinding.ts#L195 in getKeybindingForKeyCode and getKeybindingForCommand.

[svenefftinge]

Seems like it would be better rebind the Copy in case the terminal is active to like Ctrl-Shift-C and let Ctrl-C go passthough

You would simply register those two keybindings for the terminalContext and forward them to the appropriate commands.

[svenefftinge]

I opened https://github.com/theia-ide/theia/issues/819

[hexa00]

You would simply register those two keybindings for the terminalContext and forward them to the appropriate commands.

I guess we could have a generic command that is just send that key... to the term.

And we could include things like C-a C-e etc.. (term navigation in there) + C-z , C-s ... would need to check for all others..

[svenefftinge]

@simark I had a look at your approach to add ‘PassThroughKeybindings’. https://github.com/simark/theia/commit/eeab467ca56909498c86f5b805f37613c477b79a#diff-d7170acc306571b2666d36a305f828ca

As I mentioned on gitter, we need to be able to serialize keybindings and allow users to change them using keymap files. Also, we want to allow to refer to a context per keybinding to resolve conflicts (e.g. CTRL+C in terminal).

So the KeybindingContextRegistry is meant to be used by extension developers to introduce contexts, e.g. 'terminalActive'. Users can then reference that context per id in the keymaps.

Although, the idea of pass-through would simplify configuration a bit, because we won't need to introduce commands for all the terminal commands, but it will also forbid to rebind a certain command to another keybinding. So I think it would be better to explicitly redeclare all the terminal commands and always require a command id for a keybinding.

P.S.: I am aware that the KeybindingContext is currently not working and is also conceptually lacking important things like e.g. a notion of context priority. We should work on that :)

[simark]

At least with the terminal bindings that send control characters (ctrl-<key>), I don't think it makes sense to allow the user to rebind them. I don't know any terminal application that allows to do that, they are just baked in the vt100 emulation that we are all used to. That's why I preferred not to create commands for them, it greatly reduces the complexity. The more we do, the more we risk interfering with the expected terminal behavior. Our code would also have to find out which terminal instance is focused, find its associated Terminal object instance and "inject" a fake KeyboardEvent in it (I don't see how to do this with the current xterm.js API). That's basically doing the job of the browser.

I can imagine though that a user would want ctrl-p to pop open the command palette even when a terminal is in focus, because they never use ctrl-p in the terminal and they want to be able to access the command palette quickly. I guess for that we need a way to "disable" the passthrough keybinding.

In the end, if all the keybindings (including the ones shipped by default by Theia) appear in the user config file, they can just comment or remove the ctrl-p passthrough keybinding associated to the terminal. ctrl-p will then invoke the global one, opening the command palette.

On the other hand, if modules contribute default keybindings programmatically as we do today and the user keybinding config file only contains the user-defined/custom bindings, then we would need a way to "disable" the built-in terminal keybinding, so that the global one that triggers the command palette is invoked. The user could rebind ctrl-p to the command palette with a context with an even higher priority than the terminal, but it would be a bit obscure and I wouldn't expect a user to guess how to do that. Do you see a better way to do this? It often happens that users want to disable a keybinding, because they accidentally trigger it often and just want to get rid of it, without defining a new binding.

Note that this problem (how to disable a built-in keybinding) applies to all built-in keybindings, it's not related to the terminal or the concept of passthrough keybinding in particular.

As for the serialization, the concept of passthrough keybinding doesn't really matter, we can have an action field that says what to do when the keybinding is invoked. Something like:

[{
  key: "ctrl-c",
  context: "terminalActive",
  action: "passthrough"
}.
{
  key: "ctrl-c",
  action: "command",
  command: "core.copy"
}]

As for the keybinding context registry, what you said makes sense to me.

[svenefftinge]

Note that this problem (how to disable a built-in keybinding) applies to all built-in keybindings, it's not related to the terminal or the concept of passthrough keybinding in particular.

Of course, but in general you want to have those commands

• rebindable,
• addable to menus and
• show up in the command palette.

Just like we did for monaco commands.

If that all is not the case for terminal commands we might add the concept of pass-through as you suggest. It would be good if we don't encourage others to use it for the reasons I mentioned above, though. Also, Do you want to make 'disabling' and 'pass-through' behave the same? Shouldn't disable not consume the event?

[simark]

I am not sure how disabling a built-in keybinding would work (how it would look in the config), but I don't think it would be the same as passthrough. Let's say you have two keybinding registered for the same keyboard sequence, both are registered programmatically by modules. Here they are, in order of priority:

1. binding 1, context = foo
2. binding 2, context = null (global)

When the user triggers the keybinding, we invoke binding 1. Let's say the user doesn't care about binding 1, and wants the key sequence to always invoke binding 2 (even when in context foo). Disabling it would mean that when looking for a handler, we would skip binding 1 and invoke binding 2. Or maybe binding 1 would be removed from the keybindings data structures entirely. But in the end binding 2 would be invoked.

Passthrough would not be the same. If binding 1 is a passthrough, we would not invoke binding 2. We would just return and let the event propagate in the browser.

If there is a single keybinding defined for a keysequence, then disabling and passthrough would end up the same, since if we don't find a matching keybinding, we let the event propagate.

[simark]

As for serialization/deserialization, I guess that we'll need to write some code that convert the objects to and from the JSON representation, when we implement the keybinding configuration file, but that shouldn't be hard.

[akosyakov]

Regarding serialization: It is not only about keymaps, right now keybindings API is overly verbose. We want to simplify it to:

registry.registerKeybinding({
    command: ElectronCommands.RELOAD.id,
    key: "R+CmdCtrl"
});

Ideally to use the same format as for keymaps.

[simark]

I am not sure I understant what is the difference between keymaps and keybindings, can you clarify?

[epatpol]

I think keymaps refers to custom keybindings. I think we decided to stick with keymaps from custom keybindings along the way when referring to custom keyboard shortcuts.

[simark]

I had a chat with @epatpol, and there does not seem to be anything in my design that is a problem for him.

[hexa00]

@simark
I think we could have special commands like PassThrough / Disable

This would keep the interface simple while allowing what we need.

And they're won't be that much special commands like that so there's no need to encode this is object oriented design a simple if/else/switch would do.

WDTY?

[simark]

After discussing with @hexa00, what he means is that we could have:

{
  key: "ctrl-c",
  command: "passthrough",
}

I initially thought of adding a proper passthrough command, and have do nothing, but return a code to mean that the keystroke should not be swallowed by Theia. The problem is that commands know nothing about keybindings, so it doesn't really make sense to have in the commands API that is related to keybindings.

But after discussing with @hexa00, we think that we can probably just handle the special case in KeybindingRegistry.run, like:

if (binding.commandId == "passthrough") {
    // do nothing
} else {
  // invoke command as usual
  // prevent keystroke event propagation
}

As @hexa00 said, there won't be many special cases like this, so it should stay simple and clear.

[simark]

Fixed by #887.