feat(scrolling-regions)!: use terminal scrolling regions to stop Terminal::insert_before from flickering
The current implementation of Terminal::insert_before causes the viewport to flicker. This is described in #584 .
This PR removes that flickering by using terminal scrolling regions (sometimes called "scroll regions"). A terminal can have its scrolling region set to something other than the whole screen. When a scroll ANSI sequence is sent to the terminal and it has a non-default scrolling region, the terminal will scroll just inside of that region.
We use scrolling regions to implement insert_before. We create a region on the screen above the viewport, scroll that up to make room for the newly inserted lines, and then draw the new lines. We may need to repeat this process depending on how much space there is and how many lines we need to draw.
When the viewport takes up the entire screen, we take a modified approach. We create a scrolling region of just the top line (could be more) of the viewport, then use that to draw the lines we want to output. When we're done, we scroll it up by one line, into the scrollback history, and then redraw the top line from the viewport.
A final edge case is when the viewport hasn't yet reached the bottom of the screen. This case, we set up a different scrolling region, where the top is the top of the viewport, and the bottom is the viewport's bottom plus the number of lines we want to scroll by. We then scroll this region down to open up space above the viewport for drawing the inserted lines.
Regardless of what we do, we need to reset the scrolling region. This PR takes the approach of always resetting the scrolling region after every operation. So the Backend gets new scroll_region_up and scroll_region_down methods instead of set_scrolling_region, scroll_up, scroll_down, and reset_scrolling_region methods. We chose that approach for two reasons. First, we don't want Ratatui to have to remember that state and then reset the scrolling region when tearing down. Second, the pre-Windows-10 console code doesn't support scrolling regio
This PR:
- Adds a new scrolling-regions feature.
- Adds two new Backend methods: scroll_region_up and scroll_region_down.
- Implements those Backend methods on all backends in the codebase.
- The crossterm and termion implementations use raw ANSI escape sequences. I'm trying to merge changes into those two projects separately to support these functions.
- Adds code to Terminal::insert_before to choose between insert_before_scrolling_regions and insert_before_no_scrolling_regions. The latter is the old implementation.
- Adds lots of tests to the TestBackend to for the scrolling-region-related Backend methods.
- Adds versions of terminal tests that show that insert_before doesn't clobber the viewport. This is a change in behavior from before.
Codecov Report
Attention: Patch coverage is 60.00000% with 118 lines in your changes missing coverage. Please review.
Project coverage is 92.8%. Comparing base (
bc10af5) to head (9e49d10). Report is 18 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #1341 +/- ##
=======================================
- Coverage 93.5% 92.8% -0.8%
=======================================
Files 67 67
Lines 15897 16107 +210
=======================================
+ Hits 14871 14954 +83
- Misses 1026 1153 +127
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Bencher Report
| Branch | 1341/merge |
| Testbed | ubuntu-latest |
⚠️ WARNING: The following Measure does not have a Threshold. Without a Threshold, no Alerts will ever be generated!
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsCLI flag.
Click to view all benchmark results
The major concern I had with these changes was whether or not scrolling regions would be supported by all terminals, and if not, what the best way was to address #584 while not breaking Terminal::insert_after for those terminals that don't support scrolling regions.
My first attempt at that was to create a scrolling-regions feature, so that the feature could be enabled by applications that could guarantee they would only be run on appropriate terminals. I don't imagine this is a long-term solution.
From my research, it seems like anything that is vt100-related supports scrolling regions. Window's implementation of ANSI escape sequences supports them, and all Linux and Mac terminal emulators I've tried do support them. The big issue is pre-windows-10 windows support within crossterm.
As a intermediate solution, we could have both implementations compiled in. We'd start with the scrolling-regions one and fall back to the existing implementation. Thus, the but would be fixed on everything but old windows code. The only risk here is that some terminals may not support these ANSI escape sequences. As I said above, I think that's very unlikely: everything I can find that supports ANSI escape sequences supports scrolling regions.
As a long-term solution, we could implement the appropriate behavior in crossterm for windows, and then always rely on scrolling regions. This should be possible, looking at the windows documentation, but I would need to do some work to get it tested and integrated.
One advantage of the intermediate solution is that it wouldn't break external implementations of Backend. I don't know how important it is to avoid this type of breaking change. The long-term solution would require that all Backends support scrolling a region of the screen somehow, since the old code would be removed.
I should also note that building maelstrom with this feature enabled results in a much improved TUI. The flickering that was pretty bad before is completely eliminated.
I think I have addressed all of the comments.
I'm not sure what to do about the failing coverage check. It's not running coverage checks with scrolling-regions enabled.
I have created two separate, mutually exclusive, PRs for crossterm:
- https://github.com/crossterm-rs/crossterm/pull/918
- https://github.com/crossterm-rs/crossterm/pull/923 I have also created a merge request for termion:
- https://gitlab.redox-os.org/redox-os/termion/-/merge_requests/196
If/when these get merged, the appropriate backends should be updated.
What can we do to get this across the line?
Hi! Test drove this PR, the flickering is gone :)
That being said, there doesn't seem to be a way with the new insert_before to insert a line longer than the terminal width. This is useful to emulate println!() above a powerline. Lines shorter will get clipped, and while you can wrap them yourself, that's not exactly the same as the line truly being longer than the terminal width (as is common with log lines such that the emulator can wrap them).
I'm not sure if there's anything that can be done in this PR to enable that, but it's worth calling out.
Even an insert_lines_before would be nice such that each line is unbounded in length.
I've implemented a workaround using crossterm directly, but it might be a worthwhile addition to this PR to have this be a supported directly.
I've implemented a workaround using crossterm directly, but it might be a worthwhile addition to this PR to have this be a supported directly.
I don't think it would be part of this PR (mainly because if we tried to solve all the problems all at once we'd be here until the world ends reviewing things). But the idea is worth investigating more certainly. What I'd suggest is cutting another issue to list out the problems you're seeing and the specific functionality that you're trying to achieve. Include the workaround that you're using and how it acts under various circumstances.
That being said, there doesn't seem to be a way with the new
insert_beforeto insert a line longer than the terminal width. This is useful to emulateprintln!()above a powerline. Lines shorter will get clipped, and while you can wrap them yourself, that's not exactly the same as the line truly being longer than the terminal width (as is common with log lines such that the emulator can wrap them).I'm not sure if there's anything that can be done in this PR to enable that, but it's worth calling out.
Yeah, that's an interesting issue with the current API. What would your ideal API look like?
I'm curious how the scrolling would interact with the terminal's line wrapping. You might be better off emulating the wrapping yourself so you know how much to scroll by.
I've implemented a workaround using crossterm directly, but it might be a worthwhile addition to this PR to have this be a supported directly.
Can you share how your workaround works? I'm curious.
I addressed all of @joshka 's comments, I think. Please let me know what's next.
I'm not sure why the test backend::test::tests::buffer_view_with_overwrites is failing in CI. I can't reproduce it on my end and none of the changes in my PR should affect that functionality.
Yeah, that's an interesting issue with the current API. What would your ideal API look like?
Ideally I'd be able to widgets/lines into a buffer with an unbounded (or just really large) width and then let the emulator handle the wrapping of the individual lines.
If you try this today with the current API ratatui simply panics. Ideally we could just use today's API but maybe resize the viewport before we push the widget into it, and insert_before would calculate the number of lines it needs to scroll up before inserting the line.
I'm curious how the scrolling would interact with the terminal's line wrapping. You might be better off emulating the wrapping yourself so you know how much to scroll by.
As far as I can tell, you need to scroll the scroll region up to make enough room for the wrapped line (which could wrap to multiple lines) and then call println!() at the top line. The emulator apparently will wrap this properly but then also treat the line as a true line (collapsing the line breaks when the viewport is resized).
I think with scroll regions you can just simply println!() at the bottom of the region and the region will scroll up, so you wouldn't need to implement the wrapping math yourself, hence why I jumped into this PR.
Can you share how your workaround works? I'm curious.
The gist of it is that you call insert_before with the number of lines the widget will be after it's wrapped, and then you manually call println!() at the top of that free space.
This works with long lines and multilines (ignore the panic I inserted...):
I do have a bug where the grapheme calculation is slightly off when the incoming ansi sequences seem to cause the line wrapping calculation to be slightly off.
fn drain_logs(
&mut self,
terminal: &mut Terminal<CrosstermBackend<io::Stdout>>,
) -> io::Result<()> {
use unicode_segmentation::UnicodeSegmentation;
let Some(log) = self.pending_logs.pop() else {
return Ok(());
};
// Grab out the size and location of the terminal and its viewport
let frame_rect = terminal.get_frame().area();
let term_size = terminal.size().unwrap();
// Create a paragraph by escaping the contents of the log, which is already ansi escaped
let mut overflowed_lines = 0;
for line in log.content.lines() {
let grapheme_count = line.graphemes(true).count() as u16;
if grapheme_count > term_size.width {
// Subtract 1 since we already know this line will count as at least one line
overflowed_lines += grapheme_count.div_ceil(term_size.width) - 1;
}
}
let byte_count = log.content.len() as u16;
let paragraph = Paragraph::new(log.content.into_text().unwrap());
let line_count = paragraph.line_count(term_size.width) as u16;
// We want to get the escaped ansii string and then by dumping the paragraph as ascii codes (again)
// This is important because the line_count method on paragraph takes into account the width of these codes
let mut raw_ansi_buf = AnsiStringBuffer::new(3000.max(byte_count), line_count);
raw_ansi_buf.render_ref(¶graph, raw_ansi_buf.buf.area);
// Calculate how many lines we need to draw by adding the real lines to the wrapped lines
let lines_to_draw = line_count + overflowed_lines;
let space_available = term_size.height - self.viewport_current_height() - 1;
// Rendering this line will eat the frame, so just shortcut a more reliable path
// Render the new line at the top of the viewport, and then some spaces so that when we call "clear"
// The lines will have been scrolled up
if space_available < lines_to_draw {
crossterm::queue!(
std::io::stdout(),
crossterm::cursor::MoveTo(0, frame_rect.y),
crossterm::style::Print(raw_ansi_buf.to_string().trim_end()),
crossterm::style::Print("\n"),
crossterm::style::Print(
(0..self.viewport_current_height() - 1)
.map(|_| "\n")
.collect::<String>()
),
)?;
terminal.clear()?;
return Ok(());
}
// In the case where the log will fit on the screen, we want to make some room for it
// by adding some lines above the viewport. `insert_before` will eventually use scroll regions
// in ratatui, so we're just going to use that, even if it has extra flickering in the interim.
terminal.insert_before(lines_to_draw, |_| {})?;
// If the viewport is at the bottom of the screen, our new log will be inserted right above
// the viewport. If not, the viewport will shift down by lines_to_draw *or* the space available
let y_offset = match frame_rect.y - 1 < space_available {
true => 0,
false => lines_to_draw,
};
// Finally, print the log to the terminal using crossterm, not ratatui
// We are careful to handle the case where the log won't fit on the screen, since that will
// cause this code to be called with the wrong viewport and cause tearing.
raw_ansi_buf.trim_end();
let buf = raw_ansi_buf.to_string();
let mut max_idx = 0_u16;
for (idx, line) in buf.lines().enumerate() {
if line.is_empty() {
continue;
}
let start = frame_rect.y.saturating_sub(y_offset) + idx as u16;
crossterm::queue!(
std::io::stdout(),
crossterm::cursor::MoveTo(0, start),
crossterm::style::Print(line),
crossterm::style::Print("\n"),
)?;
max_idx = idx as _;
}
// Just sanity check that our math works out
assert_eq!(max_idx - line_count, 0);
Ok(())
}
Ah, I see what you're doing. Thanks for sharing.
I think you're right about doing this with scrolling regions. Assuming the viewport doesn't take up the whole screen, you'd just set the scrolling region, append a lot of lines at the bottom of the region, then reset the scrolling region. Right?
When the viewport takes up the whole screen, I think the top-line trick would work as well, wouldn't it? The terminal emulator would scroll up the top line when doing wrapping. It seems like something to play with.
To implement your function, I think we'd want to split up the backend functions into four, instead of two:
- Set scrolling region.
- Reset scrolling region (to whole screen).
- Scroll up.
- Scroll down.
I think that's do-able for all backends except for crossterm for old windows terminals (ones that don't support ANSI sequences). Currently, this crossterm implementation doesn't support old windows terminals either, but it could in theory.
I addressed all of @joshka 's comments, I think. Please let me know what's next.
I'm not sure why the test
backend::test::tests::buffer_view_with_overwritesis failing in CI. I can't reproduce it on my end and none of the changes in my PR should affect that functionality.
I also can't repro the test failures locally. I'll rerun them to see if it was some weird intermittent failure. https://github.com/ratatui/ratatui/actions/runs/10964957433?pr=1341
Edit: managed a local repro with cargo update (we need to checkin cargo.lock to make this deterministic). It looks like unicode-width released a change which broke somethings in 0.1.14. Locking unicode-width to 0.1.13 for now until we diagnose it more.
I don't have permission to edit the PR, but here's the patch to fix this:
diff --git a/Cargo.toml b/Cargo.toml
index 4a8bdac9c..7e4597202 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -38,7 +38,7 @@ termwiz = { version = "0.22.0", optional = true }
time = { version = "0.3.11", optional = true, features = ["local-offset"] }
unicode-segmentation = "1.10"
unicode-truncate = "1"
-unicode-width = "0.1.13"
+unicode-width = "=0.1.13"
[target.'cfg(not(windows))'.dependencies]
# termion is not supported on Windows
I merged in @joshka's fix from main and that seems to fix it.
Ok, then we're just working out release timing on this one. I'm not sure whether the next release will be 0.28.2 (without this) or 0.29.0 (with this).
I think it's time for a 0.29.0 but let's discuss what other features we are likely to include in it.
Yeah, that's an interesting issue with the current API. What would your ideal API look like?
Ideally I'd be able to widgets/lines into a buffer with an unbounded (or just really large) width and then let the emulator handle the wrapping of the individual lines.
If you try this today with the current API ratatui simply panics. Ideally we could just use today's API but maybe resize the viewport before we push the widget into it, and
insert_beforewould calculate the number of lines it needs to scroll up before inserting the line.I'm curious how the scrolling would interact with the terminal's line wrapping. You might be better off emulating the wrapping yourself so you know how much to scroll by.
As far as I can tell, you need to scroll the scroll region up to make enough room for the wrapped line (which could wrap to multiple lines) and then call
println!()at the top line. The emulator apparently will wrap this properly but then also treat the line as a true line (collapsing the line breaks when the viewport is resized).I think with scroll regions you can just simply println!() at the bottom of the region and the region will scroll up, so you wouldn't need to implement the wrapping math yourself, hence why I jumped into this PR.
Can you share how your workaround works? I'm curious.
The gist of it is that you call
insert_beforewith the number of lines the widget will be after it's wrapped, and then you manually callprintln!()at the top of that free space.This works with long lines and multilines (ignore the panic I inserted...):
I opened #1426.