Skip to content

Commit

Permalink
fix(terminal): some real/saved cursor bugs during resize (#3032)
Browse files Browse the repository at this point in the history
* refactor: Simplify transfer_rows_from_viewport_to_lines_above

next_lines is always consolidated to a single Row, which immediately
gets removed - we can remove some dead code as a result

* perf: Batch remove rows from the viewport for performance

Given a 1MB line catted into the terminal, a toggle-fullscreen +
toggle-fullscreen + close-pane + `run true` goes from ~9s to ~3s

* perf: Optimize Row::drain_until by splitting chars in one step

Given a 10MB line catted into the terminal, a toggle-fullscreen +
toggle-fullscreen + close-pane + `run true` goes from ~23s to ~20s

* refactor: Simplify `if let` into a `.map`

* refactor: There are only new saved coordinates when there were old ones

* refactor: Unify viewport transfer: use common variable names

* fix: Use same saved cursor logic in height resize as width

See #2182 for original introduction that only added it in one branch,
this fixes an issue where the saved cursor was incorrectly reset when
the real cursor was

* fix: Correct saved+real cursor calculations when reflowing long lines

* fix: Don't create canonical lines if cursor ends on EOL after resize

Previously if a 20 character line were split into two 10 character
lines, the cursor would be placed on the line after the two lines.
New characters would then be treated as a new canonical line. This
commit fixes this by biasing cursors to the end of the previous line.

* fix: for cursor index calculation in lines that are already wrapped

* chore: test for real/saved cursor position being handled separately

* chore: Apply cargo format

* chore(repo): update issue templates

* Bump rust version to 1.75.0 (#3039)

* rust-toolchain: Bump toolchain version to 1.69.0

which, compared to the previous 1.67.0, has the following impacts on
`zellij`:

- [Turn off debuginfo for build deps][2]: Increases build time (on my
  machine) from ~230 s in 1.67.0 to ~250 s now, *which is unexpected*

This version also changes [handling of the `default-features` flag][3]
when specifying dependencies in `Cargo.toml`. If a dependent crate
requires `default-features = true` on a crate that is required as
`default-features = false` further up the dependency tree, the `true`
setting "wins". We only specify `default-features = false` for three
crates total:

- `names`: This is used only by us
- `surf`: This is used only by us
- `vte`: This is also required by `strip-ansi-escapes`, but that has
  `default-features = false` as well

How this affects our transitive dependencies is unknown at this point.

[2]: rust-lang/cargo#11252
[3]: rust-lang/cargo#11409

* rust-toolchain: Bump toolchain version to 1.70.0

which, compared to the previous 1.69.0, as the following impacts on
`zellij`:

1. [Enable sparse registry checkout for crates.io by default][1]

This drastically increases the time to first build on a fresh rust
installation/a rust installation with a clean cargo registry cache.
Previously it took about 75s to populate the deps/cache (with `cargo
fetch --locked` and ~100 MBit/s network), whereas now the same process
takes ~10 s.

2. [The `OnceCell` type is now part of std][2]

In theory, this would allow us to cut a dependency from `zellij-utils`,
but the `once_cell` crate is pulled in by another 16 deps, so there's no
point in attempting it right now.

Build times and binary sizes are unaffected by this change compared to
the previous 1.69.0 toolchain.

[1]: rust-lang/cargo#11791
[2]: https://doc.rust-lang.org/stable/std/cell/struct.OnceCell.html

* rust-toolchain: Bump toolchain version to 1.75.0

which, compared to the previous 1.70.0, has the following impacts on
`zellij`:

1. [cross-crate inlining][8]

This should increase application performance, as functions can now be
inlined across crates.

2. [`async fn` in traits][9]

This would allow us to drop the `async_trait` dependency, but it is
currently still required by 3 other dependencies.

Build time in debug mode (on my own PC) is cut down from 256s to 189s
(for a clean build). Build time in release mode is cut down from 473s to
391s (for a clean build). Binary sizes only change minimally (825 MB ->
807 MB in debug, 29 MB -> 30 MB in release).

[8]: rust-lang/rust#116505
[9]: rust-lang/rust#115822

* chore: Apply rustfmt.

* CHANGELOG: Add PR #3039.

* feat(plugins): introduce 'pipes', allowing users to pipe data to and control plugins from the command line (#3066)

* prototype - working with message from the cli

* prototype - pipe from the CLI to plugins

* prototype - pipe from the CLI to plugins and back again

* prototype - working with better cli interface

* prototype - working after removing unused stuff

* prototype - working with launching plugin if it is not launched, also fixed event ordering

* refactor: change message to cli-message

* prototype - allow plugins to send messages to each other

* fix: allow cli messages to send plugin parameters (and implement backpressure)

* fix: use input_pipe_id to identify cli pipes instead of their message name

* fix: come cleanups and add skip_cache parameter

* fix: pipe/client-server communication robustness

* fix: leaking messages between plugins while loading

* feat: allow plugins to specify how a new plugin instance is launched when sending messages

* fix: add permissions

* refactor: adjust cli api

* fix: improve cli plugin loading error messages

* docs: cli pipe

* fix: take plugin configuration into account when messaging between plugins

* refactor: pipe message protobuf interface

* refactor: update(event) -> pipe

* refactor - rename CliMessage to CliPipe

* fix: add is_private to pipes and change some naming

* refactor - cli client

* refactor: various cleanups

* style(fmt): rustfmt

* fix(pipes): backpressure across multiple plugins

* style: some cleanups

* style(fmt): rustfmt

* style: fix merge conflict mistake

* style(wording): clarify pipe permission

* docs(changelog): introduce pipes

* fix: add some robustness and future proofing

* fix e2e tests

---------

Co-authored-by: Aram Drevekenin <aram@poor.dev>
Co-authored-by: har7an <99636919+har7an@users.noreply.github.com>
  • Loading branch information
3 people authored Jan 22, 2024
1 parent 592cabe commit 96b70e6
Show file tree
Hide file tree
Showing 7 changed files with 248 additions and 44 deletions.
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
---
source: src/tests/e2e/cases.rs
assertion_line: 1194
assertion_line: 1326
expression: last_snapshot
---
Zellij (e2e-test)  Tab #1
$$
$
Expand Down
126 changes: 85 additions & 41 deletions zellij-server/src/panes/grid.rs
Original file line number Diff line number Diff line change
Expand Up @@ -623,8 +623,8 @@ impl Grid {
cursor_canonical_line_index = i;
}
if i == self.cursor.y {
let line_wrap_position_in_line = self.cursor.y - cursor_canonical_line_index;
cursor_index_in_canonical_line = line_wrap_position_in_line + self.cursor.x;
let line_wraps = self.cursor.y.saturating_sub(cursor_canonical_line_index);
cursor_index_in_canonical_line = (line_wraps * self.width) + self.cursor.x;
break;
}
}
Expand All @@ -639,10 +639,9 @@ impl Grid {
cursor_canonical_line_index = i;
}
if i == saved_cursor_position.y {
let line_wrap_position_in_line =
saved_cursor_position.y - cursor_canonical_line_index;
let line_wraps = saved_cursor_position.y - cursor_canonical_line_index;
cursor_index_in_canonical_line =
line_wrap_position_in_line + saved_cursor_position.x;
(line_wraps * self.width) + saved_cursor_position.x;
break;
}
}
Expand Down Expand Up @@ -849,26 +848,44 @@ impl Grid {

self.viewport = new_viewport_rows;

let mut new_cursor_y = self.canonical_line_y_coordinates(cursor_canonical_line_index);
let mut new_cursor_y = self.canonical_line_y_coordinates(cursor_canonical_line_index)
+ (cursor_index_in_canonical_line / new_columns);
let mut saved_cursor_y_coordinates =
if let Some(saved_cursor) = self.saved_cursor_position.as_ref() {
Some(self.canonical_line_y_coordinates(saved_cursor.y))
} else {
None
};

let new_cursor_x = (cursor_index_in_canonical_line / new_columns)
+ (cursor_index_in_canonical_line % new_columns);
let saved_cursor_x_coordinates = if let Some(saved_cursor_index_in_canonical_line) =
saved_cursor_index_in_canonical_line.as_ref()
{
Some(
(*saved_cursor_index_in_canonical_line / new_columns)
+ (*saved_cursor_index_in_canonical_line % new_columns),
)
} else {
None
self.saved_cursor_position.as_ref().map(|saved_cursor| {
self.canonical_line_y_coordinates(saved_cursor.y)
+ saved_cursor_index_in_canonical_line.as_ref().unwrap() / new_columns
});

// A cursor at EOL has two equivalent positions - end of this line or beginning of
// next. If not already at the beginning of line, bias to EOL so add character logic
// doesn't create spurious canonical lines
let mut new_cursor_x = cursor_index_in_canonical_line % new_columns;
if self.cursor.x != 0 && new_cursor_x == 0 {
new_cursor_y = new_cursor_y.saturating_sub(1);
new_cursor_x = new_columns
}
let saved_cursor_x_coordinates = match (
saved_cursor_index_in_canonical_line.as_ref(),
self.saved_cursor_position.as_mut(),
saved_cursor_y_coordinates.as_mut(),
) {
(
Some(saved_cursor_index_in_canonical_line),
Some(saved_cursor_position),
Some(saved_cursor_y_coordinates),
) => {
let x = saved_cursor_position.x;
let mut new_x = *saved_cursor_index_in_canonical_line % new_columns;
let new_y = saved_cursor_y_coordinates;
if x != 0 && new_x == 0 {
*new_y = new_y.saturating_sub(1);
new_x = new_columns
}
Some(new_x)
},
_ => None,
};

let current_viewport_row_count = self.viewport.len();
match current_viewport_row_count.cmp(&self.height) {
Ordering::Less => {
Expand Down Expand Up @@ -919,14 +936,27 @@ impl Grid {
saved_cursor_position.x = saved_cursor_x_coordinates;
saved_cursor_position.y = saved_cursor_y_coordinates;
},
_ => {
saved_cursor_position.x = new_cursor_x;
saved_cursor_position.y = new_cursor_y;
},
_ => log::error!(
"invalid state - cannot set saved cursor to {:?} {:?}",
saved_cursor_x_coordinates,
saved_cursor_y_coordinates
),
}
};
}
if new_rows != self.height {
let mut new_cursor_y = self.cursor.y;
let mut saved_cursor_y_coordinates = self
.saved_cursor_position
.as_ref()
.map(|saved_cursor| saved_cursor.y);

let new_cursor_x = self.cursor.x;
let saved_cursor_x_coordinates = self
.saved_cursor_position
.as_ref()
.map(|saved_cursor| saved_cursor.x);

let current_viewport_row_count = self.viewport.len();
match current_viewport_row_count.cmp(&new_rows) {
Ordering::Less => {
Expand All @@ -939,25 +969,24 @@ impl Grid {
new_columns,
);
let rows_pulled = self.viewport.len() - current_viewport_row_count;
self.cursor.y += rows_pulled;
if let Some(saved_cursor_position) = self.saved_cursor_position.as_mut() {
saved_cursor_position.y += rows_pulled
new_cursor_y += rows_pulled;
if let Some(saved_cursor_y_coordinates) = saved_cursor_y_coordinates.as_mut() {
*saved_cursor_y_coordinates += rows_pulled;
};
},
Ordering::Greater => {
let row_count_to_transfer = current_viewport_row_count - new_rows;
if row_count_to_transfer > self.cursor.y {
self.cursor.y = 0;
if let Some(saved_cursor_position) = self.saved_cursor_position.as_mut() {
saved_cursor_position.y = 0
};
if row_count_to_transfer > new_cursor_y {
new_cursor_y = 0;
} else {
self.cursor.y -= row_count_to_transfer;
if let Some(saved_cursor_position) = self.saved_cursor_position.as_mut() {
saved_cursor_position.y = saved_cursor_position
.y
.saturating_sub(row_count_to_transfer);
};
new_cursor_y -= row_count_to_transfer;
}
if let Some(saved_cursor_y_coordinates) = saved_cursor_y_coordinates.as_mut() {
if row_count_to_transfer > *saved_cursor_y_coordinates {
*saved_cursor_y_coordinates = 0;
} else {
*saved_cursor_y_coordinates -= row_count_to_transfer;
}
}
transfer_rows_from_viewport_to_lines_above(
&mut self.viewport,
Expand All @@ -969,6 +998,21 @@ impl Grid {
},
Ordering::Equal => {},
}
self.cursor.y = new_cursor_y;
self.cursor.x = new_cursor_x;
if let Some(saved_cursor_position) = self.saved_cursor_position.as_mut() {
match (saved_cursor_x_coordinates, saved_cursor_y_coordinates) {
(Some(saved_cursor_x_coordinates), Some(saved_cursor_y_coordinates)) => {
saved_cursor_position.x = saved_cursor_x_coordinates;
saved_cursor_position.y = saved_cursor_y_coordinates;
},
_ => log::error!(
"invalid state - cannot set saved cursor to {:?} {:?}",
saved_cursor_x_coordinates,
saved_cursor_y_coordinates
),
}
};
}
self.height = new_rows;
self.width = new_columns;
Expand Down
121 changes: 121 additions & 0 deletions zellij-server/src/panes/unit/grid_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2400,6 +2400,127 @@ pub fn scroll_up_increase_width_and_scroll_down() {
assert_snapshot!(format!("{:?}", grid));
}

#[test]
fn saved_cursor_across_resize() {
let mut vte_parser = vte::Parser::new();
let sixel_image_store = Rc::new(RefCell::new(SixelImageStore::default()));
let terminal_emulator_color_codes = Rc::new(RefCell::new(HashMap::new()));
let debug = false;
let arrow_fonts = true;
let styled_underlines = true;
let mut grid = Grid::new(
4,
20,
Rc::new(RefCell::new(Palette::default())),
terminal_emulator_color_codes,
Rc::new(RefCell::new(LinkHandler::new())),
Rc::new(RefCell::new(None)),
sixel_image_store,
Style::default(),
debug,
arrow_fonts,
styled_underlines,
);
let mut parse = |s, grid: &mut Grid| {
for b in Vec::from(s) {
vte_parser.advance(&mut *grid, b)
}
};
let content = "
\rLine 1 >fill to 20_<
\rLine 2 >fill to 20_<
\rLine 3 >fill to 20_<
\rL\u{1b}[sine 4 >fill to 20_<";
parse(content, &mut grid);
// Move real cursor position up three lines
let content = "\u{1b}[3A";
parse(content, &mut grid);
// Truncate top of terminal, resetting cursor (but not saved cursor)
grid.change_size(3, 20);
// Wrap, resetting cursor again (but not saved cursor)
grid.change_size(3, 10);
// Restore saved cursor position and write ZZZ
let content = "\u{1b}[uZZZ";
parse(content, &mut grid);
assert_snapshot!(format!("{:?}", grid));
}

#[test]
fn saved_cursor_across_resize_longline() {
let mut vte_parser = vte::Parser::new();
let sixel_image_store = Rc::new(RefCell::new(SixelImageStore::default()));
let terminal_emulator_color_codes = Rc::new(RefCell::new(HashMap::new()));
let debug = false;
let arrow_fonts = true;
let styled_underlines = true;
let mut grid = Grid::new(
4,
20,
Rc::new(RefCell::new(Palette::default())),
terminal_emulator_color_codes,
Rc::new(RefCell::new(LinkHandler::new())),
Rc::new(RefCell::new(None)),
sixel_image_store,
Style::default(),
debug,
arrow_fonts,
styled_underlines,
);
let mut parse = |s, grid: &mut Grid| {
for b in Vec::from(s) {
vte_parser.advance(&mut *grid, b)
}
};
let content = "
\rLine 1 >fill \u{1b}[sto 20_<";
parse(content, &mut grid);
// Wrap each line precisely halfway
grid.change_size(4, 10);
// Write 'YY' at the end (ends up on a new wrapped line), restore to the saved cursor
// and overwrite 'to' with 'ZZ'
let content = "YY\u{1b}[uZZ";
parse(content, &mut grid);
assert_snapshot!(format!("{:?}", grid));
}

#[test]
fn saved_cursor_across_resize_rewrap() {
let mut vte_parser = vte::Parser::new();
let sixel_image_store = Rc::new(RefCell::new(SixelImageStore::default()));
let terminal_emulator_color_codes = Rc::new(RefCell::new(HashMap::new()));
let debug = false;
let arrow_fonts = true;
let styled_underlines = true;
let mut grid = Grid::new(
4,
4 * 8,
Rc::new(RefCell::new(Palette::default())),
terminal_emulator_color_codes,
Rc::new(RefCell::new(LinkHandler::new())),
Rc::new(RefCell::new(None)),
sixel_image_store,
Style::default(),
debug,
arrow_fonts,
styled_underlines,
);
let mut parse = |s, grid: &mut Grid| {
for b in Vec::from(s) {
vte_parser.advance(&mut *grid, b)
}
};
let content = "
\r12345678123456781234567\u{1b}[s812345678"; // 4*8 chars
parse(content, &mut grid);
// Wrap each line precisely halfway, then rewrap to halve them again
grid.change_size(4, 16);
grid.change_size(4, 8);
// Write 'Z' at the end of line 3
let content = "\u{1b}[uZ";
parse(content, &mut grid);
assert_snapshot!(format!("{:?}", grid));
}

#[test]
pub fn move_cursor_below_scroll_region() {
let mut vte_parser = vte::Parser::new();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
source: zellij-server/src/panes/./unit/grid_tests.rs
assertion_line: 2443
expression: "format!(\"{:?}\", grid)"
---
00 (W): ll to 20_<
01 (C): LZZZ 4 >fi
02 (W): ll to 20_<

Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
source: zellij-server/src/panes/./unit/grid_tests.rs
assertion_line: 2475
expression: "format!(\"{:?}\", grid)"
---
00 (C):
01 (C): Line 1 >fi
02 (W): ll ZZ 20_<
03 (W): YY

Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
source: zellij-server/src/panes/./unit/grid_tests.rs
assertion_line: 2512
expression: "format!(\"{:?}\", grid)"
---
00 (C): 12345678
01 (W): 12345678
02 (W): 1234567Z
03 (W): 12345678

12 changes: 11 additions & 1 deletion zellij-server/src/tab/unit/tab_integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2039,10 +2039,20 @@ fn save_cursor_position_across_resizes() {
1,
Vec::from("\n\n\rI am some text\n\rI am another line of text\n\rLet's save the cursor position here \u{1b}[sI should be ovewritten".as_bytes()),
).unwrap();
tab.resize_whole_tab(Size { cols: 100, rows: 3 }).unwrap();

// We check cursor and saved cursor are handled separately by:
// 1. moving real cursor up two lines
tab.handle_pty_bytes(1, Vec::from("\u{1b}[2A".as_bytes()));
// 2. resizing so real cursor gets lost above the viewport, which resets it to row 0
// The saved cursor ends up on row 1, allowing detection if it (incorrectly) gets reset too
tab.resize_whole_tab(Size { cols: 35, rows: 4 }).unwrap();

// Now overwrite
tab.handle_pty_bytes(1, Vec::from("\u{1b}[uthis overwrote me!".as_bytes()))
.unwrap();

tab.resize_whole_tab(Size { cols: 100, rows: 3 }).unwrap();

tab.render(&mut output).unwrap();
let snapshot = take_snapshot(
output.serialize().unwrap().get(&client_id).unwrap(),
Expand Down

0 comments on commit 96b70e6

Please sign in to comment.