Experience with OSC 133 and kitty keyboard protocol enabling sequences
has shown that a lot of users are still on incompatible terminals.
It's not always easy to fix those terminals straight away. There
are probably some more environments where primary device attribute
queries are not answered.
Add a feature flag (similar to keyboard-protocols and mark-prompt)
to allow users to turn this off.
When the terminal fails to respond to primary device attribute, we
already print an error pointing to "help terminal-compatibility".
Inside that document, inside the "primary device attribute" section,
point out this new feature flag.
We still have terminal-specific workarounds based on TERM_PROGRAM and
others, see test/test_driver.py. In future we should get rid of them.
They are also unreliable, potentially missing inside SSH/containers,
incorrect if a terminal was started from another terminal (#11812);
also TERM can be incorrect for many reasons.
The better criterion for terminal-specific workarounds is XTVERSION,
which has none of the above disadvantages.
Since some of the workarounds (tmux, iTerm2) need to be applied before
we draw the first prompt. This also means: before we read any config
because config may call builtin "read".
Do startup queries before reading config.
Some changes implied by this:
1. Remove a call to init_input() which is already done by env_init()
2. call initialize_tty_metadata() only after queries have returned
3. Since we call initialize_tty_metadata() before the first
call to tty.enable_tty_protocols() in Reader::readline(),
we can remove the redundant call from reader_interactive_init().
When poll() or read() on stdin fails, fish's interactive reader
pretends it has received SIGHUP, and subsequently exits.
I don't know if this is the right thing to do, or how to reproduce
this in a realistic scenario.
Unlike fish, fish_key_reader seems to ignore these failures, meaning
it will retry poll()/read() immediately. This seems less correct,
so use fish's behavior for now.
We do the same thing in several places, with lots of small differences.
Extract the most reasonable behavior and use it everywhere. Note that
we had an explictly-motivated ENOTTY check; the motivating issues
doesn't seem to reproduce anymore here though I did not bisect yet.
A process like "fish -i <somefile ..." probably shouldn't query
because it's not gonna work.
In future we could enable this by sending/receiving queries to/from
/dev/tty rather than stdout/stdin.
We are more conservative with querying on startup than we are with
querying for cursor position.
Part of this is oversight (if startup querying is not done, we
basically never get into a position where we query for cursor position,
outside extreme edge cases).
Also, some specific scenarios where we query for cursor position
inside, say, Midnight Commander, are not actually broken, because that
only happens when Midnight Commander gives us control. But let's
favor consistency for now; the Midnight Commander issue should be
fixed soon anyway.
Use the same logic in both cases.
A following commit wants to run the full initialize_tty_metadata()
only after querying XTVERSION.
But MC_TMPDIR needs to be checked before querying for XTVERSION.
Remove this cyclic dependency by extracting the MC_TMPDIR check.
Commands like
fish -C 'read'
create two top-level readers one after the other. The second one is
the fish REPL.
Both run some initialization of globals and parser variables. This is
weird; it should not be necessary.
Let's call reader_interactive_init() only once.
dvtm and abduco are two terminal session managers with the same
original.
Among other issues, they fail to reply to primary device
attribute. We have added a workaround for that based on
TERM=dvtm-256color (unreliable). A patch has been submitted for dvtm
https://lists.suckless.org/hackers/2502/19264.html
I don't know of a maintained fork (the original ones have had no
commit in 5 years) and there are better alternatives available
(shpool, tmux). They have other VT100 compatibility issues as well
(accidental DECRST; something like "ls" Tab Tab Tab causes spurious
bold and underline markup).
Also, as of the parent commit, failure to respond to primary DA is
no longer catastrophic. So let's remove the workaround. This means
that fish inside dvtm/abduco will pause for 2 seconds on startup and
print a warning (unless interrupted by ctrl-c).
Add a timeout of 2 seconds queries; if any query takes longer, warn
about that and reduce the timeout so we stop blocking the UI. This 2
second delay could also happen when network latency is momentarily
really high, so we might want relax this in future.
Note that this timeout is only triggered by a single uninterrupted
poll() (and measured from the start of poll(), which should happen
shortly after sending the query). Any polls interrupted by signals
or uvars/IO port before the timeout would be hit do not matter.
We could change this in future.
Closes#11108Closes#11117
Follow up the cursor position report query with a primary device
attribute one. When that one arrives, any cursor position response
must have arrived too. This allows us to prevent a hang on terminals
that only support primary device attribute.
Instead of switching only on the response type, switch on the
combination of that and the expected response. This helps the
following commits, which add more combinations (due to following up
cursor position report with a primary DA, and adding a timeout). No
behavior change here.
Resolves issue #3126
To match what I've been able to figure out about the existing design
philosophy, case-sensitive matches still always take priority,
but case-insensitive history suggestions precede case-insensitive
completion suggestions.
See commit 081c3282b7 (Refresh TTY timestamps also in some rare cases,
2025-01-15) and others.
Fixes d27f5a5293 (Adopt TtyHandoff in remaining places, 2025-06-21)
Fixes#11671
This adopts the tty handoff in remaining places. The idea is to rationalize
when we enable and disable tty protocols (such as CSI-U).
In particular this removes the tty protocol disabling in Parser::eval_node
- that is intended to execute pure fish script and should not be talking to
the tty.
fish-shell attempts to set up certain terminal protocols (bracketed paste,
CSI-U) while it is in control of the tty, and disable these when passing
off the tty to other processes. These terminal protocols are enabled or
disabled by emitting certain control sequences to the tty.
Today fish-shell does this in a somewhat haphazard way, tracking whether
the protocols are enabled or disabled. Functions like `Parser::exec_node`
then just toggle these, causing data to be written to the terminal in
unexpected places. In particular this is very bad for concurrent execution:
we don't want random threads talking to the tty.
Fortunately we have a controlled place where we can muck with the tty:
`TtyTransfer` which controls handoff of ownership to child processes (via
`tcsetpgrp`). Let's centralize logic around enabling and disabling terminal
protocols there. Put it in a new module and rename it to `TtyHandoff` which is a
little nicer.
This commit moves code around and does some cleanup; it doesn't actually
pull the trigger on centralizing the logic though. Next commit will do that.
Historically, `fish -C "commandline echo"` was silently ignored. Make it do
the expected thing. This won't affect subsequent readers because we only do
it for top-level ones, and reader_pop() will clear the commandline state again.
This improves consistency with the parent commit. We probably don't want to
support arbitrary readline commands before the first reader is initialized,
but setting the initial commandline seems useful: first, it would have helped
me in the past for debugging fish. Second, it would allow one to rewrite
an application launcher:
foot --app-id my-foot-launcher -e fish -C '
set fish_history launcher
bind escape exit
bind ctrl-\[ exit
- function fish_should_add_to_history
- false
- end
- for enter in enter ctrl-j
- bind $enter '\''
- history append -- "$(commandline)"
- commandline "setsid $(commandline) </dev/null >/dev/null 2>&1 & disown && exit"
- commandline -f execute
- '\''
- end
+ commandline "setsid </dev/null >/dev/null 2>&1 & disown && exit"
+ commandline --cursor $(string length "setsid ")
'
which is probably not desirable today because it will disable autosuggestions.
Though that could be fixed eventually by making autosuggestions smarter.
If we find a generally-useful use case, we should mention this in the changelog.
Ref: https://github.com/fish-shell/fish-shell/pull/11570#discussion_r2144544053
Commands like "commandline foo" silently fail, and "complete -C" fails with
a weird "option requires an argument" error.
I think at least the first one can be useful in edge cases, e.g. to test
code that does not separate the `commandline` input and output (#11570),
and to set fish's initial commandline, see the next commit.
I don't think there are super strong reasons to allow these, but if the
existing state is merely due to "no one has ever thought of doing this",
then we should try changing it.
For consistency, also allow "complete -C". I guess an argument for that is
that it's weird to make a command behave differently in non-interactive shells.
For now, keep the historical behavior of disabling access to the command
line in non-interactive shells. If we find a good reason for allowing that
(which seems unlikely), we can.
Ref: https://github.com/fish-shell/fish-shell/pull/11570#discussion_r2144544053
Co-authored-by: Johannes Altmanninger <aclopte@gmail.com>
My
$ sudo docker/docker_run_tests.sh --shell-after docker/jammy-asan.Dockerfile
shows a lot of complaints about
Direct leak of 60 byte(s) in 1 object(s) allocated from:
because some unit tests call reader_init() and reader_deinit(). Work around
this by initializing this value only once. AFAICT, OnceCell is async-signal
safe (unlike Mutex), although I don't think documentation promises that.
It doesn't feel great to change implementation code to accomodate tests but
I think for this specific issue that's what we usually do. Alternatively,
we could add to lsan_suppressions.txt.
The changes to enable terminal protocols (CSI-U, etc) also attempts to
re-disable these when fish exits. In particular it attempts to disable these
from a SIGTERM handler.
Unfortunately none of that machinery is async-signal safe. Indeed our SIGTERM
handler has gotten rather sketchy, with taking a mutex and some other stuff.
Remove the async-signal-unsafe stuff and make SIGTERM manifestly safe.
Unfortunately this means that terminal protocols will remain set after SIGTERM
but that's probably unavoidable.
Calls to redirect_tty_output were added in many places when certain tty-syscalls
returned EIO. See commit 396bf1235d
This was intended to work around a glibc bug in wide character output, but it
was never really justifiable or tested, and we no longer use glibc wide
character output.
Bravely remove these, except in the case where we got SIGHUP and we don't want
to trigger SIGTTIN or SIGTTOU.
This function does not need to run when the pager is not focused. Calling it
lazily eliminates the overhead of calling it when it is not needed.
This code runs on each keypress when entering a command, so it makes sense to
keep it as lean as possible. (At the very least it avoids spam when trying to
debug/analyze gettext behavior.)
Interactive fish with output redirected ("fish >/dev/null")
is not a common use case but it is valid.
Perhaps surprisingly, "fish >some-file" *does* print terminal escape codes
(colors, cursor movement etc.) even if terminal output is not a TTY.
This is typically harmless, and potentially useful for debugging.
We also send blocking terminal queries but those are not harmless in this case.
Since no terminal will receive the queries, we hang; indefinitely as of today,
but we should give up after a timeout and print an error. Either way that
seems needlessly surprising. Suppress queries in this case.
In future, we should probably do something similar if stdin is not a terminal;
though in that case we're even less likely to be interactive (only "-i"
I think).
This documents an invariant established by 532abaddae (Invalidate stale
autosuggestions eagerly, 2024-12-25). It was initially broken but fixed in
ba4ead6ead (Stop saving autosuggestions that we can't restore, 2025-01-17).
After nix updated to 0.30, all functions related to file descriptor accepts impl AsFd, e.g., BorrowedFd. This PR is a minimal update. It tries to use impl AsFd as long as possible, but uses BorrowedFd in some places. Yes it introduces unsafe, but doesn't introduce new unsafe code.
We can parse two different things via Ast:
1. A regular job list
2. A freestanding argument list, as used in `complete --arguments ...`
This second case is specific to one use.
Prior to this commit, we parsed the Ast and then "forgot" what we parsed,
storing a &dyn Node. Then we had to cast it to the right type, and assert,
and etc.
Make Ast generic over the Node type it parsed, and default the Node type to
JobList. This simplifies call sites.
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.
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.
