-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Make multiline prompt part of ScreenData #11911
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
Conversation
| }); | ||
| if i == 0 { | ||
| // Restore prompt start if we deleted it | ||
| self.write_command(Osc133PromptStart); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, how would we have deleted the prompt start marker?
I think the way you wrote it, we clear to the end of line/screen starting from left_prompt_width,
and we should have written Osc133PromptStart before that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can probably write a test for the presence of OSC 133 A with tmux capture-pane -e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, how would we have deleted the prompt start marker?
This only happens if prompt is not visible because of scroll.
we can probably write a test for the presence of OSC 133 A with tmux capture-pane -e
I don't think tmux supports OSC 133 and from my quick test fish doesn't print it in tmux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only happens if prompt is not visible because of scroll.
Ok, so the three places where we write Osc133PromptStart are (in source order):
- basically two cases
- 1.1 if
actual_left_promptis none (i.e. at first rendering, or if previous prompt was scrolled) and the desired prompt is scrolled out of view - 1.2 if the prompt is single line
- When we draw the first line of a multi-line prompt.
- Here; we add
Osc133PromptStartto the very first line of our screen if we clear the line due to shared prefix mismatch.
I think I get it. In
for i in cmd_start..self.desired.line_count()
if scroll_amount >= prompt_lines then we'd already have written the marker via case 1.1 above (in the other cases cmd_start will be > 0, so the marker is on an earlier line).
Since the marker is zero-width, we can't avoid deleting it here, so we re-add it.
I wonder if our prompt marker could also be accidentally deleted later instead,
specifically in the // Now actually output stuff. part.
It should be possible to tell but I haven't figured it out yet..
If it can be deleted, we would add a similar if i == 0 fix there.
I don't think tmux supports OSC 133 and from my quick test fish doesn't print it in tmux.
It should, see 5496247 (Avoid erasing OSC 133 prompt start marker with clr_eol, 2024-10-12)
Confirmed that we print it with this command. Need to press enter once to flush data through to grep:
tmux new-session 'fish | cat -v | grep --color=auto 133'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't figured it out yet
this would be less relevant if we first computed a ScreenData diff (in a way that supports zero-width elements), and then output the diff with a generic implementation
517e813 to
57c79f8
Compare
| // Output the left prompt if it has changed. | ||
| if self.scrolled() && !is_final_rendering { | ||
| self.r#move(0, 0); | ||
| self.write_command(ClearToEndOfLine); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re f154bc2 (Instead of prompt line_breaks track line_starts, 2025-10-14)
which does
- for (i, &line_break) in left_prompt_layout.line_breaks.iter().enumerate() {
+ for (i, &next_line) in left_prompt_layout.line_starts.iter().enumerate() {
self.write_command(ClearToEndOfLine);shouldn't it be iter().skip(1).enumerate() to avoid doing
ClearToEndOfLine
Osc133PromptStart
ClearToEndOfLine
which would actually (and surprisingly) break the prompt marker on some terminals, see
tmux/tmux#4183
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't it be iter().skip(1).enumerate() to avoid doing
Yes, I just forget to make a change while reworking commit history. Today tried jj, which seems to handle such things much more easily than git. Hope this will help me avoid such mistakes in the future.
which would actually (and surprisingly) break the prompt marker on some terminals
From my testing, WezTerm also can erase the prompt marker with ClearToEndOfLine, while Kitty actually prepends it to commandline (you can see that with ctrl+shift+h). One can only dream of the world there behavior of terminal emulators is standardized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jj
Nice, jj is great for this kind of stuff, I use it everywhere now.
you can see that with ctrl+shift+h
Nice -- I didn't expect that because foot strips control sequences in pipe-scrollback
| self.write_command(Osc133PromptStart); | ||
| } | ||
| let mut start; | ||
| if prompt_changed { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re 3935601 (Refactor prompt printing and var names in Screen::update, 2025-10-14)
So if we have !prompt_changed && prompt_last_line_should_wrap,
then we will no longer output Osc133PromptStart.
I think this makes sense because if only the soft wrap state changes,
then we'll definitely not clear the prompt start marker we've written earlier,
because that one was placed just before the prompt,
and we can assume that $COLUMNS is nonzero
(which is what I tried give an explanation for in #11911 (comment)).
However, what I'm not so sure about is that we still redraw the
prompt marker if the prompt is single-line here:
if left_prompt_layout.line_starts.len() <= 1 {
self.write_command(Osc133PromptStart);
}
is there a reason we still do that rather than only printing the marker when prompt_change is true?
Maybe the answer is already in a thread somewhere and I've forgotten about it, so maybe put it in the commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason we still do that rather than only printing the marker when prompt_change is true?
I think I was just overly cautious when made that decision. It is indeed unnecessary and I don't think it gives much benefit, so I removed it.
| } | ||
| } else { | ||
| start = left_prompt_layout.line_breaks.last().map_or(0, |lb| lb + 1); | ||
| self.r#move(0, prompt_last_line); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to finish my review tomorrow
src/screen.rs
Outdated
| 0, | ||
| self.desired | ||
| .line_count() | ||
| .saturating_sub(usize::try_from(termsize_last().height).unwrap()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this change to the y coordinate it either seems a bit unrelated(?) to the rest of this commit,
or not obvious enough.
The computation here
desired_line_count.saturating_sub(screen_height)
we've already done above when computing ScrolledCursor::scroll_amount.
So maybe we should pass scroll_amount to update() again, or at least extract a scroll_amount variable?
This hack exists because we want to emit \r rather than a more complicated sequence of movement commands.
AFAICT the y coordinate doesn't really matter, since we're gonna move again anyway,
but of course using the "current" line is the goal, which is probably what this change is doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this change to the y coordinate it either seems a bit unrelated(?) to the rest of this commit
I initially changed self.desired.cursor in Screen::write to track position from (0, 0) because of how my mental model of the cursor worked. However, this introduced a bug, so I introduced this code change to fix it.
But this approach don't have any advantages over original, so I reverted it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I messed up my last paragraph, I meant to say that:
if we change it from (0, 0) to (0, current_y),
then we may reduce the needless commands.
I wasn't able to tell whether that's done here or not.
Anyway, reverting seems good (and maybe doing the optimization later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we change it from (0, 0) to (0, current_y),
This somehow fails in tmux-prompt.fish. Will look at it tomorrow.
| #[derive(Clone, Debug, Default, Eq, PartialEq)] | ||
| pub struct PromptLayout { | ||
| /// line breaks when rendering the prompt | ||
| pub line_breaks: Vec<usize>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: stale comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could take this as an opportunity to document things a bit (but no big deal, I'm not sure if this is the best place)
/// Line start offsets for each logical line in the prompt, or equivalently, screen line, because prompt lines are not wrapped but truncated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I shortened it to /// Start offsets for each line in the truncated prompt.
I also was thinking about refactoring prompt handling in the future (e.g. creating TruncPrompt with Index implementation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(e.g. creating TruncPrompt with Index implementation)
That sounds reasonable; it's passed through PromptCacheEntry and ScreenLayout as generic WString,
which is probably not very helpful.
FWIW I think the only reason why PromptLayout is public is that we use it for testing.
Rust wants us to put unit tests into the same file, but I think that's weird,
it's much easier to ignore test code (in grep etc.) when it's in a different file.
I think it's also weird that we do
let layout = compute_layout(...)
...
let left_prompt = layout.left_prompt
...
let left_prompt_layout = cached_layouts.calc_prompt_layout(left_prompt, None, usize::MAX);
Specifically, I'm wondering why we do it piecemeal rather than have a single function compute the entire layout.
For testing, I wonder if we can swap out terminal output with a test-only output class that produces structured data
similar to tests like this https://github.com/mawww/kakoune/blob/master/test/highlight/rust/let/script
But there is also some terminal-related functionality like querying where we want to have at least a mocked terminal to read from
Maybe we should just make the tmux-based tests easier to work with;
it should be made easy to attach a debugger, or attach a tmux client without affecting the behavior of the test.
(also we should replace sleeps with things like sleep-until eventually).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also we should replace sleeps with things like
sleep-untileventually
I actually looked at that and this is my attempt at creating synchronization functions using tmux wait-for:
diff --git a/tests/test_functions/isolated-tmux-start.fish b/tests/test_functions/isolated-tmux-start.fish
index d057af744b..1b1340d7ba 100644
--- a/tests/test_functions/isolated-tmux-start.fish
+++ b/tests/test_functions/isolated-tmux-start.fish
@@ -26,6 +26,22 @@
or sleep 0.3
end
+ function tmux-wait
+ isolated-tmux send-keys f12
+ isolated-tmux wait-for prompt
+ end
+
+ function tmux-capture
+ tmux-wait
+ isolated-tmux capture-pane -p
+ end
+
+ function tmux-exec
+ isolated-tmux send-keys Enter
+ isolated-tmux wait-for postexec
+ tmux-wait
+ end
+
function sleep-until
set -l cmd $argv[1]
set -l i 0
@@ -48,6 +64,13 @@
set fish_history ""
# No transient prompt.
set fish_transient_prompt 0
+
+ function __tmux_postexec_notify --on-event fish_postexec
+ tmux wait-for -S postexec
+ end
+
+ bind f12 "tmux wait-for -S prompt;"
+ bind -M insert f12 "tmux wait-for -S prompt"
' $argv
# Set the correct permissions for the newly created socket to allow future connections.
# This is required at least under WSL or else each invocation will return a permissions error.But after stress testing it by spawning ~300 python test drivers simultaneously, it sadly sometimes fails.
So is repeatedly checking captured stdout only consistent option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow I didn't know about tmux wait-for.
I tested some variations of
sh -c 'for i in $(seq 100); do
tmux wait-for -S topic
done' &
for i in $(seq 100); do
tmux wait-for topic
done
but nothing worked.
There's probably a lot things that could be improved about these tests
It's weird that the test script is a fish script,
when these tests mainly want to test the fish running inside tmux.
I wonder if it would be slightly better to write the test script in Python or Rust, without littlecheck.
littlecheck is a reimplementation of LLVM's lit+FileCheck combo,
which allows to add RUN-commands and CHECK-assertions to test files,
rather than requiring those test files to be able to say, do import pytest; assert actual == expected.
I guess that makes a bit more sense for script tests like tests/checks/abbr.fish,
where an assert_eq $(some command) "expected" currently can't print the line number that failed,
but we could either add enough reflection to fish or write the script in Python or Rust and use their standard test framework.
Anyway, I think we need to synchronize with fish, so using blocking
fifos seems like a reasonable choice (I guess flock() would work too
one some systems):
Start adding proper waiting to tmux tests
I assume that
tmux send-keys a
tmux send-keys b
are always processed in this order by the tmux server.
However the actual processing of keys (by tmux/fish etc.) is async,
which motivates a lot of sleeps. Replace the sleeps with a synchronous
command, that only returns once fish has finished executing the
command.
We still don't know when tmux has actually rendered what's been written
to the TTY, but given that the following works without sleeps now:
for x in $(seq 300); do
printf %s\\n 'tests/test_driver.py target/debug tests/checks/tmux-abbr.fish'
done | parallel
I'd guess that "tmux capture-pane" already forces a render.
diff --git a/tests/checks/tmux-abbr.fish b/tests/checks/tmux-abbr.fish
index 1d713c8425..10ce1e3485 100644
--- a/tests/checks/tmux-abbr.fish
+++ b/tests/checks/tmux-abbr.fish
@@ -10,34 +10,34 @@
isolated-tmux-start
# Expand abbreviations on space.
-isolated-tmux send-keys abbr-test Space arg1 Enter
-tmux-sleep
+isolated-tmux send-keys abbr-test Space arg1
+tmux-execute
# CHECK: prompt {{\d+}}> abbr-test [expanded] arg1
# Expand abbreviations at the cursor when executing.
-isolated-tmux send-keys abbr-test Enter
-tmux-sleep
+isolated-tmux send-keys abbr-test
+tmux-execute
# CHECK: prompt {{\d+}}> abbr-test [expanded]
# Use Control+Z right after abbreviation expansion, to keep going without expanding.
-isolated-tmux send-keys abbr-test Space C-z arg2 Enter
-tmux-sleep
+isolated-tmux send-keys abbr-test Space C-z arg2
+tmux-execute
# CHECK: prompt {{\d+}}> abbr-test arg2
# Same with a redundant space; it does not expand abbreviations.
-isolated-tmux send-keys C-u abbr-test Space C-z Space arg2 Enter
-tmux-sleep
+isolated-tmux send-keys C-u abbr-test Space C-z Space arg2
+tmux-execute
# CHECK: prompt {{\d+}}> abbr-test arg2
# Or use Control+Space to the same effect.
-isolated-tmux send-keys abbr-test C-Space arg3 Enter
-tmux-sleep
+isolated-tmux send-keys abbr-test C-Space arg3
+tmux-execute
# CHECK: prompt {{\d+}}> abbr-test arg3
# Do not expand abbreviation if the cursor is not at the command, even if it's just white space.
# This makes the behavior more consistent with the above two scenarios.
-isolated-tmux send-keys abbr-test C-Space Enter
-tmux-sleep
+isolated-tmux send-keys abbr-test C-Space
+tmux-execute
# CHECK: prompt {{\d+}}> abbr-test
# CHECK: prompt {{\d+}}>
diff --git a/tests/test_functions/isolated-tmux-start.fish b/tests/test_functions/isolated-tmux-start.fish
index faddeff3f9..2bcc835dc9 100644
--- a/tests/test_functions/isolated-tmux-start.fish
+++ b/tests/test_functions/isolated-tmux-start.fish
@@ -39,8 +39,21 @@
end
end
+ mkfifo fifo
+
+ function tmux-execute
+ isolated-tmux send-keys \e\]fish-test-execute\a
+ open_fifo '<'
+ or exit
+ end
+
set -l fish (status fish-path)
isolated-tmux new-session -x 80 -y 10 -d $fish -C '
+ function tmux-execute-completed
+ open_fifo ">"
+ or exit
+ end
+ bind \e\]fish-test-execute\a execute tmux-execute-completed
# This is similar to "tests/interactive.config".
function fish_greeting; end
function fish_prompt; printf "prompt $status_generation> "; end
diff --git a/tests/test_functions/open_fifo.fish b/tests/test_functions/open_fifo.fish
new file mode 100644
index 0000000000..11b94c1850
--- /dev/null
+++ b/tests/test_functions/open_fifo.fish
@@ -0,0 +1,8 @@
+function open_fifo -a redirection
+ timeout 180 sh -c "exec $redirection fifo"
+ set -l saved_status $status
+ if test $saved_status -eq 124
+ echo >&2 "error: timed out in 'exec $redirection fifo'"
+ end
+ return $saved_status
+endThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but nothing worked.
It because 'tmux wait-for -S` is non-blocking and has backlog of 1 for given channel. E.g:
tmux wait-for -S channel
tmux wait-for -S channel
tmux wait-for -S channel
tmux wait-for channel
echo 1
tmux wait-for channel
echo 2
tmux wait-for channel
echo 3will only print 1.
This works:
fish -c 'for i in (seq 100); tmux wait-for -S $i; end' &
for i in (seq 100)
tmux wait-for $i
endThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that would explain it. If possible, we can use either tmux wait-for or fifos to synchronize, probably no big difference.
Though I wonder why tmux wait-for didn't work for you with the hardcoded channel name,
because each tmux session should be isolated, and only use one channel at a time.
Maybe we can reduce it down to a tmux bug.
We probably still want a (high) timeout on the blocking wait, so CI systems don't hang for hours.
fish_postexec sounds good too, seems more reliable.
For the cases where we are capturing a command line state before execution, we'll use your tmux-wait approach.
Probably we can speed up overall test runtime by a lot.
Commit 7db9553 (Fix regression causing off-by-one pager height on truncated autosuggestion) adds line to pager if cursor located at the start of line and previous line is softwrapped, even if line was softwrapped normally and not by autosuggestion, giving pager too much space. Fix that.
It makes it easier to do manipulations with prompt lines.
6e17d83 to
b924405
Compare
Instead of pretending that prompt is always 1 line, track multiline prompt in ScreenData.visible_prompt_lines and ScreenData.line_datas as empty lines. This enables: - Trimming part of the prompt that leaves the viewport. - Removing of the old hack needed for locating first prompt line. - Fixing fish-shell#11875.
First, print prompt marker on repaint even if prompt is not visible.
Second, if we issue a clear at (0, 0) we need to restore marker.
This is necessary for features like Kitty's prompt navigation (`ctrl-shift-{jk}`),
which requires the prompt marker at the top of the scrolled command line
to actually jump back to the original state.
| && self.desired.line_datas[y] | ||
| .color_at(self.desired.line_datas[y].len() - 1) | ||
| .foreground | ||
| == HighlightRole::autosuggestion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks reasonable, might be a small layering violation though.
I wonder if we can improve this future, maybe is_soft_wrapped should be an enum with different values for "actual softwrap" or "softwrapped autosuggestion".
This stuff is super complicated..
| self.write_command(ClearToEndOfScreen); | ||
| if i == 0 && current_width == 0 { | ||
| // Restore prompt marker if we deleted it | ||
| self.write_command(Osc133PromptStart); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find a bug in the Osc133PromptStart placement, but it sure is hard to understand.
Maybe we can use the approach of drawing everything else first,
and then move the cursor to (0, 0) and draw Osc133PromptStart there.
Except in the cases where we didn't delete an earlier marker,
assuming that detecting this case is less complicated than what we have now.
If we're very confident in the correctness, we could do that change first,
to make the main change smaller.
But it's also fine as a follow-up commit/PR.
| /// line breaks when rendering the prompt | ||
| pub line_breaks: Vec<usize>, | ||
| /// width of the longest line | ||
| pub max_line_width: usize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that's the problem with using pub for testing,
we currently don't get "unused" warnings (though that might change), maybe we can write a clippy lint..
| isolated-tmux capture-pane -p | ||
| isolated-tmux capture-pane -p -S -12 | ||
| # CHECK: prompt 4> commandline -i ": '$(seq (math $LINES \* 2))'" right-prompt | ||
| # CHECK: prompt 5> : '1 right-prompt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So -S -12 tests that we no longer have spurious lines in tmux scrollback. Nice.
Commit 7db9553 (Fix regression causing off-by-one pager height on truncated autosuggestion) adds line to pager if cursor located at the start of line and previous line is softwrapped, even if line was softwrapped normally and not by autosuggestion, giving pager too much space. Fix that. Part of fish-shell#11911
It makes it easier to do manipulations with prompt lines. Part of fish-shell#11911
Instead of pretending that prompt is always 1 line, track multiline prompt in ScreenData.visible_prompt_lines and ScreenData.line_datas as empty lines. This enables: - Trimming part of the prompt that leaves the viewport. - Removing of the old hack needed for locating first prompt line. - Fixing fish-shell#11875. Part of fish-shell#11911
Description
Sorry for my sudden disappearance. I kept increasing the scope of my PR and eventually burned out.
Like #11901, this fixes #11875. It also makes fish hide the part of a multiline prompt that leaves the viewport and removes an old hack that was previously necessary for locating the first prompt line.
I have tried to avoid introducing bugs this time, but I would be grateful for any help with testing.