The input queue doesn't want to be blocked, so let's decide later what to
do with a mouse click. This fixes the potential problem where a mouse click
is ignored if it is received while we're waiting for the terminal to repond
to a query.
While we are waiting for a query response from the terminal, we defer any
input processing until we receive our query response
Then that response is promoted to the front of the input queue, and
remaining inputs are processed in order.
We accidentally process focus events, which may run arbitrary code. We can't
do this; it breaks a lot of invariants (for example the it can invalidate
the cursor positions for CursorPositionQuery, or it can cause fish to exit
before fully consumeing a query response).
Make sure we only process known-safe events. We definitely need to process
CheckExit (in case we received SIGHUP). I guess we should also process Eof,
in case the terminal is buggy.
These code paths have a tiny bit of logic in common, share that.
Maybe this should be nested in ImplicitEvent, and maybe Eof and CheckExit
shouldn't be..
This object is initialized once just before we start reading from the terminal.
Once seems to be the appropriate type for this. This gets rid of an awkward
enum variant.
We never need to access this from other threads, so a Mutex is overkill.
Leave behind stale variable names like "wait_guard" to be cleaned up by the
next commit.
Since TestInputEventQueuer is used concurrently in tests,
give it its own private object, to avoid borrowing conflicts.
Same for fish_key_reader; this fixes the issue that fish_key_reader potentially
reads keyboard input before a query is finished.
Old versions of ConHost and Putty can't parse DCS sequences.
For this reason, we briefly switch to the alternate screen buffer while sending
DCS-format (e.g. XTGETTCAP) queries. For extra paranoia, we wrapped this
procedure in a synchronized update. This doesn't seem to be needed; neither
ConHost nor Putty show glitches when the synchronized update is omitted.
As of today, every terminal that implements XTGETTCAP also implements
synchronized updates but that might change.
Let's remove it, to reduce surprise for users and terminal developers.
As a bonus, this also fixes a glitch on Terminal.app which fails to parse
the synchronized-update query (`printf '\x1b[?2026$p'`) and echoes the "p"
(bug report ID FB17141059). Else we could work around this with another
alternate screen buffer.
Unfortunately, this change surfaces two issues with GNU screen. For one,
they don't allow apps to use the alternate screen features (the user may
allow it with "altscreen on"). Second, screen unconditionally echoes
the payload of DCS commands. A possible fix has been suggested at
https://lists.gnu.org/archive/html/screen-devel/2025-04/msg00010.html
I think this combination of behaviors is unique among terminals. I'm sure
there are more terminals that don't parse DCS commands yet, but I think almost
all terminals implement alternate screen buffer. Probably only terminal
multiplexers are prone to this issue. AFAICT very few multiplexers exists,
so we can work around those until they are fixed.
Disable XTGETTCAP queries for GNU screen specifically. Unfortunately screen
does not implement XTVERSION, so I don't know how to reliably identify
it. Instead, check STY and some commonly-used values TERM values.
This has false negatives in some edge cases.
But the worst thing that happens is that "+q696e646e" will be echoed once
at startup, which is easy to ignore, or work around with something like
function workaround --on-event fish_prompt
commandline -f repaint
functions --erase workaround
end
which I don't think we should apply by default, because it can mask other
issues.
We should give screen more time to respond. I guess I'll open an issue so
we don't forget. In doubt, we can always go back to the previous approach
(but implement it in fish script).
Alternative workaround: instead of the alternative screen buffer, we could
try something like clr_eol/clr_bol to erase the spuriously echoed text. I
tried to do this in various ways but (surprisingly) failed.
Two issues:
1. typing the codepoint 0x123456 into fish_key_reader:
$ fish_key_reader -cV
# decoded from: \xf4\xa3\x91
bind \xf4 'do something'
# decoded from:
bind \xa3 'do something'
# decoded from:
bind \x91 'do something'
The invalid codepoint is represented in its original encoding, which leaks
to the UI. This was more or less intentionally added by b77d1d0e2b (Stop
crashing on invalid Unicode input, 2024-02-27). That commit rendered it
as replacement byte, but that was removed for other reasons in e25a1358e6
(Work around broken rendering of pasted multibyte chars in non-UTF-8-ish
locale, 2024-08-03).
We no longer insert such (PUA) codepoints into the commandline. The "bind"
comes above would work however. I don't think this is something we want
to support. Discard invalid codepoints in the reader, so they can't be
bound and fish_key_reader shows nothing.
2. builtin read silently drops invalid encodings This builtin is not really
suited to read binary data (#11383 is an error scenario), but I guess it can
be bent to do that. Some of its code paths use str2wcstring which passes
through e.g. invalid UTF-8. The read-one-char-at-a-time code path doesn't.
Fix this.
As mentioned in
https://github.com/fish-shell/fish-shell/pull/9688#discussion_r1155089596,
commit b77d1d0e2b (Stop crashing on invalid Unicode input, 2024-02-27), Rust's
char type doesn't support arbitrary 32-bit values. Out-of-range Unicode
codepoints would cause crashes. That commit addressed this by converting
the encoded bytes (e.g. UTF-8) to special private-use-area characters that
fish knows about. It didn't bother to update the code path in builtin read
that relies on mbrtowc as well.
Fix that. Move and rename parse_codepoint() and rename/reorder its input/output
parameters.
Note that the behavior is still wrong if builtin read can't decode the
input; see the next commit.
Fixes#11383
This also changes the single-byte locale code path to treat keyboard input
like "\x1ba" as alt-a instead of "escape,a". I can't off-hand reproduce
a problem with "LC_ALL=C fish_key_reader", I guess we always use a UTF-8
locale if available?
Our use of the terminfo database in /usr/share/terminfo/$TERM is both
1. a way for users to configure app behavior in their terminal (by
setting TERM, copying around and modifying terminfo files)
2. a way for terminal emulator developers to advertise support for
backwards-incompatible features that are not otherwise easily observable.
To 1: this is not ideal (it's very easy to break things). There's not many
things that realistically need configuration; let's use shell variables
instead.
To 2: in practice, feature-probing via terminfo is often wrong. There's not
many backwards-incompatible features that need this; for the ones that do
we can still use terminfo capabilities but query the terminal via XTGETTCAP
directly, skipping the file (which may not exist on the same system as
the terminal).
---
Get rid of terminfo. If anyone finds a $TERM where we need different behavior,
we can hardcode that into fish.
* Allow to override this with `fish_features=no-ignore-terminfo fish`
Not sure if we should document this, since it's supposed to be removed soon,
and if someone needs this (which we don't expect), we'd like to know.
* This is supported on a best-effort basis; it doesn't match the previous
behavior exactly. For simplicity of implementation, it will not change
the fact that we now:
* use parm_left_cursor (CSI Ps D) instead of cursor_left (CSI D) if
terminfo claims the former is supported
* no longer support eat_newline_glitch, which seems no longer present
on today's ConEmu and ConHost
* Tested as described in https://github.com/fish-shell/fish-shell/pull/11345#discussion_r2030121580
* add `man fish-terminal-compatibility` to state our assumptions.
This could help terminal emulator developers.
* assume `parm_up_cursor` is supported if the terminal supports XTGETTCAP
* Extract all control sequences to src/terminal_command.rs.
* Remove the "\x1b(B" prefix from EXIT_ATTRIBUTE_MODE. I doubt it's really
needed.
* assume it's generally okay to output 256 colors
Things have improved since commit 3669805627 (Improve compatibility with
0-16 color terminals., 2016-07-21).
Apparently almost every actively developed terminal supports it, including
Terminal.app and GNU screen.
* That is, we default `fish_term256` to true and keep it only as a way to
opt out of the the full 256 palette (e.g. switching to the 16-color
palette).
* `TERM=xterm-16color` has the same opt-out effect.
* `TERM` is generally ignored but add back basic compatiblity by turning
off color for "ansi-m", "linux-m" and "xterm-mono"; these are probably
not set accidentally.
* Since `TERM` is (mostly) ignored, we don't need the magic "xterm" in
tests. Unset it instead.
* Note that our pexpect tests used a dumb terminal because:
1. it makes fish do a full redraw of the commandline everytime, making it
easier to write assertions.
2. it disables all control sequences for colors, etc, which we usually
don't want to test explicitly.
I don't think TERM=dumb has any other use, so it would be better
to print escape sequences unconditionally, and strip them in
the test driver (leaving this for later, since it's a bit more involved).
Closes#11344Closes#11345
I don't think we want to support terminals that implement XTGETTCAP but for
some reason don't use CSI Ps S for scroll forward; that would be a needless
complication.
Let's make ctrl-l / scrollback-push fail hard if a terminal does this.
Confusingly kitty and foot use different response formats, but happily we
no longer care.
An upcoming commit will document that we require the CSI Ps S style.
We canonicalize "ctrl-shift-i" to "ctrl-I".
Both when deciphering this notation (as given to builtin bind),
and when receiving it as a key event ("\e[105;73;6u")
This has problems:
A. Our bind notation canonicalization only works for 26 English letters.
For example, "ctrl-shift-ä" is not supported -- only "ctrl-Ä" is.
We could try to fix that but this depends on the keyboard layout.
For example "bind alt-shift-=" and "bind alt-+" are equivalent on a "us"
layout but not on a "de" layout.
B. While capslock is on, the key event won't include a shifted key ("73" here).
This is due a quirk in the kitty keyboard protocol[^1]. This means that
fish_key_reader's canonicalization doesn't work (unless we call toupper()
ourselves).
I think we want to support both notations.
It's recommended to match all of these (in this order) when pressing
"ctrl-shift-i".
1. bind ctrl-shift-i do-something
2. bind ctrl-shift-I do-something
3. bind ctrl-I do-something
4. bind ctrl-i do-something
Support 1 and 3 for now, allowing both bindings to coexist. No priorities
for now. This solves problem A, and -- if we take care to use the explicit
shift notation -- problem B.
For keys that are not affected by capslock, problem B does not apply. In this
case, recommend the shifted notation ("alt-+" instead of "alt-shift-=")
since that seems more intuitive.
Though if we prioritized "alt-shift-=" over "alt-+" as per the recommendation,
that's an argument against the shifted key.
Example output for some key events:
$ fish_key_reader -cV
# decoded from: \e\[61:43\;4u
bind alt-+ 'do something' # recommended notation
bind alt-shift-= 'do something'
# decoded from: \e\[61:43\;68u
bind alt-+ 'do something' # recommended notation
bind alt-shift-= 'do something'
# decoded from: \e\[105:73\;6u
bind ctrl-I 'do something'
bind ctrl-shift-i 'do something' # recommended notation
# decoded from: \e\[105\;70u
bind ctrl-shift-i 'do something'
Due to the capslock quirk, the last one has only one matching representation
since there is no shifted key. We could decide to match ctrl-shift-i events
(that don't have a shifted key) to ctrl-I bindings (for ASCII letters), as
before this patch. But that case is very rare, it should only happen when
capslock is on, so it's probably not even a breaking change.
The other way round is supported -- we do match ctrl-I events (typically
with shifted key) to ctrl-shift-i bindings (but only for ASCII letters).
This is mainly for backwards compatibility.
Also note that, bindings without other modifiers currently need to use the
shifted key (like "Ä", not "shift-ä"), since we still get a legacy encoding,
until we request "Report all keys as escape codes".
[^1]: <https://github.com/kovidgoyal/kitty/issues/8493>
While at it, don't escape "?", I don't know why 68e167d576 (f-k-r should
use the user's locale, 2016-06-29) did that. Question mark is only special
in combination with redirections.
This add two commands history-last-token-search-backward and
history-last-token-search-forward which behaves like bash's yank-last-arg. So
similar to history-token-search-* but only considers the last argument for
each command.
Closes#10756Closes#11258
As reported on gitter, fish running inside a qemu console randomly fails to
recognize multi-byte sequences like "\e[D" (right); it sometimes recognizes
the first two bytes as "alt-[" and the last byte as the "D" key.
This because 8bf8b10f68 (Extended & human-friendly keys, 2024-03-30) changed
our approach to reading multi-byte key sequences. Previously, we'd wait
forever (or rather fish_sequence_key_delay_ms) for the "D" byte.
As of 8bf8b10f68, we assume the entire sequence is already present in the
input buffer; and stop parsing the sequence if stdin is not readable.
It would be more technically correct to implement the VT state machine but
then we'd probably want to to figure out a timeout or a reset key, in case
of transport or terminal issues.
Returning early is also what we have historically done for multi-byte code
points. Also, other terminal programs have been using it for many years
without problems.
I don't know why this happens in qemu but it seems we can work around by
setting a 1ms timeout. This timeout should be small enough two keys "escape"
and "[" typed by a human will still be seen separate.
Refs:
https://matrix.to/#/!YLTeaulxSDauOOxBoR:matrix.org/$Cfi9wL8FGLAI6_VAQWG2mG_VxsADUPvdPB46P41Jdbshttps://matrix.to/#/!YLTeaulxSDauOOxBoR:matrix.org/$O_-LZ1W7Dk6L_4Rj0MyCry6GtO2JQlEas8fH9PrSYT8
As reported in b5736c5535 (Extend iTerm CSI u workaround to < 3.5.12,
2025-02-20), iTerm 3.5.12 has resolved our issues related to the kitty
keyboard protocol. Enable it here too, matching the release branch.
The flag to gate this is set for versions of iTerm that don't have sufficient
support for the kitty keyboard protocol. CSI u is (more or less) the encoding
used by that protocol. Let's name things accordingly. My bad.
Whenever we add logic to print a control sequence that we hadn't printed
before, there is a nonzero risk that a terminal mishandles it.
Terminal-specific workarounds cause pain but are probably better than not
being able to use any new commands provided by terminals.
There is no universal way to identify a terminal. Device attributes (primary
through tertiary) typically get spoofed responses, likely not good enough
for working around bugs in specific versions of a terminal.
The de-facto standard for the terminal name and version is XTVERSION.
It's usually specific to the terminal, except for something like VTE-based
terminals, where we get this (which seems good enough also)
printf '\x1b[>0q'; cat
^[P>|VTE(7803)^[\
Of course querying for XTVERSION can trigger terminal bugs just as well. Let's
start querying for it now -- even without a concrete use case -- to increase
the chance we can use it during crunch time when we don't want to test
anymore. (We typically discover buggy terminals only very late in the release
cycle, most prominently after a release).
When we enable/disable terminal protocols,
we use atomic operations because of issues like
1. halfway through enabling, we might be interrupted by a signal handler.
2. our SIGTERM handler runs the (idempotent) disabling sequences,
so the operations must be async-signal safe.
The flags to keep track of whether things like kitty keyboard protocol are enabled
are "mirrored" between the enabling and disabling logic:
- the enabling logic marks it as enabled *before* enabling anything
- the disabling logic marks it as disabled *after* everything has been disabled
This ensures that we are well-behaved in issue 1; we will always (perhaps
redundantly) disable the kitty keyboard protocol.
We forgot to use the same ordering for bracketed paste.
If we get SIGTERM after this line
BRACKETED_PASTE.store(false, Ordering::Release);
we might exit with bracketed paste still turned on.
e697add5b5 (Feature flag to prevent executing off buffered keys, 2025-01-02)
breaks my expectations/habits, and it breaks Midnight Commander.
Additionally, I'm not aware of any case where it actually adds security.
We generally assume that terminal echoback sequences do not contain
control characters except for well-known escape sequences.
This backs out commit e697add5b5.
See #10987, #10991
Every reader gets their own wait handle which is wrong and not actually
needed - it's a singleton. We should probaly make it global. Let's
do an intermediate solution for now -- not much time this weekend ;).
Fixes#11110
The two terminals Midnight Commander and dvtm are special in that
they filter requests (or perhaps responses) like
printf "\x1b[0c"
and don't implement the response themselves -- so we never get
one. Let's work around that until we can fix it.
Disable the kitty protocol in mc for now (to keep the code simple),
though we could certainly re-enable it.
Fixes 64859fc242 (Blocking wait for responses to startup queries, 2025-01-25).
This was copied from C++ code but we have overflow checks, which
forces us to actually handle errors.
While at it, add some basic error logging.
Fixes#11092
Today we might
1. enable modifyOtherKeys
2. get a reply indicating the kitty keyboard protocol is supported
3. because of 2, we never turn off modifyOtherKeys again
Let's get rid of this weird issue by enabling either modifyOtherKeys
or the kitty enhancements only after we know whether the kitty protocol
is supported.
This means we need to call terminal_protocols_enable_ifn() before every
call to readch() until the querying is done. Fortunately, this is
already in place in read_normal_chars(); there are other places that
call readch() but none of those is executed until querying has completed.
At startup we query for
- the cursor position (CSI 6 n)
- kitty keyboard protocol support (CSI ? u)
- terminfo capabilities via XTGETTCAP
Since we don't wait for responses, those can leak into child processes.
Some child processes like fzf cannot decode DCS replies. Plug the
leak by ending each round of querying by asking for the Primary Device
Attribute, and resume input processing only after a response has been
received, (or ctrl-c as an escape hatch).
This is a nice simplification. Tested with the lowest common
denominator (putty, Terminal.app and st).
Fixes#11079
The st terminal wrongly parses CSI ? u as DECRC. A fix has been
proposed upstream. Let's also work around it I guess (not to mention
that querying in the first place is also sort of a workaround).
This reverts commit ebdc3a0393.
Not discussed, includes a new thing that queries the terminal for the client OS
when what is really needed is just a `uname` - which would also work on Terminal.app.
As mentioned in
https://github.com/fish-shell/fish-shell/pull/11045#discussion_r1915994998,
we need to refresh TTY timestamps to avoid timing-based issues.
For some context see
git log --grep='[Rr]efresh.* TTY'
Make things more consistent again. I don't know if all of these are
absolutely necessary, hoping to find out later (and consolidate this
logic in outputter).
And leave the old behavior under the name "cancel-commandline".
This renames "cancel-commandline-traditional" back to
"cancel-commandline", so the old name triggers the old behavior.
Fixes#10935
Zellij 0.41.2 has a bug where it responds to
printf '\x1b[?2026$p'; cat -v
with '^[[2026;2$y' (DECRQM) instead of '^[[?2026;2$y' (DECRPM).
This is fixed by https://github.com/zellij-org/zellij/pull/3884
We fail to parse it, leading to an extra y added to the input queue.
Since it seems easy to work around for us, let's do that, I guess.
Some terminals such as conhost and putty cannot parse DCS commands,
and will echo them back.
Work around this by making sure that this echoed text will not
be visible.
Do so by temporarily enabling the alternative screen buffer when
sending DCS queries (in this case only XTGETTCAP). The alternative
screen buffer feature seems widely supported, and easier to get right
than trying to clear individual lines etc.
The alternative screen may still be visible for a
short time. Luckily we can use [Synchronized Output](
https://gist.github.com/christianparpart/d8a62cc1ab659194337d73e399004036)
to make sure the screen change is never visible to the user.
Querying support for that is deemed safe since it only requires a
CSI command.
Note that it seems that every terminal that supports Synchronized
Output also parses DCS commands successfully. This means that we
could get away without the alternative screen buffer in practice.
Not sure yet.
The implementation is slightly more complex than necessary in that it
defines a redundant ImplicitEvent. This is for two reasons: 1. I have
a pending change that wants to use it, so this removes diff noise and
2. we historically have sc/input_common.rs not depend on src/output.rs.
I dont' think any are strong reasons though.
