Skip to content

Commit 83d0279

Browse files
aidanhsimsnifhar7an
authored andcommitted
fix(terminal): some real/saved cursor bugs during resize (zellij-org#3032)
* 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 zellij-org#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 (zellij-org#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 zellij-org#3039. * feat(plugins): introduce 'pipes', allowing users to pipe data to and control plugins from the command line (zellij-org#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 <[email protected]> Co-authored-by: har7an <[email protected]>
1 parent 69089dd commit 83d0279

7 files changed

+248
-44
lines changed

src/tests/e2e/snapshots/zellij__tests__e2e__cases__start_without_pane_frames.snap

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
---
22
source: src/tests/e2e/cases.rs
3-
assertion_line: 1194
3+
assertion_line: 1326
44
expression: last_snapshot
55
---
66
Zellij (e2e-test)  Tab #1
77
$$
8-
8+
$
99
1010
1111

zellij-server/src/panes/grid.rs

+85-41
Original file line numberDiff line numberDiff line change
@@ -623,8 +623,8 @@ impl Grid {
623623
cursor_canonical_line_index = i;
624624
}
625625
if i == self.cursor.y {
626-
let line_wrap_position_in_line = self.cursor.y - cursor_canonical_line_index;
627-
cursor_index_in_canonical_line = line_wrap_position_in_line + self.cursor.x;
626+
let line_wraps = self.cursor.y.saturating_sub(cursor_canonical_line_index);
627+
cursor_index_in_canonical_line = (line_wraps * self.width) + self.cursor.x;
628628
break;
629629
}
630630
}
@@ -639,10 +639,9 @@ impl Grid {
639639
cursor_canonical_line_index = i;
640640
}
641641
if i == saved_cursor_position.y {
642-
let line_wrap_position_in_line =
643-
saved_cursor_position.y - cursor_canonical_line_index;
642+
let line_wraps = saved_cursor_position.y - cursor_canonical_line_index;
644643
cursor_index_in_canonical_line =
645-
line_wrap_position_in_line + saved_cursor_position.x;
644+
(line_wraps * self.width) + saved_cursor_position.x;
646645
break;
647646
}
648647
}
@@ -849,26 +848,44 @@ impl Grid {
849848

850849
self.viewport = new_viewport_rows;
851850

852-
let mut new_cursor_y = self.canonical_line_y_coordinates(cursor_canonical_line_index);
851+
let mut new_cursor_y = self.canonical_line_y_coordinates(cursor_canonical_line_index)
852+
+ (cursor_index_in_canonical_line / new_columns);
853853
let mut saved_cursor_y_coordinates =
854-
if let Some(saved_cursor) = self.saved_cursor_position.as_ref() {
855-
Some(self.canonical_line_y_coordinates(saved_cursor.y))
856-
} else {
857-
None
858-
};
859-
860-
let new_cursor_x = (cursor_index_in_canonical_line / new_columns)
861-
+ (cursor_index_in_canonical_line % new_columns);
862-
let saved_cursor_x_coordinates = if let Some(saved_cursor_index_in_canonical_line) =
863-
saved_cursor_index_in_canonical_line.as_ref()
864-
{
865-
Some(
866-
(*saved_cursor_index_in_canonical_line / new_columns)
867-
+ (*saved_cursor_index_in_canonical_line % new_columns),
868-
)
869-
} else {
870-
None
854+
self.saved_cursor_position.as_ref().map(|saved_cursor| {
855+
self.canonical_line_y_coordinates(saved_cursor.y)
856+
+ saved_cursor_index_in_canonical_line.as_ref().unwrap() / new_columns
857+
});
858+
859+
// A cursor at EOL has two equivalent positions - end of this line or beginning of
860+
// next. If not already at the beginning of line, bias to EOL so add character logic
861+
// doesn't create spurious canonical lines
862+
let mut new_cursor_x = cursor_index_in_canonical_line % new_columns;
863+
if self.cursor.x != 0 && new_cursor_x == 0 {
864+
new_cursor_y = new_cursor_y.saturating_sub(1);
865+
new_cursor_x = new_columns
866+
}
867+
let saved_cursor_x_coordinates = match (
868+
saved_cursor_index_in_canonical_line.as_ref(),
869+
self.saved_cursor_position.as_mut(),
870+
saved_cursor_y_coordinates.as_mut(),
871+
) {
872+
(
873+
Some(saved_cursor_index_in_canonical_line),
874+
Some(saved_cursor_position),
875+
Some(saved_cursor_y_coordinates),
876+
) => {
877+
let x = saved_cursor_position.x;
878+
let mut new_x = *saved_cursor_index_in_canonical_line % new_columns;
879+
let new_y = saved_cursor_y_coordinates;
880+
if x != 0 && new_x == 0 {
881+
*new_y = new_y.saturating_sub(1);
882+
new_x = new_columns
883+
}
884+
Some(new_x)
885+
},
886+
_ => None,
871887
};
888+
872889
let current_viewport_row_count = self.viewport.len();
873890
match current_viewport_row_count.cmp(&self.height) {
874891
Ordering::Less => {
@@ -919,14 +936,27 @@ impl Grid {
919936
saved_cursor_position.x = saved_cursor_x_coordinates;
920937
saved_cursor_position.y = saved_cursor_y_coordinates;
921938
},
922-
_ => {
923-
saved_cursor_position.x = new_cursor_x;
924-
saved_cursor_position.y = new_cursor_y;
925-
},
939+
_ => log::error!(
940+
"invalid state - cannot set saved cursor to {:?} {:?}",
941+
saved_cursor_x_coordinates,
942+
saved_cursor_y_coordinates
943+
),
926944
}
927945
};
928946
}
929947
if new_rows != self.height {
948+
let mut new_cursor_y = self.cursor.y;
949+
let mut saved_cursor_y_coordinates = self
950+
.saved_cursor_position
951+
.as_ref()
952+
.map(|saved_cursor| saved_cursor.y);
953+
954+
let new_cursor_x = self.cursor.x;
955+
let saved_cursor_x_coordinates = self
956+
.saved_cursor_position
957+
.as_ref()
958+
.map(|saved_cursor| saved_cursor.x);
959+
930960
let current_viewport_row_count = self.viewport.len();
931961
match current_viewport_row_count.cmp(&new_rows) {
932962
Ordering::Less => {
@@ -939,25 +969,24 @@ impl Grid {
939969
new_columns,
940970
);
941971
let rows_pulled = self.viewport.len() - current_viewport_row_count;
942-
self.cursor.y += rows_pulled;
943-
if let Some(saved_cursor_position) = self.saved_cursor_position.as_mut() {
944-
saved_cursor_position.y += rows_pulled
972+
new_cursor_y += rows_pulled;
973+
if let Some(saved_cursor_y_coordinates) = saved_cursor_y_coordinates.as_mut() {
974+
*saved_cursor_y_coordinates += rows_pulled;
945975
};
946976
},
947977
Ordering::Greater => {
948978
let row_count_to_transfer = current_viewport_row_count - new_rows;
949-
if row_count_to_transfer > self.cursor.y {
950-
self.cursor.y = 0;
951-
if let Some(saved_cursor_position) = self.saved_cursor_position.as_mut() {
952-
saved_cursor_position.y = 0
953-
};
979+
if row_count_to_transfer > new_cursor_y {
980+
new_cursor_y = 0;
954981
} else {
955-
self.cursor.y -= row_count_to_transfer;
956-
if let Some(saved_cursor_position) = self.saved_cursor_position.as_mut() {
957-
saved_cursor_position.y = saved_cursor_position
958-
.y
959-
.saturating_sub(row_count_to_transfer);
960-
};
982+
new_cursor_y -= row_count_to_transfer;
983+
}
984+
if let Some(saved_cursor_y_coordinates) = saved_cursor_y_coordinates.as_mut() {
985+
if row_count_to_transfer > *saved_cursor_y_coordinates {
986+
*saved_cursor_y_coordinates = 0;
987+
} else {
988+
*saved_cursor_y_coordinates -= row_count_to_transfer;
989+
}
961990
}
962991
transfer_rows_from_viewport_to_lines_above(
963992
&mut self.viewport,
@@ -969,6 +998,21 @@ impl Grid {
969998
},
970999
Ordering::Equal => {},
9711000
}
1001+
self.cursor.y = new_cursor_y;
1002+
self.cursor.x = new_cursor_x;
1003+
if let Some(saved_cursor_position) = self.saved_cursor_position.as_mut() {
1004+
match (saved_cursor_x_coordinates, saved_cursor_y_coordinates) {
1005+
(Some(saved_cursor_x_coordinates), Some(saved_cursor_y_coordinates)) => {
1006+
saved_cursor_position.x = saved_cursor_x_coordinates;
1007+
saved_cursor_position.y = saved_cursor_y_coordinates;
1008+
},
1009+
_ => log::error!(
1010+
"invalid state - cannot set saved cursor to {:?} {:?}",
1011+
saved_cursor_x_coordinates,
1012+
saved_cursor_y_coordinates
1013+
),
1014+
}
1015+
};
9721016
}
9731017
self.height = new_rows;
9741018
self.width = new_columns;

zellij-server/src/panes/unit/grid_tests.rs

+121
Original file line numberDiff line numberDiff line change
@@ -2400,6 +2400,127 @@ pub fn scroll_up_increase_width_and_scroll_down() {
24002400
assert_snapshot!(format!("{:?}", grid));
24012401
}
24022402

2403+
#[test]
2404+
fn saved_cursor_across_resize() {
2405+
let mut vte_parser = vte::Parser::new();
2406+
let sixel_image_store = Rc::new(RefCell::new(SixelImageStore::default()));
2407+
let terminal_emulator_color_codes = Rc::new(RefCell::new(HashMap::new()));
2408+
let debug = false;
2409+
let arrow_fonts = true;
2410+
let styled_underlines = true;
2411+
let mut grid = Grid::new(
2412+
4,
2413+
20,
2414+
Rc::new(RefCell::new(Palette::default())),
2415+
terminal_emulator_color_codes,
2416+
Rc::new(RefCell::new(LinkHandler::new())),
2417+
Rc::new(RefCell::new(None)),
2418+
sixel_image_store,
2419+
Style::default(),
2420+
debug,
2421+
arrow_fonts,
2422+
styled_underlines,
2423+
);
2424+
let mut parse = |s, grid: &mut Grid| {
2425+
for b in Vec::from(s) {
2426+
vte_parser.advance(&mut *grid, b)
2427+
}
2428+
};
2429+
let content = "
2430+
\rLine 1 >fill to 20_<
2431+
\rLine 2 >fill to 20_<
2432+
\rLine 3 >fill to 20_<
2433+
\rL\u{1b}[sine 4 >fill to 20_<";
2434+
parse(content, &mut grid);
2435+
// Move real cursor position up three lines
2436+
let content = "\u{1b}[3A";
2437+
parse(content, &mut grid);
2438+
// Truncate top of terminal, resetting cursor (but not saved cursor)
2439+
grid.change_size(3, 20);
2440+
// Wrap, resetting cursor again (but not saved cursor)
2441+
grid.change_size(3, 10);
2442+
// Restore saved cursor position and write ZZZ
2443+
let content = "\u{1b}[uZZZ";
2444+
parse(content, &mut grid);
2445+
assert_snapshot!(format!("{:?}", grid));
2446+
}
2447+
2448+
#[test]
2449+
fn saved_cursor_across_resize_longline() {
2450+
let mut vte_parser = vte::Parser::new();
2451+
let sixel_image_store = Rc::new(RefCell::new(SixelImageStore::default()));
2452+
let terminal_emulator_color_codes = Rc::new(RefCell::new(HashMap::new()));
2453+
let debug = false;
2454+
let arrow_fonts = true;
2455+
let styled_underlines = true;
2456+
let mut grid = Grid::new(
2457+
4,
2458+
20,
2459+
Rc::new(RefCell::new(Palette::default())),
2460+
terminal_emulator_color_codes,
2461+
Rc::new(RefCell::new(LinkHandler::new())),
2462+
Rc::new(RefCell::new(None)),
2463+
sixel_image_store,
2464+
Style::default(),
2465+
debug,
2466+
arrow_fonts,
2467+
styled_underlines,
2468+
);
2469+
let mut parse = |s, grid: &mut Grid| {
2470+
for b in Vec::from(s) {
2471+
vte_parser.advance(&mut *grid, b)
2472+
}
2473+
};
2474+
let content = "
2475+
\rLine 1 >fill \u{1b}[sto 20_<";
2476+
parse(content, &mut grid);
2477+
// Wrap each line precisely halfway
2478+
grid.change_size(4, 10);
2479+
// Write 'YY' at the end (ends up on a new wrapped line), restore to the saved cursor
2480+
// and overwrite 'to' with 'ZZ'
2481+
let content = "YY\u{1b}[uZZ";
2482+
parse(content, &mut grid);
2483+
assert_snapshot!(format!("{:?}", grid));
2484+
}
2485+
2486+
#[test]
2487+
fn saved_cursor_across_resize_rewrap() {
2488+
let mut vte_parser = vte::Parser::new();
2489+
let sixel_image_store = Rc::new(RefCell::new(SixelImageStore::default()));
2490+
let terminal_emulator_color_codes = Rc::new(RefCell::new(HashMap::new()));
2491+
let debug = false;
2492+
let arrow_fonts = true;
2493+
let styled_underlines = true;
2494+
let mut grid = Grid::new(
2495+
4,
2496+
4 * 8,
2497+
Rc::new(RefCell::new(Palette::default())),
2498+
terminal_emulator_color_codes,
2499+
Rc::new(RefCell::new(LinkHandler::new())),
2500+
Rc::new(RefCell::new(None)),
2501+
sixel_image_store,
2502+
Style::default(),
2503+
debug,
2504+
arrow_fonts,
2505+
styled_underlines,
2506+
);
2507+
let mut parse = |s, grid: &mut Grid| {
2508+
for b in Vec::from(s) {
2509+
vte_parser.advance(&mut *grid, b)
2510+
}
2511+
};
2512+
let content = "
2513+
\r12345678123456781234567\u{1b}[s812345678"; // 4*8 chars
2514+
parse(content, &mut grid);
2515+
// Wrap each line precisely halfway, then rewrap to halve them again
2516+
grid.change_size(4, 16);
2517+
grid.change_size(4, 8);
2518+
// Write 'Z' at the end of line 3
2519+
let content = "\u{1b}[uZ";
2520+
parse(content, &mut grid);
2521+
assert_snapshot!(format!("{:?}", grid));
2522+
}
2523+
24032524
#[test]
24042525
pub fn move_cursor_below_scroll_region() {
24052526
let mut vte_parser = vte::Parser::new();
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
source: zellij-server/src/panes/./unit/grid_tests.rs
3+
assertion_line: 2443
4+
expression: "format!(\"{:?}\", grid)"
5+
---
6+
00 (W): ll to 20_<
7+
01 (C): LZZZ 4 >fi
8+
02 (W): ll to 20_<
9+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
source: zellij-server/src/panes/./unit/grid_tests.rs
3+
assertion_line: 2475
4+
expression: "format!(\"{:?}\", grid)"
5+
---
6+
00 (C):
7+
01 (C): Line 1 >fi
8+
02 (W): ll ZZ 20_<
9+
03 (W): YY
10+
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
source: zellij-server/src/panes/./unit/grid_tests.rs
3+
assertion_line: 2512
4+
expression: "format!(\"{:?}\", grid)"
5+
---
6+
00 (C): 12345678
7+
01 (W): 12345678
8+
02 (W): 1234567Z
9+
03 (W): 12345678
10+

zellij-server/src/tab/unit/tab_integration_tests.rs

+11-1
Original file line numberDiff line numberDiff line change
@@ -2039,10 +2039,20 @@ fn save_cursor_position_across_resizes() {
20392039
1,
20402040
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()),
20412041
).unwrap();
2042-
tab.resize_whole_tab(Size { cols: 100, rows: 3 }).unwrap();
2042+
2043+
// We check cursor and saved cursor are handled separately by:
2044+
// 1. moving real cursor up two lines
2045+
tab.handle_pty_bytes(1, Vec::from("\u{1b}[2A".as_bytes()));
2046+
// 2. resizing so real cursor gets lost above the viewport, which resets it to row 0
2047+
// The saved cursor ends up on row 1, allowing detection if it (incorrectly) gets reset too
2048+
tab.resize_whole_tab(Size { cols: 35, rows: 4 }).unwrap();
2049+
2050+
// Now overwrite
20432051
tab.handle_pty_bytes(1, Vec::from("\u{1b}[uthis overwrote me!".as_bytes()))
20442052
.unwrap();
20452053

2054+
tab.resize_whole_tab(Size { cols: 100, rows: 3 }).unwrap();
2055+
20462056
tab.render(&mut output).unwrap();
20472057
let snapshot = take_snapshot(
20482058
output.serialize().unwrap().get(&client_id).unwrap(),

0 commit comments

Comments
 (0)