From c1a931551b0d136a8f0c436bdaad9f10f7b04f86 Mon Sep 17 00:00:00 2001 From: dailz Date: Thu, 11 Jun 2026 16:08:57 +0800 Subject: [PATCH] fix(tui): handle settings save errors and rewrite truncate_to_columns (closes #30) - Report save failures in settings instead of silently discarding errors - Save draft first, commit color_config only on success (prevents split-brain) - Show error message in red on status bar, stay in Settings mode on failure - Clear error on Esc, entering settings, or any settings edit - Rewrite truncate_to_columns as dedicated O(prefix) truncator - Tab: stop before tab if expansion would exceed width - Fixes pre-existing test_truncate_to_columns_tab failure --- Cargo.lock | 1 + crates/tui/Cargo.toml | 1 + crates/tui/src/app.rs | 152 ++++++++++++++++++++++++++++++++++++++++-- crates/tui/src/ui.rs | 18 ++++- 4 files changed, 164 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7cea9de..23b5690 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2358,6 +2358,7 @@ dependencies = [ "ratatui", "serde_json", "tempfile", + "unicode-width", ] [[package]] diff --git a/crates/tui/Cargo.toml b/crates/tui/Cargo.toml index dcf3756..a5d4d12 100644 --- a/crates/tui/Cargo.toml +++ b/crates/tui/Cargo.toml @@ -11,6 +11,7 @@ anyhow.workspace = true log-viewer-core.workspace = true serde_json.workspace = true crossbeam-channel.workspace = true +unicode-width.workspace = true [dev-dependencies] tempfile = { workspace = true } diff --git a/crates/tui/src/app.rs b/crates/tui/src/app.rs index ff79d16..075e031 100644 --- a/crates/tui/src/app.rs +++ b/crates/tui/src/app.rs @@ -9,6 +9,7 @@ use log_viewer_core::io::progressive_reader::{ use log_viewer_core::io::wrap::{format_json_line, wrap_line_chars, MAX_WRAP_INPUT_LEN}; use log_viewer_core::types::LogLevel; use log_viewer_core::watcher::file_watcher::{FileEvent, FileWatcher}; +use unicode_width::UnicodeWidthChar; use crate::color::AVAILABLE_COLORS; @@ -114,6 +115,7 @@ pub struct App { // Settings panel state pub(crate) settings_cursor: usize, pub(crate) settings_draft: ColorConfig, + pub(crate) settings_error: Option, // File watcher file_watcher: Option, @@ -141,6 +143,7 @@ impl App { color_config: ColorConfig::default(), settings_cursor: 0, settings_draft: ColorConfig::default(), + settings_error: None, file_watcher: None, reload_after_loading: false, } @@ -707,6 +710,7 @@ impl App { if !key.modifiers.contains(KeyModifiers::CONTROL) => { self.settings_draft = self.color_config.clone(); + self.settings_error = None; self.mode = AppMode::Settings; } _ => { @@ -719,12 +723,22 @@ impl App { use crossterm::event::KeyCode; match key.code { KeyCode::Esc | KeyCode::Char('q') => { + self.settings_error = None; self.mode = AppMode::Normal; } KeyCode::Enter => { - self.color_config = self.settings_draft.clone(); - let _ = self.color_config.save(); - self.mode = AppMode::Normal; + let draft = self.settings_draft.clone(); + match draft.save() { + Ok(()) => { + self.color_config = draft; + self.settings_error = None; + self.mode = AppMode::Normal; + } + Err(e) => { + self.settings_error = + Some(format!("Failed to save settings: {e}")); + } + } } KeyCode::Char('j') | KeyCode::Down => { if self.settings_cursor < 5 { @@ -784,6 +798,7 @@ impl App { } fn set_color(&mut self, level_idx: usize, color_name: &str) { + self.settings_error = None; match level_idx { 0 => self.settings_draft.error = color_name.to_string(), 1 => self.settings_draft.warn = color_name.to_string(), @@ -1187,11 +1202,41 @@ impl App { } } +const TRUNCATE_TAB_WIDTH: usize = 4; + fn truncate_to_columns(s: &str, max_cols: usize) -> String { if max_cols == 0 || s.is_empty() { return String::new(); } - wrap_line_chars(s, max_cols).into_iter().next().unwrap_or_default() + + let mut out = String::new(); + let mut col = 0; + + for ch in s.chars() { + if ch == '\t' { + let tab_stop = TRUNCATE_TAB_WIDTH - (col % TRUNCATE_TAB_WIDTH); + if col + tab_stop > max_cols { + break; + } + for _ in 0..tab_stop { + out.push(' '); + } + col += tab_stop; + } else { + let w = if ch.is_control() { + 0 + } else { + ch.width().unwrap_or(0) + }; + if col + w > max_cols { + break; + } + out.push(ch); + col += w; + } + } + + out } #[cfg(test)] @@ -2020,6 +2065,71 @@ plain text line ); } + #[test] + fn test_settings_save_failure_stays_in_settings() { + let mut app = App::new(); + let original = app.color_config.clone(); + + let impossible_path = std::path::PathBuf::from("/nonexistent/deep/nested/config.toml"); + enter_settings(&mut app); + app.handle_key(make_key(crossterm::event::KeyCode::Right)); + assert_ne!(app.settings_draft.error, original.error); + + let draft = app.settings_draft.clone(); + let result = draft.save_to(&impossible_path); + assert!(result.is_err(), "Save to impossible path should fail"); + + let err = result.unwrap_err(); + assert!( + err.to_string().contains("config") || err.to_string().contains("creating"), + "Error should mention config or creating directory" + ); + } + + #[test] + fn test_settings_error_cleared_on_edit() { + let mut app = App::new(); + enter_settings(&mut app); + + app.settings_error = Some("test error".to_string()); + + app.handle_key(make_key(crossterm::event::KeyCode::Right)); + assert!( + app.settings_error.is_none(), + "Editing a setting should clear settings_error" + ); + } + + #[test] + fn test_settings_error_cleared_on_esc() { + let mut app = App::new(); + enter_settings(&mut app); + + app.settings_error = Some("test error".to_string()); + app.handle_key(make_key(crossterm::event::KeyCode::Esc)); + + assert_eq!(app.mode, AppMode::Normal); + assert!( + app.settings_error.is_none(), + "Esc should clear settings_error" + ); + } + + #[test] + fn test_settings_error_cleared_on_enter_settings() { + let mut app = App::new(); + enter_settings(&mut app); + app.settings_error = Some("stale error".to_string()); + app.handle_key(make_key(crossterm::event::KeyCode::Esc)); + assert!(app.settings_error.is_none()); + + enter_settings(&mut app); + assert!( + app.settings_error.is_none(), + "Entering settings should clear stale errors" + ); + } + #[test] fn test_color_change_affects_build_line_spans() { use crate::ui::build_line_spans; @@ -3553,7 +3663,39 @@ plain text line #[test] fn test_truncate_to_columns_tab() { - // Tab expands to 4 spaces assert_eq!(truncate_to_columns("a\tb", 3), "a"); } + + #[test] + fn test_truncate_tab_fits_fully() { + assert_eq!(truncate_to_columns("a\tb", 5), "a b"); + } + + #[test] + fn test_truncate_tab_exact_boundary() { + assert_eq!(truncate_to_columns("a\tb", 4), "a "); + } + + #[test] + fn test_truncate_cjk_at_boundary() { + assert_eq!(truncate_to_columns("你好", 3), "你"); + assert_eq!(truncate_to_columns("你好", 4), "你好"); + } + + #[test] + fn test_truncate_control_char() { + assert_eq!(truncate_to_columns("a\x07b", 5), "a\x07b"); + } + + #[test] + fn test_truncate_exact_width() { + assert_eq!(truncate_to_columns("abcde", 5), "abcde"); + assert_eq!(truncate_to_columns("abcdef", 5), "abcde"); + } + + #[test] + fn test_truncate_only_tab() { + assert_eq!(truncate_to_columns("\t", 4), " "); + assert_eq!(truncate_to_columns("\t", 3), ""); + } } \ No newline at end of file diff --git a/crates/tui/src/ui.rs b/crates/tui/src/ui.rs index 6c57748..f92a540 100644 --- a/crates/tui/src/ui.rs +++ b/crates/tui/src/ui.rs @@ -88,9 +88,21 @@ pub fn render(frame: &mut ratatui::Frame, app: &mut App) { } // ── Status bar ───────────────────────────────────────────────── - let status_text = if app.mode == AppMode::Settings { - " j/k:navigate ←/→:change 1-8:jump Enter:save Esc:cancel" - } else if app.is_error() { + if app.mode == AppMode::Settings { + if let Some(ref err) = app.settings_error { + frame.render_widget( + Paragraph::new(err.as_str()).style(Style::default().fg(Color::Red)), + outer[2], + ); + } else { + frame.render_widget( + Paragraph::new(" j/k:navigate ←/→:change 1-8:jump Enter:save Esc:cancel"), + outer[2], + ); + } + return; + } + let status_text = if app.is_error() { " Press q to quit" } else if app.is_loading() { let pct = app.loading_progress().map_or(0, |p| p as usize);