Skip to content

Extended keyboard input handling#10359

Closed
krobelus wants to merge 8 commits into
fish-shell:masterfrom
krobelus:keys
Closed

Extended keyboard input handling#10359
krobelus wants to merge 8 commits into
fish-shell:masterfrom
krobelus:keys

Conversation

@krobelus

@krobelus krobelus commented Mar 10, 2024

Copy link
Copy Markdown
Contributor

Improve keyboard handling using the CSI u protocol with fixes/extensions from the kitty keyboard protocol.

This allows to

  • bind more keys, especially ones with ctrl/alt/shift modifiers (on a modern terminal)
  • give keys a user-friendly name (no more \e\[1\;5C)

Changelog additions:

  • Fish now asks the terminal to speak the keyboard input protocol CSI
    u in combination with XTerm's modifyOtherKeys and Kitty's
    progressive enhancements. This allows to bind almost any key,
    including arbitrary combinations of modifiers ctrl, alt and shift.

  • There is a new syntax for specifying keys for builtin bind. The
    new syntax introduces modifier names and names for some keys that
    don't have an obvious and printable Unicode code point. The old
    syntax remains mostly supported but the new one is preferred.

    • Existing bindings that use the new names have a different
      meaning now. For example
      • bind up 'do something' maps the up arrow instead of the
        two keys u and p.
      • bind ctrl-x,alt-c 'do something' maps a sequence of two
        key chords.

    - To minimize breakage, KEYS argument to bind is parsed in the old syntax in two cases:

    • If KEYS starts with a raw escape character
      (\e) or a raw ASCII control
      character (\c).
    • If it contains none of , or - and it is not a named key.

    Since , and - act as separators, they need to be given as
    comma and minus in ambiguous cases.

  • Specifying key names as terminfo name (bind -k) is deprecated and
    may be removed in a future version.

  • Legacy bindings that use raw escape sequences instead of named
    modifiers/keys continue to be included in the default configuration.
    If your terminal supports CSI u, you can remove legacy bindings
    (including terminfo bindings) by setting the
    fish_legacy_bindings variable to
    false (on shell startup or before switching key bindings).

  • The fish_escape_delay_ms configuration knob has been obsolete (for
    a while I think) and has been removed.

  • fish_key_reader --verbose is now ignored, so it no longer shows
    codepoint values or timing information (though the latter might be
    useful for users of fish_sequence_key_delay_ms).

  • When a terminal pastes text into fish using bracketed paste, fish
    used to switch to a special paste bind mode. This bind mode has
    been removed. The behavior on paste is not intended to be
    configurable.

  • Focus reporting is enabled unconditionally, not just for tmux. To
    use it, define functions that handle events
    forfish_focus_in` andfish_focus_out``.

  • When fish is stopped or terminated by a signal that cannot be caught
    (SIGSTOP or SIGKILL), it may leave the terminal in a state where
    keypresses with modifiers send CSI u sequences instead of
    traditional ones (that are recognized by bash). If this happens, you
    can use the reset command from ncurses to restore the terminal
    state.

  • If the terminal supports CSI u, shift-enter now inserts a newline
    instead of executing the command line..


Original description, somewhat outdated:

Details

This is a WIP implementation of better keyboard handling, using the CSI u protocol with fixes/extensions from the kitty keyboard protocol.

It allows to

  • bind more keys, especially ones with ctrl/alt/shift modifiers (on a modern terminal)
  • give keys a user-friendly name (no more \e\[1\;5C)

image

The implementation works for me but has lots of glaring issues; I'm sharing in case there is early feedback on UI/design.

UI Changes

Today, builtin bind supports binding either raw sequences (like \e\[1\;5C), or singular named keys (like --key up) as provided by terminfo.

With extended keys, we probably want to support binding sequences of (named) keys where each key might have a number of modifiers. This is not possible with today's syntax. Raw sequences do not support named keys. Terminfo keys don't support sequences or modifiers.

Syntax precedents are the Emacs family (C-M-x and <next>) and Vim (<C-M-x> and <PageDown>). Unfortunately neither are very natural for our bind usage.
Use [c-a-x] and [pagedown].

Key names

Introduce names for keys that don't map to a standard/unique Unicode code point (see src/key.rs).
I don't know if there is a standard set of names, for now I copied them from Kakoune.
This list currently includes some control characters, meaning they can be specified in two ways, for example bind [ret] means the same as bind \n. Not sure about this - the redundancy might be confusing.

Sequencing

Binding sequences of extended keys is somewhat rare. Concatenation seems free:

bind x[c-y][a-up][s-pageup][lbrack] 'do something' # 5 keystrokes

One alternative is to separate by spaces but that's ambiguous.

Backwards compat

If the terminal agrees to talk CSI u, the user's customized bindings of raw non-CSI-u escape sequences will no longer work.
We can probably remedy this by translating the escape sequences that are used in practice.
For example, hard-code \e\[1\;5C as alias for [c-right] (by keeping the existing binding).

Additionally the syntax change affects bindings that contain both [ and ].
This is completely avoidable but actual breakage seems very unlikely.

Most still work as-is

  • bind [ 'echo foo'
  • bind [x 'echo foo'
  • bind \cX 'echo foo' # note this is [c-x], not [c-X]!
  • bind \ex 'echo foo' # aka [a-x]

Some throw an error:

  • bind [no-such-named-key] 'echo foo'

Some silently do something different:

  • bind [x] 'echo foo' # binds a single key (x), not a sequence of three keys. Could also make it an error.
  • bind [ret] 'echo foo' # binds Return, not a sequence

Future direction

  • These terminal protocol works without feature probing because most terminals are sufficiently compatible with xterm. Maybe one day we can drop terminfo.
  • We currently support a configurable "sequence delay" - originally only for the escape key. This is hacky, mostly exists due to historical reasons. Would be nice to get rid of this I think today the main benefactor is Vi mode in screen/tmux.

The implementation is in large parts copied from here (and terminal-agnostic parts from here).
I recently realized that there is a Rust crate called crossterm that might cover some of the terminal-specific parts.

@faho

faho commented Mar 10, 2024

Copy link
Copy Markdown
Member

We currently support a configurable "sequence delay"

We support two delays. One is, as you said, a historical bogosity to distinguish alt+x from escape, wait, x.

The other is a delay to decide if we should use what you already pressed or wait for more - this is not just historical cruft, it's needed if you want to bind anything to longer sequences. E.g. you can bind escape to one thing and escape+x to another, or a reasonably popular idea in vi-mode: Bind "jk" in insert mode to switch to normal mode.

The former could only go if we decided to only support terminals with CSI u (given my faith in the overall terminal ecosystem I do not believe that's likely to happen anytime soon). The latter has to stay.

This list currently includes some control characters, meaning they can be specified in two ways, for example bind [ret] means the same as bind \n

\n or \r?

Anyway: Honestly this is fine. We already have \r being the same as \cm, and that's much worse because it's actually two key chords on my keyboard.

Additionally the syntax change affects bindings that contain both [ and ].
This is completely avoidable but actual breakage seems very unlikely.

Yeah, that's fine.

I recently realized that there is a Rust crate called crossterm that might cover some of the terminal-specific parts.

The readme does not fill me with confidence that it fits our requirements. The "Tested Terminals" list is much too short for my taste, especially considering Console Host is, as I understand WSL, not relevant to us.

We can probably remedy this by translating the escape sequences that are used in practice.
For example, hard-code \e[1;5C as alias for [c-right] (by keeping the existing binding).

I would like to have this one step deeper and translate the xterm sequences to keys internally. I don't want to ask people to bind \e\[1\;5C and [c-right] in case someone uses a terminal that doesn't have CSI u - which, currently, is a lot of them.


Alright, now for the obvious bikeshedding about key names:

Syntax precedents are the Emacs family (C-M-x and ) and Vim ( and ). Unfortunately neither are very natural for our bind usage.

Agreed, and tbh both are pretty jargony (why does "M" stand for alt? What on earth is a "meta" key?).

bind x[c-y][a-up][s-pageup][lbrack]

I'm not enamored with these either.

The bracket notation is clever - the alternative I proposed would require --key: bind --key 'x ctrl+y alt+up shift+pageup [' 'do something'.

But I don't like the abbreviations here - "lbrack" and "ret" in particular. I would appreciate "LeftBracket" and "Return". I would also spell out the modifiers.

So this would be

bind x[Ctrl-y][Alt-Up][Shift-PageUp][LeftBracket]

I would also probably make the names case-insensitive. Of course that might conflict with allowing you do use uppercase letters instead of "Shift-" - is [Ctrl-Y] ctrl-shift-y or just ctrl-y? If it is the latter, is Y shift-y or y?

Bindings are read much more often than they are written, so it's fine if something is a bit longer.

As a weird sidenote: I probably have a different idea of how often people would bind the [ key because I use a german keyboard layout and that makes it awkward to type (right alt + 8). I'm reasonably sure it's used more by people with keyboards that have it in the ö spot.

@krobelus

krobelus commented Mar 10, 2024 via email

Copy link
Copy Markdown
Contributor Author

@faho

faho commented Mar 10, 2024

Copy link
Copy Markdown
Member

Ok sure. I just think that overloading bindings like that is overly complex. When I use bindings that share prefixes I make sure that they are not a prefix of one another.

It is a feature we have, that had people asking for it. It's pretty new, but I do not believe it is unreasonable to have.

I guess we should keep aliasing Ctrl-[ to Escape. Not sure.

Not sure if you're running the german qwertz layout, on that Ctrl-[ is basically unusable.

So I would need the opinion of people with other layouts to see if that is a thing that people do.

Perhaps only named keys (so not the single-char ones) should be case-insensitive.

That makes sense.

@postsolar

Copy link
Copy Markdown

Is this meant to be working now? fish_key_reader would accurately report [s-backspace] or [c-R] but binding any such key has no effect, the key read seems to be the key that would be read the old way (e.g. backspace or c-r).

@krobelus

Copy link
Copy Markdown
Contributor Author

works for me, do you have steps to reproduce?

@postsolar

postsolar commented Mar 16, 2024

Copy link
Copy Markdown

It now works for me too, I think the issue was running ./fish -N instead of ./fish. My old set of keybindings (set with fish_user_key_bindings.fish) was somehow making fish unusable, so I tested with the -N flag.

fish_user_key_bindings.fish
function fish_user_key_bindings
# wipe out the default keybindings
bind --erase --all
bind --erase --all --preset

# add the trivial ones
bind '' self-insert
bind -M paste '' self-insert
bind -M paste ' ' self-insert-notfirst
bind -m paste \e\[200\~ __fish_start_bracketed_paste
bind -M paste \e\[201\~ __fish_stop_bracketed_paste
bind -M paste \' __fish_commandline_insert_escaped\ \\\'\ \$__fish_paste_quoted
bind -M paste \\ __fish_commandline_insert_escaped\ \\\\\ \$__fish_paste_quoted

# add all the other ones
bind \b "backward-kill-line"
bind \cd "exit"
bind \cz "fg 2>/dev/null; commandline -f repaint"
bind \e, "history-token-search-backward"
bind \e. "history-token-search-forward"
bind \eC "cancel-commandline"
bind \eO "insert-line-over"
bind \eU "redo"
bind \eY "commandline | wl-copy -n"
bind \e\# "__fish_toggle_comment_commandline"
bind \e\[1\;2C "forward-word"
bind \e\[1\;2D "backward-word"
bind \e\[1\;3A "cd_up"
bind \e\[1\;3B "cd_into_child_directory"
bind \e\[1\;3C "forward-bigword"
bind \e\[1\;3D "backward-bigword"
bind \e\[1\;4C "end-of-line"
bind \e\[1\;4D "beginning-of-line"
bind \e\[1\;5C "end-of-buffer"
bind \e\[1\;5D "beginning-of-buffer"
bind \e\[3\;2~ "kill-word"
bind \e\[3\;3~ "kill-bigword"
bind \e\[3\;4~ "kill-line"
bind \e\[3~ "history-pager-delete or delete-char"
bind \e\[A "up-or-search"
bind \e\[B "down-or-search"
bind \e\[C "forward-char"
bind \e\[D "backward-char"
bind \e\b "backward-kill-path-component"
bind \e\x7F "backward-kill-word"
bind \e` "togglecase-char"
bind \ec "cancel"
bind \ed "kill-whole-line"
bind \ee "edit_command_buffer"
bind \eg "__fish_paginate"
bind \eh "history-pager"
bind \el "__fish_list_current_token"
bind \em "__fish_man_page"
bind \eo "insert-line-under"
bind \ep "__fish_preview_current_file"
bind \er "clear-screen"
bind \es "fish_commandline_prepend sudo"
bind \eu "undo"
bind \ey "commandline | wl-copy -p -n"
bind \r "execute"
bind \x7F "backward-delete-char"
end

@krobelus

Copy link
Copy Markdown
Contributor Author

yeah fish -N is currently broken by this PR (and you probably don't want to use -N currently anyway).
Your old key bindings should keep working but the compatibiltiy work is not finished (missing CSI sequences and things like alt-backspace). We can't cover all cases but most of the relevant ones

@krobelus

Copy link
Copy Markdown
Contributor Author

I don't think fish_sequence_delay is the ideal long-term UI. I'd rather have fish wait indefinitely but (after some initial delay?) somehow echo the currently pending sequence (but crucially, don't execute it until it's complete). Similar to how Emacs and others do it.
Of course this is totally unrelated, I'll try to fix the delay tests and get this ready relatively soon.

I think I'd like to stick to lowercase key names, not sure the details yet.

@postsolar

Copy link
Copy Markdown

From the OP it's not clear to me whether it's planned to deprecate current, escapes-based syntax. I would argue that this is desirable. Consider the scenario which I run into recently:

  1. I erase all standard keybindings and set new ones based on the new syntax from this PR
  2. I reboot. I don't use a login manager so I boot directly into a TTY
  3. Fish is currently my login shell. TTY doesn't support any of my keybindings, so nothing works

While this is, technically, no different than simply erasing all keybindings, and thus a user's failure, still, there are programs which handle this fine (among them Kakoune). So, would it be desirable to automatically map new syntax to old syntax?

@faho

faho commented Mar 23, 2024

Copy link
Copy Markdown
Member

From the OP it's not clear to me whether it's planned to deprecate current, escapes-based syntax

Not for quite a while.

So, would it be desirable to automatically map new syntax to old syntax?

Those are two separate questions.

We should probably map most keys also to the typical xterm sequences, yes. And again "We can't cover all cases but most of the relevant ones". This should probably cover:

  • Regular keys that are directly represented (pressing "a" yields "a") - pretty sure these are already covered
  • Return / backspace / delete - it's fine if these are swapped (because terminals often swap them)
  • Arrows
  • Ctrl plus regular keys, alt plus regular keys
  • Ctrl plus arrows, alt plus arrows, shift plus arrows

It's fine if windows/option etc, "hyper", the F1 etc function keys, ctrl+alt+shift+left and the numpad aren't mapped.

It's fine if some of these use the xterm sequence and don't work in some terminals, as long as the basic "enter text and press return to execute" works.

@krobelus

krobelus commented Mar 29, 2024

Copy link
Copy Markdown
Contributor Author

One option would be to use the names from /usr/include/xkbcommon/xkbcommon-names.h but unfortunately XKB_MOD_NAME_ALT is Mod1, which is not great depending on the audience

Probably kitty key names (like shift+enter) are reasonable although I think I'd prefer shift-enter (could allow both of course).

To optimize for the common case we could make bind shift+enter foo do the right thing (instead of requiring [shift+enter]).
That will make it less obvious how to bind sequences of named/modified keys..
actually we can make it a bit more obvious by getting rid of brackets entirely in favor of space separation:

bind 'x ctrl-y alt-up shift-pageup [' 'do something' # 5 keystrokes

as a bonus this frees us from needing to find a name for ].

bind ' ' foo would still work I guess, but the canonical version (that bind prints) will be bind space foo because that works with modifiers.
(I don't think that `bind 'ctrl-\ ' is supposed to work, I think it should be an error)

EDIT: actually I think comma-separation is better than space-separation.

Of course it's possible to don't invent new names for control and alt but keep using \c and \e but I think that would be weird, especially for \e which doesn't look like Alt.

I guess we should keep aliasing Ctrl-[ to Escape. Not sure.

Not sure if you're running the german qwertz layout, on that Ctrl-[ is basically unusable.

Yes I use qwerty (alternative layouts are not great with Vim and I need
[]{} way more often than umlauts). I don't think it's a must that we
provide ctrl-[ but I personally always use it rather than escape, so I'd
keep the alias.

@krobelus krobelus changed the title WIP: Extended keyboard input handling Extended keyboard input handling Mar 31, 2024
@krobelus

Copy link
Copy Markdown
Contributor Author

I've compiled a list of changelog additions (same in PR description and in the last commit), we can use that as guidance for deciding about the user-visible changes. This is ready as far as I'm concerned (of course there's a bunch more things I could document or improve, LMK)

@faho faho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Put escape-delay back
  • Remove $fish_legacy_bindings

Comment thread CHANGELOG.rst Outdated
Comment thread CHANGELOG.rst Outdated
Comment thread CHANGELOG.rst Outdated
Comment thread CHANGELOG.rst Outdated
Comment thread doc_src/cmds/bind.rst Outdated
Comment thread doc_src/cmds/bind.rst
Terminal Limitations
--------------------

Unix terminals, like the ones fish operates in, are at heart 70s technology. They have some limitations that applications running inside them can't workaround.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would need something on CSI u and then still something on the limitation of the legacy system.

I do not believe that CSI u is that well-supported that we can ignore the older system almost entirely.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you're right but I don't want to spend too much time on this.
Given that, I might not even mention CSI u - it's an implementation detail and it should just work, the same as in Vim and elsewhere.
So I'd prefer to let someone else write the documentation if they feel like it's useful.
I guess I can avoid touching the existing docs.
I plan to switch to a different flavor of the protocol in future.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like:

fish works best in terminal emulators that support the modern CSI u key sequence protocol.

It will try to enable this automatically, and if your terminal supports it everything should just work.

If your terminal does not support CSI u, key bindings will have some limitations:

- being incapable of telling many characters + control from other keys, like Control+I from tab or Control+J from newline.
- Control and Shift can't be used simultaneously. 
- not knowing some modifier+key sequences

These limitations are caused by the old encoding simply not giving the application more information, which is why the CSI u protocol was created.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah those are good things to mention.
It's not exactly CSI u but a mix of two protocols with kitty extensions. It's subject to change in future, so I'm not sure if I want to document the specifics. But I think linking to the kitty page is a good idea.

@faho faho Apr 1, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not exactly CSI u but a mix of two protocols with kitty extensions.

Okay, so which things do we need, what terminals does this work properly on? What can we mention here?

I don't really want to link to kitty documentation because that makes it look like a single-terminal thing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Features like disambiguation of keys ctrl-j or shift-enter work only on terminals that implement kitty extensions.

(But users of all terminals benefit from having a human-friendly, (terminal/protocol)-agnostic key binding syntax)

I don't really want to link to kitty documentation because that makes it look like a single-terminal thing.

I think this is very easy to avoid, like "if your terminal implements the kitty keyboard protocol ...".
I don't think it's very important to document the specifics because it's always easy to get the ground truth by trying fish on some terminal.
It should be obvious that "if the terminal disambiguates a key, then you can map it" because I don't think there is any other cross-terminal protocol around (not counting terminfo).
When a new protocol comes along we can implement that etc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XTerm users also get some benefits from modifyOtherKeys. I dropped this doc change for now

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be obvious that "if the terminal disambiguates a key, then you can map it" because I don't think there is any other cross-terminal protocol around (not counting terminfo).

What I'm looking for here is some feature we can tell people to look for in a terminal without having to try it out and see.

Ideally this would just be a bullet point like "Keyboard Input Protocol 2024". I don't love calling it the "kitty keyboard protocol" because that pushes people towards kitty specifically, but if that's the best description we have I guess that's it.

I dropped this doc change for now

That's fine.

@faho

faho commented Apr 1, 2024

Copy link
Copy Markdown
Member

Okay, running fish_key_reader from this in a terminal that does not support CSI u (konsole), it seems to move the cursor up constantly?

EDIT:

Oh god, starting fish from this will move up and clear the scrollback buffer?

On every prompt.

@postsolar

postsolar commented Apr 2, 2024

Copy link
Copy Markdown

I'm getting this branch' fish_key_reader and fish to crash with Rio terminal.

rust backtrace
 -  RUST_BACKTRACE=full ./.local/bin/fish-shell/build/fish_key_reader
thread 'main' panicked at /home/me/.cargo/git/checkouts/rust-terminfo-0cd910809fe82bc7/870327d/src/parser/compiled.rs:44:64:
called `Option::unwrap()` on a `None` value
stack backtrace:
   0:     0x56274548e947 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hc7227e0e319a0b38
   1:     0x5627454a5c30 - core::fmt::write::hab7a2cb9fb1eea10
   2:     0x5627454771bf - std::io::Write::write_fmt::h5d9ea37d99fc8ea3
   3:     0x56274548e714 - std::sys_common::backtrace::print::h7ef6b9088506adf3
   4:     0x56274548f2ca - std::panicking::default_hook::{{closure}}::h2be91e085f955e55
   5:     0x56274548eff3 - std::panicking::default_hook::hd222121dd148ba89
   6:     0x56274548f7d8 - std::panicking::rust_panic_with_hook::hcb0f9a863644f6c2
   7:     0x56274548f67e - std::panicking::begin_panic_handler::{{closure}}::hb3736c891f535df7
   8:     0x56274548eb76 - std::sys_common::backtrace::__rust_end_short_backtrace::h5c867a2182e2e40a
   9:     0x56274548f410 - rust_begin_unwind
  10:     0x562744e9b495 - core::panicking::panic_fmt::h71234a7826c31033
  11:     0x562744e9b553 - core::panicking::panic::h457defa9287ba8c7
  12:     0x562745398995 - core::option::Option<T>::unwrap::ha5e1ef58ac97e522
                               at /build/rustc-1.76.0-src/library/core/src/option.rs:931:21
  13:     0x56274539d75e - terminfo::parser::compiled::<impl core::convert::From<terminfo::parser::compiled::Database> for terminfo::database::Database>::from::hc90eab182a8eefa9
                               at /home/me/.cargo/git/checkouts/rust-terminfo-0cd910809fe82bc7/870327d/src/parser/compiled.rs:44:46
  14:     0x5627453af18e - <T as core::convert::Into<U>>::into::h8ea6cfdee6c25bed
                               at /build/rustc-1.76.0-src/library/core/src/convert/mod.rs:757:9
  15:     0x5627453a0ba4 - terminfo::database::Database::from_buffer::hc2f6aaab7cb3dc57
                               at /home/me/.cargo/git/checkouts/rust-terminfo-0cd910809fe82bc7/870327d/src/database.rs:230:7
  16:     0x5627453a0a2b - terminfo::database::Database::from_path::h3701716254c36854
                               at /home/me/.cargo/git/checkouts/rust-terminfo-0cd910809fe82bc7/870327d/src/database.rs:224:3
  17:     0x5627453a0825 - terminfo::database::Database::from_name::hf61f08a24dc8f902
                               at /home/me/.cargo/git/checkouts/rust-terminfo-0cd910809fe82bc7/870327d/src/database.rs:199:13
  18:     0x56274539f6c1 - terminfo::database::Database::from_env::h9527e9a466e43106
                               at /home/me/.cargo/git/checkouts/rust-terminfo-0cd910809fe82bc7/870327d/src/database.rs:144:4
  19:     0x5627452e20b5 - fish::curses::setup::h27c9bced04e1b6bd
                               at /home/me/.local/bin/fish-shell/src/curses.rs:381:9
  20:     0x562744fcd992 - fish::env_dispatch::init_curses::he94df56b88351047
                               at /home/me/.local/bin/fish-shell/src/env_dispatch.rs:653:8
  21:     0x562744fc81ed - fish::env_dispatch::run_inits::h764728521a2f37f3
                               at /home/me/.local/bin/fish-shell/src/env_dispatch.rs:352:5
  22:     0x562744fc81ba - fish::env_dispatch::env_dispatch_init::h15cc32200b25eb4c
                               at /home/me/.local/bin/fish-shell/src/env_dispatch.rs:343:5
  23:     0x562744edbd8e - fish::env::environment::env_init::hfe5a15dd645bf0a2
                               at /home/me/.local/bin/fish-shell/src/env/environment.rs:734:5
  24:     0x562744e9fbe6 - fish_key_reader::setup_and_process_keys::h43673a8d4216061b
                               at /home/me/.local/bin/fish-shell/src/bin/fish_key_reader.rs:157:5
  25:     0x562744ea14d5 - fish_key_reader::main::hfa298dc3881ab4f8
                               at /home/me/.local/bin/fish-shell/src/bin/fish_key_reader.rs:256:5
  26:     0x562744ea5fbb - core::ops::function::FnOnce::call_once::hcb33d9a4a69e7002
                               at /build/rustc-1.76.0-src/library/core/src/ops/function.rs:250:5
  27:     0x562744ea651e - std::sys_common::backtrace::__rust_begin_short_backtrace::h37a3c85bf668ac55
                               at /build/rustc-1.76.0-src/library/std/src/sys_common/backtrace.rs:155:18
  28:     0x562744ea6591 - std::rt::lang_start::{{closure}}::h2239127d050ad6a5
                               at /build/rustc-1.76.0-src/library/std/src/rt.rs:166:18
  29:     0x56274548f304 - std::panicking::try::h45c8379f4866ecc7
  30:     0x56274548ab35 - std::rt::lang_start_internal::hf358ddc7f05da9a0
  31:     0x562744ea656a - std::rt::lang_start::h151a11f5b07e5660
                               at /build/rustc-1.76.0-src/library/std/src/rt.rs:165:17
  32:     0x562744ea15ee - main
  33:     0x7f18ec0b70ce - __libc_start_call_main
  34:     0x7f18ec0b7189 - __libc_start_main@GLIBC_2.2.5
  35:     0x562744e9bea5 - _start
  36:                0x0 - <unknown>

krobelus added a commit to krobelus/rust-terminfo that referenced this pull request Apr 2, 2024
This fixes a problem with the rio terminal as reported in
fish-shell/fish-shell#10359 (comment)
krobelus added a commit to krobelus/rust-terminfo that referenced this pull request Apr 2, 2024
This fixes a problem with the rio terminal as reported in
fish-shell/fish-shell#10359 (comment)
krobelus added 5 commits April 2, 2024 08:01
We need to give fish time to render I think.
Terminal titles are set with an OSC 0 sequence.  I don't think we want to
support terminals that react badly to unknown OSC (or CSI) sequences.

So let's remove our feature detection.

This will fix future false negatives along the lines of
fish-shell#10037
Use generic shell commands instead.  This keeps us honest.

No functional change expected.
To do so add an ad-hoc "commandline --search-field" to operate on pager
search field.

This is primarily motivated because a following commit reuses the
fish_clipboard_paste logic for bracketed paste. This avoids a regression.
krobelus added 2 commits April 2, 2024 08:01
I don't know why we're inconsistent about this, and at least asan fails
frequently due to timeouts.
@krobelus

krobelus commented Apr 2, 2024

Copy link
Copy Markdown
Contributor Author

that's unrelated but I've pushed a fix (to master and here).
I've fixed the other problems mentioned, anything else?

@postsolar

postsolar commented Apr 2, 2024

Copy link
Copy Markdown

Thank you @krobelus, I can confirm that the crash issue is solved now.

That said, keys detection seems to be inconsistent across different terminals.

Those that can accurately report [s-backspace]:

  • kitty 0.33.1
  • foot (d3b348a5b183b0e6295dc985b1d5dcd939cb5c6e)
  • wezterm 20240203-110809-5046fc22

Those that cannot report it accurately and instead report [backspace] (but all work as expected with kitten show_key -m kitty):

  • alacritty 0.13.2
  • foot (master)
  • rioterm 0.0.36

With regards to foot it maybe might sound familiar to this issue?

@krobelus

krobelus commented Apr 2, 2024

Copy link
Copy Markdown
Contributor Author

yeah the plan for the future is to also use the other progressive enhancements

See the changelog additions for user-visible changes.

Since we enable/disable terminal protocols whenever we pass terminal ownership,
tests can no longer run in parallel on the same terminal.

Since fish asks the terminal to speak CSI u now, using gdb may be less
ergonomic.  As a remedy, use gdbserver, or lobby for CSI u support in
libreadline[1].

[1]: https://lists.gnu.org/archive/html/bug-readline/2022-04/msg00010.html
@krobelus

krobelus commented Apr 2, 2024

Copy link
Copy Markdown
Contributor Author

Okay, running fish_key_reader from this in a terminal that does not support CSI u (konsole), it seems to move the cursor up constantly?

EDIT:

Oh god, starting fish from this will move up and clear the scrollback buffer?

On every prompt.

Not a fan of the harsh tone, I don't think yelling at some inanimate thing is helpful

@faho

faho commented Apr 2, 2024

Copy link
Copy Markdown
Member

Not a fan of the harsh tone, I don't think yelling at some inanimate thing is helpful

Sorry, yeah, I was just surprised by things jumping around and immediately typed it in here.

It wasn't my intention for it to sound harsh or like I'm yelling, it was the equivalent of a jumpscare.

@krobelus

krobelus commented Apr 2, 2024

Copy link
Copy Markdown
Contributor Author

ASan test_terminal.py is already flaky on master; not sure about macOS test_histfile.py, that one just seems to hang sometimes

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants