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
This commit is contained in:
1
Cargo.lock
generated
1
Cargo.lock
generated
@@ -2358,6 +2358,7 @@ dependencies = [
|
|||||||
"ratatui",
|
"ratatui",
|
||||||
"serde_json",
|
"serde_json",
|
||||||
"tempfile",
|
"tempfile",
|
||||||
|
"unicode-width",
|
||||||
]
|
]
|
||||||
|
|
||||||
[[package]]
|
[[package]]
|
||||||
|
|||||||
@@ -11,6 +11,7 @@ anyhow.workspace = true
|
|||||||
log-viewer-core.workspace = true
|
log-viewer-core.workspace = true
|
||||||
serde_json.workspace = true
|
serde_json.workspace = true
|
||||||
crossbeam-channel.workspace = true
|
crossbeam-channel.workspace = true
|
||||||
|
unicode-width.workspace = true
|
||||||
|
|
||||||
[dev-dependencies]
|
[dev-dependencies]
|
||||||
tempfile = { workspace = true }
|
tempfile = { workspace = true }
|
||||||
|
|||||||
@@ -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::io::wrap::{format_json_line, wrap_line_chars, MAX_WRAP_INPUT_LEN};
|
||||||
use log_viewer_core::types::LogLevel;
|
use log_viewer_core::types::LogLevel;
|
||||||
use log_viewer_core::watcher::file_watcher::{FileEvent, FileWatcher};
|
use log_viewer_core::watcher::file_watcher::{FileEvent, FileWatcher};
|
||||||
|
use unicode_width::UnicodeWidthChar;
|
||||||
|
|
||||||
use crate::color::AVAILABLE_COLORS;
|
use crate::color::AVAILABLE_COLORS;
|
||||||
|
|
||||||
@@ -114,6 +115,7 @@ pub struct App {
|
|||||||
// Settings panel state
|
// Settings panel state
|
||||||
pub(crate) settings_cursor: usize,
|
pub(crate) settings_cursor: usize,
|
||||||
pub(crate) settings_draft: ColorConfig,
|
pub(crate) settings_draft: ColorConfig,
|
||||||
|
pub(crate) settings_error: Option<String>,
|
||||||
|
|
||||||
// File watcher
|
// File watcher
|
||||||
file_watcher: Option<FileWatcher>,
|
file_watcher: Option<FileWatcher>,
|
||||||
@@ -141,6 +143,7 @@ impl App {
|
|||||||
color_config: ColorConfig::default(),
|
color_config: ColorConfig::default(),
|
||||||
settings_cursor: 0,
|
settings_cursor: 0,
|
||||||
settings_draft: ColorConfig::default(),
|
settings_draft: ColorConfig::default(),
|
||||||
|
settings_error: None,
|
||||||
file_watcher: None,
|
file_watcher: None,
|
||||||
reload_after_loading: false,
|
reload_after_loading: false,
|
||||||
}
|
}
|
||||||
@@ -707,6 +710,7 @@ impl App {
|
|||||||
if !key.modifiers.contains(KeyModifiers::CONTROL) =>
|
if !key.modifiers.contains(KeyModifiers::CONTROL) =>
|
||||||
{
|
{
|
||||||
self.settings_draft = self.color_config.clone();
|
self.settings_draft = self.color_config.clone();
|
||||||
|
self.settings_error = None;
|
||||||
self.mode = AppMode::Settings;
|
self.mode = AppMode::Settings;
|
||||||
}
|
}
|
||||||
_ => {
|
_ => {
|
||||||
@@ -719,12 +723,22 @@ impl App {
|
|||||||
use crossterm::event::KeyCode;
|
use crossterm::event::KeyCode;
|
||||||
match key.code {
|
match key.code {
|
||||||
KeyCode::Esc | KeyCode::Char('q') => {
|
KeyCode::Esc | KeyCode::Char('q') => {
|
||||||
|
self.settings_error = None;
|
||||||
self.mode = AppMode::Normal;
|
self.mode = AppMode::Normal;
|
||||||
}
|
}
|
||||||
KeyCode::Enter => {
|
KeyCode::Enter => {
|
||||||
self.color_config = self.settings_draft.clone();
|
let draft = self.settings_draft.clone();
|
||||||
let _ = self.color_config.save();
|
match draft.save() {
|
||||||
self.mode = AppMode::Normal;
|
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 => {
|
KeyCode::Char('j') | KeyCode::Down => {
|
||||||
if self.settings_cursor < 5 {
|
if self.settings_cursor < 5 {
|
||||||
@@ -784,6 +798,7 @@ impl App {
|
|||||||
}
|
}
|
||||||
|
|
||||||
fn set_color(&mut self, level_idx: usize, color_name: &str) {
|
fn set_color(&mut self, level_idx: usize, color_name: &str) {
|
||||||
|
self.settings_error = None;
|
||||||
match level_idx {
|
match level_idx {
|
||||||
0 => self.settings_draft.error = color_name.to_string(),
|
0 => self.settings_draft.error = color_name.to_string(),
|
||||||
1 => self.settings_draft.warn = 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 {
|
fn truncate_to_columns(s: &str, max_cols: usize) -> String {
|
||||||
if max_cols == 0 || s.is_empty() {
|
if max_cols == 0 || s.is_empty() {
|
||||||
return String::new();
|
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)]
|
#[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]
|
#[test]
|
||||||
fn test_color_change_affects_build_line_spans() {
|
fn test_color_change_affects_build_line_spans() {
|
||||||
use crate::ui::build_line_spans;
|
use crate::ui::build_line_spans;
|
||||||
@@ -3553,7 +3663,39 @@ plain text line
|
|||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn test_truncate_to_columns_tab() {
|
fn test_truncate_to_columns_tab() {
|
||||||
// Tab expands to 4 spaces
|
|
||||||
assert_eq!(truncate_to_columns("a\tb", 3), "a");
|
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), "");
|
||||||
|
}
|
||||||
}
|
}
|
||||||
@@ -88,9 +88,21 @@ pub fn render(frame: &mut ratatui::Frame, app: &mut App) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// ── Status bar ─────────────────────────────────────────────────
|
// ── Status bar ─────────────────────────────────────────────────
|
||||||
let status_text = if app.mode == AppMode::Settings {
|
if app.mode == AppMode::Settings {
|
||||||
" j/k:navigate ←/→:change 1-8:jump Enter:save Esc:cancel"
|
if let Some(ref err) = app.settings_error {
|
||||||
} else if app.is_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"
|
" Press q to quit"
|
||||||
} else if app.is_loading() {
|
} else if app.is_loading() {
|
||||||
let pct = app.loading_progress().map_or(0, |p| p as usize);
|
let pct = app.loading_progress().map_or(0, |p| p as usize);
|
||||||
|
|||||||
Reference in New Issue
Block a user