fix(tui): decouple cursor from viewport + correct width calc in no-VHI paths

Three intertwined fixes for the no-VHI scroll path that together eliminate
the 'switching to a new entry scrolls it to viewport top' behavior observed
during Loading state, Loading->Ready transition window, and Tab-toggle
VHI rebuild window.

Cursor/viewport decoupling (the user-visible bug):
* scroll_down_line / scroll_up_line no-VHI branches now advance only the
  cursor (cursor_line, cursor_sub_offset), then call ensure_cursor_visible_no_vhi
  to adjust the viewport minimally if needed. Previously they locked
  v_offset = cursor_line, forcing the cursor's entry to the top of the
  viewport on every keypress.
* New helpers: advance_visual_pos, retreat_visual_pos (bounded visual-row
  walks using compute_visual_height), ensure_cursor_visible_no_vhi (matches
  the VHI-path semantics - top-align when cursor is above viewport,
  bottom-align with minimal scroll when below, no movement when within).
* Same pattern applied to scroll_down/up_half_page and scroll_down/up_page.
* scroll_to_bottom sets cursor_sub_offset to the last visual row of the
  last line in no-VHI state (was 0).
* ensure_viewport_cache hidden anchors removed: the params-change recenter
  and the Loading post-check that did v_offset = cursor_line are replaced
  with the visibility helper.
* 4 transition sites (Tab handler, ensure_visual_height_index, reload,
  poll VHI complete) now clamp cursor_sub_offset against the current
  line height instead of blindly zeroing.

Width calculation correctness (root cause of the residual j/k bug):
* gutter_width() pub(crate) method extracted from ui.rs's inline formula,
  shared between renderer and App to prevent drift.
* get_content_width() now returns effective text-wrap width (area minus
  gutter), matching the renderer's actual_content_width. Previously it
  returned raw area width, causing compute_visual_height to under-count
  wrap rows for lines that wrapped only after gutter subtraction.
* Free function gutter_width_for(total_lines, is_loading) added so
  handle_file_appended and reload_ready_reader can recompute width
  after line-count changes without conflicting borrows on self.
* handle_file_appended: width now captured AFTER update_for_append
  (was before), so digit-boundary crossings (9->10, 99->100) don't
  leave stale gutter widths.

ui.rs render_content now calls app.gutter_width() instead of inlining
the formula (DRY).

