fix(tui): rebase v_offset before VHI invalidation to prevent viewport jump (closes #25)
Ready state can have VHI=None during async rebuild (Tab toggle, resize, file append, Loading→Ready transition). Without rebase, v_offset retains a visual-row value that gets treated as a logical-line offset, causing viewport jumps and cursor drift. Changes: - Add rebase_offset_for_invalidate() to convert v_offset from visual-row to logical-line + sub-row before VHI invalidation - Call it at all 6 invalidation sites in Ready state - Recalibrate v_offset from viewport top when VHI rebuild completes - Clamp preserved sub_row to new line height after JSON/width changes Regression tests: 6 new tests covering rebase conversion, Tab toggle, VHI rebuild recalibration, sub-row clamping, and Loading→Ready transition.
This commit is contained in:
@@ -659,12 +659,15 @@ impl App {
|
||||
self.json_format = !self.json_format;
|
||||
self.viewport_cache.invalidate();
|
||||
let width = self.viewport_cache.width;
|
||||
let (new_offset, new_sub) = self.rebase_offset_for_invalidate();
|
||||
if let AppLoadingState::Ready { reader } = &mut self.loading_state {
|
||||
reader.invalidate_visual_height_index();
|
||||
if width > 0 {
|
||||
reader.start_visual_height_rebuild(width, self.json_format);
|
||||
}
|
||||
}
|
||||
self.v_offset = new_offset;
|
||||
self.v_sub_offset = new_sub;
|
||||
self.last_g_press = None;
|
||||
}
|
||||
KeyCode::Char('s') | KeyCode::Char('S')
|
||||
@@ -823,6 +826,25 @@ impl App {
|
||||
}
|
||||
}
|
||||
|
||||
/// Compute the rebased offset pair (logical_line, sub_row) from the
|
||||
/// current visual-row `v_offset`. Returns `(v_offset, v_sub_offset)`
|
||||
/// suitable for the no-VHI fallback scrolling path.
|
||||
///
|
||||
/// MUST be called before borrowing `&mut self.loading_state` for
|
||||
/// `invalidate_visual_height_index`, because it reads VHI through
|
||||
/// `&self`.
|
||||
fn rebase_offset_for_invalidate(&self) -> (usize, usize) {
|
||||
if self.get_visual_height_index().is_some() {
|
||||
let top_visual = self.v_offset;
|
||||
let top_line = self.visual_row_to_logical_row(top_visual);
|
||||
let line_first_visual = self.cursor_to_first_visual_row(top_line);
|
||||
let sub = top_visual.saturating_sub(line_first_visual);
|
||||
(top_line, sub)
|
||||
} else {
|
||||
(self.v_offset, self.v_sub_offset)
|
||||
}
|
||||
}
|
||||
|
||||
fn ensure_visual_height_index(&mut self, width: usize) {
|
||||
let needs_rebuild = match self.get_visual_height_index() {
|
||||
Some(idx) => !idx.is_valid_for(self.json_format, width),
|
||||
@@ -830,10 +852,13 @@ impl App {
|
||||
};
|
||||
|
||||
if needs_rebuild {
|
||||
let (new_offset, new_sub) = self.rebase_offset_for_invalidate();
|
||||
if let AppLoadingState::Ready { reader } = &mut self.loading_state {
|
||||
reader.invalidate_visual_height_index();
|
||||
reader.start_visual_height_rebuild(width, self.json_format);
|
||||
}
|
||||
self.v_offset = new_offset;
|
||||
self.v_sub_offset = new_sub;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -916,6 +941,7 @@ impl App {
|
||||
|
||||
fn handle_file_appended(&mut self) {
|
||||
let width = self.get_content_width();
|
||||
let rebased = self.rebase_offset_for_invalidate();
|
||||
match &mut self.loading_state {
|
||||
AppLoadingState::Ready { reader } => {
|
||||
let old_reader_line_count = reader.line_count();
|
||||
@@ -966,6 +992,9 @@ impl App {
|
||||
index.extend_from_heights(&new_heights);
|
||||
}
|
||||
} else {
|
||||
let (new_offset, new_sub) = rebased;
|
||||
self.v_offset = new_offset;
|
||||
self.v_sub_offset = new_sub;
|
||||
reader.invalidate_visual_height_index();
|
||||
reader.start_visual_height_rebuild(width, self.json_format);
|
||||
}
|
||||
@@ -974,9 +1003,11 @@ impl App {
|
||||
}
|
||||
Ok(log_viewer_core::io::file_reader::AppendStatus::Reloaded) => {
|
||||
let _ = reader.save_cache();
|
||||
let (new_offset, _new_sub) = rebased;
|
||||
reader.invalidate_visual_height_index();
|
||||
reader.start_visual_height_rebuild(width, self.json_format);
|
||||
self.cursor_line = self.cursor_line.min(self.total_lines().saturating_sub(1));
|
||||
self.v_offset = new_offset;
|
||||
self.v_sub_offset = 0;
|
||||
self.viewport_cache.invalidate();
|
||||
self.clamp_v_offset();
|
||||
@@ -993,12 +1024,14 @@ impl App {
|
||||
|
||||
fn reload_ready_reader(&mut self) {
|
||||
let width = self.get_content_width();
|
||||
let (new_offset, _new_sub) = self.rebase_offset_for_invalidate();
|
||||
if let AppLoadingState::Ready { reader } = &mut self.loading_state {
|
||||
let _ = reader.reload();
|
||||
let _ = reader.save_cache();
|
||||
reader.invalidate_visual_height_index();
|
||||
reader.start_visual_height_rebuild(width, self.json_format);
|
||||
self.cursor_line = self.cursor_line.min(self.total_lines().saturating_sub(1));
|
||||
self.v_offset = new_offset;
|
||||
self.v_sub_offset = 0;
|
||||
self.viewport_cache.invalidate();
|
||||
self.clamp_v_offset();
|
||||
@@ -1030,10 +1063,23 @@ impl App {
|
||||
} = &mut reader.state
|
||||
{
|
||||
*visual_height_index = Some(index);
|
||||
self.viewport_cache.invalidate();
|
||||
}
|
||||
}
|
||||
}
|
||||
// Recalibrate v_offset from logical → visual-row now that VHI is available.
|
||||
// Must be outside the `reader` borrow above.
|
||||
if self.get_visual_height_index().is_some() {
|
||||
let logical_top = self.v_offset.min(self.total_lines().saturating_sub(1));
|
||||
let sub = self.v_sub_offset;
|
||||
let first_visual = self.cursor_to_first_visual_row(logical_top);
|
||||
let line_height = self
|
||||
.get_visual_height_index()
|
||||
.map_or(1, |idx| idx.visual_height_of_line(logical_top));
|
||||
self.v_offset = first_visual.saturating_add(sub.min(line_height.saturating_sub(1)));
|
||||
self.v_sub_offset = 0;
|
||||
self.clamp_v_offset();
|
||||
self.viewport_cache.invalidate();
|
||||
}
|
||||
|
||||
// Poll main indexer (Loading state)
|
||||
let old_state = std::mem::replace(&mut self.loading_state, AppLoadingState::Empty);
|
||||
@@ -1078,9 +1124,12 @@ impl App {
|
||||
|
||||
// Gutter width changes (~N → N) shift content_width, so any
|
||||
// VisualHeightIndex built with the old width is stale.
|
||||
let (new_offset, new_sub) = self.rebase_offset_for_invalidate();
|
||||
if let AppLoadingState::Ready { reader } = &mut self.loading_state {
|
||||
reader.invalidate_visual_height_index();
|
||||
}
|
||||
self.v_offset = new_offset;
|
||||
self.v_sub_offset = new_sub;
|
||||
|
||||
if self.reload_after_loading {
|
||||
self.reload_after_loading = false;
|
||||
@@ -3126,4 +3175,248 @@ plain text line
|
||||
});
|
||||
assert!(result.is_ok());
|
||||
}
|
||||
|
||||
// ── Issue #25 regression tests ──────────────────────────────────
|
||||
// Ready state without VHI must keep v_offset self-consistent.
|
||||
|
||||
#[test]
|
||||
fn test_rebase_offset_converts_visual_to_logical_with_sub() {
|
||||
let content = "line0\nline1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\n";
|
||||
let path = make_temp_file(content);
|
||||
let result = std::panic::catch_unwind(|| {
|
||||
let mut app = App::new();
|
||||
load_file_ready(&mut app, &path);
|
||||
app.content_height = 10;
|
||||
|
||||
// 10 lines × 3 visual rows each = 30 visual rows
|
||||
install_vhi(&mut app, &[3usize; 10]);
|
||||
|
||||
// v_offset=7 → visual row 7 is inside line2 (rows 6-8), sub=1
|
||||
app.v_offset = 7;
|
||||
app.v_sub_offset = 0;
|
||||
|
||||
let (logical, sub) = app.rebase_offset_for_invalidate();
|
||||
assert_eq!(logical, 2, "visual row 7 → logical line 2 (rows 6-8)");
|
||||
assert_eq!(sub, 1, "visual row 7 is sub-row 1 within line 2");
|
||||
|
||||
// v_offset=0 → line 0, sub=0
|
||||
app.v_offset = 0;
|
||||
let (logical, sub) = app.rebase_offset_for_invalidate();
|
||||
assert_eq!(logical, 0);
|
||||
assert_eq!(sub, 0);
|
||||
|
||||
// v_offset=5 → line1 (rows 3-5), sub=2
|
||||
app.v_offset = 5;
|
||||
let (logical, sub) = app.rebase_offset_for_invalidate();
|
||||
assert_eq!(logical, 1, "visual row 5 → logical line 1 (rows 3-5)");
|
||||
assert_eq!(sub, 2);
|
||||
|
||||
cleanup(&path);
|
||||
});
|
||||
assert!(result.is_ok());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_rebase_offset_passthrough_when_no_vhi() {
|
||||
let content = "line0\nline1\nline2\n";
|
||||
let path = make_temp_file(content);
|
||||
let result = std::panic::catch_unwind(|| {
|
||||
let mut app = App::new();
|
||||
load_file_ready(&mut app, &path);
|
||||
|
||||
// No VHI installed — rebase should return current values unchanged
|
||||
app.v_offset = 42;
|
||||
app.v_sub_offset = 3;
|
||||
let (logical, sub) = app.rebase_offset_for_invalidate();
|
||||
assert_eq!(logical, 42);
|
||||
assert_eq!(sub, 3);
|
||||
|
||||
cleanup(&path);
|
||||
});
|
||||
assert!(result.is_ok());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tab_toggle_rebases_offset_before_invalidate() {
|
||||
let content = "line0\nline1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\n";
|
||||
let path = make_temp_file(content);
|
||||
let result = std::panic::catch_unwind(|| {
|
||||
let mut app = App::new();
|
||||
load_file_ready(&mut app, &path);
|
||||
app.content_height = 10;
|
||||
app.content_width = 20;
|
||||
|
||||
install_vhi(&mut app, &[3usize; 10]);
|
||||
|
||||
// Scroll to visual row 7 → line2 sub-row 1
|
||||
app.v_offset = 7;
|
||||
app.cursor_line = 2;
|
||||
app.v_sub_offset = 0;
|
||||
|
||||
// Simulate Tab toggle: rebase, then invalidate
|
||||
let rebased = app.rebase_offset_for_invalidate();
|
||||
let (new_offset, new_sub) = rebased;
|
||||
app.v_offset = new_offset;
|
||||
app.v_sub_offset = new_sub;
|
||||
|
||||
assert_eq!(app.v_offset, 2, "v_offset should be logical line 2 after rebase");
|
||||
assert_eq!(app.v_sub_offset, 1, "v_sub_offset should preserve sub-row 1");
|
||||
|
||||
// Key invariant: v_offset is now a valid logical line number,
|
||||
// not a stale visual-row offset that would cause a jump.
|
||||
// Before the fix, v_offset would still be 7 (visual) treated as logical → jump.
|
||||
assert!(
|
||||
app.v_offset < app.total_lines(),
|
||||
"v_offset ({}) must be a valid logical line index < {}",
|
||||
app.v_offset, app.total_lines()
|
||||
);
|
||||
|
||||
cleanup(&path);
|
||||
});
|
||||
assert!(result.is_ok());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_vhi_rebuild_recalibrates_from_viewport_top() {
|
||||
let content = "line0\nline1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\n";
|
||||
let path = make_temp_file(content);
|
||||
let result = std::panic::catch_unwind(|| {
|
||||
let mut app = App::new();
|
||||
load_file_ready(&mut app, &path);
|
||||
app.content_height = 10;
|
||||
app.content_width = 20;
|
||||
|
||||
// Install VHI with varying heights
|
||||
install_vhi(&mut app, &[2, 4, 3, 1, 2, 5, 1, 3, 2, 1]);
|
||||
|
||||
// Scroll to visual row 8 → line2 (rows 6-8), sub=2
|
||||
app.v_offset = 8;
|
||||
app.cursor_line = 2;
|
||||
app.v_sub_offset = 0;
|
||||
|
||||
// Rebase (simulates invalidate)
|
||||
let (logical_top, sub) = app.rebase_offset_for_invalidate();
|
||||
assert_eq!(logical_top, 2);
|
||||
assert_eq!(sub, 2);
|
||||
|
||||
// Simulate what happens: v_offset becomes logical, VHI cleared
|
||||
app.v_offset = logical_top;
|
||||
app.v_sub_offset = sub;
|
||||
|
||||
// Now simulate VHI rebuild completion — the recalibration block
|
||||
// Install a fresh VHI (same heights, simulates rebuild result)
|
||||
install_vhi(&mut app, &[2, 4, 3, 1, 2, 5, 1, 3, 2, 1]);
|
||||
|
||||
// The recalibration logic from poll_background_indexer
|
||||
let logical_top = app.v_offset.min(app.total_lines().saturating_sub(1));
|
||||
let sub = app.v_sub_offset;
|
||||
let first_visual = app.cursor_to_first_visual_row(logical_top);
|
||||
let line_height = app
|
||||
.get_visual_height_index()
|
||||
.map_or(1, |idx| idx.visual_height_of_line(logical_top));
|
||||
app.v_offset = first_visual.saturating_add(sub.min(line_height.saturating_sub(1)));
|
||||
app.v_sub_offset = 0;
|
||||
|
||||
// line2 starts at visual row 6, sub was 2 → visual row 8
|
||||
assert_eq!(app.v_offset, 8, "v_offset should map back to visual row 8");
|
||||
assert_eq!(app.v_sub_offset, 0, "v_sub_offset should be 0 after recalibration");
|
||||
|
||||
// Scrolling should work normally with VHI
|
||||
app.scroll_down_line();
|
||||
assert_eq!(app.v_offset, 9, "scroll down should advance visual row");
|
||||
|
||||
cleanup(&path);
|
||||
});
|
||||
assert!(result.is_ok());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_vhi_rebuild_clamps_sub_to_new_line_height() {
|
||||
let content = "line0\nline1\nline2\n";
|
||||
let path = make_temp_file(content);
|
||||
let result = std::panic::catch_unwind(|| {
|
||||
let mut app = App::new();
|
||||
load_file_ready(&mut app, &path);
|
||||
app.content_height = 24;
|
||||
app.content_width = 20;
|
||||
|
||||
// Line heights: line0=1, line1=5 (wraps), line2=1
|
||||
install_vhi(&mut app, &[1, 5, 1]);
|
||||
|
||||
// Position at line1 sub-row 4 (last sub-row of 5-row line)
|
||||
app.v_offset = 5;
|
||||
app.cursor_line = 1;
|
||||
app.v_sub_offset = 0;
|
||||
|
||||
// Rebase
|
||||
let (logical_top, sub) = app.rebase_offset_for_invalidate();
|
||||
assert_eq!(logical_top, 1, "visual row 5 → line 1 (rows 1-5)");
|
||||
assert_eq!(sub, 4, "sub-row 4 within line 1");
|
||||
|
||||
app.v_offset = logical_top;
|
||||
app.v_sub_offset = sub;
|
||||
|
||||
// Simulate VHI rebuild with SHRUNK line height (e.g., JSON toggle)
|
||||
// line1 now only 2 visual rows instead of 5
|
||||
install_vhi(&mut app, &[1, 2, 1]);
|
||||
|
||||
// Recalibration should clamp sub=4 to new height-1=1
|
||||
let logical_top = app.v_offset.min(app.total_lines().saturating_sub(1));
|
||||
let sub = app.v_sub_offset;
|
||||
let first_visual = app.cursor_to_first_visual_row(logical_top);
|
||||
let line_height = app
|
||||
.get_visual_height_index()
|
||||
.map_or(1, |idx| idx.visual_height_of_line(logical_top));
|
||||
let clamped_sub = sub.min(line_height.saturating_sub(1));
|
||||
app.v_offset = first_visual.saturating_add(clamped_sub);
|
||||
app.v_sub_offset = 0;
|
||||
|
||||
// line1 starts at visual row 1, clamped sub=1 → visual row 2
|
||||
assert_eq!(app.v_offset, 2, "v_offset should be clamped to visual row 2 (line1 row 1 of 2)");
|
||||
assert_eq!(app.v_sub_offset, 0);
|
||||
|
||||
cleanup(&path);
|
||||
});
|
||||
assert!(result.is_ok());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_loading_to_ready_rebases_visual_offset() {
|
||||
let content = "line0\nline1\nline2\nline3\nline4\n";
|
||||
let path = make_temp_file(content);
|
||||
let result = std::panic::catch_unwind(|| {
|
||||
let mut app = App::new();
|
||||
load_file_ready(&mut app, &path);
|
||||
app.content_height = 10;
|
||||
|
||||
// Install VHI: 2 rows per line
|
||||
install_vhi(&mut app, &[2usize; 5]);
|
||||
|
||||
// Scroll to visual row 3 → line1 sub-row 1
|
||||
app.v_offset = 3;
|
||||
app.cursor_line = 1;
|
||||
app.v_sub_offset = 0;
|
||||
|
||||
// Simulate Loading→Ready invalidation rebase
|
||||
let (new_offset, new_sub) = app.rebase_offset_for_invalidate();
|
||||
assert_eq!(new_offset, 1, "visual row 3 → line 1");
|
||||
assert_eq!(new_sub, 1, "sub-row 1 within line 1");
|
||||
|
||||
// Apply rebased values and clear VHI (simulate invalidate)
|
||||
app.v_offset = new_offset;
|
||||
app.v_sub_offset = new_sub;
|
||||
if let AppLoadingState::Ready { reader } = &mut app.loading_state {
|
||||
reader.invalidate_visual_height_index();
|
||||
}
|
||||
|
||||
// VHI is now None → scroll_down_line uses else branch (v_sub_offset path)
|
||||
// "line1" at width=80 → compute_visual_height=1, sub=1+1 >= 1 → advance
|
||||
app.scroll_down_line();
|
||||
assert_eq!(app.v_offset, 2, "should advance to line 2 (line1 height=1, sub overflow)");
|
||||
assert_eq!(app.v_sub_offset, 0);
|
||||
|
||||
cleanup(&path);
|
||||
});
|
||||
assert!(result.is_ok());
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user