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

Keybinding to ASCII characters #421

Closed
fdncred opened this issue Jul 27, 2020 · 13 comments
Closed

Keybinding to ASCII characters #421

fdncred opened this issue Jul 27, 2020 · 13 comments

Comments

@fdncred
Copy link

fdncred commented Jul 27, 2020

I'm trying to understand rustyline's keybindings better because I'm not sure what we have implemented in nushell is working.

This backspaces a character at a time. It seems like it should backspace a word at a time.

    rl.bind_sequence(
        KeyPress::Char('\x08'), // The ASCII code for backspace is 08
        Cmd::Kill(Movement::BackwardWord(1, Word::Vi)),
        );

This backspaces a word at a time. I think that is correct.

    rl.bind_sequence(
        KeyPress::Backspace,
        Cmd::Kill(Movement::BackwardWord(1, Word::Vi)),
        );

This backspaces a character at a time. It seems like this should backspace a word at a time with ctrl+backspace.

    rl.bind_sequence(
        KeyPress::Ctrl('\x08'), // The ASCII code for backspace is 08
        Cmd::Kill(Movement::BackwardWord(1, Word::Vi)),
        );

Any ideas why only the middle one seems to work?

@gwenn
Copy link
Collaborator

gwenn commented Jul 28, 2020

Because of this:

        '\x08' => KeyPress::Backspace, // '\b'

Control characters received from stdin are "normalized" before looking for associated command.

@fdncred
Copy link
Author

fdncred commented Jul 28, 2020

Right. I've seen that. So there is no way to use char literals for keybindings?

@gwenn
Copy link
Collaborator

gwenn commented Jul 28, 2020

And I guess we cannot retrieved a Ctrl-backspace from a Unix terminal.
See #318 (comment) and #333

@gwenn
Copy link
Collaborator

gwenn commented Jul 28, 2020

You may already know but you can patch this example.
And then run:

RUST_LOG=rustyline=debug cargo run --example example 2> debug.log
# tail -f debug.log in another terminal...

to trace all keys pressed.

@fdncred
Copy link
Author

fdncred commented Jul 28, 2020

oh, I didn't know that. Thanks for the tip!

@gwenn
Copy link
Collaborator

gwenn commented Jul 28, 2020

Right. I've seen that. So there is no way to use char literals for keybindings?

We should (have) "normalize" the first parameter of bind_sequence (KeyPress::Char('\x08') => KeyPress::Backspace) and warn if one binding is overwritten after normalisation.

@fdncred
Copy link
Author

fdncred commented Jul 28, 2020

We want to be able to bind Ctrl + Backspace to Cmd::Kill::BackwardWord(1) and Alt + Backspace to Cmd::Kill::BackwardChar(1). I see no way to do that at all. Even if I hard code it, it doesn't work.

@gwenn
Copy link
Collaborator

gwenn commented Jul 28, 2020

We want to be able to bind Ctrl + Backspace to Cmd::Kill::BackwardWord(1) and Alt + Backspace to Cmd::Kill::BackwardChar(1). I see no way to do that at all. Even if I hard code it, it doesn't work.

For me, you cannot retrieve a Ctrl-backspace from a Unix terminal so I would prefer to keep it as unsupported (even if it is possible on Windows).

@gwenn
Copy link
Collaborator

gwenn commented Jul 28, 2020

Right. I've seen that. So there is no way to use char literals for keybindings?

We should (have) "normalize" the first parameter of bind_sequence (KeyPress::Char('\x08') => KeyPress::Backspace) and warn if one binding is overwritten after normalisation.

Something like:

diff --git a/src/lib.rs b/src/lib.rs
index faa8da0..968fb87 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -887,7 +887,21 @@ impl<H: Helper> Editor<H> {
     /// Bind a sequence to a command.
     pub fn bind_sequence(&mut self, key_seq: KeyPress, cmd: Cmd) -> Option<Cmd> {
         if let Ok(mut bindings) = self.custom_bindings.write() {
-            bindings.insert(key_seq, cmd)
+            let key = if let KeyPress::Char(ref c) = key_seq {
+                if c.is_control() {
+                    keys::char_to_key_press(*c)
+                } else {
+                    key_seq
+                }
+            } else if let KeyPress::Ctrl(ref c) = key_seq {
+                if c.is_control() {
+                    warn!(target: "rustyline", "KeyPress::Ctrl({:?}) may not work on unix", c)
+                }
+                key_seq
+            } else {
+                key_seq
+            };
+            bindings.insert(key, cmd)
         } else {
             None
         }

@fdncred
Copy link
Author

fdncred commented Jul 28, 2020

what would be even better, is to allow the windows customization, and warn like you do here. Also, vice-versa, i assume there are keys that unix can bind that windows cannot.

@gwenn
Copy link
Collaborator

gwenn commented Jul 28, 2020

If you want to support Ctrl-backspace on windows, you will have to fix that:

} else if key == KeyPress::Backspace && ctrl {
...

(not tested)

@fdncred
Copy link
Author

fdncred commented Jul 28, 2020

Here's a PR to look at for this stuff.

@gwenn
Copy link
Collaborator

gwenn commented Nov 22, 2020

Version 7.0.0 released

@gwenn gwenn closed this as completed Nov 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants