Skip to content

fix(painter): stop the typing flicker from synchronized updates #1304

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.

Sign up for GitHub

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

Open
kronberger-droid wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(painter): stop the typing flicker from synchronized updates #1304

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 42 additions & 75 deletions src/painting/painter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,9 @@ pub struct Painter {
pub(crate) last_layout: Option<PromptLayout>,
/// Set by `hide_cursor`, cleared by every Show. See [`Painter::show_cursor`].
cursor_hidden: bool,
/// Set by `begin_frame`, cleared by `end_frame`, so a paint ends only a
/// frame that was opened and a plain one sends no marker at all (#1302).
frame_open: bool,
/// The screen height whose last row an external-message commit measured
/// as the real bottom. Until then a commit that scrolls past the last row
/// asks where it ended, since a short reported height would put the real
Expand Down Expand Up @@ -497,6 +500,7 @@ impl Painter {
semantic_markers: None,
last_layout: None,
cursor_hidden: false,
frame_open: false,
bottom_confirmed_at: None,
status_rows: 0,
menu_room: 0,
Expand Down Expand Up @@ -982,9 +986,12 @@ impl Painter {
};
let required_lines = block_rows + status_rows;

// Cursor queries above must finish before the frame begins. End the frame even if
// drawing fails, so a recoverable write error cannot leave updates suspended.
self.begin_frame()?;
// No frame of its own: a paint is far below the writer's buffer and
// goes out as one write, and a multiplexer that handles the frame
// itself redraws the pane at every end marker, which flickered on
// each keystroke (#1302). A frame `print_external_message` left open
// is still ended below, even if drawing fails, so a recoverable
// write error cannot leave updates suspended.
let painted = (|| -> Result<()> {
// Marking the painter state as larger buffer to avoid animations
self.large_buffer = required_lines >= screen_height;
Expand Down Expand Up @@ -1644,23 +1651,28 @@ impl Painter {
fn begin_frame(&mut self) -> Result<()> {
if !self.term_is_dumb {
self.stdout.queue(BeginSynchronizedUpdate)?;
self.frame_open = true;
}
Ok(())
}

/// Queue the end of the frame [`Painter::begin_frame`] opened. Queue it
/// before the flush that sends the frame and before any cursor query: an
/// end left in the buffer holds the screen until the terminal times out.
/// Queue the end of the frame [`Painter::begin_frame`] opened, nothing
/// when none is open. Queue it before the flush that sends the frame and
/// before any cursor query: an end left in the buffer holds the screen
/// until the terminal times out.
fn end_frame(&mut self) -> Result<()> {
if !self.term_is_dumb {
if self.frame_open {
// Cleared only once the end is queued: after a failed write the
// next paint tries again, since a second end marker is harmless
// and a missing one holds the screen.
self.stdout.queue(EndSynchronizedUpdate)?;
self.frame_open = false;
}
Ok(())
}

/// End a frame and send it, for a caller that has nothing to paint after
/// [`Painter::print_external_message`] left one open. With no frame open
/// the end resets a mode that is already off, which no terminal shows.
/// [`Painter::print_external_message`] left one open.
pub(crate) fn finish_frame(&mut self) -> Result<()> {
self.end_frame()?;
self.stdout.flush()
Expand Down Expand Up @@ -2305,71 +2317,34 @@ mod tests {
let out = String::from_utf8_lossy(p.stdout.captured()).into_owned();
let (exit, next) = out.split_at(exit_end);
assert!(
exit.ends_with("\x1b[?25h\x1b[?2026l\x1b[J\r\n"),
exit.ends_with("\x1b[?25h\x1b[J\r\n"),
"exit path changed what it leaves the cursor as: {exit:?}"
);
assert!(
next.starts_with("\x1b[?25l"),
"anchor did not hide the cursor before anything else: {next:?}"
);
assert!(
next.matches("\x1b[?25h").count() == 1 && next.ends_with("\x1b[?25h\x1b[?2026l"),
next.matches("\x1b[?25h").count() == 1 && next.ends_with("\x1b[?25h"),
"cursor shown between the exit newline and the end of the next paint: {next:?}"
);
}

/// The repaint after an external-message commit is what ends the commit's
/// frame. A write that fails while it draws must not skip that end.
#[test]
fn large_repaint_is_bracketed_as_one_synchronized_frame() {
let mut painter = make_painter(1000, 20, false);
painter.stdout = W::capture();
let content = "x".repeat(9000); // Larger than the terminal writer's 8 KiB buffer.
let lines = make_lines(">", " ", "", &content, "");
fn failed_repaint_ends_a_frame_left_open_for_it() {
let mut painter = make_painter(80, 10, false);
painter.term_is_dumb = false;
// Room for the commit and for the reset and the hide that open the
// paint, so the write that fails is one of the draw's.
painter.stdout = W::FailOnce(Vec::new(), 60, false);
painter.prompt_start_row.mark_verified(0);
painter
.repaint_buffer(
&TestPrompt,
&lines,
PromptEditMode::Default,
None,
false,
&None,
)
.expect("large repaint");

let output = painter.stdout.captured();
let start = b"\x1b[?2026h";
let end = b"\x1b[?2026l";
let starts = output
.windows(start.len())
.filter(|bytes| *bytes == start)
.count();
let ends = output
.windows(end.len())
.filter(|bytes| *bytes == end)
.count();
let first_content = output
.iter()
.position(|byte| *byte == b'x')
.expect("painted text");
let last_content = output
.iter()
.rposition(|byte| *byte == b'x')
.expect("painted text");
let frame_start = output
.windows(start.len())
.position(|bytes| bytes == start)
.expect("frame start");
let frame_end = output
.windows(end.len())
.position(|bytes| bytes == end)
.expect("frame end");
assert_eq!((starts, ends), (1, 1));
assert!(frame_start < first_content && last_content < frame_end);
}
.print_external_message(vec!["one".into()])
.expect("print_external_message failed");
assert!(painter.frame_open);

#[test]
fn failed_repaint_ends_the_synchronized_frame() {
let mut painter = make_painter(80, 10, false);
painter.stdout = W::FailOnce(Vec::new(), 25, false);
let lines = make_lines(">", " ", "", "prompt", "");
assert!(painter
.repaint_buffer(
Expand All @@ -2386,8 +2361,8 @@ mod tests {
panic!("test writer changed");
};
assert!(*failed);
assert!(output.windows(8).any(|bytes| bytes == b"\x1b[?2026h"));
assert!(output.ends_with(b"\x1b[?2026l"));
assert!(output.ends_with(b"\x1b[?2026l"), "{output:?}");
assert!(!painter.frame_open);
}

#[test]
Expand Down Expand Up @@ -3190,26 +3165,18 @@ mod tests {
assert!(painter.exit_right_prompt.is_none());
}

/// A paint is one synchronized frame: the terminal holds the erase and
/// the redraw and shows them together. The markers wrap everything that
/// draws, on the small- and the large-buffer path alike. The capture
/// writer ignores flushes, so where the end sits relative to the final
/// flush is not checked here.
/// A paint on its own sends no synchronized-update marker, on the small-
/// and the large-buffer path alike: a multiplexer redraws the whole pane
/// at each end marker, which flickered on every keystroke (#1302).
#[rstest]
#[case::small_buffer("abc", false)]
#[case::large_buffer("l1\nl2\nl3\nl4\nl5\nl6\nl7\nl8\nl9\nl10", true)]
fn a_paint_is_one_synchronized_frame(#[case] before: &str, #[case] expect_large: bool) {
fn a_plain_paint_sends_no_frame_marker(#[case] before: &str, #[case] expect_large: bool) {
let lines = make_lines("> ", "", "", before, "");
let (out, _, large) = capture_repaint(&lines, 0);

assert_eq!(large, expect_large);
// Only the style reset and the cursor hide, which draw nothing, may
// come before the frame; the erase must not.
let begin = out.find("\x1b[?2026h").expect("no begin marker");
let erase = out.find("\x1b[J").expect("no erase");
assert!(begin < erase, "{out:?}");
assert!(out.ends_with("\x1b[?2026l"), "{out:?}");
assert_eq!(out.matches("\x1b[?2026").count(), 2, "{out:?}");
assert!(!out.contains("\x1b[?2026"), "{out:?}");
}

/// `TERM=dumb` would print the markers as text.
Expand Down
Loading