Skip to content
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

Jump to bottom

Fix alternate-screen scrolling #6186

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix alternate-screen scrolling #6186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
19a7942
Region scrolling tests
loops Sep 27, 2024
4f22aab
Region scrolling tests and a fix
loops Sep 27, 2024
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
21 changes: 14 additions & 7 deletions term/src/screen.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,6 +649,8 @@ impl Screen {
) {
let phys_scroll = self.phys_range(scroll_region);
let num_rows = num_rows.min(phys_scroll.end - phys_scroll.start);
let scrollback_ok = scroll_region.start == 0 && self.allow_scrollback;
let insert_at_end = scroll_region.end as usize == self.physical_rows;

debug!(
"scroll_up {:?} num_rows={} phys_scroll={:?}",
Expand All @@ -660,15 +662,15 @@ impl Screen {
// changed by the scroll operation. For normal newline at the bottom
// of the screen based scrolling, the StableRowIndex does not change,
// so we use the scroll region bounds to gate the invalidation.
if scroll_region.start != 0 || scroll_region.end as usize != self.physical_rows {
if !scrollback_ok {
for y in phys_scroll.clone() {
self.line_mut(y).update_last_change_seqno(seqno);
}
}

// if we're going to remove lines due to lack of scrollback capacity,
// remember how many so that we can adjust our insertion point later.
let lines_removed = if scroll_region.start > 0 {
let lines_removed = if !scrollback_ok {
// No scrollback available for these;
// Remove the scrolled lines
num_rows
Expand Down Expand Up @@ -708,7 +710,7 @@ impl Screen {
line.update_last_change_seqno(seqno);
line
};
if scroll_region.end as usize == self.physical_rows {
if insert_at_end {
self.lines.push_back(line);
} else {
self.lines.insert(phys_scroll.end - 1, line);
Expand All @@ -724,12 +726,10 @@ impl Screen {
self.lines.remove(remove_idx);
}

if remove_idx == 0 && self.allow_scrollback {
if remove_idx == 0 && scrollback_ok {
self.stable_row_index_offset += lines_removed;
}

// It's cheaper to push() than it is insert() at the end
let push = scroll_region.end as usize == self.physical_rows;
for _ in 0..to_add {
let mut line = if default_blank == blank_attr {
Line::new(seqno)
Expand All @@ -741,12 +741,19 @@ impl Screen {
)
};
bidi_mode.apply_to_line(&mut line, seqno);
if push {
if insert_at_end {
self.lines.push_back(line);
} else {
self.lines.insert(phys_scroll.end, line);
}
}

// If we have invalidated the StableRowIndex, mark all subsequent lines as dirty
if to_remove > 0 || (to_add > 0 && !insert_at_end) {
for y in self.phys_range(&(scroll_region.end..self.physical_rows as VisibleRowIndex)) {
self.line_mut(y).update_last_change_seqno(seqno);
}
}
}

pub fn erase_scrollback(&mut self) {
Expand Down
108 changes: 108 additions & 0 deletions term/src/test/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1181,6 +1181,114 @@ fn test_1573() {
assert_eq!(graphemes, vec![sequence]);
}

#[test]
fn test_region_scroll() {
let mut term = TestTerm::new(5, 1, 10);
term.print("1\n2\n3\n4\n5");

// Test scroll region that doesn't start on first row, scrollback not used
term.set_scroll_region(1, 2);
term.cup(0, 2);
let seqno = term.current_seqno();
term.print("\na");
assert_all_contents(&term, file!(), line!(), &["1", "3", "a", "4", "5"]);
term.assert_dirty_lines(seqno, &[1, 2], None);
assert_eq!(term.screen().visible_row_to_stable_row(0), 0);
assert_eq!(term.screen().visible_row_to_stable_row(4), 4);

// Scroll region starting on first row, but is smaller than screen (see #6099)
// Scrollback will be used, which means lines below the scroll region
// have their stable index invalidated, and so need to be marked dirty
term.set_scroll_region(0, 1);
term.cup(0, 1);
let seqno = term.current_seqno();
term.print("\nb");
assert_all_contents(&term, file!(), line!(), &["1", "3", "b", "a", "4", "5"]);
term.assert_dirty_lines(seqno, &[2, 3, 4, 5], None);
assert_eq!(term.screen().visible_row_to_stable_row(0), 1);
assert_eq!(term.screen().visible_row_to_stable_row(4), 5);

// Test deletion of more lines than exist in scroll region
term.cup(0, 1);
let seqno = term.current_seqno();
term.delete_lines(3);
assert_all_contents(&term, file!(), line!(), &["1", "3", "", "a", "4", "5"]);
term.assert_dirty_lines(seqno, &[2], None);
assert_eq!(term.screen().visible_row_to_stable_row(0), 1);
assert_eq!(term.screen().visible_row_to_stable_row(4), 5);

// Return to normal, entire-screen scrolling, optimal number of lines marked dirty
term.set_scroll_region(0, 4);
term.cup(0, 4);
let seqno = term.current_seqno();
term.print("\nX");
assert_all_contents(&term, file!(), line!(), &["1", "3", "", "a", "4", "5", "X"]);
term.assert_dirty_lines(seqno, &[6], None);
assert_eq!(term.screen().visible_row_to_stable_row(4), 6);
}

#[test]
fn test_alt_screen_region_scroll() {
// Test that scrollback is never used, and lines below the scroll region
// aren't made dirty or invalid. Only the scroll region is marked dirty.
let mut term = TestTerm::new(5, 1, 10);
term.print("M\no\nn\nk\ne\ny");

// Enter alternate-screen mode, saving current state
term.set_mode("?1049", true);
term.print("1\n2\n3\n4\n5");

// Test scroll region that doesn't start on first row
term.set_scroll_region(1, 2);
term.cup(0, 2);
let seqno = term.current_seqno();
term.print("\na");
assert_all_contents(&term, file!(), line!(), &["1", "3", "a", "4", "5"]);
term.assert_dirty_lines(seqno, &[1, 2], None);
assert_eq!(term.screen().visible_row_to_stable_row(4), 4);

// Test scroll region that starts on first row, still no scrollback
term.set_scroll_region(0, 1);
term.cup(0, 1);
let seqno = term.current_seqno();
term.print("\nb");
assert_all_contents(&term, file!(), line!(), &["3", "b", "a", "4", "5"]);
term.assert_dirty_lines(seqno, &[0, 1], None);
assert_eq!(term.screen().visible_row_to_stable_row(4), 4);

// Return to normal, entire-screen scrolling
// Not optimal, the entire screen is marked dirty for every line scrolled
term.set_scroll_region(0, 4);
term.cup(0, 4);
let seqno = term.current_seqno();
term.print("\nX");
assert_all_contents(&term, file!(), line!(), &["b", "a", "4", "5", "X"]);
term.assert_dirty_lines(seqno, &[0, 1, 2, 3, 4], None);
assert_eq!(term.screen().visible_row_to_stable_row(4), 4);

// Leave alternate-mode and ensure screen is restored, with all lines marked dirty
let seqno = term.current_seqno();
term.set_mode("?1049", false);
assert_all_contents(&term, file!(), line!(), &["M", "o", "n", "k", "e", "y"]);
term.assert_dirty_lines(seqno, &[0, 1, 2, 3, 4], None);
assert_eq!(term.screen().visible_row_to_stable_row(0), 1);
}

#[test]
fn test_region_scrollback_limit() {
// Ensure scrollback is truncated properly, when it reaches the line limit
let mut term = TestTerm::new(4, 1, 2);
term.print("1\n2\n3\n4");
term.set_scroll_region(0, 1);
term.cup(0, 1);

let seqno = term.current_seqno();
term.print("A\nB\nC\nD");
assert_all_contents(&term, file!(), line!(), &["A", "B", "C", "D", "3", "4"]);
term.assert_dirty_lines(seqno, &[0, 1, 2, 3, 4, 5], None);
assert_eq!(term.screen().visible_row_to_stable_row(4), 7);
}

#[test]
fn test_hyperlinks() {
let mut term = TestTerm::new(3, 5, 0);
Expand Down
Loading