Tests: 7 existing tests that locked in the old buggy v_offset = cursor_line
coupling have been rewritten to assert the new correct behavior. New
property tests verify wrap_line_count matches wrap_line_chars across
CJK/emoji/tab/combining-mark/control-char inputs.
This commit is contained in:
dailz
2026-06-22 14:52:32 +08:00
parent 4421da35f4
commit e69b7af32a
2 changed files with 1012 additions and 350 deletions
+944 -311
View File
File diff suppressed because it is too large Load Diff
+48 -19
View File
@@ -6,6 +6,7 @@ use crate::color::level_fg;
use log_viewer_core::config::ColorConfig; use log_viewer_core::config::ColorConfig;
use log_viewer_core::types::LogLevel; use log_viewer_core::types::LogLevel;
#[allow(dead_code)]
pub(crate) fn build_line_spans( pub(crate) fn build_line_spans(
gutter_text: String, gutter_text: String,
content_text: String, content_text: String,
@@ -28,6 +29,10 @@ pub(crate) fn build_line_spans(
]) ])
} }
fn is_cursor_visual_row(app: &App, logical_line: usize, visual_row: usize) -> bool {
logical_line == app.cursor_line && visual_row == app.cursor_sub_offset
}
pub fn render(frame: &mut ratatui::Frame, app: &mut App) { pub fn render(frame: &mut ratatui::Frame, app: &mut App) {
use ratatui::layout::{Constraint, Layout}; use ratatui::layout::{Constraint, Layout};
use ratatui::widgets::Paragraph; use ratatui::widgets::Paragraph;
@@ -123,7 +128,10 @@ pub fn render(frame: &mut ratatui::Frame, app: &mut App) {
let name = app.file_name().unwrap_or("unknown"); let name = app.file_name().unwrap_or("unknown");
let total = app.total_lines(); let total = app.total_lines();
let cursor_display = if total == 0 { 0 } else { app.cursor_line + 1 }; let cursor_display = if total == 0 { 0 } else { app.cursor_line + 1 };
let status = format!(" {} [{}/{}] | j/k:scroll d/u:half-page f/b:page G/gg:jump Tab:format S:settings q:quit", name, cursor_display, total); let status = format!(
" {} [{}/{}] | j/k:scroll d/u:half-page f/b:page G/gg:jump Tab:format S:settings q:quit",
name, cursor_display, total
);
frame.render_widget(Paragraph::new(status), outer[2]); frame.render_widget(Paragraph::new(status), outer[2]);
return; return;
} else { } else {
@@ -142,8 +150,12 @@ pub fn render_settings(frame: &mut ratatui::Frame, app: &mut App, area: ratatui:
let popup_w = ((area.width as u32 * 4 / 5).max(40)).min(area.width as u32) as u16; let popup_w = ((area.width as u32 * 4 / 5).max(40)).min(area.width as u32) as u16;
let popup_h = ((area.height as u32 * 4 / 5).max(14)).min(area.height as u32) as u16; let popup_h = ((area.height as u32 * 4 / 5).max(14)).min(area.height as u32) as u16;
let popup_x = area.x.saturating_add(area.width.saturating_sub(popup_w) / 2); let popup_x = area
let popup_y = area.y.saturating_add(area.height.saturating_sub(popup_h) / 2); .x
.saturating_add(area.width.saturating_sub(popup_w) / 2);
let popup_y = area
.y
.saturating_add(area.height.saturating_sub(popup_h) / 2);
let popup = ratatui::layout::Rect::new(popup_x, popup_y, popup_w, popup_h); let popup = ratatui::layout::Rect::new(popup_x, popup_y, popup_w, popup_h);
let block = Block::new().borders(Borders::ALL).title(" Color Settings "); let block = Block::new().borders(Borders::ALL).title(" Color Settings ");
@@ -221,14 +233,8 @@ fn render_content(frame: &mut ratatui::Frame, app: &mut App, area: ratatui::layo
} else { } else {
0 0
}; };
let is_loading = app.is_loading(); let is_loading = app.is_loading();
let gutter_prefix_extra = if is_loading { 1 } else { 0 }; let gutter_width = app.gutter_width();
let gutter_width = if total_lines > 0 {
line_num_width + gutter_prefix_extra + 1 + 1
} else {
0
};
let actual_content_width = content_width.saturating_sub(gutter_width); let actual_content_width = content_width.saturating_sub(gutter_width);
@@ -263,7 +269,7 @@ fn render_content(frame: &mut ratatui::Frame, app: &mut App, area: ratatui::layo
break; break;
} }
let is_cursor = logical_line == app.cursor_line; let is_cursor = is_cursor_visual_row(app, logical_line, visual_row);
let level = entry.level.as_ref(); let level = entry.level.as_ref();
let bg_color = if is_cursor { let bg_color = if is_cursor {
@@ -375,6 +381,17 @@ mod tests {
assert_eq!(line.spans[1].style.bg, Some(Color::Reset)); assert_eq!(line.spans[1].style.bg, Some(Color::Reset));
} }
#[test]
fn test_cursor_highlight_predicate_uses_cursor_sub_offset() {
let mut app = App::new();
app.cursor_line = 4;
app.cursor_sub_offset = 2;
assert!(!is_cursor_visual_row(&app, 4, 0));
assert!(!is_cursor_visual_row(&app, 3, 2));
assert!(is_cursor_visual_row(&app, 4, 2));
}
fn render_to_buffer(app: &mut App, width: u16, height: u16) -> ratatui::buffer::Buffer { fn render_to_buffer(app: &mut App, width: u16, height: u16) -> ratatui::buffer::Buffer {
let backend = ratatui::backend::TestBackend::new(width, height); let backend = ratatui::backend::TestBackend::new(width, height);
let mut terminal = ratatui::Terminal::new(backend).unwrap(); let mut terminal = ratatui::Terminal::new(backend).unwrap();
@@ -481,7 +498,8 @@ mod tests {
let result = std::panic::catch_unwind(|| { let result = std::panic::catch_unwind(|| {
let data = std::fs::read(&path).unwrap(); let data = std::fs::read(&path).unwrap();
let index = log_viewer_core::io::line_index::LineIndex::from_bytes(&data); let index = log_viewer_core::io::line_index::LineIndex::from_bytes(&data);
let _ = log_viewer_core::io::index_cache::IndexCache::save_with_hash(&path, &index, &data); let _ =
log_viewer_core::io::index_cache::IndexCache::save_with_hash(&path, &index, &data);
let mut app = App::new(); let mut app = App::new();
app.load_file(path.to_str().unwrap()).unwrap(); app.load_file(path.to_str().unwrap()).unwrap();
@@ -509,14 +527,22 @@ mod tests {
// ── Issue #31: Settings popup area offset tests ──────────────── // ── Issue #31: Settings popup area offset tests ────────────────
/// Helper: enter settings mode and render to buffer. /// Helper: enter settings mode and render to buffer.
fn render_settings_to_buffer(app: &mut App, width: u16, height: u16) -> ratatui::buffer::Buffer { fn render_settings_to_buffer(
app: &mut App,
width: u16,
height: u16,
) -> ratatui::buffer::Buffer {
app.mode = crate::app::AppMode::Settings; app.mode = crate::app::AppMode::Settings;
app.settings_draft = app.color_config.clone(); app.settings_draft = app.color_config.clone();
render_to_buffer(app, width, height) render_to_buffer(app, width, height)
} }
/// Find the top-left corner of the popup border by scanning for '┌'. /// Find the top-left corner of the popup border by scanning for '┌'.
fn find_popup_top_left(buf: &ratatui::buffer::Buffer, width: u16, height: u16) -> Option<(u16, u16)> { fn find_popup_top_left(
buf: &ratatui::buffer::Buffer,
width: u16,
height: u16,
) -> Option<(u16, u16)> {
for row in 0..height { for row in 0..height {
for col in 0..width { for col in 0..width {
if buf.cell((col, row)).unwrap().symbol() == "" { if buf.cell((col, row)).unwrap().symbol() == "" {
@@ -537,8 +563,8 @@ mod tests {
let mut app = App::new(); let mut app = App::new();
let buf = render_settings_to_buffer(&mut app, 80, 24); let buf = render_settings_to_buffer(&mut app, 80, 24);
let (_px, py) = find_popup_top_left(&buf, 80, 24) let (_px, py) =
.expect("popup border '┌' should be rendered"); find_popup_top_left(&buf, 80, 24).expect("popup border '┌' should be rendered");
// outer[1].y == 1; the popup is centered inside a 22-row area, // outer[1].y == 1; the popup is centered inside a 22-row area,
// so popup_y must be at least 1 (not 0). // so popup_y must be at least 1 (not 0).
@@ -556,8 +582,8 @@ mod tests {
let mut app = App::new(); let mut app = App::new();
let buf = render_settings_to_buffer(&mut app, 80, 24); let buf = render_settings_to_buffer(&mut app, 80, 24);
let (px, _py) = find_popup_top_left(&buf, 80, 24) let (px, _py) =
.expect("popup border '┌' should be rendered"); find_popup_top_left(&buf, 80, 24).expect("popup border '┌' should be rendered");
// popup_w = 80*4/5 = 64, centered: (80-64)/2 = 8 // popup_w = 80*4/5 = 64, centered: (80-64)/2 = 8
assert_eq!( assert_eq!(
@@ -573,6 +599,9 @@ mod tests {
let mut app = App::new(); let mut app = App::new();
let _buf = render_settings_to_buffer(&mut app, 30, 10); let _buf = render_settings_to_buffer(&mut app, 30, 10);
})); }));
assert!(result.is_ok(), "rendering settings in a small frame should not panic"); assert!(
result.is_ok(),
"rendering settings in a small frame should not panic"
);
} }
} }