From 822a9a504cfd9e525fa8af190a90f7595825c711 Mon Sep 17 00:00:00 2001 From: Leszek Date: Mon, 15 Jun 2026 00:13:08 +0200 Subject: [PATCH 1/3] refactor(input): EventContext, dispatch split, bugfixes & test quality Implements docs/pr-plan/pr-08-input-main-infra.md (PR8). Bugs (with value-asserting tests): - dialogs: pass trimmed dest to resolve_user_path (extract/create archive) - menu: surface menu-bar width overflow (debug_log + saturate) not silent 0 - mouse: guard panel_bounds for tiny terminals (no impossible row range) - main: recover_terminal_state no longer reports a working terminal as a failure Refactor: - EventContext bundles loop state; collapses dispatch_event/dispatch_key_event (drops too_many_arguments), per-mode dispatch table, run_app split - handlers migrated to &mut EventContext; drop dead _viewer_state params - mouse/mode_dispatch/normal/dialogs/pickers/menu_actions/directory_tree: helper extraction, shared matchers, named constants, fewer hot-path clones - lib.rs: drop dead root re-exports (module-path public API surface) - input/mod.rs: pub(crate) Build: - Cargo.toml: [profile.release] lto + codegen-units=1, [profile.dev] opt-level=1 Tests: - value-asserting key/mouse outcomes, picker/history coverage, parameterized dialog cases, loader-exposing harness No behavior change beyond the listed fixes. Gate green: fmt, clippy -D warnings, 1225 tests, release build. --- Cargo.toml | 12 + src/input/command_line.rs | 32 ++- src/input/dialogs.rs | 375 ++++++++++++++++------------ src/input/directory_tree.rs | 68 +++-- src/input/menu_actions.rs | 104 +++++--- src/input/mod.rs | 41 ++- src/input/mode_dispatch.rs | 164 ++++++------ src/input/mouse.rs | 201 ++++++++++----- src/input/mouse/tests.rs | 280 ++++++++++----------- src/input/normal.rs | 486 +++++++++++++++++++++--------------- src/input/pickers.rs | 290 +++++++++++---------- src/lib.rs | 18 +- src/main.rs | 359 +++++++++++++------------- src/menu.rs | 118 ++++++--- src/tests/dialogs.rs | 40 +-- src/tests/helpers.rs | 91 +++++-- src/tests/history.rs | 91 ++++++- src/tests/keybinds.rs | 189 +++++++++++++- src/tests/keyevents.rs | 130 ++++++++-- src/tests/menu.rs | 246 +++++++----------- src/tests/mod.rs | 49 ++-- src/tests/overwrite.rs | 21 +- src/tests/pickers.rs | 138 +++++++++- src/tests/user_menu.rs | 99 ++++---- src/tests/viewer.rs | 40 +-- 25 files changed, 2266 insertions(+), 1416 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 47d515b..374b35f 100755 --- a/Cargo.toml +++ b/Cargo.toml @@ -39,6 +39,18 @@ zstd = "0.13" [dev-dependencies] tempfile = "3" +# Release: maximize runtime performance of the single binary. +# LTO + a single codegen unit let the optimizer inline across crate +# boundaries (slower link, faster/smaller binary). +[profile.release] +lto = true +codegen-units = 1 + +# Dev: light optimization keeps TUI interaction responsive while +# preserving full debug info and fast incremental rebuilds. +[profile.dev] +opt-level = 1 + [target.'cfg(target_os = "macos")'.dependencies] notify = { version = "8", default-features = false, features = ["macos_fsevent"] } diff --git a/src/input/command_line.rs b/src/input/command_line.rs index dd63ee4..6e408e4 100644 --- a/src/input/command_line.rs +++ b/src/input/command_line.rs @@ -8,6 +8,15 @@ fn reset_history(state: &mut AppState) { state.input.history_index = None; } +/// Reset history navigation only if the editing op actually changed the line. +/// Collapses the repeated `if { reset_history }` pattern at +/// every text-mutating key branch into a single call-site. +fn reset_history_if(state: &mut AppState, edited: bool) { + if edited { + reset_history(state); + } +} + fn cancel_command_input(state: &mut AppState) { state.mode = AppMode::Normal; state.input.command_line.clear(); @@ -41,9 +50,8 @@ pub(crate) fn handle_command_line(state: &mut AppState, key: KeyEvent) { return; } KeyCode::Char('w') => { - if state.input.command_line.delete_word_backward() { - reset_history(state); - } + let edited = state.input.command_line.delete_word_backward(); + reset_history_if(state, edited); return; } KeyCode::Char('c') => { @@ -56,8 +64,9 @@ pub(crate) fn handle_command_line(state: &mut AppState, key: KeyEvent) { } if key.modifiers.contains(KeyModifiers::ALT) { - if key.code == KeyCode::Backspace && state.input.command_line.delete_word_backward() { - reset_history(state); + if key.code == KeyCode::Backspace { + let edited = state.input.command_line.delete_word_backward(); + reset_history_if(state, edited); } return; } @@ -69,8 +78,9 @@ pub(crate) fn handle_command_line(state: &mut AppState, key: KeyEvent) { KeyCode::Enter => { command_execute(state); } - KeyCode::Backspace if state.input.command_line.backspace() => { - reset_history(state); + KeyCode::Backspace => { + let edited = state.input.command_line.backspace(); + reset_history_if(state, edited); } KeyCode::Left => { state.input.command_line.cursor_left(); @@ -105,10 +115,10 @@ pub(crate) fn handle_command_line(state: &mut AppState, key: KeyEvent) { .set_text_at_end(state.input.command_history[idx + 1].clone()); } else { state.input.history_index = None; - state - .input - .command_line - .set_text_at_end(state.input.command_draft.clone()); + // Move the draft out instead of cloning: leaving Down restores + // the draft, and the next Up overwrites it before any read. + let draft = std::mem::take(&mut state.input.command_draft); + state.input.command_line.set_text_at_end(draft); } } } diff --git a/src/input/dialogs.rs b/src/input/dialogs.rs index 7179ec9..02df87e 100644 --- a/src/input/dialogs.rs +++ b/src/input/dialogs.rs @@ -11,10 +11,15 @@ use lc::ops; use lc::ops::archive::ArchiveFormat; use lc::ui::{dialogs, viewer}; +use super::EventContext; use crate::app::panel_ops::{ panel_visible_height, rebuild_visible_entries, refresh_active, refresh_both, set_active_panel, }; +/// Upper bound on the byte length of any dialog text field (paths, names, octal +/// modes, search queries). Caps memory for pathological pasted input and keeps +/// the single-line input renderable; enforced per inserted char in +/// [`apply_dialog_text_edit`]. const MAX_DIALOG_INPUT_BYTES: usize = 4096; pub(crate) fn parse_octal_mode(input: &str) -> Option { @@ -68,10 +73,6 @@ fn reset_dialog_state(state: &mut AppState) { } } -fn dismiss_dialog_and_restore(state: &mut AppState) { - reset_dialog_state(state); -} - pub(crate) fn finish_confirmed_action(state: &mut AppState) { state.input.dialog_selection = 0; if state.ui.status_message.is_some() @@ -143,11 +144,7 @@ pub(crate) fn check_overwrite_conflict(state: &AppState) -> Option> } }) .collect(); - if conflicting.is_empty() { - None - } else { - Some(conflicting) - } + (!conflicting.is_empty()).then_some(conflicting) } PendingAction::CreateArchive { dest, overwrite, .. @@ -190,11 +187,7 @@ pub(crate) fn check_overwrite_conflict(state: &AppState) -> Option> } }) .collect(); - if conflicting.is_empty() { - None - } else { - Some(conflicting) - } + (!conflicting.is_empty()).then_some(conflicting) } PendingAction::Delete { .. } => None, } @@ -310,6 +303,84 @@ fn handle_quick_cd(state: &mut AppState, input: &str) { } } +/// What the common epilogue should do after a per-action handler runs. +enum InputOutcome { + /// The handler already finalized the dialog; skip the common epilogue. + Finalized, + /// Reset to Normal mode and refresh the active panel. + ResetWithRefresh, + /// Reset to Normal mode without refreshing (Filter rebuilds in place). + ResetNoRefresh, +} + +fn input_action_viewer_search( + state: &mut AppState, + viewer_state: &mut Option, + input: &str, + terminal_height: u16, +) -> InputOutcome { + if let Some(vs) = viewer_state.as_mut() { + vs.search(input, terminal_height.saturating_sub(3) as usize); + } + state.mode = AppMode::Viewing; + state.input.dialog_input.clear(); + InputOutcome::Finalized +} + +fn input_action_create_directory(state: &mut AppState, input: &str) -> InputOutcome { + if let Err(msg) = validate_create_or_rename(input, "Directory name") { + state.ui.status_message = Some(msg); + return InputOutcome::Finalized; + } + let target = fs::path::resolve_user_path(state.active_panel().path(), input); + if let Err(err) = ops::create_directory(&target) { + state.ui.status_message = Some(format!("Create directory failed: {err}")); + } + InputOutcome::ResetWithRefresh +} + +fn input_action_rename(state: &mut AppState, input: &str) -> InputOutcome { + if let Err(msg) = validate_create_or_rename(input, "New name") { + state.ui.status_message = Some(msg); + return InputOutcome::Finalized; + } + if let Some(entry) = state.active_panel().current_entry() + && input != entry.name + && let Err(err) = ops::rename_entry(&entry.path, input) + { + state.ui.status_message = Some(format!("Rename failed: {err}")); + } + InputOutcome::ResetWithRefresh +} + +fn input_action_chmod(state: &mut AppState, input: &str) -> InputOutcome { + let Some(mode) = parse_octal_mode(input) else { + if input.trim().is_empty() { + state.ui.status_message = Some("Octal mode cannot be empty".to_string()); + } else { + state.ui.status_message = Some(format!("Invalid octal mode '{input}'")); + } + return InputOutcome::Finalized; + }; + if let Some(entry) = state.active_panel().current_entry() + && let Err(err) = ops::chmod(&entry.path, mode) + { + state.ui.status_message = Some(format!("Chmod failed: {err}")); + } + InputOutcome::ResetWithRefresh +} + +fn input_action_filter(state: &mut AppState, input: String, terminal_height: u16) -> InputOutcome { + let panel = state.active_panel_mut(); + panel.set_filter((!input.trim().is_empty()).then_some(input)); + if panel.listing.needs_full_read() || panel.listing.unfiltered().is_empty() { + refresh_active(state); + } else { + rebuild_visible_entries(panel, panel_visible_height(terminal_height)); + } + InputOutcome::ResetNoRefresh +} + fn handle_input_action( state: &mut AppState, viewer_state: &mut Option, @@ -318,77 +389,32 @@ fn handle_input_action( terminal_height: u16, ) { let input = state.input.dialog_input.text().to_owned(); - match action { + let outcome = match action { InputAction::ViewerSearch => { - if let Some(vs) = viewer_state.as_mut() { - vs.search(&input, terminal_height.saturating_sub(3) as usize); - } - state.mode = AppMode::Viewing; - state.input.dialog_input.clear(); - return; - } - InputAction::CreateDirectory => { - if let Err(msg) = validate_create_or_rename(&input, "Directory name") { - state.ui.status_message = Some(msg); - return; - } - let target = fs::path::resolve_user_path(state.active_panel().path(), &input); - if let Err(err) = ops::create_directory(&target) { - state.ui.status_message = Some(format!("Create directory failed: {err}")); - } - } - InputAction::Rename => { - if let Err(msg) = validate_create_or_rename(&input, "New name") { - state.ui.status_message = Some(msg); - return; - } - if let Some(entry) = state.active_panel().current_entry() - && input != entry.name - && let Err(err) = ops::rename_entry(&entry.path, &input) - { - state.ui.status_message = Some(format!("Rename failed: {err}")); - } - } - InputAction::Chmod => { - let mode = match parse_octal_mode(&input) { - Some(m) => m, - None => { - if input.trim().is_empty() { - state.ui.status_message = Some("Octal mode cannot be empty".to_string()); - } else { - state.ui.status_message = Some(format!("Invalid octal mode '{input}'")); - } - return; - } - }; - if let Some(entry) = state.active_panel().current_entry() - && let Err(err) = ops::chmod(&entry.path, mode) - { - state.ui.status_message = Some(format!("Chmod failed: {err}")); - } + input_action_viewer_search(state, viewer_state, &input, terminal_height) } - InputAction::Filter => { - let panel = state.active_panel_mut(); - panel.set_filter(if input.trim().is_empty() { - None - } else { - Some(input) - }); - if panel.listing.needs_full_read() || panel.listing.unfiltered().is_empty() { - refresh_active(state); - } else { - rebuild_visible_entries(panel, panel_visible_height(terminal_height)); - } + InputAction::CreateDirectory => input_action_create_directory(state, &input), + InputAction::Rename => input_action_rename(state, &input), + InputAction::Chmod => input_action_chmod(state, &input), + InputAction::Filter => input_action_filter(state, input, terminal_height), + InputAction::QuickCd => { + handle_quick_cd(state, &input); + InputOutcome::ResetWithRefresh } - InputAction::QuickCd => handle_quick_cd(state, &input), InputAction::FindFile => { handle_find_file(state, running_job, &input); - return; + InputOutcome::Finalized } - } + }; + + let refresh = match outcome { + InputOutcome::Finalized => return, + InputOutcome::ResetWithRefresh => true, + InputOutcome::ResetNoRefresh => false, + }; state.mode = AppMode::Normal; state.input.dialog_input.clear(); - if !matches!(action, InputAction::Filter) { + if refresh { refresh_active(state); } if let Some(panel) = state.ui.menu_restore_panel.take() { @@ -439,7 +465,7 @@ fn handle_input_dialog( fn handle_error_dialog(state: &mut AppState, key: KeyCode) { if matches!(key, KeyCode::Enter | KeyCode::Esc) { - dismiss_dialog_and_restore(state); + reset_dialog_state(state); } } @@ -454,7 +480,7 @@ fn handle_progress_dialog(state: &mut AppState, running_job: &Option fn handle_properties_dialog(state: &mut AppState, key: KeyCode) { if matches!(key, KeyCode::Enter | KeyCode::Esc) { - dismiss_dialog_and_restore(state); + reset_dialog_state(state); } } @@ -509,15 +535,23 @@ fn apply_dialog_text_edit(dest_input: &mut TextInput, key: KeyCode) { } } -fn handle_archive_extract_dialog( - state: &mut AppState, - running_job: &mut Option, - key: KeyCode, -) { +/// Navigation outcome shared by the archive extract/create dialogs, which use an +/// identical OK/Cancel button layout and text-input field. +enum ArchiveNav { + /// User confirmed (OK): build and dispatch the pending action. + Commit, + /// User cancelled (Esc, Cancel button) — dialog already dismissed. + Dismissed, + /// A non-committing key (toggle/no-op) was handled; fall through to text edit. + Continue, +} + +/// Handle the OK/Cancel button navigation common to both archive dialogs. +fn archive_dialog_nav(state: &mut AppState, key: KeyCode) -> ArchiveNav { match key { KeyCode::Esc => { dismiss_dialog(state); - return; + ArchiveNav::Dismissed } KeyCode::Left | KeyCode::Right => { state.input.dialog_selection = if state.input.dialog_selection == 0 { @@ -525,93 +559,99 @@ fn handle_archive_extract_dialog( } else { 0 }; - return; + ArchiveNav::Continue } KeyCode::Enter if state.input.dialog_selection == 1 => { dismiss_dialog(state); - return; - } - KeyCode::Enter => { - let (source, dest_text) = - if let AppMode::Dialog(DialogKind::ArchiveExtract(ref details)) = state.mode { - (details.source.clone(), details.dest_input.text().to_owned()) - } else { - return; - }; - if dest_text.trim().is_empty() { - state.ui.status_message = Some("Destination path cannot be empty".to_string()); - return; - } - let dest = fs::path::resolve_user_path(state.active_panel().path(), &dest_text); - state.ui.pending_action = Some(PendingAction::ExtractArchive { - source, - dest, - overwrite: false, - }); - dispatch_with_overwrite_check(state, running_job); - return; + ArchiveNav::Dismissed } - _ => {} + KeyCode::Enter => ArchiveNav::Commit, + _ => ArchiveNav::Continue, } - if let AppMode::Dialog(DialogKind::ArchiveExtract(ref mut details)) = state.mode { - apply_dialog_text_edit(&mut details.dest_input, key); +} + +/// Build the `ExtractArchive` pending action from the active extract dialog and +/// dispatch it. Returns early (without dispatch) on empty/invalid input. +fn commit_archive_extract(state: &mut AppState, running_job: &mut Option) { + let AppMode::Dialog(DialogKind::ArchiveExtract(details)) = &state.mode else { + return; + }; + let source = details.source.clone(); + let dest_text = details.dest_input.text().trim().to_owned(); + if dest_text.is_empty() { + state.ui.status_message = Some("Destination path cannot be empty".to_string()); + return; } + let dest = fs::path::resolve_user_path(state.active_panel().path(), &dest_text); + state.ui.pending_action = Some(PendingAction::ExtractArchive { + source, + dest, + overwrite: false, + }); + dispatch_with_overwrite_check(state, running_job); } -fn handle_archive_create_dialog( +/// Build the `CreateArchive` pending action from the active create dialog and +/// dispatch it. Returns early (without dispatch) on empty input or unknown format. +fn commit_archive_create(state: &mut AppState, running_job: &mut Option) { + let AppMode::Dialog(DialogKind::ArchiveCreate(details)) = &state.mode else { + return; + }; + let sources = details.sources.clone(); + let dest_text = details.dest_input.text().trim().to_owned(); + if dest_text.is_empty() { + state.ui.status_message = Some("Archive path cannot be empty".to_string()); + return; + } + let dest = fs::path::resolve_user_path(state.active_panel().path(), &dest_text); + let Some(format) = archive_format_from_path(&dest) else { + state.ui.status_message = Some( + "Unsupported archive format. Use: zip, tar, tar.gz, tar.bz2, tar.xz, tar.zst, 7z" + .to_string(), + ); + return; + }; + state.ui.pending_action = Some(PendingAction::CreateArchive { + sources, + dest, + format, + overwrite: false, + }); + dispatch_with_overwrite_check(state, running_job); +} + +/// Borrow the destination text input of whichever archive dialog is active. +fn active_archive_dest_input(state: &mut AppState) -> Option<&mut TextInput> { + match &mut state.mode { + AppMode::Dialog(DialogKind::ArchiveExtract(details)) => Some(&mut details.dest_input), + AppMode::Dialog(DialogKind::ArchiveCreate(details)) => Some(&mut details.dest_input), + _ => None, + } +} + +/// Shared handler for the archive extract/create dialogs. They share identical +/// OK/Cancel navigation and a single text field; only the committed pending +/// action differs (selected by `is_extract`). +fn handle_archive_dialog( state: &mut AppState, running_job: &mut Option, key: KeyCode, + is_extract: bool, ) { - match key { - KeyCode::Esc => { - dismiss_dialog(state); - return; - } - KeyCode::Left | KeyCode::Right => { - state.input.dialog_selection = if state.input.dialog_selection == 0 { - 1 + match archive_dialog_nav(state, key) { + ArchiveNav::Dismissed => return, + ArchiveNav::Commit => { + if is_extract { + commit_archive_extract(state, running_job); } else { - 0 - }; - return; - } - KeyCode::Enter if state.input.dialog_selection == 1 => { - dismiss_dialog(state); - return; - } - KeyCode::Enter => { - let (sources, dest_text) = - if let AppMode::Dialog(DialogKind::ArchiveCreate(ref details)) = state.mode { - ( - details.sources.clone(), - details.dest_input.text().to_owned(), - ) - } else { - return; - }; - if dest_text.trim().is_empty() { - state.ui.status_message = Some("Archive path cannot be empty".to_string()); - return; + commit_archive_create(state, running_job); } - let dest = fs::path::resolve_user_path(state.active_panel().path(), &dest_text); - let Some(format) = archive_format_from_path(&dest) else { - state.ui.status_message = Some("Unsupported archive format. Use: zip, tar, tar.gz, tar.bz2, tar.xz, tar.zst, 7z".to_string()); - return; - }; - state.ui.pending_action = Some(PendingAction::CreateArchive { - sources, - dest, - format, - overwrite: false, - }); - dispatch_with_overwrite_check(state, running_job); return; } - _ => {} + ArchiveNav::Continue => {} } - if let AppMode::Dialog(DialogKind::ArchiveCreate(ref mut details)) = state.mode { - apply_dialog_text_edit(&mut details.dest_input, key); + if let Some(dest_input) = active_archive_dest_input(state) { + apply_dialog_text_edit(dest_input, key); } } @@ -646,13 +686,11 @@ fn handle_copymove_dialog( } } -pub(crate) fn handle_dialog( - state: &mut AppState, - viewer_state: &mut Option, - running_job: &mut Option, - key: KeyCode, - terminal_size: ratatui::layout::Size, -) { +pub(crate) fn handle_dialog(ctx: &mut EventContext, key: KeyCode) { + let terminal_size = ctx.term_size; + let state = &mut *ctx.state; + let viewer_state = &mut *ctx.viewer_state; + let running_job = &mut *ctx.running_job; if let AppMode::Dialog(DialogKind::Help { message, scroll_offset, @@ -698,20 +736,25 @@ pub(crate) fn handle_dialog( _ => false, }; if should_exit { - dismiss_dialog_and_restore(state); + reset_dialog_state(state); } return; } + // Match on a reference and dispatch by variant. Each handler re-borrows + // `state.mode` for the data it needs, so cloning the whole `DialogKind` + // (`Box`/`Vec`/`TextInput`) on every key press is avoided. Only the + // `Copy` `InputAction` is lifted out here before the borrow is released. let AppMode::Dialog(dk) = &state.mode else { return; }; - match dk.clone() { + match dk { DialogKind::Confirm(_) => { handle_confirm_dialog(state, running_job, key); } DialogKind::Input { action, .. } => { + let action = *action; handle_input_dialog( state, viewer_state, @@ -737,10 +780,10 @@ pub(crate) fn handle_dialog( handle_overwrite_dialog(state, running_job, key); } DialogKind::ArchiveExtract(..) => { - handle_archive_extract_dialog(state, running_job, key); + handle_archive_dialog(state, running_job, key, true); } DialogKind::ArchiveCreate(..) => { - handle_archive_create_dialog(state, running_job, key); + handle_archive_dialog(state, running_job, key, false); } // unreachable: Help handled above; arm kept for match exhaustiveness DialogKind::Help { .. } => {} diff --git a/src/input/directory_tree.rs b/src/input/directory_tree.rs index 4e0229e..ef99b74 100644 --- a/src/input/directory_tree.rs +++ b/src/input/directory_tree.rs @@ -3,16 +3,13 @@ use crossterm::event::KeyCode; use lc::app::{dir_tree, types::*}; use lc::ui::{DIR_TREE_OVERHEAD_ROWS, viewer}; +use super::EventContext; use crate::app::panel_ops::refresh_active; -pub(crate) fn handle_directory_tree( - state: &mut AppState, - _viewer_state: &mut Option, - viewer_loader: &mut Option, - key: KeyCode, - terminal_height: u16, -) { - let visible_height = directory_tree_visible_height(terminal_height); +pub(crate) fn handle_directory_tree(ctx: &mut EventContext, key: KeyCode) { + let visible_height = directory_tree_visible_height(ctx.term_size.height); + let state = &mut *ctx.state; + let viewer_loader = &mut *ctx.viewer_loader; match key { KeyCode::Esc => { state.mode = AppMode::Normal; @@ -84,11 +81,20 @@ pub(crate) fn set_tree_diagnostic_status( return; } - let items: Vec = diagnostics - .iter() - .map(|d| format!("{}: {}", d.path.display(), d.message)) - .collect(); - *status_message = Some(format!("Directory tree warnings: [{}]", items.join("] ["))); + // Build the message directly into a single buffer instead of allocating an + // intermediate Vec plus a join() pass (avoids N+1 allocations). + use std::fmt::Write as _; + + let mut buf = String::from("Directory tree warnings: ["); + for (i, d) in diagnostics.iter().enumerate() { + if i > 0 { + buf.push_str("] ["); + } + // Writing to a String is infallible; the Result is intentionally ignored. + let _ = write!(buf, "{}: {}", d.path.display(), d.message); + } + buf.push(']'); + *status_message = Some(buf); } fn handle_tree_enter(state: &mut AppState, viewer_loader: &mut Option) { @@ -144,6 +150,24 @@ mod tests { use super::*; use std::path::PathBuf; + /// Drive `handle_directory_tree` with a freshly built `EventContext` so the + /// per-test call-sites stay as terse as the old positional form. + fn tree_key(state: &mut AppState, key: KeyCode, height: u16) { + let mut viewer_state = None; + let mut viewer_loader = None; + let mut image_preview_loader = None; + let mut running_job = None; + let mut ctx = EventContext { + state, + viewer_state: &mut viewer_state, + viewer_loader: &mut viewer_loader, + image_preview_loader: &mut image_preview_loader, + running_job: &mut running_job, + term_size: ratatui::layout::Size::new(80, height), + }; + handle_directory_tree(&mut ctx, key); + } + fn make_tree_entries(count: usize) -> Vec { (0..count) .map(|i| { @@ -167,7 +191,7 @@ mod tests { let mut state = AppState::default(); state.mode = AppMode::DirectoryTree; state.tree.entries = make_tree_entries(10); - handle_directory_tree(&mut state, &mut None, &mut None, KeyCode::Esc, 24); + tree_key(&mut state, KeyCode::Esc, 24); assert_eq!(state.mode, AppMode::Normal); } @@ -176,7 +200,7 @@ mod tests { let mut state = AppState::default(); state.mode = AppMode::DirectoryTree; state.tree.entries = make_tree_entries(10); - handle_directory_tree(&mut state, &mut None, &mut None, KeyCode::Up, 24); + tree_key(&mut state, KeyCode::Up, 24); assert_eq!(state.tree.selected, 0); } @@ -185,7 +209,7 @@ mod tests { let mut state = AppState::default(); state.mode = AppMode::DirectoryTree; state.tree.entries = make_tree_entries(10); - handle_directory_tree(&mut state, &mut None, &mut None, KeyCode::Down, 24); + tree_key(&mut state, KeyCode::Down, 24); assert_eq!(state.tree.selected, 1); } @@ -195,7 +219,7 @@ mod tests { state.mode = AppMode::DirectoryTree; state.tree.entries = make_tree_entries(5); state.tree.selected = 4; - handle_directory_tree(&mut state, &mut None, &mut None, KeyCode::Down, 24); + tree_key(&mut state, KeyCode::Down, 24); assert_eq!(state.tree.selected, 4); } @@ -206,7 +230,7 @@ mod tests { state.tree.entries = make_tree_entries(50); state.tree.selected = 25; state.tree.scroll = 20; - handle_directory_tree(&mut state, &mut None, &mut None, KeyCode::Home, 24); + tree_key(&mut state, KeyCode::Home, 24); assert_eq!(state.tree.selected, 0); assert_eq!(state.tree.scroll, 0); } @@ -216,7 +240,7 @@ mod tests { let mut state = AppState::default(); state.mode = AppMode::DirectoryTree; state.tree.entries = make_tree_entries(50); - handle_directory_tree(&mut state, &mut None, &mut None, KeyCode::End, 24); + tree_key(&mut state, KeyCode::End, 24); assert_eq!(state.tree.selected, 49); } @@ -225,11 +249,11 @@ mod tests { let mut state = AppState::default(); state.mode = AppMode::DirectoryTree; state.tree.entries = vec![]; - handle_directory_tree(&mut state, &mut None, &mut None, KeyCode::Down, 24); + tree_key(&mut state, KeyCode::Down, 24); assert_eq!(state.tree.selected, 0); - handle_directory_tree(&mut state, &mut None, &mut None, KeyCode::End, 24); + tree_key(&mut state, KeyCode::End, 24); assert_eq!(state.tree.selected, 0); - handle_directory_tree(&mut state, &mut None, &mut None, KeyCode::Enter, 24); + tree_key(&mut state, KeyCode::Enter, 24); } #[test] diff --git a/src/input/menu_actions.rs b/src/input/menu_actions.rs index 7165a8e..1fc7cfd 100644 --- a/src/input/menu_actions.rs +++ b/src/input/menu_actions.rs @@ -1,6 +1,6 @@ -use crossterm::event::{KeyCode, KeyModifiers}; +use std::fmt::Write as _; -const TREE_INITIAL_EXPAND_DEPTH: usize = 2; +use crossterm::event::{KeyCode, KeyModifiers}; use lc::app::user_menu::MenuSource; use lc::app::{config, dir_tree, types::*, user_menu}; @@ -10,6 +10,52 @@ use lc::ops; use super::directory_tree::set_tree_diagnostic_status; use crate::app::panel_ops::{current_visible_height, rebuild_visible_entries, with_menu_panel}; +const TREE_INITIAL_EXPAND_DEPTH: usize = 2; + +/// Mask for the permission bits (`setuid`/`setgid`/`sticky` + rwx triplets) +/// extracted from a raw `st_mode` value when prefilling the chmod dialog. +const PERMISSION_MASK: u32 = 0o7777; + +/// Status-bar labels for the two listing layouts. +const LISTING_MODE_LONG_LABEL: &str = "Long"; +const LISTING_MODE_BRIEF_LABEL: &str = "Brief"; + +/// Status-bar labels for a toggled boolean flag (e.g. permission column). +const FLAG_ON_LABEL: &str = "ON"; +const FLAG_OFF_LABEL: &str = "OFF"; + +/// Returns the listing layout that toggling `mode` switches to. +/// +/// Models the binary `Long`/`Brief` switch as a total function over the type, +/// so the compiler enforces exhaustiveness if a third variant is ever added. +/// (Lives here rather than as a `ListingMode` method because that enum is owned +/// by `app::types::sorting`; promote to an inherent method when touching it.) +fn toggle_listing_mode(mode: ListingMode) -> ListingMode { + match mode { + ListingMode::Long => ListingMode::Brief, + ListingMode::Brief => ListingMode::Long, + } +} + +/// Human-readable status-bar label for a listing layout. +fn listing_mode_label(mode: ListingMode) -> &'static str { + match mode { + ListingMode::Long => LISTING_MODE_LONG_LABEL, + ListingMode::Brief => LISTING_MODE_BRIEF_LABEL, + } +} + +/// Enters a list-picker mode with a freshly reset selection cursor. +fn enter_picker(state: &mut AppState, kind: PickerKind) { + state.ui.picker_selected = 0; + state.mode = AppMode::ListPicker(kind); +} + +/// Name of the panel's current entry, cloned if one is selected. +fn current_entry_name(panel: &PanelState) -> Option { + panel.current_entry().map(|e| e.name.clone()) +} + pub fn execute_menu_action(state: &mut AppState) -> Option<(KeyCode, KeyModifiers, bool)> { let action = menu_action_at(state.ui.menu_selected, state.ui.menu_item_selected)?; match action { @@ -87,15 +133,9 @@ fn execute_panel_config_action( MenuAction::ToggleListingMode => { with_menu_panel(state, |state| { let panel = state.active_panel_mut(); - let new_mode = match panel.listing_mode() { - ListingMode::Long => ListingMode::Brief, - ListingMode::Brief => ListingMode::Long, - }; + let new_mode = toggle_listing_mode(panel.listing_mode()); panel.set_listing_mode(new_mode); - let label = match new_mode { - ListingMode::Long => "Long", - ListingMode::Brief => "Brief", - }; + let label = listing_mode_label(new_mode); state.ui.status_message = Some(format!("Layout changed to {label}")); }); None @@ -138,8 +178,8 @@ fn execute_panel_config_action( let panel = state.active_panel_mut(); let show = !panel.show_permissions(); panel.set_show_permissions(show); - state.ui.status_message = - Some(format!("Permissions: {}", if show { "ON" } else { "OFF" })); + let flag_label = if show { FLAG_ON_LABEL } else { FLAG_OFF_LABEL }; + state.ui.status_message = Some(format!("Permissions: {flag_label}")); }); None } @@ -179,18 +219,15 @@ fn execute_nav_action( None } MenuAction::CompareDirs => { - state.ui.picker_selected = 0; - state.mode = AppMode::ListPicker(PickerKind::CompareMode); + enter_picker(state, PickerKind::CompareMode); None } MenuAction::History => { - state.ui.picker_selected = 0; - state.mode = AppMode::ListPicker(PickerKind::History); + enter_picker(state, PickerKind::History); None } MenuAction::DirectoryHotlist => { - state.ui.picker_selected = 0; - state.mode = AppMode::ListPicker(PickerKind::Hotlist); + enter_picker(state, PickerKind::Hotlist); None } MenuAction::CommandLine => { @@ -208,7 +245,7 @@ fn execute_dialog_action( match action { MenuAction::Rename => { with_menu_panel(state, |state| { - let entry_name = state.active_panel().current_entry().map(|e| e.name.clone()); + let entry_name = current_entry_name(state.active_panel()); if let Some(name) = entry_name && name != ".." { @@ -233,7 +270,7 @@ fn execute_dialog_action( state .input .dialog_input - .set_text_at_end(format!("{:o}", permissions & 0o7777)); + .set_text_at_end(format!("{:o}", permissions & PERMISSION_MASK)); state.mode = AppMode::Dialog(DialogKind::Input { prompt: "Chmod (octal):".to_string(), action: InputAction::Chmod, @@ -248,26 +285,17 @@ fn execute_dialog_action( pub fn open_user_menu(state: &mut AppState) { let panel_dir = state.active_panel().path().to_path_buf(); - let current_file = state - .active_panel() - .current_entry() - .map(|e| e.name.clone()) - .unwrap_or_default(); + let current_file = current_entry_name(state.active_panel()).unwrap_or_default(); match user_menu::load_menu_with_warnings(&panel_dir, ¤t_file) { Ok(loaded) if loaded.entries.is_empty() => { - let message = if loaded.warnings.is_empty() { - "No matching menu entries found.".to_string() - } else { - format!( - "No matching menu entries found.\n{}", - loaded - .warnings - .iter() - .map(|warning| format!("Line {}: {}", warning.line, warning.message)) - .collect::>() - .join("\n") - ) - }; + let message = loaded.warnings.iter().fold( + "No matching menu entries found.".to_string(), + |mut acc, warning| { + // Infallible: writing into a String never errors. + let _ = write!(acc, "\nLine {}: {}", warning.line, warning.message); + acc + }, + ); state.mode = AppMode::Dialog(DialogKind::Error(message)); } Ok(loaded) => { diff --git a/src/input/mod.rs b/src/input/mod.rs index f908066..9042673 100644 --- a/src/input/mod.rs +++ b/src/input/mod.rs @@ -1,8 +1,33 @@ -pub mod command_line; -pub mod dialogs; -pub mod directory_tree; -pub mod menu_actions; -pub mod mode_dispatch; -pub mod mouse; -pub mod normal; -pub mod pickers; +pub(crate) mod command_line; +pub(crate) mod dialogs; +pub(crate) mod directory_tree; +pub(crate) mod menu_actions; +pub(crate) mod mode_dispatch; +pub(crate) mod mouse; +pub(crate) mod normal; +pub(crate) mod pickers; + +use ratatui::layout::Size; + +use lc::app::job_runner::RunningJob; +use lc::app::types::AppState; +use lc::ui::viewer; + +/// Aggregates the long-lived, per-frame mutable state that the event-dispatch +/// layer threads through the key/mouse handlers. Replaces the 7-8 argument +/// parameter lists that `dispatch_event` and friends used to carry by hand. +/// +/// Deliberately *not* generic over the terminal backend: the `Terminal` is +/// passed as a separate argument only to the handlers that actually need it +/// (normal/menu mode spawning an external editor), so the backend type never +/// leaks into this struct and infects the whole call graph with a `B` parameter. +/// Per-event data (key code/modifiers, mouse event) is likewise kept out of the +/// struct and passed separately. +pub(crate) struct EventContext<'a> { + pub state: &'a mut AppState, + pub viewer_state: &'a mut Option, + pub viewer_loader: &'a mut Option, + pub image_preview_loader: &'a mut Option, + pub running_job: &'a mut Option, + pub term_size: Size, +} diff --git a/src/input/mode_dispatch.rs b/src/input/mode_dispatch.rs index adb06a3..b7edb1d 100644 --- a/src/input/mode_dispatch.rs +++ b/src/input/mode_dispatch.rs @@ -1,11 +1,10 @@ use crossterm::event::{KeyCode, KeyModifiers}; -use ratatui::prelude::*; use lc::app::panel_ops; use lc::app::types::{AppMode, AppState, DialogKind, InputAction}; use lc::menu::{MENUS, menu_item_count}; -use lc::ui::viewer; +use super::EventContext; use crate::{ handle_alt_keys, handle_ctrl_keys, handle_enter_key, handle_function_keys, handle_navigation_keys, @@ -14,6 +13,12 @@ use crate::{ const VIEWER_CHROME_HEIGHT: u16 = 3; const HORIZONTAL_SCROLL_STEP: usize = 4; +/// Keys that close the file viewer (both the loading and loaded states). +/// Single source of truth so the loader and viewer-state branches can't drift. +fn is_viewer_exit_key(key: KeyCode) -> bool { + matches!(key, KeyCode::Esc | KeyCode::F(3 | 10) | KeyCode::Char('q')) +} + fn refresh_or_rebuild(state: &mut AppState, visible_height: usize) { let needs_refresh = { let panel = state.active_panel(); @@ -34,13 +39,13 @@ pub(crate) fn clear_search_state(state: &mut AppState, visible_height: usize) { refresh_or_rebuild(state, visible_height); } -fn set_active_panel_filter(state: &mut AppState, filter: String) { - state.active_panel_mut().set_filter(Some(filter)); -} - +/// Applies the current `search_query` as the active panel's filter and +/// refreshes/rebuilds the visible entries. Shared by `initiate_search` and +/// `handle_search_mode` so the clone-query → set-filter → refresh sequence +/// lives in one place. fn apply_search_filter(state: &mut AppState, visible: usize) { let filter_query = state.input.search_query.clone(); - set_active_panel_filter(state, filter_query); + state.active_panel_mut().set_filter(Some(filter_query)); refresh_or_rebuild(state, visible); } @@ -53,25 +58,22 @@ pub(crate) fn initiate_search( state.prev_mode = Some(prev_mode); state.input.search_query.push(c); state.input.search_cursor = state.input.search_query.len(); - let filter_query = state.input.search_query.clone(); state.mode = AppMode::Search; - set_active_panel_filter(state, filter_query); - refresh_or_rebuild(state, visible_height); + apply_search_filter(state, visible_height); } pub(crate) fn handle_normal_mode( - state: &mut AppState, - _viewer_state: &mut Option, - viewer_loader: &mut Option, + ctx: &mut EventContext, key: KeyCode, modifiers: KeyModifiers, - terminal_height: u16, terminal: &mut ratatui::Terminal, ) { + let terminal_height = ctx.term_size.height; let visible = panel_ops::panel_visible_height(terminal_height); + let state = &mut *ctx.state; match key { KeyCode::F(_) => { - handle_function_keys(state, viewer_loader, key, terminal); + handle_function_keys(state, ctx.viewer_loader, key, terminal); } KeyCode::Up | KeyCode::Down @@ -86,7 +88,7 @@ pub(crate) fn handle_normal_mode( handle_navigation_keys(state, key, modifiers, visible); } KeyCode::Enter if !modifiers.contains(KeyModifiers::ALT) => { - handle_enter_key(state, viewer_loader, visible, terminal); + handle_enter_key(state, ctx.viewer_loader, visible, terminal); } KeyCode::Char('u' | 's' | 'h' | 'r' | 'o') if modifiers.contains(KeyModifiers::CONTROL) => { handle_ctrl_keys(state, key, terminal_height); @@ -106,33 +108,28 @@ pub(crate) fn handle_normal_mode( } } -pub(crate) fn handle_viewer_mode( - state: &mut AppState, - viewer_state: &mut Option, - viewer_loader: &mut Option, - image_preview_loader: &mut Option, - key: KeyCode, - terminal_size: Size, -) { - if viewer_loader.is_some() { - if matches!(key, KeyCode::Esc | KeyCode::F(3 | 10) | KeyCode::Char('q')) { - viewer_loader.take(); - *image_preview_loader = None; +pub(crate) fn handle_viewer_mode(ctx: &mut EventContext, key: KeyCode) { + let terminal_size = ctx.term_size; + let state = &mut *ctx.state; + if ctx.viewer_loader.is_some() { + if is_viewer_exit_key(key) { + ctx.viewer_loader.take(); + *ctx.image_preview_loader = None; state.restore_prev_mode(); - *viewer_state = None; + *ctx.viewer_state = None; } return; } - if let Some(vs) = viewer_state.as_mut() { + if let Some(vs) = ctx.viewer_state.as_mut() { let page_height = terminal_size.height.saturating_sub(VIEWER_CHROME_HEIGHT) as usize; let content_width = terminal_size.width as usize; vs.update_wrap_layout(content_width); vs.clamp_scroll(); match key { - KeyCode::Esc | KeyCode::F(3 | 10) | KeyCode::Char('q') => { - *image_preview_loader = None; + _ if is_viewer_exit_key(key) => { + *ctx.image_preview_loader = None; state.restore_prev_mode(); - *viewer_state = None; + *ctx.viewer_state = None; } KeyCode::Up | KeyCode::Char('k') => vs.scroll_up(1), KeyCode::Down | KeyCode::Char('j') => vs.scroll_down(1), @@ -151,7 +148,7 @@ pub(crate) fn handle_viewer_mode( state .input .dialog_input - .set_text_at_end(vs.search_query.as_deref().unwrap_or("").to_owned()); + .set_text_at_end(vs.search_query.clone().unwrap_or_default()); state.mode = AppMode::Dialog(DialogKind::Input { prompt: "Find in viewer:".to_string(), action: InputAction::ViewerSearch, @@ -160,7 +157,7 @@ pub(crate) fn handle_viewer_mode( _ => {} } } else { - *image_preview_loader = None; + *ctx.image_preview_loader = None; state.mode = AppMode::Normal; } } @@ -193,93 +190,86 @@ pub(crate) fn handle_search_mode(state: &mut AppState, key: KeyCode, terminal_he } pub(crate) fn run_selected_menu_action( - state: &mut AppState, - viewer_state: &mut Option, - viewer_loader: &mut Option, - terminal_height: u16, + ctx: &mut EventContext, terminal: &mut ratatui::Terminal, ) { - let prev = state.mode.clone(); - if let Some((key, modifiers, for_menu_panel)) = super::menu_actions::execute_menu_action(state) - { - state.mode = AppMode::Normal; - if for_menu_panel { - panel_ops::with_menu_panel(state, |state| { - handle_normal_mode( - state, - viewer_state, - viewer_loader, - key, - modifiers, - terminal_height, - terminal, - ); - }); - } else { - handle_normal_mode( + let prev = ctx.state.mode.clone(); + let Some((key, modifiers, for_menu_panel)) = + super::menu_actions::execute_menu_action(ctx.state) + else { + if ctx.state.mode == prev { + ctx.state.restore_prev_mode(); + } + return; + }; + ctx.state.mode = AppMode::Normal; + if for_menu_panel { + // `with_menu_panel` must own `&mut state` for the duration of the + // closure (it flips the active panel before/after). The other context + // fields are disjoint, so we reborrow them into a fresh `EventContext` + // bound to the closure's `state` rather than passing `ctx` (whose + // `state` field is moved into `with_menu_panel`). + let viewer_state = &mut *ctx.viewer_state; + let viewer_loader = &mut *ctx.viewer_loader; + let image_preview_loader = &mut *ctx.image_preview_loader; + let running_job = &mut *ctx.running_job; + let term_size = ctx.term_size; + panel_ops::with_menu_panel(ctx.state, |state| { + let mut inner = EventContext { state, viewer_state, viewer_loader, - key, - modifiers, - terminal_height, - terminal, - ); - } - } else if state.mode == prev { - state.restore_prev_mode(); + image_preview_loader, + running_job, + term_size, + }; + handle_normal_mode(&mut inner, key, modifiers, terminal); + }); + } else { + handle_normal_mode(ctx, key, modifiers, terminal); } } pub(crate) fn handle_menu_mode( - state: &mut AppState, - viewer_state: &mut Option, - viewer_loader: &mut Option, + ctx: &mut EventContext, key: KeyCode, - terminal_height: u16, terminal: &mut ratatui::Terminal, ) { let total = MENUS.len(); - let max_items = menu_item_count(state.ui.menu_selected); + let max_items = menu_item_count(ctx.state.ui.menu_selected); if max_items == 0 { - state.mode = AppMode::Normal; + ctx.state.mode = AppMode::Normal; return; } match key { KeyCode::Esc | KeyCode::F(9 | 10) => { - state.restore_prev_mode(); + ctx.state.restore_prev_mode(); } KeyCode::Left => { - state.ui.menu_selected = if state.ui.menu_selected == 0 { + ctx.state.ui.menu_selected = if ctx.state.ui.menu_selected == 0 { total - 1 } else { - state.ui.menu_selected - 1 + ctx.state.ui.menu_selected - 1 }; - state.ui.menu_item_selected = 0; + ctx.state.ui.menu_item_selected = 0; } KeyCode::Right => { - state.ui.menu_selected = (state.ui.menu_selected + 1) % total; - state.ui.menu_item_selected = 0; + ctx.state.ui.menu_selected = (ctx.state.ui.menu_selected + 1) % total; + ctx.state.ui.menu_item_selected = 0; } KeyCode::Up => { - state.ui.menu_item_selected = if state.ui.menu_item_selected == 0 { + ctx.state.ui.menu_item_selected = if ctx.state.ui.menu_item_selected == 0 { max_items - 1 } else { - state.ui.menu_item_selected - 1 + ctx.state.ui.menu_item_selected - 1 }; } KeyCode::Down => { - state.ui.menu_item_selected = (state.ui.menu_item_selected + 1) % max_items; + ctx.state.ui.menu_item_selected = (ctx.state.ui.menu_item_selected + 1) % max_items; } KeyCode::Enter => { - run_selected_menu_action( - state, - viewer_state, - viewer_loader, - terminal_height, - terminal, - ); + run_selected_menu_action(ctx, terminal); } _ => {} } diff --git a/src/input/mouse.rs b/src/input/mouse.rs index 5b6bed3..21faeb5 100644 --- a/src/input/mouse.rs +++ b/src/input/mouse.rs @@ -12,6 +12,7 @@ use crate::menu::{MENUS, menu_dropdown_x, menu_title_width, menu_title_x}; use crate::ui::dialogs; use crate::ui::viewer; +use super::EventContext; use super::dialogs::{check_overwrite_conflict, dismiss_dialog, finish_confirmed_action}; use crate::app::panel_ops::{refresh_active, refresh_both, refresh_panel}; @@ -20,6 +21,22 @@ const DOUBLE_CLICK_THRESHOLD_MS: u64 = 300; const ARCHIVE_EXTRACT_INPUT_ROW_OFFSET: u16 = 3; const ARCHIVE_CREATE_INPUT_ROW_OFFSET: u16 = 4; +/// Fallback dropdown width (in cells) when a menu has no items to measure. +const DEFAULT_DROPDOWN_WIDTH: usize = 10; +/// Chrome rows reserved by a panel's border + header before the file rows +/// (top border, header row, bottom border, function bar). Used by +/// [`panel_bounds`] to derive the file-row range from the terminal height. +const PANEL_CHROME_ROWS: u16 = 4; +/// Minimum terminal height that can host at least one panel file row. Below +/// this, [`panel_bounds`] yields an empty range and clicks/scrolls in the panel +/// body are no-ops (see [`in_panel_file_rows`]). +const MIN_PANEL_HEIGHT: u16 = PANEL_CHROME_ROWS + 1; +/// The function bar is split into 10 equal-width F-key buttons (F1..=F10). +const FUNCTION_BAR_BUTTONS: u32 = 10; +/// Highest selectable function-bar button index (`FUNCTION_BAR_BUTTONS - 1`). +const FUNCTION_BAR_MAX_INDEX: u32 = FUNCTION_BAR_BUTTONS - 1; + +#[derive(Debug)] pub enum MouseOutcome { Consumed, NormalKey(KeyCode), @@ -33,14 +50,15 @@ pub(crate) struct MousePosition { pub(crate) height: u16, } -pub fn handle_mouse_event( - state: &mut AppState, - viewer_state: &mut Option, - viewer_loader: &mut Option, - running_job: &mut Option, +pub(crate) fn handle_mouse_event( + ctx: &mut EventContext, mouse_event: crossterm::event::MouseEvent, - terminal_size: ratatui::layout::Size, ) -> Option { + let terminal_size = ctx.term_size; + let state = &mut *ctx.state; + let viewer_state = &mut *ctx.viewer_state; + let viewer_loader = &mut *ctx.viewer_loader; + let running_job = &mut *ctx.running_job; let pos = MousePosition { col: mouse_event.column, row: mouse_event.row, @@ -108,8 +126,7 @@ fn handle_middle_down(state: &mut AppState, pos: &MousePosition) -> Option Option u16 { + width / 2 +} + +/// File-row range (`start`, `end`) for a terminal of `height`. The interior +/// file rows are those strictly between `start` and `end` (see +/// [`in_panel_file_rows`]). +/// +/// On a terminal too short to host any file row (`height < MIN_PANEL_HEIGHT`) +/// this returns an empty `(1, 1)` range so callers treat the panel body as +/// having no rows, rather than relying on `saturating_sub` underflowing to 0 +/// and producing a malformed `(1, 0)` range. fn panel_bounds(height: u16) -> (u16, u16) { - (1u16, height.saturating_sub(4)) + if height < MIN_PANEL_HEIGHT { + return (1, 1); + } + (1u16, height - PANEL_CHROME_ROWS) +} + +/// Inclusive panel row span (`end - start + 1`) for the file-list area. +fn panel_height(start: u16, end: u16) -> u16 { + end.saturating_sub(start) + 1 +} + +/// Number of visible file rows in the list area: the panel height minus the +/// two rows of chrome (header + bottom border) that frame the scrollable body. +/// Centralized here so the scroll and click paths cannot drift apart. +fn visible_rows(start: u16, end: u16) -> usize { + panel_height(start, end).saturating_sub(2) as usize } fn in_panel_file_rows(pos: &MousePosition) -> bool { @@ -192,10 +237,8 @@ fn handle_mouse_scroll( return; } let (panel_start_row, panel_end_row) = panel_bounds(pos.height); - let panel_height = panel_end_row.saturating_sub(panel_start_row) + 1; - let visible_rows = panel_height.saturating_sub(2) as usize; - let mid_col = pos.width / 2; - if pos.col < mid_col { + let visible = visible_rows(panel_start_row, panel_end_row); + if pos.col < mid_col(pos.width) { state.active_panel = ActivePanel::Left; } else { state.active_panel = ActivePanel::Right; @@ -205,7 +248,7 @@ fn handle_mouse_scroll( match kind { MouseEventKind::ScrollUp => { panel.cursor = panel.cursor.saturating_sub(SCROLL_LINES); - panel.ensure_cursor_visible(visible_rows); + panel.ensure_cursor_visible(visible); } MouseEventKind::ScrollDown => { if panel.cursor + SCROLL_LINES < len { @@ -213,7 +256,7 @@ fn handle_mouse_scroll( } else { panel.cursor = len.saturating_sub(1); } - panel.ensure_cursor_visible(visible_rows); + panel.ensure_cursor_visible(visible); } _ => {} } @@ -236,45 +279,50 @@ fn handle_mouse_dialog( running_job: &mut Option, pos: &MousePosition, ) -> Option { - if let AppMode::Dialog(DialogKind::Progress { .. }) = state.mode { - return handle_progress_click(state, running_job, pos); - } - - if let AppMode::Dialog(DialogKind::Input { .. }) = state.mode { - return Some(MouseOutcome::Consumed); - } - - if let AppMode::Dialog(DialogKind::ArchiveExtract(ref mut details)) = state.mode { - position_text_input_cursor( - &mut details.dest_input, - pos, - archive_input_rect(pos, ARCHIVE_EXTRACT_INPUT_ROW_OFFSET), - ); - return Some(MouseOutcome::Consumed); - } - - if let AppMode::Dialog(DialogKind::ArchiveCreate(ref mut details)) = state.mode { - position_text_input_cursor( - &mut details.dest_input, - pos, - archive_input_rect(pos, ARCHIVE_CREATE_INPUT_ROW_OFFSET), - ); - return Some(MouseOutcome::Consumed); - } + // Dispatch on the active dialog kind. The two archive variants mutate their + // boxed `details` in place, so they are handled inside this borrow of + // `state.mode`; the click-handler variants (Progress/Confirm/Overwrite) need + // a fresh `&mut AppState`, so they delegate *after* the match releases the + // borrow. Any other dialog swallows the click without acting on it. + let AppMode::Dialog(kind) = &mut state.mode else { + return None; + }; - if let AppMode::Dialog(DialogKind::Confirm(_)) = state.mode { - return handle_confirm_click(state, running_job, pos); + enum Delegate { + Progress, + Confirm, + Overwrite, } - if let AppMode::Dialog(DialogKind::OverwriteConfirm(..)) = state.mode { - return handle_overwrite_click(state, running_job, pos); - } + let delegate = match kind { + DialogKind::ArchiveExtract(details) => { + position_text_input_cursor( + &mut details.dest_input, + pos, + archive_input_rect(pos, ARCHIVE_EXTRACT_INPUT_ROW_OFFSET), + ); + return Some(MouseOutcome::Consumed); + } + DialogKind::ArchiveCreate(details) => { + position_text_input_cursor( + &mut details.dest_input, + pos, + archive_input_rect(pos, ARCHIVE_CREATE_INPUT_ROW_OFFSET), + ); + return Some(MouseOutcome::Consumed); + } + DialogKind::Progress { .. } => Delegate::Progress, + DialogKind::Confirm(_) => Delegate::Confirm, + DialogKind::OverwriteConfirm(..) => Delegate::Overwrite, + // Input and every other dialog: consume the click, change nothing. + _ => return Some(MouseOutcome::Consumed), + }; - if let AppMode::Dialog(_) = state.mode { - return Some(MouseOutcome::Consumed); + match delegate { + Delegate::Progress => handle_progress_click(state, running_job, pos), + Delegate::Confirm => handle_confirm_click(state, running_job, pos), + Delegate::Overwrite => handle_overwrite_click(state, running_job, pos), } - - None } fn archive_input_rect(pos: &MousePosition, row_offset: u16) -> Rect { @@ -475,7 +523,7 @@ fn handle_mouse_menu_dropdown(state: &mut AppState, pos: &MousePosition) -> Opti .iter() .map(|s| UnicodeWidthStr::width(*s)) .max() - .unwrap_or(10) as u16 + .unwrap_or(DEFAULT_DROPDOWN_WIDTH) as u16 + 4; let menu_bar_area = Rect::new(0, 0, pos.width, 1); let dropdown_x = menu_dropdown_x(menu_bar_area, state.ui.menu_selected, dropdown_width); @@ -485,7 +533,10 @@ fn handle_mouse_menu_dropdown(state: &mut AppState, pos: &MousePosition) -> Opti let inner_width = dropdown_width.saturating_sub(2); let max_visible = pos.height.saturating_sub(1); - let dropdown_height = ((items.len().min(u16::MAX as usize - 2)) as u16 + 2).min(max_visible); + // Clamp the item count so `+ 2` (the dropdown's top/bottom border) cannot + // overflow `u16` before the `.min(max_visible)` clamp. + const MAX_DROPDOWN_ITEMS: usize = u16::MAX as usize - 2; + let dropdown_height = ((items.len().min(MAX_DROPDOWN_ITEMS)) as u16 + 2).min(max_visible); let visible_items = dropdown_height.saturating_sub(2) as usize; let clamped_selected = state .ui @@ -521,7 +572,8 @@ fn handle_mouse_function_bar(state: &mut AppState, pos: &MousePosition) -> Optio if pos.width == 0 { return Some(MouseOutcome::Consumed); } - let btn_idx = (u32::from(pos.col) * 10 / u32::from(pos.width)).min(9) as u16; + let btn_idx = (u32::from(pos.col) * FUNCTION_BAR_BUTTONS / u32::from(pos.width)) + .min(FUNCTION_BAR_MAX_INDEX) as u16; let fkey = match btn_idx { 0 => KeyCode::F(1), 1 => KeyCode::F(2), @@ -553,9 +605,8 @@ fn handle_mouse_panels( } let (panel_start_row, panel_end_row) = panel_bounds(pos.height); - let panel_height = panel_end_row.saturating_sub(panel_start_row) + 1; - let mid_col = pos.width / 2; - let clicked_left = pos.col < mid_col; + let panel_rows = panel_height(panel_start_row, panel_end_row); + let clicked_left = pos.col < mid_col(pos.width); if clicked_left { state.active_panel = ActivePanel::Left; @@ -607,7 +658,12 @@ fn handle_mouse_panels( panel_mut.set_path(path); panel_mut.cursor = 0; panel_mut.scroll_offset = 0; - refresh_panel(panel_mut, panel_height as usize); + // Contract: `refresh_panel` takes the *full* inclusive panel height + // (header + body + bottom border) because it recomputes the whole + // listing layout, whereas `ensure_cursor_visible` below takes only + // the count of scrollable body rows (`visible_rows`). The two are + // intentionally different units; keep them in sync via the helpers. + refresh_panel(panel_mut, panel_rows as usize); } else { *viewer_loader = Some(viewer::ViewerState::open_background(path)); state.prev_mode = Some(state.mode.clone()); @@ -619,7 +675,7 @@ fn handle_mouse_panels( let panel_mut = state.active_panel_mut(); panel_mut.cursor = clicked_index; - panel_mut.ensure_cursor_visible(panel_height.saturating_sub(2) as usize); + panel_mut.ensure_cursor_visible(visible_rows(panel_start_row, panel_end_row)); } } @@ -638,8 +694,7 @@ fn handle_mouse_drag(state: &mut AppState, pos: &MousePosition) { None => return, }; - let mid_col = pos.width / 2; - let clicked_left = pos.col < mid_col; + let clicked_left = pos.col < mid_col(pos.width); let same_panel = clicked_left == matches!(state.active_panel, ActivePanel::Left); if !same_panel { return; @@ -662,13 +717,29 @@ fn handle_mouse_drag(state: &mut AppState, pos: &MousePosition) { let panel_mut = state.active_panel_mut(); let start = anchor.min(current_index); let end = anchor.max(current_index); - panel_mut.clear_selection(); - for i in start..=end { - panel_mut.set_selection_at(i, true); - } + set_selection_range(panel_mut, start..=end); panel_mut.cursor = current_index; - let visible_rows = (panel_end_row - panel_start_row).saturating_sub(1) as usize; - panel_mut.ensure_cursor_visible(visible_rows); + panel_mut.ensure_cursor_visible(visible_rows(panel_start_row, panel_end_row)); +} + +/// Replace the panel's selection with exactly the filtered indices in `range`. +/// +/// Clears any prior selection, then selects `range` in a single pass. Kept as a +/// dedicated helper so the drag path has one well-named entry point and the +/// clear-then-select sequence cannot be reordered by accident. +/// +/// Note: it still routes each index through [`PanelState::set_selection_at`], +/// which maps the filtered index to the backing store per call. A true +/// single-scan batch would need a `PanelState`-side method with access to the +/// private filtered-index table; that lives outside this module. +fn set_selection_range( + panel: &mut crate::app::types::PanelState, + range: std::ops::RangeInclusive, +) { + panel.clear_selection(); + for i in range { + panel.set_selection_at(i, true); + } } fn handle_mouse_up(state: &mut AppState) { diff --git a/src/input/mouse/tests.rs b/src/input/mouse/tests.rs index 2ac6375..c527e26 100644 --- a/src/input/mouse/tests.rs +++ b/src/input/mouse/tests.rs @@ -1,4 +1,9 @@ -#![allow(clippy::unwrap_used)] +// `unwrap_used`/`expect_used` are allowed for this whole module because it is a +// `#[cfg(test)]` `mod tests` (AGENTS.md permits the allow only on test modules). +// The unwraps are all on `TempDir`/filesystem setup or on building a known-valid +// `FileEntry`, where a failure means the test environment is broken and +// panicking with a backtrace is the right outcome. +#![allow(clippy::unwrap_used, clippy::expect_used)] use super::*; use crate::app::types::{ @@ -58,36 +63,21 @@ fn setup_copy_dirs() -> TestDirs { } } +/// A regular-file `FileEntry` at `/{name}`. Built via the production +/// `FileEntry::builder()` rather than a hand-rolled struct literal so it tracks +/// the real field set. +/// +/// (The richer `TestEntry` builder lives behind `#[cfg(test)]` in the *library* +/// crate and is not reachable from these binary-side unit tests, so the plain +/// `FileEntry::builder()` is used here.) fn mk_entry(name: &str) -> crate::app::types::FileEntry { - use std::sync::Arc; - crate::app::types::FileEntry { - name: name.to_string(), - path: std::path::PathBuf::from(format!("/{}", name)), - cha: crate::fs::cha::Cha { - kind: crate::fs::cha::ChaKind::empty(), - mode: crate::fs::cha::ChaMode::new(0o100644), - len: 0, - mtime: None, - btime: None, - ctime: None, - atime: None, - uid: 0, - gid: 0, - dev: 0, - nlink: 0, - }, - owner: Arc::from(""), - group: Arc::from(""), - selected: false, - mime_type: None, - time_str: String::new(), - size_str: String::new(), - name_width: unicode_width::UnicodeWidthStr::width(name), - size_width: 0, - time_width: 0, - category: crate::app::types::FileCategory::Other, - sanitized_name: None, - } + crate::app::types::FileEntry::builder() + .name(name) + .path(std::path::PathBuf::from(format!("/{name}"))) + .is_dir(false) + .size(0) + .build() + .expect("valid test file entry") } #[test] @@ -220,117 +210,118 @@ fn mouse_function_bar_zero_width_does_not_panic() { assert!(matches!(outcomes, Some(MouseOutcome::Consumed))); } -#[test] -fn mouse_error_dialog_click_does_not_dismiss() { - let mut state = AppState { - mode: AppMode::Dialog(DialogKind::Error("error".to_string())), - ..Default::default() - }; - let mut running_job = None; - let outcomes = handle_mouse_dialog(&mut state, &mut running_job, &mp(1, 1, 80, 24)); - assert!(outcomes.is_some()); - assert!(matches!(state.mode, AppMode::Dialog(DialogKind::Error(_)))); -} - -#[test] -fn mouse_properties_dialog_click_does_not_dismiss() { - let mut state = AppState { - mode: AppMode::Dialog(DialogKind::Properties(Box::new(PropertiesDetails { - name: "file.txt".to_string(), - size: 0, - mtime: std::time::SystemTime::UNIX_EPOCH, - permissions: 0o644, - owner: String::new(), - group: String::new(), - kind: FileKind::from_metadata_flags(false, false), - }))), - ..Default::default() - }; - let mut running_job = None; - let outcomes = handle_mouse_dialog(&mut state, &mut running_job, &mp(1, 1, 80, 24)); - assert!(outcomes.is_some()); - assert!(matches!( - state.mode, - AppMode::Dialog(DialogKind::Properties(..)) - )); -} - -#[test] -fn mouse_help_dialog_click_does_not_dismiss() { - let mut state = AppState { - mode: AppMode::Dialog(DialogKind::Help { - message: "help".to_string(), - scroll_offset: 0, - }), - ..Default::default() - }; - let mut running_job = None; - let outcomes = handle_mouse_dialog(&mut state, &mut running_job, &mp(1, 1, 80, 24)); - assert!(outcomes.is_some()); - assert!(matches!( - state.mode, - AppMode::Dialog(DialogKind::Help { .. }) - )); -} - -#[test] -fn mouse_confirm_dialog_keeps_existing_behavior() { - let mut state = AppState { - mode: AppMode::Dialog(DialogKind::Confirm(ConfirmDetails::simple( - "Confirm", "Run?", - ))), - input: InputState { - dialog_selection: 1, - ..Default::default() - }, - ..Default::default() - }; - let mut running_job = None; - let outcomes = handle_mouse_dialog(&mut state, &mut running_job, &mp(79, 23, 80, 24)); - assert!(outcomes.is_some()); - assert!(matches!( - state.mode, - AppMode::Dialog(DialogKind::Confirm(_)) - )); -} - -#[test] -fn mouse_overwrite_confirm_dialog_handled() { - let mut state = AppState { - mode: AppMode::Dialog(DialogKind::OverwriteConfirm(Box::new( - OverwriteConfirmDetails { - conflicting: vec![], +/// One parameterized passive-dialog case: a label, the initial state, the click +/// position (chosen to miss any action button), and a matcher asserting the +/// dialog mode survived the click. +type PassiveDialogCase = (&'static str, AppState, MousePosition, fn(&AppMode) -> bool); + +/// A non-actionable click on each of these dialogs must be `Consumed` and must +/// leave the dialog mode unchanged. Parameterized over the six passive dialog +/// kinds (the former six near-identical `*_does_not_dismiss` / `_handled` / +/// `_is_consumed` tests) so the shared assertion lives in one place. +fn passive_dialog_cases() -> Vec { + vec![ + ( + "error", + AppState { + mode: AppMode::Dialog(DialogKind::Error("error".to_string())), + ..Default::default() }, - ))), - input: InputState { - dialog_selection: 0, - ..Default::default() - }, - ..Default::default() - }; - let mut running_job = None; - let outcomes = handle_mouse_dialog(&mut state, &mut running_job, &mp(1, 1, 80, 24)); - assert!(outcomes.is_some()); - assert!(matches!( - state.mode, - AppMode::Dialog(DialogKind::OverwriteConfirm(..)) - )); + mp(1, 1, 80, 24), + |m| matches!(m, AppMode::Dialog(DialogKind::Error(_))), + ), + ( + "properties", + AppState { + mode: AppMode::Dialog(DialogKind::Properties(Box::new(PropertiesDetails { + name: "file.txt".to_string(), + size: 0, + mtime: std::time::SystemTime::UNIX_EPOCH, + permissions: 0o644, + owner: String::new(), + group: String::new(), + kind: FileKind::from_metadata_flags(false, false), + }))), + ..Default::default() + }, + mp(1, 1, 80, 24), + |m| matches!(m, AppMode::Dialog(DialogKind::Properties(..))), + ), + ( + "help", + AppState { + mode: AppMode::Dialog(DialogKind::Help { + message: "help".to_string(), + scroll_offset: 0, + }), + ..Default::default() + }, + mp(1, 1, 80, 24), + |m| matches!(m, AppMode::Dialog(DialogKind::Help { .. })), + ), + ( + "confirm", + AppState { + mode: AppMode::Dialog(DialogKind::Confirm(ConfirmDetails::simple( + "Confirm", "Run?", + ))), + input: InputState { + dialog_selection: 1, + ..Default::default() + }, + ..Default::default() + }, + // Outside the button row: click must not trigger the action. + mp(79, 23, 80, 24), + |m| matches!(m, AppMode::Dialog(DialogKind::Confirm(_))), + ), + ( + "overwrite", + AppState { + mode: AppMode::Dialog(DialogKind::OverwriteConfirm(Box::new( + OverwriteConfirmDetails { + conflicting: vec![], + }, + ))), + input: InputState { + dialog_selection: 0, + ..Default::default() + }, + ..Default::default() + }, + mp(1, 1, 80, 24), + |m| matches!(m, AppMode::Dialog(DialogKind::OverwriteConfirm(..))), + ), + ( + "progress", + AppState { + mode: AppMode::Dialog(DialogKind::Progress { + message: "Copying".to_string(), + progress_fraction: 0.5, + cancellable: true, + }), + ..Default::default() + }, + mp(40, 21, 80, 24), + |m| matches!(m, AppMode::Dialog(DialogKind::Progress { .. })), + ), + ] } #[test] -fn mouse_progress_click_is_consumed() { - let mut state = AppState { - mode: AppMode::Dialog(DialogKind::Progress { - message: "Copying".to_string(), - progress_fraction: 0.5, - cancellable: true, - }), - ..Default::default() - }; - let mut running_job = None; - let outcomes = handle_mouse_dialog(&mut state, &mut running_job, &mp(40, 21, 80, 24)); - assert!(outcomes.is_some()); - assert!(matches!(outcomes, Some(MouseOutcome::Consumed))); +fn passive_dialog_click_is_consumed_and_preserves_mode() { + for (label, mut state, pos, mode_ok) in passive_dialog_cases() { + let mut running_job = None; + let outcome = handle_mouse_dialog(&mut state, &mut running_job, &pos); + assert!( + matches!(outcome, Some(MouseOutcome::Consumed)), + "{label}: expected Consumed, got {outcome:?}" + ); + assert!( + mode_ok(&state.mode), + "{label}: dialog mode was not preserved" + ); + } } #[test] @@ -398,10 +389,21 @@ fn drag_select_range() { ..Default::default() }; + // anchor = index 0; click at row 5 maps to filtered index 3 + // (list starts at row 2, so relative row 3). The drag selects the inclusive + // range 0..=3, i.e. entries a, b, c, d — and must leave e untouched. handle_mouse_drag(&mut state, &mp(1, 5, 80, 24)); - let selected_count = state.left_panel.selected_entries().count(); - assert_eq!(selected_count, 4); + let selected: Vec<&str> = state + .left_panel + .selected_entries() + .map(|e| e.name.as_str()) + .collect(); + assert_eq!(selected, ["a", "b", "c", "d"], "wrong entries selected"); + assert_eq!(state.left_panel.cursor, 3, "cursor should land on drag end"); + // The right panel shares identical entries but is not the active panel, so + // the drag must not touch its selection. + assert_eq!(state.right_panel.selected_entries().count(), 0); } #[test] diff --git a/src/input/normal.rs b/src/input/normal.rs index 2ec2ef3..0ed0d02 100644 --- a/src/input/normal.rs +++ b/src/input/normal.rs @@ -1,14 +1,16 @@ +use std::io; use std::path::PathBuf; use crossterm::event::{KeyCode, KeyModifiers}; use lc::app::file_type; +use lc::app::paths::terminal_state_file_path; use lc::app::types::{AppMode, AppState, FileEntry, InputAction, PickerKind}; use lc::app::{panel_ops, shell}; use lc::ops::archive; use lc::ui::viewer; -use crate::{enter_tui_stdout, file_name_str, leave_tui_stdout, terminal_state_file_path}; +use crate::{enter_tui_stdout, file_name_str, leave_tui_stdout}; pub(crate) fn handle_function_keys( state: &mut AppState, @@ -17,75 +19,77 @@ pub(crate) fn handle_function_keys( terminal: &mut ratatui::Terminal, ) { match key { - KeyCode::F(1) => { - state.mode = AppMode::Dialog(lc::app::types::DialogKind::Help { - message: lc::app::keymap::build_help_message().to_string(), - scroll_offset: 0, - }); - } - KeyCode::F(2) => { - super::menu_actions::open_user_menu(state); - } - KeyCode::F(3) => { - if let Some(entry) = state.active_panel().current_entry() - && !entry.is_dir() - { - open_in_viewer(state, viewer_loader, entry.path.clone()); - } - } - KeyCode::F(4) => { - launch_editor(state, terminal); - } - KeyCode::F(5) => { - confirm_file_transfer(state, "Copy Confirm", "Copy", |sources, dest| { - lc::app::types::PendingAction::Copy(lc::app::types::TransferAction { - sources, - dest, - overwrite: false, - }) - }); - } - KeyCode::F(6) => { - confirm_file_transfer(state, "Move Confirm", "Move", |sources, dest| { - lc::app::types::PendingAction::Move(lc::app::types::TransferAction { - sources, - dest, - overwrite: false, - }) - }); - } - KeyCode::F(7) => { - handle_f7_key(state); - } - KeyCode::F(8) => { - confirm_delete(state); - } - KeyCode::F(9) => { - state.prev_mode = Some(std::mem::replace(&mut state.mode, AppMode::Menu)); - state.ui.menu_item_selected = 0; - } - KeyCode::F(10) => { - state.request_quit(); - } - KeyCode::F(11) => { - let entry_name = state.active_panel().current_entry().map(|e| e.name.clone()); - if let Some(name) = entry_name - && name != ".." - { - state.input.dialog_input.set_text_at_end(name); - state.mode = AppMode::Dialog(lc::app::types::DialogKind::Input { - prompt: "Rename to:".to_string(), - action: InputAction::Rename, - }); - } - } - KeyCode::F(12) => { - handle_f12_key(state); - } + KeyCode::F(1) => open_help_dialog(state), + KeyCode::F(2) => super::menu_actions::open_user_menu(state), + KeyCode::F(3) => view_current_entry(state, viewer_loader), + KeyCode::F(4) => launch_editor(state, terminal), + KeyCode::F(5) => confirm_copy(state), + KeyCode::F(6) => confirm_move(state), + KeyCode::F(7) => handle_f7_key(state), + KeyCode::F(8) => confirm_delete(state), + KeyCode::F(9) => open_menu_bar(state), + KeyCode::F(10) => state.request_quit(), + KeyCode::F(11) => open_rename_dialog(state), + KeyCode::F(12) => handle_f12_key(state), _ => {} } } +fn open_help_dialog(state: &mut AppState) { + state.mode = AppMode::Dialog(lc::app::types::DialogKind::Help { + message: lc::app::keymap::build_help_message().to_string(), + scroll_offset: 0, + }); +} + +fn view_current_entry(state: &mut AppState, viewer_loader: &mut Option) { + if let Some(entry) = state.active_panel().current_entry() + && !entry.is_dir() + { + open_in_viewer(state, viewer_loader, entry.path.clone()); + } +} + +fn confirm_copy(state: &mut AppState) { + confirm_file_transfer(state, "Copy Confirm", "Copy", |sources, dest| { + lc::app::types::PendingAction::Copy(lc::app::types::TransferAction { + sources, + dest, + overwrite: false, + }) + }); +} + +fn confirm_move(state: &mut AppState) { + confirm_file_transfer(state, "Move Confirm", "Move", |sources, dest| { + lc::app::types::PendingAction::Move(lc::app::types::TransferAction { + sources, + dest, + overwrite: false, + }) + }); +} + +fn open_menu_bar(state: &mut AppState) { + state.prev_mode = Some(std::mem::replace(&mut state.mode, AppMode::Menu)); + state.ui.menu_item_selected = 0; +} + +fn open_rename_dialog(state: &mut AppState) { + let entry_name = state + .active_panel() + .current_entry() + .filter(|e| e.name != "..") + .map(|e| e.name.clone()); + if let Some(name) = entry_name { + state.input.dialog_input.set_text_at_end(name); + state.mode = AppMode::Dialog(lc::app::types::DialogKind::Input { + prompt: "Rename to:".to_string(), + action: InputAction::Rename, + }); + } +} + fn handle_f7_key(state: &mut AppState) { if let Some(entry) = state.active_panel().current_entry() && is_archive_file(entry) @@ -126,71 +130,108 @@ pub(crate) fn launch_editor( state: &mut AppState, terminal: &mut ratatui::Terminal, ) { - let entry_info = state + let Some((is_dir, path)) = state .active_panel() .current_entry() - .map(|e| (e.is_dir(), e.path.clone())); - if let Some((is_dir, path)) = entry_info - && !is_dir - { - let editor = std::env::var("EDITOR").unwrap_or_else(|_| "vi".to_string()); - if let Err(e) = leave_tui_stdout() { - state.ui.status_message = Some(format!("Terminal suspend failed: {e}")); - return; - } - if let Some(terminal_state_file) = terminal_state_file_path() { - if let Some(parent) = terminal_state_file.parent() - && let Err(e) = std::fs::create_dir_all(parent) - { - lc::debug_log!("failed to create terminal state dir: {e}"); - } - if let Err(e) = std::fs::write(&terminal_state_file, "alternate_screen") { - lc::debug_log!("failed to write terminal state file: {e}"); - } - } - let parts: Vec = shlex::split(&editor).unwrap_or_else(|| { - let fallback: Vec = editor.split_whitespace().map(String::from).collect(); - if fallback.is_empty() { - vec!["vi".to_string()] - } else { - fallback - } - }); - let cmd = parts.first().map_or("vi", |s| s.as_str()); - let status = std::process::Command::new(cmd) - .args(parts.get(1..).unwrap_or_default()) - .arg(&path) - .stdin(std::process::Stdio::inherit()) - .stdout(std::process::Stdio::inherit()) - .stderr(std::process::Stdio::inherit()) - .status(); - let resume_result = enter_tui_stdout(); - if let Some(terminal_state_file) = terminal_state_file_path() - && let Err(e) = std::fs::remove_file(&terminal_state_file) - { - lc::debug_log!("failed to remove terminal state file: {e}"); + .map(|e| (e.is_dir(), e.path.clone())) + else { + return; + }; + if is_dir { + return; + } + + let editor = std::env::var("EDITOR").unwrap_or_else(|_| "vi".to_string()); + if let Err(e) = leave_tui_stdout() { + state.ui.status_message = Some(format!("Terminal suspend failed: {e}")); + return; + } + + write_terminal_state_marker(); + let spawn_result = spawn_editor(&editor, &path); + let resume_result = enter_tui_stdout(); + clear_terminal_state_marker(); + + if let Some(message) = editor_status_message(spawn_result, resume_result) { + state.ui.status_message = Some(message); + } + if let Err(e) = terminal.clear() { + lc::debug_log!("terminal.clear() failed after editor: {e}"); + } + panel_ops::refresh_active(state); +} + +/// Parse the `EDITOR` value into argv parts, falling back to `vi` when the +/// value cannot be split or is empty. +fn editor_command_parts(editor: &str) -> Vec { + shlex::split(editor).unwrap_or_else(|| { + let fallback: Vec = editor.split_whitespace().map(String::from).collect(); + if fallback.is_empty() { + vec!["vi".to_string()] + } else { + fallback } - match (status, resume_result) { - (Err(e), _) => { - state.ui.status_message = Some(format!("Editor error: {e}")); - } - (Ok(s), Err(e)) => { - let mut parts = Vec::new(); - if !s.success() { - parts.push(format!("Editor exited with status: {s}")); - } - parts.push(format!("Terminal restore failed: {e}")); - state.ui.status_message = Some(parts.join("; ")); - } - (Ok(s), Ok(())) if !s.success() => { - state.ui.status_message = Some(format!("Editor exited with status: {s}")); + }) +} + +/// Spawn the configured editor on `path`, inheriting the parent stdio so the +/// editor takes over the terminal. Returns the editor's exit status. +fn spawn_editor(editor: &str, path: &std::path::Path) -> io::Result { + let parts = editor_command_parts(editor); + let cmd = parts.first().map_or("vi", |s| s.as_str()); + std::process::Command::new(cmd) + .args(parts.get(1..).unwrap_or_default()) + .arg(path) + .stdin(std::process::Stdio::inherit()) + .stdout(std::process::Stdio::inherit()) + .stderr(std::process::Stdio::inherit()) + .status() +} + +/// Persist a marker file so a crash mid-edit can detect we left the alternate +/// screen. Best-effort: failures are logged but never surfaced to the user. +fn write_terminal_state_marker() { + let Some(terminal_state_file) = terminal_state_file_path() else { + return; + }; + if let Some(parent) = terminal_state_file.parent() + && let Err(e) = std::fs::create_dir_all(parent) + { + lc::debug_log!("failed to create terminal state dir: {e}"); + } + if let Err(e) = std::fs::write(&terminal_state_file, "alternate_screen") { + lc::debug_log!("failed to write terminal state file: {e}"); + } +} + +/// Remove the terminal-state marker written before launching the editor. +/// Best-effort: a missing or unremovable file is only logged. +fn clear_terminal_state_marker() { + if let Some(terminal_state_file) = terminal_state_file_path() + && let Err(e) = std::fs::remove_file(&terminal_state_file) + { + lc::debug_log!("failed to remove terminal state file: {e}"); + } +} + +/// Build the user-facing status message (if any) from the editor spawn result +/// and the terminal-resume result. Returns `None` when everything succeeded. +fn editor_status_message( + spawn_result: io::Result, + resume_result: io::Result<()>, +) -> Option { + match (spawn_result, resume_result) { + (Err(e), _) => Some(format!("Editor error: {e}")), + (Ok(s), Err(e)) => { + let mut parts = Vec::new(); + if !s.success() { + parts.push(format!("Editor exited with status: {s}")); } - (Ok(_), Ok(_)) => {} - } - if let Err(e) = terminal.clear() { - lc::debug_log!("terminal.clear() failed after editor: {e}"); + parts.push(format!("Terminal restore failed: {e}")); + Some(parts.join("; ")) } - panel_ops::refresh_active(state); + (Ok(s), Ok(())) if !s.success() => Some(format!("Editor exited with status: {s}")), + (Ok(_), Ok(())) => None, } } @@ -205,10 +246,9 @@ pub(crate) fn confirm_file_transfer( return; } let dest_dir = state.inactive_panel().path().to_path_buf(); - let file_names: Vec = display_file_names(&paths); - let msg = if paths.len() == 1 { - let name = file_names[0].as_str(); - format!("{verb} '{name}' to '{}'?", dest_dir.display()) + let file_names = display_file_names(&paths); + let msg = if let [single] = file_names.as_slice() { + format!("{verb} '{single}' to '{}'?", dest_dir.display()) } else { format!( "{verb} {} entries to '{}'?", @@ -216,11 +256,8 @@ pub(crate) fn confirm_file_transfer( dest_dir.display() ) }; - state.input.dialog_selection = 0; - state.mode = AppMode::Dialog(lc::app::types::DialogKind::Confirm( - lc::app::types::ConfirmDetails::with_files(label, &msg, file_names), - )); - state.ui.pending_action = Some(make_pending(paths, dest_dir)); + let pending = make_pending(paths, dest_dir); + open_confirm_dialog(state, label, &msg, file_names, pending); } pub(crate) fn confirm_delete(state: &mut AppState) { @@ -228,18 +265,31 @@ pub(crate) fn confirm_delete(state: &mut AppState) { if paths.is_empty() { return; } - let file_names: Vec = display_file_names(&paths); - let msg = if paths.len() == 1 { - let name = file_names[0].as_str(); - format!("Delete '{name}'?") + let file_names = display_file_names(&paths); + let msg = if let [single] = file_names.as_slice() { + format!("Delete '{single}'?") } else { format!("Delete {} entries?", paths.len()) }; + let pending = lc::app::types::PendingAction::Delete { paths }; + open_confirm_dialog(state, "Delete Confirm", &msg, file_names, pending); +} + +/// Open a confirmation dialog with the given label, message and affected file +/// names, and arm `pending_action` to run once the user confirms. Shared by +/// the copy/move/delete confirmation entry points. +fn open_confirm_dialog( + state: &mut AppState, + label: &str, + msg: &str, + file_names: Vec, + pending: lc::app::types::PendingAction, +) { state.input.dialog_selection = 0; state.mode = AppMode::Dialog(lc::app::types::DialogKind::Confirm( - lc::app::types::ConfirmDetails::with_files("Delete Confirm", &msg, file_names), + lc::app::types::ConfirmDetails::with_files(label, msg, file_names), )); - state.ui.pending_action = Some(lc::app::types::PendingAction::Delete { paths }); + state.ui.pending_action = Some(pending); } pub(crate) fn handle_navigation_keys( @@ -248,76 +298,94 @@ pub(crate) fn handle_navigation_keys( modifiers: KeyModifiers, visible: usize, ) { + let shift = modifiers.contains(KeyModifiers::SHIFT); match key { - KeyCode::Up if modifiers.contains(KeyModifiers::SHIFT) => { - let panel = state.active_panel_mut(); - if panel.listing.filtered_is_empty() { - return; - } - panel.toggle_selection_at(panel.cursor); - panel.move_cursor_up(visible); - } - KeyCode::Down if modifiers.contains(KeyModifiers::SHIFT) => { - let panel = state.active_panel_mut(); - if panel.listing.filtered_is_empty() { - return; - } - panel.toggle_selection_at(panel.cursor); - panel.move_cursor_down(visible); - } + KeyCode::Up if shift => select_and_move(state, visible, Direction::Up), + KeyCode::Down if shift => select_and_move(state, visible, Direction::Down), KeyCode::Up | KeyCode::Char('k') => { - let panel = state.active_panel_mut(); - panel.move_cursor_up(visible); + state.active_panel_mut().move_cursor_up(visible); } KeyCode::Down | KeyCode::Char('j') => { - let panel = state.active_panel_mut(); - panel.move_cursor_down(visible); - } - KeyCode::Home => { - let p = state.active_panel_mut(); - p.cursor = 0; - p.scroll_offset = 0; - } - KeyCode::End => { - let len = state.active_panel().listing.filtered_len(); - if len > 0 { - let p = state.active_panel_mut(); - p.cursor = len - 1; - p.ensure_cursor_visible(visible); - } - } - KeyCode::PageUp => { - let p = state.active_panel_mut(); - p.cursor = p.cursor.saturating_sub(visible); - p.scroll_offset = p.scroll_offset.saturating_sub(visible); - } - KeyCode::PageDown => { - let len = state.active_panel().listing.filtered_len(); - let p = state.active_panel_mut(); - p.cursor = (p.cursor + visible).min(len.saturating_sub(1)); - p.scroll_offset = (p.scroll_offset + visible).min(len.saturating_sub(visible)); - } - KeyCode::Tab => { - state.active_panel = state.active_panel.toggle(); - let p = state.active_panel_mut(); - let max = p.listing.filtered_len().saturating_sub(1); - p.cursor = p.cursor.min(max); - p.ensure_cursor_visible(visible); - } - KeyCode::Insert => { - let panel = state.active_panel_mut(); - if panel.listing.filtered_is_empty() { - return; - } - panel.toggle_selection(); - if panel.cursor < panel.listing.filtered_len() - 1 { - panel.move_cursor_down(visible); - } - } + state.active_panel_mut().move_cursor_down(visible); + } + KeyCode::Home => move_cursor_home(state), + KeyCode::End => move_cursor_end(state, visible), + KeyCode::PageUp => page_up(state, visible), + KeyCode::PageDown => page_down(state, visible), + KeyCode::Tab => switch_active_panel(state, visible), + KeyCode::Insert => toggle_selection_and_advance(state, visible), _ => {} } } +/// Vertical movement direction for Shift+selection navigation. +#[derive(Clone, Copy)] +enum Direction { + Up, + Down, +} + +/// Toggle the selection at the cursor, then move one row in `direction`. +/// No-op on an empty (filtered) listing. +fn select_and_move(state: &mut AppState, visible: usize, direction: Direction) { + let panel = state.active_panel_mut(); + if panel.listing.filtered_is_empty() { + return; + } + panel.toggle_selection_at(panel.cursor); + match direction { + Direction::Up => panel.move_cursor_up(visible), + Direction::Down => panel.move_cursor_down(visible), + } +} + +fn move_cursor_home(state: &mut AppState) { + let p = state.active_panel_mut(); + p.cursor = 0; + p.scroll_offset = 0; +} + +fn move_cursor_end(state: &mut AppState, visible: usize) { + let len = state.active_panel().listing.filtered_len(); + if len > 0 { + let p = state.active_panel_mut(); + p.cursor = len - 1; + p.ensure_cursor_visible(visible); + } +} + +fn page_up(state: &mut AppState, visible: usize) { + let p = state.active_panel_mut(); + p.cursor = p.cursor.saturating_sub(visible); + p.scroll_offset = p.scroll_offset.saturating_sub(visible); +} + +fn page_down(state: &mut AppState, visible: usize) { + let len = state.active_panel().listing.filtered_len(); + let p = state.active_panel_mut(); + p.cursor = (p.cursor + visible).min(len.saturating_sub(1)); + p.scroll_offset = (p.scroll_offset + visible).min(len.saturating_sub(visible)); +} + +fn switch_active_panel(state: &mut AppState, visible: usize) { + state.active_panel = state.active_panel.toggle(); + let p = state.active_panel_mut(); + let max = p.listing.filtered_len().saturating_sub(1); + p.cursor = p.cursor.min(max); + p.ensure_cursor_visible(visible); +} + +fn toggle_selection_and_advance(state: &mut AppState, visible: usize) { + let panel = state.active_panel_mut(); + if panel.listing.filtered_is_empty() { + return; + } + panel.toggle_selection(); + if panel.cursor < panel.listing.filtered_len() - 1 { + panel.move_cursor_down(visible); + } +} + pub(crate) fn reposition_cursor_to_entry( state: &mut AppState, prev_dir_name: Option<&str>, @@ -528,9 +596,17 @@ fn open_in_viewer( state.mode = AppMode::Viewing; } +/// Render the display name (last path component, falling back to the full path) +/// for each path. Builds the strings directly to avoid an intermediate +/// `Vec` and the extra path clone it would require. fn display_file_names(paths: &[PathBuf]) -> Vec { - panel_ops::file_names_from_paths(paths) + paths .iter() - .map(|p| p.display().to_string()) + .map(|p| { + p.file_name().map_or_else( + || p.display().to_string(), + |name| name.to_string_lossy().into_owned(), + ) + }) .collect() } diff --git a/src/input/pickers.rs b/src/input/pickers.rs index f75a3a0..bd719af 100644 --- a/src/input/pickers.rs +++ b/src/input/pickers.rs @@ -36,42 +36,61 @@ fn move_cursor(entries_len: usize, selected: &mut usize, direction: MoveDirectio } } -fn handle_history_picker(state: &mut AppState, key: KeyCode, len: usize) { - match key { +/// Result of the shared picker key dispatch. +enum NavOutcome { + /// The key was fully handled here (Esc closed the picker, or the cursor + /// moved). The caller should stop processing. + Handled, + /// Not a shared navigation key; the caller handles it (e.g. Enter, hotkeys). + Passthrough, +} + +/// Handle the navigation keys common to every list picker: `Esc` closes the +/// picker, and Up/Down/Home/End move the shared `picker_selected` cursor. +/// +/// Centralizing these arms keeps the per-picker handlers focused on their own +/// `Enter`/hotkey behaviour instead of repeating five identical match arms. +fn handle_nav_key(state: &mut AppState, key: KeyCode, len: usize) -> NavOutcome { + let direction = match key { KeyCode::Esc => { state.mode = AppMode::Normal; + return NavOutcome::Handled; } - KeyCode::Up => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::Up), - KeyCode::Down => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::Down), - KeyCode::Home => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::Home), - KeyCode::End => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::End), - KeyCode::Enter => { - if state.ui.picker_selected >= len { - state.mode = AppMode::Normal; - return; - } - // History displays most-recent-first; reverse visual index to VecDeque position - let idx = len - 1 - state.ui.picker_selected; - if let Some(cmd) = state.input.command_history.get(idx).cloned() { - state.input.command_line.set_text_at_end(cmd); - state.mode = AppMode::CommandLine; - } else { - state.mode = AppMode::Normal; - } + KeyCode::Up => MoveDirection::Up, + KeyCode::Down => MoveDirection::Down, + KeyCode::Home => MoveDirection::Home, + KeyCode::End => MoveDirection::End, + _ => return NavOutcome::Passthrough, + }; + move_cursor(len, &mut state.ui.picker_selected, direction); + NavOutcome::Handled +} + +fn handle_history_picker(state: &mut AppState, key: KeyCode, len: usize) { + if let NavOutcome::Handled = handle_nav_key(state, key, len) { + return; + } + if key == KeyCode::Enter { + if state.ui.picker_selected >= len { + state.mode = AppMode::Normal; + return; + } + // History displays most-recent-first; reverse visual index to VecDeque position + let idx = len - 1 - state.ui.picker_selected; + if let Some(cmd) = state.input.command_history.get(idx).cloned() { + state.input.command_line.set_text_at_end(cmd); + state.mode = AppMode::CommandLine; + } else { + state.mode = AppMode::Normal; } - _ => {} } } fn handle_hotlist_picker(state: &mut AppState, key: KeyCode, len: usize) { + if let NavOutcome::Handled = handle_nav_key(state, key, len) { + return; + } match key { - KeyCode::Esc => { - state.mode = AppMode::Normal; - } - KeyCode::Up => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::Up), - KeyCode::Down => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::Down), - KeyCode::Home => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::Home), - KeyCode::End => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::End), KeyCode::Enter => { let idx = state.ui.picker_selected; state.mode = AppMode::Normal; @@ -80,15 +99,16 @@ fn handle_hotlist_picker(state: &mut AppState, key: KeyCode, len: usize) { KeyCode::Char('a') => { let cur = state.active_panel().path().to_path_buf(); if state.hotlist().iter().any(|p| p == &cur) { - state.ui.status_message = Some("Directory already in hotlist".to_string()); + state.set_status("Directory already in hotlist"); } else { state.hotlist_push(cur); - state.ui.status_message = Some("Added current directory to hotlist".to_string()); + state.set_status("Added current directory to hotlist"); } } KeyCode::Char('d') if state.ui.picker_selected < state.hotlist().len() => { state.hotlist_remove(state.ui.picker_selected); - if state.ui.picker_selected > 0 && state.ui.picker_selected >= state.hotlist().len() { + let hotlist_len = state.hotlist().len(); + if state.ui.picker_selected > 0 && state.ui.picker_selected >= hotlist_len { state.ui.picker_selected -= 1; } } @@ -99,121 +119,126 @@ fn handle_hotlist_picker(state: &mut AppState, key: KeyCode, len: usize) { fn handle_compare_mode_picker(state: &mut AppState, key: KeyCode) { let modes = CompareMode::ALL; let len = modes.len(); - match key { - KeyCode::Esc => { - state.mode = AppMode::Normal; - } - KeyCode::Up => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::Up), - KeyCode::Down => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::Down), - KeyCode::Home => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::Home), - KeyCode::End => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::End), - KeyCode::Enter => { - let Some(&chosen) = modes.get(state.ui.picker_selected) else { - state.mode = AppMode::Normal; - return; - }; + if let NavOutcome::Handled = handle_nav_key(state, key, len) { + return; + } + if key == KeyCode::Enter { + let Some(&chosen) = modes.get(state.ui.picker_selected) else { state.mode = AppMode::Normal; - compare_directories(state, chosen); - } - _ => {} + return; + }; + state.mode = AppMode::Normal; + compare_directories(state, chosen); } } fn handle_user_menu_picker(state: &mut AppState, key: KeyCode) { let len = state.ui.user_menu_entries.len(); - match key { - KeyCode::Esc => { - state.mode = AppMode::Normal; - } - KeyCode::Up => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::Up), - KeyCode::Down => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::Down), - KeyCode::Home => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::Home), - KeyCode::End => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::End), - KeyCode::Enter => { - let idx = state.ui.picker_selected.min(len.saturating_sub(1)); - state.mode = AppMode::Normal; - if let Some(entry) = state.ui.user_menu_entries.get(idx).cloned() { - let active_dir = state.active_panel().path().to_path_buf(); - let other_dir = state.inactive_panel().path().to_path_buf(); - let current_file = state - .active_panel() - .current_entry() - .map(|e| e.name.clone()) - .unwrap_or_default(); - let tagged: Vec = state - .active_panel() - .selected_entries() - .filter(|e| e.name != "..") - .map(|e| e.path.clone()) - .collect(); - let ctx = user_menu::SubstContext { - current_file: std::path::Path::new(¤t_file), - active_dir: &active_dir, - other_dir: &other_dir, - tagged: &tagged, - }; - let cmd = match user_menu::apply_substitutions(&entry.command, &ctx) { - Ok(c) => c, - Err(e) => { - state.ui.status_message = Some(e); - return; - } - }; - if state.ui.user_menu_source == MenuSource::Local { - state.ui.pending_menu_command = Some(cmd); - state.input.dialog_selection = 0; - state.mode = AppMode::Dialog(DialogKind::Confirm(ConfirmDetails::simple( - "Trust Local Menu?", - "This menu comes from the current directory.\n\ - Running untrusted commands may be dangerous.\n\n\ - Execute?", - ))); - } else { - lc::app::shell::run_shell_command(state, &cmd, true, refresh_active); - } - } + if let NavOutcome::Handled = handle_nav_key(state, key, len) { + return; + } + if key == KeyCode::Enter { + activate_user_menu_entry(state, len); + } +} + +/// Run the user-menu entry under the cursor: build the substitution context, +/// expand the command template, then either confirm (local menus) or execute. +fn activate_user_menu_entry(state: &mut AppState, len: usize) { + let idx = state.ui.picker_selected.min(len.saturating_sub(1)); + state.mode = AppMode::Normal; + let cmd = match resolve_user_menu_command(state, idx) { + Some(Ok(cmd)) => cmd, + Some(Err(err)) => { + state.ui.status_message = Some(err); + return; } - _ => {} + None => return, + }; + dispatch_user_menu_command(state, cmd); +} + +/// Expand the substitution template of the entry at `idx` against the current +/// panels. Returns `None` when no entry exists at `idx`, otherwise the +/// expansion result. Only the command template is cloned (not the whole +/// `MenuEntry`), so the compiled condition is never duplicated. +fn resolve_user_menu_command(state: &AppState, idx: usize) -> Option> { + let command = state.ui.user_menu_entries.get(idx)?.command.clone(); + let active_dir = state.active_panel().path().to_path_buf(); + let other_dir = state.inactive_panel().path().to_path_buf(); + let current_file = state + .active_panel() + .current_entry() + .map(|e| e.name.clone()) + .unwrap_or_default(); + let tagged: Vec = state + .active_panel() + .selected_entries() + .filter(|e| e.name != "..") + .map(|e| e.path.clone()) + .collect(); + let ctx = user_menu::SubstContext { + current_file: std::path::Path::new(¤t_file), + active_dir: &active_dir, + other_dir: &other_dir, + tagged: &tagged, + }; + Some(user_menu::apply_substitutions(&command, &ctx)) +} + +/// Either prompt for trust (local menus, which may carry untrusted commands) +/// or run the expanded command straight away (global menus). +fn dispatch_user_menu_command(state: &mut AppState, cmd: String) { + if state.ui.user_menu_source == MenuSource::Local { + state.ui.pending_menu_command = Some(cmd); + state.input.dialog_selection = 0; + state.mode = AppMode::Dialog(DialogKind::Confirm(ConfirmDetails::simple( + "Trust Local Menu?", + "This menu comes from the current directory.\n\ + Running untrusted commands may be dangerous.\n\n\ + Execute?", + ))); + } else { + lc::app::shell::run_shell_command(state, &cmd, true, refresh_active); } } fn handle_archive_menu_picker(state: &mut AppState, key: KeyCode) { const ITEMS: [&str; 2] = ["Extract Archive", "Create Archive"]; let len = ITEMS.len(); - match key { - KeyCode::Esc => { - state.mode = AppMode::Normal; - } - KeyCode::Up => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::Up), - KeyCode::Down => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::Down), - KeyCode::Home => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::Home), - KeyCode::End => move_cursor(len, &mut state.ui.picker_selected, MoveDirection::End), - KeyCode::Enter => { - let choice = state.ui.picker_selected; - state.mode = AppMode::Normal; - match choice { - 0 => { - if let Some(entry) = state.active_panel().current_entry() { - if entry.name != ".." && file_type::is_archive(&entry.name) { - super::normal::show_archive_dialog(state); - } else { - state.ui.status_message = - Some("Cursor is not on an archive file".to_string()); - } - } - } - 1 => { - let paths = super::normal::selected_or_current_paths(state); - if paths.is_empty() { - state.ui.status_message = Some("No files selected".to_string()); + if let NavOutcome::Handled = handle_nav_key(state, key, len) { + return; + } + if key == KeyCode::Enter { + let choice = state.ui.picker_selected; + state.mode = AppMode::Normal; + match choice { + 0 => { + // Preserve original behaviour: do nothing when the panel has no + // current entry; only report when an entry exists but is not an + // archive. + if let Some(on_archive) = state + .active_panel() + .current_entry() + .map(|e| e.name != ".." && file_type::is_archive(&e.name)) + { + if on_archive { + super::normal::show_archive_dialog(state); } else { - show_create_dialog(state, paths); + state.set_status("Cursor is not on an archive file"); } } - _ => {} } + 1 => { + let paths = super::normal::selected_or_current_paths(state); + if paths.is_empty() { + state.set_status("No files selected"); + } else { + show_create_dialog(state, paths); + } + } + _ => {} } - _ => {} } } @@ -251,15 +276,16 @@ pub(crate) fn handle_list_picker(state: &mut AppState, key: KeyCode) { } } -fn effective_entries(panel: &PanelState) -> &[FileEntry] { - // Single backing store: always compare over the full set, regardless of - // any active filter. +/// Entries to feed into a directory comparison: always the full, unfiltered +/// listing. Comparison must reflect the whole directory, not whatever subset a +/// user-applied filter happens to show, so any active filter is ignored here. +fn comparable_entries(panel: &PanelState) -> &[FileEntry] { panel.listing.unfiltered() } pub(crate) fn compare_directories(state: &mut AppState, mode: CompareMode) { - let left_entries = effective_entries(&state.left_panel); - let right_entries = effective_entries(&state.right_panel); + let left_entries = comparable_entries(&state.left_panel); + let right_entries = comparable_entries(&state.right_panel); let report = ops::compare_entries(left_entries, right_entries, mode); ops::apply_compare_to_panels(&mut state.left_panel, &mut state.right_panel, &report); diff --git a/src/lib.rs b/src/lib.rs index 9dcc084..6ea81c7 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -2,20 +2,20 @@ //! //! Built with Ratatui + Crossterm. Single binary, no runtime dependencies. +// Public API surface. +// +// The `lc` binary (`main.rs`, `render*`, `input/*`, integration tests under +// `src/tests/`) consumes this library as an external crate and always reaches +// items through their module path (e.g. `lc::app::types::AppState`, +// `lc::ops::compare::compare_entries`). The crate-root re-exports that used to +// live here were never referenced via `lc::` by any consumer, so they +// were a redundant, inconsistent second surface. The intended public API is the +// set of top-level modules below; navigate into them for concrete items. pub mod app; pub mod fs; pub mod menu; pub mod ops; pub mod ui; -pub use app::types::{ - ActivePanel, AppMode, AppState, CompareMode, DialogKind, Direction, FileCategory, FileEntry, - FileSize, ListingMode, PanelState, PendingAction, SortField, SortMode, SortOptions, ViewMode, -}; -pub use menu::MenuAction; -pub use ops::compare::{CompareReport, apply_compare_to_panels, compare_entries}; #[cfg(unix)] pub use ops::file_ops::chmod; -pub use ops::file_ops::{create_directory, rename_entry}; -pub use ops::search::{SearchError, SearchErrorKind, SearchOutcome, TruncationReason}; -pub use ops::sorting::{cycle_sort_mode, sort_entries}; diff --git a/src/main.rs b/src/main.rs index 122ed83..99aafc5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,5 +1,4 @@ use std::io::{self, Write}; -use std::path::PathBuf; use std::sync::Arc; use std::sync::mpsc; use std::time::Duration; @@ -13,7 +12,9 @@ use crossterm::{ execute, terminal::{EnterAlternateScreen, LeaveAlternateScreen, disable_raw_mode, enable_raw_mode}, }; -use ratatui::prelude::*; +use ratatui::Terminal; +use ratatui::backend::CrosstermBackend; +use ratatui::layout::Size; use lc::{app, fs, menu, ui}; @@ -85,10 +86,6 @@ pub(crate) fn leave_tui_stdout() -> io::Result<()> { raw_result.and(screen_result) } -fn terminal_state_file_path() -> Option { - paths::terminal_state_file_path() -} - fn fatal(msg: &str, err: &dyn std::fmt::Display) -> ! { lc::debug_log!("{msg}: {err}"); let _ = writeln!(io::stderr(), "Error: {err}"); @@ -231,9 +228,9 @@ fn pre_draw( } } -fn run_app(terminal: &mut Terminal>) -> io::Result<()> { - recover_terminal_state()?; - +/// Build the initial `AppState`: load config + apply theme. Any recoverable +/// error is surfaced through `status_message` rather than aborting startup. +fn init_app_state() -> AppState { let mut state = AppState::new(); let config_raw = match app::config::load_setup(&mut state) { Ok(raw) => raw, @@ -247,13 +244,20 @@ fn run_app(terminal: &mut Terminal>) -> io::Result< { state.ui.status_message = Some(e); } + state +} - let mut viewer_state: Option = None; - let mut viewer_loader: Option = None; - let mut image_preview_loader: Option = None; - let mut running_job: Option = None; +/// Start the filesystem watcher, returning the receiver and `Option`. +/// A failed watcher is non-fatal: the app runs without live updates and the +/// reason is appended to `status_message`. +fn init_watcher( + state: &mut AppState, +) -> ( + Option, + mpsc::Receiver, +) { let (watch_tx, watch_rx) = mpsc::sync_channel(WATCH_CHANNEL_CAPACITY); - let mut watcher = match fs::watcher::Watcher::new(Arc::new(watch_tx)) { + let watcher = match fs::watcher::Watcher::new(Arc::new(watch_tx)) { Ok(w) => Some(w), Err(err) => { let msg = format!("watcher disabled: {err}"); @@ -264,44 +268,127 @@ fn run_app(terminal: &mut Terminal>) -> io::Result< None } }; - let mut watcher_paused = false; - let mut watcher_sync_state = watcher_sync::WatcherSyncState::default(); + (watcher, watch_rx) +} + +/// Long-lived loop state that the async-poll and render seams operate on. Groups +/// the watcher bookkeeping + the per-frame viewer/job handles so the per-iter +/// helpers take a small parameter list instead of a dozen `&mut` locals. +struct AppLoop { + viewer_state: Option, + viewer_loader: Option, + image_preview_loader: Option, + running_job: Option, + watcher: Option, + watcher_paused: bool, + watcher_sync_state: watcher_sync::WatcherSyncState, +} + +/// Drain every async source feeding the UI for one loop iteration: watcher +/// events, the running job, and the viewer/image-preview loaders. Returns +/// `true` if anything changed and the frame must be redrawn. +fn poll_async( + loop_state: &mut AppLoop, + state: &mut AppState, + watch_rx: &mpsc::Receiver, +) -> bool { + let mut dirty = false; + panel_ops::sync_watcher_job_state( + &loop_state.watcher, + loop_state.running_job.is_some(), + &mut loop_state.watcher_paused, + ); + watcher_sync::sync_watcher_paths( + &mut loop_state.watcher, + state, + &mut loop_state.watcher_sync_state, + ); + if watcher_sync::poll_watcher_events(state, watch_rx) { + if let Some(ref w) = loop_state.watcher { + w.flush_pending(); + } + dirty = true; + } + if poll_running_job(state, &mut loop_state.running_job, panel_ops::refresh_both) { + panel_ops::sync_watcher_job_state( + &loop_state.watcher, + loop_state.running_job.is_some(), + &mut loop_state.watcher_paused, + ); + dirty = true; + } + if poll_viewer_loader( + state, + &mut loop_state.viewer_state, + &mut loop_state.viewer_loader, + ) || poll_image_preview( + state, + &mut loop_state.viewer_state, + &mut loop_state.image_preview_loader, + ) { + dirty = true; + } + dirty +} + +/// Run image-preview prep then draw a single frame. Keeps the `pre_draw` → +/// `terminal.draw` ordering (image preview / wrap layout must be computed +/// before the immediate-mode render reads them) in one place. +fn render_frame( + terminal: &mut Terminal>, + loop_state: &mut AppLoop, + state: &AppState, + term_size: Size, +) -> io::Result<()> { + pre_draw( + state, + &mut loop_state.viewer_state, + &mut loop_state.image_preview_loader, + term_size, + ); + terminal.draw(|f| { + render::render_ui( + f, + state, + loop_state.viewer_state.as_ref(), + loop_state.viewer_loader.as_ref(), + ) + })?; + Ok(()) +} + +fn run_app(terminal: &mut Terminal>) -> io::Result<()> { + recover_terminal_state()?; + + let mut state = init_app_state(); + let (watcher, watch_rx) = init_watcher(&mut state); + let mut loop_state = AppLoop { + viewer_state: None, + viewer_loader: None, + image_preview_loader: None, + running_job: None, + watcher, + watcher_paused: false, + watcher_sync_state: watcher_sync::WatcherSyncState::default(), + }; panel_ops::refresh_panel(&mut state.left_panel, 0); panel_ops::refresh_panel(&mut state.right_panel, 0); - watcher_sync::sync_watcher_paths(&mut watcher, &state, &mut watcher_sync_state); + watcher_sync::sync_watcher_paths( + &mut loop_state.watcher, + &state, + &mut loop_state.watcher_sync_state, + ); let mut dirty = true; let mut term_size = terminal.size()?; loop { - panel_ops::sync_watcher_job_state(&watcher, running_job.is_some(), &mut watcher_paused); - watcher_sync::sync_watcher_paths(&mut watcher, &state, &mut watcher_sync_state); - if watcher_sync::poll_watcher_events(&mut state, &watch_rx) { - if let Some(ref w) = watcher { - w.flush_pending(); - } - dirty = true; - } - if poll_running_job(&mut state, &mut running_job, panel_ops::refresh_both) { - panel_ops::sync_watcher_job_state(&watcher, running_job.is_some(), &mut watcher_paused); - dirty = true; - } - if poll_viewer_loader(&mut state, &mut viewer_state, &mut viewer_loader) - || poll_image_preview(&mut state, &mut viewer_state, &mut image_preview_loader) - { + if poll_async(&mut loop_state, &mut state, &watch_rx) { dirty = true; } if dirty { - pre_draw( - &state, - &mut viewer_state, - &mut image_preview_loader, - term_size, - ); - if let Err(e) = terminal.draw(|f| { - render::render_ui(f, &state, viewer_state.as_ref(), viewer_loader.as_ref()) - }) { - shutdown_job(&mut running_job); + if let Err(e) = render_frame(terminal, &mut loop_state, &state, term_size) { + shutdown_job(&mut loop_state.running_job); return Err(e); } dirty = false; @@ -310,24 +397,26 @@ fn run_app(terminal: &mut Terminal>) -> io::Result< let key = match event::read() { Ok(k) => k, Err(e) => { - shutdown_job(&mut running_job); + shutdown_job(&mut loop_state.running_job); return Err(e); } }; - dirty = dispatch_event( - &mut state, - &mut viewer_state, - &mut viewer_loader, - &mut image_preview_loader, - &mut running_job, - terminal, - &mut term_size, - &key, - )?; + let mut ctx = input::EventContext { + state: &mut state, + viewer_state: &mut loop_state.viewer_state, + viewer_loader: &mut loop_state.viewer_loader, + image_preview_loader: &mut loop_state.image_preview_loader, + running_job: &mut loop_state.running_job, + term_size, + }; + dirty = dispatch_event(&mut ctx, terminal, &key)?; + // The dispatch may have processed a `Resize`, which lives only in + // the context; carry it back so the next render uses the new size. + term_size = ctx.term_size; } if state.should_quit() { - shutdown_job(&mut running_job); + shutdown_job(&mut loop_state.running_job); return Ok(()); } } @@ -340,141 +429,90 @@ fn shutdown_job(job: &mut Option) { } fn recover_terminal_state() -> io::Result<()> { - let Some(terminal_state_file) = terminal_state_file_path() else { + let Some(terminal_state_file) = paths::terminal_state_file_path() else { return Ok(()); }; if std::fs::metadata(&terminal_state_file).is_ok() { + // Bounce the terminal to recover from a previous external-process exit. + // The `enter` result is what determines whether the terminal is usable + // going forward, so it is the one we propagate. A failed `leave` is + // logged but must NOT mask a successful `enter`: reporting an error + // while the terminal is actually working would abort the app needlessly. let leave = leave_tui_stdout(); let enter = enter_tui_stdout(); if let Err(e) = std::fs::remove_file(&terminal_state_file) { lc::debug_log!("failed to remove terminal state file: {e}"); } - leave.and(enter)?; + if let Err(e) = leave { + lc::debug_log!("failed to leave terminal during recovery: {e}"); + } + enter?; } Ok(()) } -#[allow(clippy::too_many_arguments)] fn dispatch_event( - state: &mut AppState, - viewer_state: &mut Option, - viewer_loader: &mut Option, - image_preview_loader: &mut Option, - running_job: &mut Option, + ctx: &mut input::EventContext, terminal: &mut Terminal, - term_size: &mut Size, event: &Event, ) -> Result { match event { - Event::Key(key) => dispatch_key_event( - state, - viewer_state, - viewer_loader, - image_preview_loader, - running_job, - terminal, - *term_size, - key, - ), - Event::Mouse(mouse_event) => dispatch_mouse_event( - state, - viewer_state, - viewer_loader, - running_job, - mouse_event, - terminal, - *term_size, - ), + Event::Key(key) => dispatch_key_event(ctx, terminal, key), + Event::Mouse(mouse_event) => dispatch_mouse_event(ctx, terminal, mouse_event), Event::Resize(cols, rows) => { - *term_size = Size::new(*cols, *rows); + ctx.term_size = Size::new(*cols, *rows); Ok(true) } _ => Ok(false), } } -#[allow(clippy::too_many_arguments)] fn dispatch_key_event( - state: &mut AppState, - viewer_state: &mut Option, - viewer_loader: &mut Option, - image_preview_loader: &mut Option, - running_job: &mut Option, + ctx: &mut input::EventContext, terminal: &mut Terminal, - term_size: Size, key: &KeyEvent, ) -> Result { match key.kind { KeyEventKind::Press => {} - KeyEventKind::Repeat if key_repeat_allowed(&state.mode, key.code) => {} + KeyEventKind::Repeat if key_repeat_allowed(&ctx.state.mode, key.code) => {} _ => return Ok(false), } - let size = term_size; - match &state.mode { + // Per-mode dispatch: each arm forwards the shared `EventContext` (plus the + // terminal/key data each handler needs) to exactly one mode handler. Match + // on `ctx.state.mode` as a place expression with only non-binding patterns + // (`Dialog(_)`, `ListPicker(_)`, unit variants): this reads the discriminant + // without holding a borrow into the arm, so each handler is free to reborrow + // `ctx.state` mutably. No clone -- the previous `mode.clone()` deep-allocated + // `Dialog`/`ListPicker` payloads on every keypress. + match ctx.state.mode { AppMode::Normal => { - input::mode_dispatch::handle_normal_mode( - state, - viewer_state, - viewer_loader, - key.code, - key.modifiers, - size.height, - terminal, - ); + input::mode_dispatch::handle_normal_mode(ctx, key.code, key.modifiers, terminal); } AppMode::Viewing => { - input::mode_dispatch::handle_viewer_mode( - state, - viewer_state, - viewer_loader, - image_preview_loader, - key.code, - size, - ); + input::mode_dispatch::handle_viewer_mode(ctx, key.code); } AppMode::CommandLine => { - input::command_line::handle_command_line(state, *key); + input::command_line::handle_command_line(ctx.state, *key); } AppMode::Dialog(_) => { - input::dialogs::handle_dialog(state, viewer_state, running_job, key.code, size); + input::dialogs::handle_dialog(ctx, key.code); } AppMode::Search if matches!(key.code, KeyCode::F(_)) => { - let visible = panel_ops::panel_visible_height(size.height); - input::mode_dispatch::clear_search_state(state, visible); - input::mode_dispatch::handle_normal_mode( - state, - viewer_state, - viewer_loader, - key.code, - key.modifiers, - size.height, - terminal, - ); + let visible = panel_ops::panel_visible_height(ctx.term_size.height); + input::mode_dispatch::clear_search_state(ctx.state, visible); + input::mode_dispatch::handle_normal_mode(ctx, key.code, key.modifiers, terminal); } AppMode::Search => { - input::mode_dispatch::handle_search_mode(state, key.code, size.height); + input::mode_dispatch::handle_search_mode(ctx.state, key.code, ctx.term_size.height); } AppMode::Menu => { - input::mode_dispatch::handle_menu_mode( - state, - viewer_state, - viewer_loader, - key.code, - size.height, - terminal, - ); + input::mode_dispatch::handle_menu_mode(ctx, key.code, terminal); } AppMode::ListPicker(_) => { - input::pickers::handle_list_picker(state, key.code); + input::pickers::handle_list_picker(ctx.state, key.code); } AppMode::DirectoryTree => { - input::directory_tree::handle_directory_tree( - state, - viewer_state, - viewer_loader, - key.code, - size.height, - ); + input::directory_tree::handle_directory_tree(ctx, key.code); } } Ok(true) @@ -514,49 +552,24 @@ fn key_repeat_allowed(mode: &AppMode, key: KeyCode) -> bool { } fn dispatch_mouse_event( - state: &mut AppState, - viewer_state: &mut Option, - viewer_loader: &mut Option, - running_job: &mut Option, - mouse_event: &MouseEvent, + ctx: &mut input::EventContext, terminal: &mut Terminal, - term_size: Size, + mouse_event: &MouseEvent, ) -> Result { - let Some(outcome) = input::mouse::handle_mouse_event( - state, - viewer_state, - viewer_loader, - running_job, - *mouse_event, - term_size, - ) else { + let Some(outcome) = input::mouse::handle_mouse_event(ctx, *mouse_event) else { return Ok(false); }; match outcome { input::mouse::MouseOutcome::Consumed => {} input::mouse::MouseOutcome::NormalKey(key) => { - if matches!(state.mode, AppMode::Search) { - let visible = panel_ops::panel_visible_height(term_size.height); - input::mode_dispatch::clear_search_state(state, visible); + if matches!(ctx.state.mode, AppMode::Search) { + let visible = panel_ops::panel_visible_height(ctx.term_size.height); + input::mode_dispatch::clear_search_state(ctx.state, visible); } - input::mode_dispatch::handle_normal_mode( - state, - viewer_state, - viewer_loader, - key, - KeyModifiers::NONE, - term_size.height, - terminal, - ); + input::mode_dispatch::handle_normal_mode(ctx, key, KeyModifiers::NONE, terminal); } input::mouse::MouseOutcome::MenuAction => { - input::mode_dispatch::run_selected_menu_action( - state, - viewer_state, - viewer_loader, - term_size.height, - terminal, - ); + input::mode_dispatch::run_selected_menu_action(ctx, terminal); } } Ok(true) diff --git a/src/menu.rs b/src/menu.rs index 2438294..256bd5b 100644 --- a/src/menu.rs +++ b/src/menu.rs @@ -43,21 +43,29 @@ pub struct MenuEntry { pub actions: &'static [MenuAction], } +// The "Left" and "Right" top-level menus are identical apart from their title: +// both drive the panel of the same name via `with_menu_panel`. Share a single +// definition of the item labels and their actions so the two entries cannot +// drift out of sync. +const PANEL_MENU_ITEMS: &[&str] = &[ + "Listing mode...", + "Sort order...", + "Filter...", + "Refresh panel", +]; + +const PANEL_MENU_ACTIONS: &[MenuAction] = &[ + MenuAction::ToggleListingMode, + MenuAction::CycleSortOrder, + MenuAction::OpenFilter, + MenuAction::RefreshPanel, +]; + pub const MENUS: [MenuEntry; 5] = [ MenuEntry { title: "Left", - items: &[ - "Listing mode...", - "Sort order...", - "Filter...", - "Refresh panel", - ], - actions: &[ - MenuAction::ToggleListingMode, - MenuAction::CycleSortOrder, - MenuAction::OpenFilter, - MenuAction::RefreshPanel, - ], + items: PANEL_MENU_ITEMS, + actions: PANEL_MENU_ACTIONS, }, MenuEntry { title: "File", @@ -128,21 +136,16 @@ pub const MENUS: [MenuEntry; 5] = [ }, MenuEntry { title: "Right", - items: &[ - "Listing mode...", - "Sort order...", - "Filter...", - "Refresh panel", - ], - actions: &[ - MenuAction::ToggleListingMode, - MenuAction::CycleSortOrder, - MenuAction::OpenFilter, - MenuAction::RefreshPanel, - ], + items: PANEL_MENU_ITEMS, + actions: PANEL_MENU_ACTIONS, }, ]; +// Compile-time guard: every menu must have exactly as many actions as labels, +// so `menu_action_at` can index `actions` with a validated `items` position. +// NOTE: this only checks the *lengths* line up — it cannot verify that each +// label is semantically paired with the right action; that mapping is the +// author's responsibility and is covered by the unit tests below. const _: () = const { let mut i = 0; while i < MENUS.len() { @@ -162,32 +165,81 @@ pub fn menu_item_count(menu: usize) -> usize { MENUS.get(menu).map_or(0, |entry| entry.items.len()) } -pub fn menu_bar_text_width() -> u16 { +/// Display width of one rendered title cell: the title plus its padding. +/// +/// `const fn`, so `MENU_BAR_TEXT_WIDTH` below and the per-index prefix sums can +/// be folded at compile time instead of re-walking `MENUS` on every frame. +/// Menu titles are ASCII (enforced by `_TITLES_ARE_ASCII`), so byte length +/// equals the unicode display width here. +const fn const_title_width(title: &str) -> u16 { + (title.len() as u16).saturating_add(MENU_TITLE_PADDING as u16) +} + +/// Total rendered width of the whole menu bar (all titles + the separators +/// between them). Precomputed once at compile time from `MENUS`; this is the +/// single source of truth shared by `menu_bar_start_x` and `menu_title_x`. +const MENU_BAR_TEXT_WIDTH: u16 = { let mut total: u16 = 0; - for (i, entry) in MENUS.iter().enumerate() { + let mut i = 0; + while i < MENUS.len() { if i > 0 { total = total.saturating_add(MENU_TITLE_SEPARATOR as u16); } - total = total.saturating_add(menu_title_width(entry.title)); + total = total.saturating_add(const_title_width(MENUS[i].title)); + i += 1; } total +}; + +// Guard the ASCII assumption baked into `const_title_width`: if a non-ASCII +// title is ever added, byte length would diverge from the unicode display +// width and the precomputed offsets would be wrong, so fail the build instead. +const _TITLES_ARE_ASCII: () = const { + let mut i = 0; + while i < MENUS.len() { + assert!(MENUS[i].title.is_ascii(), "menu titles must be ASCII"); + i += 1; + } +}; + +pub fn menu_bar_text_width() -> u16 { + MENU_BAR_TEXT_WIDTH } pub fn menu_bar_start_x(width: u16) -> u16 { - width.saturating_sub(menu_bar_text_width()) / 2 + width.saturating_sub(MENU_BAR_TEXT_WIDTH) / 2 } pub fn menu_title_width(title: &str) -> u16 { - let title_w: u16 = UnicodeWidthStr::width(title).try_into().unwrap_or(0); + let title_w: u16 = match u16::try_from(UnicodeWidthStr::width(title)) { + Ok(w) => w, + Err(_) => { + // A title wider than u16::MAX columns cannot really happen (titles + // are short ASCII labels), but if it ever did, silently clamping to + // 0 used to collapse the title to zero width and mis-position every + // following menu with no trace. Saturate to the visible maximum and + // leave a breadcrumb instead. + crate::debug_log!("menu_title_width: title width overflowed u16, clamping: {title:?}"); + u16::MAX + } + }; title_w.saturating_add(MENU_TITLE_PADDING as u16) } -pub fn menu_title_x(width: u16, index: usize) -> u16 { - let mut x = menu_bar_start_x(width); +/// Horizontal offset of menu `index`'s title relative to `menu_bar_start_x`: +/// the summed width of every preceding title plus the separators between them. +fn menu_prefix_width(index: usize) -> u16 { + let mut prefix: u16 = 0; for entry in MENUS.iter().take(index) { - x = x.saturating_add(menu_title_width(entry.title) + MENU_TITLE_SEPARATOR as u16); + prefix = prefix + .saturating_add(menu_title_width(entry.title)) + .saturating_add(MENU_TITLE_SEPARATOR as u16); } - x + prefix +} + +pub fn menu_title_x(width: u16, index: usize) -> u16 { + menu_bar_start_x(width).saturating_add(menu_prefix_width(index)) } pub fn menu_dropdown_x(menu_bar_area: Rect, selected_menu: usize, dropdown_width: u16) -> u16 { diff --git a/src/tests/dialogs.rs b/src/tests/dialogs.rs index 734e591..bd44ee0 100644 --- a/src/tests/dialogs.rs +++ b/src/tests/dialogs.rs @@ -56,13 +56,7 @@ fn confirm_enter_without_pending_action_dismisses_dialog() { ..Default::default() }; - dialogs::handle_dialog( - &mut state, - &mut None, - &mut None, - KeyCode::Enter, - Size::new(80, 24), - ); + dialog_key(&mut state, KeyCode::Enter, Size::new(80, 24)); assert_eq!(state.mode, AppMode::Normal); } @@ -92,13 +86,7 @@ fn confirm_enter_with_pending_action_starts_action() { .set_entries(vec![entry("delme.txt").build()]); state.left_panel.cursor = 0; - dialogs::handle_dialog( - &mut state, - &mut None, - &mut None, - KeyCode::Enter, - Size::new(80, 24), - ); + dialog_key(&mut state, KeyCode::Enter, Size::new(80, 24)); assert!(!matches!( state.mode, @@ -336,13 +324,7 @@ fn chmod_valid_input_applies_mode_and_dismisses() { state.left_panel.cursor = 0; state.active_panel = app::types::ActivePanel::Left; - dialogs::handle_dialog( - &mut state, - &mut None, - &mut None, - KeyCode::Enter, - Size::new(80, 24), - ); + dialog_key(&mut state, KeyCode::Enter, Size::new(80, 24)); assert_eq!(state.mode, AppMode::Normal); let meta = std::fs::metadata(&file).unwrap(); @@ -365,13 +347,7 @@ fn chmod_invalid_input_shows_error_stays_in_dialog() { state.left_panel.cursor = 0; state.active_panel = app::types::ActivePanel::Left; - dialogs::handle_dialog( - &mut state, - &mut None, - &mut None, - KeyCode::Enter, - Size::new(80, 24), - ); + dialog_key(&mut state, KeyCode::Enter, Size::new(80, 24)); assert!( matches!(state.mode, AppMode::Dialog(DialogKind::Input { .. })), @@ -411,13 +387,7 @@ fn chmod_esc_dismisses_without_changing_mode() { state.left_panel.cursor = 0; state.active_panel = app::types::ActivePanel::Left; - dialogs::handle_dialog( - &mut state, - &mut None, - &mut None, - KeyCode::Esc, - Size::new(80, 24), - ); + dialog_key(&mut state, KeyCode::Esc, Size::new(80, 24)); assert_eq!(state.mode, AppMode::Normal); let meta = std::fs::metadata(&file).unwrap(); diff --git a/src/tests/helpers.rs b/src/tests/helpers.rs index 20cad62..b1a215c 100644 --- a/src/tests/helpers.rs +++ b/src/tests/helpers.rs @@ -15,9 +15,21 @@ pub fn test_size() -> Size { Size::new(TERMINAL_WIDTH, TERMINAL_HEIGHT) } +/// Everything `dispatch_event` may have produced as a side effect, captured for +/// inspection. The struct is constructed *only* by [`dispatch_test_event`], so +/// fields can be added freely: every call-site destructures with a `..` rest +/// pattern (`DispatchResult { handled, .. }`) and stays source-compatible. +/// +/// `viewer_loader` is the background viewer-loader handle that `dispatch_event` +/// leaves in the `EventContext`. Exposing it lets a test assert that an action +/// *did* (or did not) kick off a viewer load: +/// `dispatch_test_event_exposes_viewer_loader_on_f3` drives F3 on a file and +/// asserts `viewer_loader` is populated. Previously it was a hardcoded throwaway +/// inside the harness and thus unobservable. pub struct DispatchResult { pub handled: Result, pub viewer: Option, + pub viewer_loader: Option, pub job: Option, } @@ -27,21 +39,27 @@ pub fn dispatch_test_event( event: &Event, ) -> DispatchResult { let mut viewer: Option = None; + let mut viewer_loader: Option = None; + let mut image_preview_loader: Option = None; let mut job: Option = None; - let mut size = test_size(); - let handled = super::super::dispatch_event( - state, - &mut viewer, - &mut None, - &mut None, - &mut job, - terminal, - &mut size, - event, - ); + let handled = { + let mut ctx = crate::input::EventContext { + state, + viewer_state: &mut viewer, + viewer_loader: &mut viewer_loader, + image_preview_loader: &mut image_preview_loader, + running_job: &mut job, + term_size: test_size(), + }; + super::super::dispatch_event(&mut ctx, terminal, event) + }; + // `image_preview_loader` is bound only to satisfy `EventContext`'s `&mut` + // field; it is intentionally not exposed on `DispatchResult` because no test + // reads it (and any loader it captured is dropped here, cancelling its worker). DispatchResult { handled, viewer, + viewer_loader, job, } } @@ -50,6 +68,13 @@ pub fn test_terminal() -> Terminal { Terminal::new(TestBackend::new(TERMINAL_WIDTH, TERMINAL_HEIGHT)).unwrap() } +// `TestEntry` is defined under `src/app/types/` because it builds a +// `crate::app::types::FileEntry` via the lib-private `FileEntry::builder()` — +// machinery only reachable from inside the `lc` library crate under `cfg(test)`. +// This integration suite is a *separate* test crate, so it cannot name that +// module directly. The `#[path]` attribute mounts the very same source file into +// this crate's module tree, re-using one `TestEntry` builder across both worlds +// instead of duplicating it (which would drift out of sync with `FileEntry`). #[path = "../app/types/test_helpers.rs"] mod test_helpers; @@ -61,6 +86,11 @@ pub fn test_path(name: impl AsRef) -> std::path::PathBuf { pub fn buffer_to_string(buffer: &ratatui::buffer::Buffer) -> String { let area = buffer.area(); + // Pre-size for the worst case: every cell contributes one char (`width` + // per row) plus one `'\n'` separator per row — hence the `+ 1`. (Strictly + // there are only `height - 1` separators since the last row has no trailing + // newline, but rounding up by one row avoids a reallocation and keeps the + // arithmetic obvious. Wide glyphs may push past this, but it is just a hint.) let mut result = String::with_capacity((area.width as usize + 1) * area.height as usize); for y in 0..area.height { if y > 0 { @@ -81,15 +111,38 @@ pub fn dispatch_key( modifiers: KeyModifiers, terminal: &mut Terminal, ) { - handle_normal_mode( + let mut viewer_state = None; + let mut viewer_loader = None; + let mut image_preview_loader = None; + let mut running_job = None; + let mut ctx = crate::input::EventContext { + state, + viewer_state: &mut viewer_state, + viewer_loader: &mut viewer_loader, + image_preview_loader: &mut image_preview_loader, + running_job: &mut running_job, + term_size: test_size(), + }; + handle_normal_mode(&mut ctx, key, modifiers, terminal); +} + +/// Drive `handle_dialog` against a freshly built `EventContext`. The viewer and +/// running-job handles are local throwaways for the all-`None` call-sites; tests +/// that need to inspect the job after the call should build the context inline. +pub fn dialog_key(state: &mut AppState, key: KeyCode, size: Size) { + let mut viewer_state = None; + let mut viewer_loader = None; + let mut image_preview_loader = None; + let mut running_job = None; + let mut ctx = crate::input::EventContext { state, - &mut None, - &mut None, - key, - modifiers, - TERMINAL_HEIGHT, - terminal, - ); + viewer_state: &mut viewer_state, + viewer_loader: &mut viewer_loader, + image_preview_loader: &mut image_preview_loader, + running_job: &mut running_job, + term_size: size, + }; + crate::input::dialogs::handle_dialog(&mut ctx, key); } pub fn dummy_tree_entries(count: usize) -> Vec { diff --git a/src/tests/history.rs b/src/tests/history.rs index 264f336..e24acff 100644 --- a/src/tests/history.rs +++ b/src/tests/history.rs @@ -65,11 +65,16 @@ fn history_picker_navigate_up_down() { assert_eq!(state.ui.picker_selected, 0); } +// The picker IS open (mode is ListPicker); the old name "does_not_open" was +// misleading. What actually happens: pressing Enter with an empty history has +// nothing to select, so the handler closes the picker back to Normal mode and +// loads no command line. #[test] -fn empty_history_does_not_open_picker() { +fn history_picker_enter_on_empty_history_closes_to_normal() { let mut state = make_history_picker(&[], 0); pickers::handle_list_picker(&mut state, KeyCode::Enter); assert_eq!(state.mode, AppMode::Normal); + assert!(state.input.command_line.text().is_empty()); } #[test] @@ -146,3 +151,87 @@ fn history_picker_empty_list_all_directions_clamped() { assert_eq!(state.ui.picker_selected, 0, "failed on {key:?}"); } } + +// PageUp/PageDown are not wired into the picker's navigation (only +// Up/Down/Home/End are), so they fall through as no-ops and leave the cursor +// where it was — they do not page through history entries. +#[test] +fn history_picker_page_up_down_are_noops() { + let mut state = make_history_picker(&["cmd1", "cmd2", "cmd3"], 1); + + pickers::handle_list_picker(&mut state, KeyCode::PageDown); + assert_eq!(state.ui.picker_selected, 1, "PageDown does not move cursor"); + + pickers::handle_list_picker(&mut state, KeyCode::PageUp); + assert_eq!(state.ui.picker_selected, 1, "PageUp does not move cursor"); +} + +// A typed character inside the history picker is inert: the history picker does +// not filter, so a char neither moves the cursor nor closes the picker nor +// loads a command line. +#[test] +fn history_picker_typed_char_is_noop() { + let mut state = make_history_picker(&["git status", "git log"], 0); + + pickers::handle_list_picker(&mut state, KeyCode::Char('g')); + + assert_eq!(state.ui.picker_selected, 0, "char does not move cursor"); + assert_eq!( + state.mode, + AppMode::ListPicker(PickerKind::History), + "char does not close the picker" + ); + assert!( + state.input.command_line.text().is_empty(), + "char does not load a command line" + ); +} + +// Dedup interaction with the cap: with the buffer full at MAX_HISTORY (100) +// unique entries, re-pushing one that already exists (here the oldest, +// "cmd_0", currently at the front) must NOT drop an unrelated entry. `retain` +// removes the existing copy first (len 99), then push_back re-appends it +// (len 100), so no pop_front fires. Net effect: still 100 entries, the +// duplicate moved to the most-recent slot, and "cmd_1" is now the oldest. No +// unique command is lost to the cap. +#[test] +fn history_dedup_of_existing_entry_at_cap_preserves_all_uniques() { + let mut state = AppState::default(); + for i in 0..100 { + shell::push_history(&mut state, &format!("cmd_{i}")); + } + assert_eq!(state.input.command_history.len(), 100); + assert_eq!(state.input.command_history[0], "cmd_0"); + + // Re-push the entry currently at the front (oldest) while the buffer is full. + shell::push_history(&mut state, "cmd_0"); + + assert_eq!( + state.input.command_history.len(), + 100, + "dedup keeps the buffer at cap without growing" + ); + assert_eq!( + state.input.command_history[0], "cmd_1", + "cmd_1 becomes the oldest after cmd_0 was deduped to the front" + ); + assert_eq!( + state.input.command_history[99], "cmd_0", + "the re-pushed duplicate is now the most recent" + ); + // No unique command was evicted by the cap: cmd_1..=cmd_99 plus cmd_0 all + // still present exactly once. + for i in 0..100 { + let cmd = format!("cmd_{i}"); + assert_eq!( + state + .input + .command_history + .iter() + .filter(|e| **e == cmd) + .count(), + 1, + "{cmd} present exactly once" + ); + } +} diff --git a/src/tests/keybinds.rs b/src/tests/keybinds.rs index 1eb5509..627a1e8 100644 --- a/src/tests/keybinds.rs +++ b/src/tests/keybinds.rs @@ -10,6 +10,12 @@ use lc::app::types::{ActivePanel, AppMode, AppState, DialogKind, InputAction}; use ratatui::{Terminal, backend::TestBackend}; use std::path::PathBuf; +/// Terminal height used by the directory-tree paging tests. +const TREE_TERM_HEIGHT: u16 = 12; +/// Visible tree rows for `TREE_TERM_HEIGHT` (height minus the tree chrome +/// overhead). One PageDown/PageUp moves the selection by this many rows. +const TREE_PAGE_STEP: usize = (TREE_TERM_HEIGHT - lc::ui::DIR_TREE_OVERHEAD_ROWS) as usize; + fn setup_ctrl_test() -> (Terminal, AppState) { (test_terminal(), AppState::default()) } @@ -375,28 +381,59 @@ fn directory_tree_page_down_uses_terminal_height() { ..Default::default() }; - directory_tree::handle_directory_tree(&mut state, &mut None, &mut None, KeyCode::PageDown, 12); - - assert_eq!(state.tree.selected, 9); - assert_eq!(state.tree.scroll, 9); + { + let mut no_viewer = None; + let mut no_loader = None; + let mut no_image = None; + let mut no_job = None; + let mut ctx = crate::input::EventContext { + state: &mut state, + viewer_state: &mut no_viewer, + viewer_loader: &mut no_loader, + image_preview_loader: &mut no_image, + running_job: &mut no_job, + term_size: ratatui::layout::Size::new(80, TREE_TERM_HEIGHT), + }; + directory_tree::handle_directory_tree(&mut ctx, KeyCode::PageDown); + } + + // From the top, one PageDown advances by a full visible page. + assert_eq!(state.tree.selected, TREE_PAGE_STEP); + assert_eq!(state.tree.scroll, TREE_PAGE_STEP); } #[test] fn directory_tree_page_up_uses_terminal_height() { + const START: usize = 25; let mut state = AppState { tree: app::types::TreeState { entries: dummy_tree_entries(50), - selected: 25, - scroll: 25, + selected: START, + scroll: START, ..Default::default() }, ..Default::default() }; - directory_tree::handle_directory_tree(&mut state, &mut None, &mut None, KeyCode::PageUp, 12); - - assert_eq!(state.tree.selected, 16); - assert_eq!(state.tree.scroll, 16); + { + let mut no_viewer = None; + let mut no_loader = None; + let mut no_image = None; + let mut no_job = None; + let mut ctx = crate::input::EventContext { + state: &mut state, + viewer_state: &mut no_viewer, + viewer_loader: &mut no_loader, + image_preview_loader: &mut no_image, + running_job: &mut no_job, + term_size: ratatui::layout::Size::new(80, TREE_TERM_HEIGHT), + }; + directory_tree::handle_directory_tree(&mut ctx, KeyCode::PageUp); + } + + // One PageUp retreats by a full visible page. + assert_eq!(state.tree.selected, START - TREE_PAGE_STEP); + assert_eq!(state.tree.scroll, START - TREE_PAGE_STEP); } #[test] @@ -411,3 +448,135 @@ fn command_line_up_loads_last_history_entry() { assert_eq!(state.input.command_line.text(), "git status"); } + +#[test] +fn f4_no_current_entry_does_not_launch() { + let mut state = AppState::default(); + let mut viewer = None; + let mut terminal = test_terminal(); + handle_function_keys(&mut state, &mut viewer, KeyCode::F(4), &mut terminal); + assert!(matches!(state.mode, AppMode::Normal)); +} + +#[test] +fn f5_opens_copy_confirm_dialog() { + let mut state = AppState::default(); + state + .left_panel + .set_entries(vec![entry("file.txt").file(10).build()]); + state.left_panel.cursor = 0; + state.active_panel = ActivePanel::Left; + state.right_panel.set_path(PathBuf::from("/tmp/dest")); + let mut viewer = None; + let mut terminal = test_terminal(); + + handle_function_keys(&mut state, &mut viewer, KeyCode::F(5), &mut terminal); + + assert!(matches!( + state.mode, + AppMode::Dialog(app::types::DialogKind::Confirm(_)) + )); + assert!(matches!( + state.ui.pending_action, + Some(app::types::PendingAction::Copy(_)) + )); +} + +#[test] +fn f6_opens_move_confirm_dialog() { + let mut state = AppState::default(); + state + .left_panel + .set_entries(vec![entry("file.txt").file(10).build()]); + state.left_panel.cursor = 0; + state.active_panel = ActivePanel::Left; + state.right_panel.set_path(PathBuf::from("/tmp/dest")); + let mut viewer = None; + let mut terminal = test_terminal(); + + handle_function_keys(&mut state, &mut viewer, KeyCode::F(6), &mut terminal); + + assert!(matches!( + state.mode, + AppMode::Dialog(app::types::DialogKind::Confirm(_)) + )); + assert!(matches!( + state.ui.pending_action, + Some(app::types::PendingAction::Move(_)) + )); +} + +#[test] +fn f5_with_empty_panel_does_nothing() { + let mut state = AppState { + active_panel: ActivePanel::Left, + ..Default::default() + }; + state.right_panel.set_path(PathBuf::from("/tmp/dest")); + let mut viewer = None; + let mut terminal = test_terminal(); + + handle_function_keys(&mut state, &mut viewer, KeyCode::F(5), &mut terminal); + + assert!(matches!(state.mode, AppMode::Normal)); + assert!(state.ui.pending_action.is_none()); +} + +#[test] +fn ctrl_down_arrow_falls_through_to_navigation() { + // Navigation ignores Ctrl on arrow keys: Ctrl+Down behaves like a plain + // Down and advances the cursor. + let mut state = AppState::default(); + state + .left_panel + .set_entries(vec![entry("a").build(), entry("b").build()]); + state.active_panel = ActivePanel::Left; + state.left_panel.cursor = 0; + + handle_navigation_keys( + &mut state, + KeyCode::Down, + KeyModifiers::CONTROL, + VISIBLE_HEIGHT, + ); + + assert_eq!(state.left_panel.cursor, 1); +} + +#[test] +fn alt_up_arrow_falls_through_to_navigation() { + // Navigation ignores Alt on arrow keys: Alt+Up behaves like a plain Up. + let mut state = AppState::default(); + state + .left_panel + .set_entries(vec![entry("a").build(), entry("b").build()]); + state.active_panel = ActivePanel::Left; + state.left_panel.cursor = 1; + + handle_navigation_keys(&mut state, KeyCode::Up, KeyModifiers::ALT, VISIBLE_HEIGHT); + + assert_eq!(state.left_panel.cursor, 0); +} + +#[test] +fn page_down_clamps_to_last_entry() { + // A panel shorter than one page must clamp the cursor at the final entry + // rather than overshoot past the end. + let mut state = AppState::default(); + state.left_panel.set_entries(vec![ + entry("a").build(), + entry("b").build(), + entry("c").build(), + ]); + state.active_panel = ActivePanel::Left; + state.left_panel.cursor = 0; + + handle_navigation_keys( + &mut state, + KeyCode::PageDown, + KeyModifiers::NONE, + VISIBLE_HEIGHT, + ); + + assert_eq!(state.left_panel.cursor, 2); +} diff --git a/src/tests/keyevents.rs b/src/tests/keyevents.rs index ca94edd..15d9629 100644 --- a/src/tests/keyevents.rs +++ b/src/tests/keyevents.rs @@ -1,7 +1,10 @@ use super::helpers::*; use crossterm::event::KeyCode; use crossterm::event::{Event, KeyEvent, KeyEventKind, KeyModifiers, MouseButton, MouseEventKind}; -use lc::app::types::{ActivePanel, AppMode, AppState, DialogKind, InputAction, TextInput}; +use lc::app::types::{ + ActivePanel, AppMode, AppState, ConfirmDetails, DialogKind, InputAction, PendingAction, + TextInput, +}; fn panel_with_files(names: &[&str]) -> (tempfile::TempDir, AppState) { let tmp = tempfile::tempdir().unwrap(); @@ -43,9 +46,9 @@ fn dispatch_unhandled_event_returns_false() { #[test] fn dispatch_mouse_click_moves_cursor() { - let (tmp, mut state) = panel_with_files(&["a.txt", "b.txt"]); - std::fs::write(tmp.path().join("a.txt"), b"a").unwrap(); - std::fs::write(tmp.path().join("b.txt"), b"b").unwrap(); + // Cursor positioning is pure logic over the in-memory entries, so no + // on-disk files are needed for this test. + let (_tmp, mut state) = panel_with_files(&["a.txt", "b.txt"]); state.left_panel.cursor = 1; // row=2, col=2 maps to first visible entry (index 0) @@ -59,7 +62,10 @@ fn dispatch_mouse_click_moves_cursor() { let DispatchResult { handled, .. } = dispatch_test_event(&mut state, &mut terminal, &event); - assert!(handled.is_ok()); + // The click is consumed as a side effect (cursor move) but the mouse + // handler reports it as not producing a follow-up key event, so dispatch + // returns Ok(false). + assert_eq!(handled, Ok(false)); assert_eq!(state.left_panel.cursor, 0); assert_eq!(state.active_panel, ActivePanel::Left); } @@ -73,7 +79,7 @@ fn key_press_triggers_search_initiation() { let DispatchResult { handled, .. } = dispatch_test_event(&mut state, &mut terminal, &Event::Key(key)); - assert!(handled.is_ok()); + assert_eq!(handled, Ok(true)); assert!(matches!(state.mode, AppMode::Search)); } @@ -90,23 +96,25 @@ fn key_release_is_ignored() { let DispatchResult { handled, .. } = dispatch_test_event(&mut state, &mut terminal, &Event::Key(key)); - assert!(handled.is_ok()); + // Release events are gated out before any handler runs, so dispatch reports + // the event as unhandled. + assert_eq!(handled, Ok(false)); assert!(matches!(state.mode, AppMode::Normal)); } #[test] fn key_repeat_navigation_moves_cursor() { - let (tmp, mut state) = panel_with_files(&["a.txt", "b.txt", "c.txt"]); - std::fs::write(tmp.path().join("a.txt"), b"a").unwrap(); - std::fs::write(tmp.path().join("b.txt"), b"b").unwrap(); - std::fs::write(tmp.path().join("c.txt"), b"c").unwrap(); + // Navigation is pure cursor math over the in-memory entries; no on-disk + // files are required. + let (_tmp, mut state) = panel_with_files(&["a.txt", "b.txt", "c.txt"]); let mut terminal = test_terminal(); let key = KeyEvent::new_with_kind(KeyCode::Down, KeyModifiers::NONE, KeyEventKind::Repeat); let DispatchResult { handled, .. } = dispatch_test_event(&mut state, &mut terminal, &Event::Key(key)); - assert!(handled.is_ok()); + // Repeat is allowed for navigation keys, so the event is handled. + assert_eq!(handled, Ok(true)); assert_eq!(state.left_panel.cursor, 1); } @@ -131,7 +139,7 @@ fn key_repeat_text_edit_updates_input_dialog() { let DispatchResult { handled, .. } = dispatch_test_event(&mut state, &mut terminal, &Event::Key(key)); - assert!(handled.is_ok()); + assert_eq!(handled, Ok(true)); assert_eq!(state.input.dialog_input.text(), "a"); assert_eq!(state.input.dialog_input.cursor(), 1); } @@ -139,25 +147,115 @@ fn key_repeat_text_edit_updates_input_dialog() { #[test] fn key_repeat_destructive_is_ignored() { let (tmp, mut state) = panel_with_files(&["victim.txt"]); - std::fs::write(tmp.path().join("victim.txt"), b"x").unwrap(); + let victim = tmp.path().join("victim.txt"); + std::fs::write(&victim, b"x").unwrap(); + state.left_panel.cursor = 0; let mut terminal = test_terminal(); let key = KeyEvent::new_with_kind(KeyCode::F(8), KeyModifiers::NONE, KeyEventKind::Repeat); let DispatchResult { handled, .. } = dispatch_test_event(&mut state, &mut terminal, &Event::Key(key)); - assert!(handled.is_ok()); + // F8 (delete) is destructive: repeat is gated out, so dispatch reports the + // event as unhandled and no confirm dialog or pending action is created. + assert_eq!(handled, Ok(false)); assert!(matches!(state.mode, AppMode::Normal)); assert!(state.ui.pending_action.is_none()); + // The victim file must survive an ignored destructive repeat. + assert!( + victim.exists(), + "victim file must not be deleted by an ignored F8 repeat" + ); +} + +#[test] +fn dispatch_test_event_exposes_job_on_confirmed_delete() { + // Confirming a pending Delete spawns a background job, which dispatch + // surfaces through `DispatchResult.job`. Drive the full key path + // (Confirm dialog + Enter) so the job handle is produced by the real + // handler rather than constructed by the test. + let (tmp, mut state) = panel_with_files(&["victim.txt"]); + let victim = tmp.path().join("victim.txt"); + std::fs::write(&victim, b"x").unwrap(); + state.left_panel.cursor = 0; + state.mode = AppMode::Dialog(DialogKind::Confirm(ConfirmDetails::simple( + "Delete", + "Delete selected?", + ))); + state.ui.pending_action = Some(PendingAction::Delete { + paths: vec![victim], + }); + + let mut terminal = test_terminal(); + let key = KeyEvent::new_with_kind(KeyCode::Enter, KeyModifiers::NONE, KeyEventKind::Press); + + let mut res = dispatch_test_event(&mut state, &mut terminal, &Event::Key(key)); + + assert_eq!(res.handled, Ok(true)); + assert!( + res.job.is_some(), + "confirmed Delete must produce a running job" + ); + assert!(matches!( + state.mode, + AppMode::Dialog(DialogKind::Progress { .. }) + )); + + // Join the spawned worker so the temp dir outlives the background delete. + if let Some(mut job) = res.job.take() { + job.shutdown(); + } +} + +#[test] +fn dispatch_test_event_exposes_viewer_loader_on_f3() { + // F3 on a regular file runs `view_current_entry`, which passes its `!is_dir` + // guard and calls `open_in_viewer` -> `ViewerState::open_background`. Dispatch + // surfaces the spawned background loader through `DispatchResult.viewer_loader`. + // The entry must be a *file* (default `TestEntry` kind is `Directory`, which + // the guard rejects), so build it with `.file(..)` and back it with a real + // on-disk file the loader can open. + let tmp = tempfile::tempdir().unwrap(); + let file = tmp.path().join("doc.txt"); + std::fs::write(&file, b"hello").unwrap(); + + let mut state = AppState { + active_panel: ActivePanel::Left, + ..Default::default() + }; + state.left_panel.set_path(tmp.path().to_path_buf()); + state + .left_panel + .set_entries(vec![TestEntry::new("doc.txt").path(file).file(5).build()]); + state.left_panel.cursor = 0; + + let mut terminal = test_terminal(); + let key = KeyEvent::new_with_kind(KeyCode::F(3), KeyModifiers::NONE, KeyEventKind::Press); + + let mut res = dispatch_test_event(&mut state, &mut terminal, &Event::Key(key)); + + assert!( + res.viewer_loader.is_some(), + "F3 on a file must start a background viewer loader" + ); + assert!(matches!(state.mode, AppMode::Viewing)); + + // Drop the loader explicitly: `ViewerLoader`'s `Drop` sets the cancel flag and + // detaches the worker (no thread leak, no blocking join), so scope exit is the + // whole cleanup. Drop before `tmp` so the worker is cancelled while the file + // still exists. + drop(res.viewer_loader.take()); } #[test] -fn dispatch_test_event_exposes_viewer_and_job() { +fn dispatch_test_event_viewer_and_job_default_none() { + // A no-op event (FocusGained) leaves both the viewer and job handles unset. let mut state = AppState::default(); let mut terminal = test_terminal(); let res = dispatch_test_event(&mut state, &mut terminal, &Event::FocusGained); + assert_eq!(res.handled, Ok(false)); assert!(res.viewer.is_none()); assert!(res.job.is_none()); } diff --git a/src/tests/menu.rs b/src/tests/menu.rs index 23b8080..577a336 100644 --- a/src/tests/menu.rs +++ b/src/tests/menu.rs @@ -2,19 +2,84 @@ use super::helpers::*; use crate::input::mode_dispatch::{handle_menu_mode, run_selected_menu_action}; use crossterm::event::KeyCode; use lc::app; -use lc::app::types::{ActivePanel, AppMode, AppState, PickerKind}; +use lc::app::types::{ActivePanel, AppMode, AppState, PickerKind, UiState}; -const _TEST_WIDTH: u16 = 24; const TEST_HEIGHT: u16 = 24; +// Named `(menu, item)` coordinates into the top menu bar (`lc::menu::MENUS`). +// Top-level order is 0:Left 1:File 2:Command 3:Options 4:Right; the second index +// is the position of the item within that menu's `items`/`actions` slice. These +// replace the bare numeric literals the tests used to set on `menu_selected` / +// `menu_item_selected`, so each test reads as the action it actually exercises. +const LEFT_SORT_ORDER: (usize, usize) = (0, 1); +const LEFT_RESET_FILTER: (usize, usize) = (0, 4); +const FILE_USER_MENU: (usize, usize) = (1, 0); +const FILE_RENAME: (usize, usize) = (1, 7); +const COMMAND_HISTORY: (usize, usize) = (2, 5); +const COMMAND_HOTLIST: (usize, usize) = (2, 6); +const COMMAND_COMMAND_LINE: (usize, usize) = (2, 7); +const OPTIONS_SHOW_HIDDEN: (usize, usize) = (3, 0); +const RIGHT_LISTING_MODE: (usize, usize) = (4, 0); +const RIGHT_SORT_ORDER: (usize, usize) = (4, 1); +const RIGHT_FILTER: (usize, usize) = (4, 2); +const RIGHT_REFRESH: (usize, usize) = (4, 3); + +/// Build an `AppState` parked in `Menu` mode with the given menu/item selected. +/// Collapses the `AppState { mode: Menu, ui: UiState { .. }, .. }` boilerplate +/// repeated across these tests into one factory. +fn menu_state((menu, item): (usize, usize)) -> AppState { + AppState { + mode: AppMode::Menu, + ui: UiState { + menu_selected: menu, + menu_item_selected: item, + ..Default::default() + }, + ..Default::default() + } +} + +/// Like `menu_state`, but also records the mode the menu was opened from +/// (`prev_mode`) so cancel/restore behaviour can be exercised. +fn menu_state_from((menu, item): (usize, usize), prev_mode: AppMode) -> AppState { + AppState { + prev_mode: Some(prev_mode), + ..menu_state((menu, item)) + } +} + fn dispatch_menu(state: &mut AppState, key: KeyCode) { let mut terminal = test_terminal(); - handle_menu_mode(state, &mut None, &mut None, key, TEST_HEIGHT, &mut terminal); + let mut viewer_state = None; + let mut viewer_loader = None; + let mut image_preview_loader = None; + let mut running_job = None; + let mut ctx = crate::input::EventContext { + state, + viewer_state: &mut viewer_state, + viewer_loader: &mut viewer_loader, + image_preview_loader: &mut image_preview_loader, + running_job: &mut running_job, + term_size: ratatui::layout::Size::new(80, TEST_HEIGHT), + }; + handle_menu_mode(&mut ctx, key, &mut terminal); } fn run_menu_action(state: &mut AppState) { let mut terminal = test_terminal(); - run_selected_menu_action(state, &mut None, &mut None, TEST_HEIGHT, &mut terminal); + let mut viewer_state = None; + let mut viewer_loader = None; + let mut image_preview_loader = None; + let mut running_job = None; + let mut ctx = crate::input::EventContext { + state, + viewer_state: &mut viewer_state, + viewer_loader: &mut viewer_loader, + image_preview_loader: &mut image_preview_loader, + running_job: &mut running_job, + term_size: ratatui::layout::Size::new(80, TEST_HEIGHT), + }; + run_selected_menu_action(&mut ctx, &mut terminal); } fn entry(name: &str) -> TestEntry { @@ -32,8 +97,7 @@ fn menu_toggle_hidden_files() { state.left_panel.set_path(temp_dir.path().to_path_buf()); state.left_panel.set_show_hidden(initial); state.mode = AppMode::Menu; - state.ui.menu_selected = 3; - state.ui.menu_item_selected = 0; + (state.ui.menu_selected, state.ui.menu_item_selected) = OPTIONS_SHOW_HIDDEN; dispatch_menu(&mut state, KeyCode::Enter); @@ -51,8 +115,7 @@ fn menu_rename_opens_input_dialog_with_current_name() { .build(), ]); state.mode = AppMode::Menu; - state.ui.menu_selected = 1; - state.ui.menu_item_selected = 7; + (state.ui.menu_selected, state.ui.menu_item_selected) = FILE_RENAME; dispatch_menu(&mut state, KeyCode::Enter); @@ -78,8 +141,7 @@ fn menu_rename_confirms_and_renames_file() { state.left_panel.cursor = 0; state.active_panel = ActivePanel::Left; state.mode = AppMode::Menu; - state.ui.menu_selected = 1; - state.ui.menu_item_selected = 7; + (state.ui.menu_selected, state.ui.menu_item_selected) = FILE_RENAME; dispatch_menu(&mut state, KeyCode::Enter); @@ -95,10 +157,8 @@ fn menu_rename_confirms_and_renames_file() { .dialog_input .set_text_at_end("new.txt".to_string()); - crate::input::dialogs::handle_dialog( + dialog_key( &mut state, - &mut None, - &mut None, KeyCode::Enter, ratatui::layout::Size::new(80, TEST_HEIGHT), ); @@ -114,15 +174,7 @@ fn menu_rename_confirms_and_renames_file() { #[test] fn menu_history_opens_picker() { - let mut state = AppState { - mode: AppMode::Menu, - ui: lc::app::types::UiState { - menu_selected: 2, - menu_item_selected: 5, - ..Default::default() - }, - ..Default::default() - }; + let mut state = menu_state(COMMAND_HISTORY); state.input.command_history.push_back("ls -la".to_string()); dispatch_menu(&mut state, KeyCode::Enter); @@ -134,15 +186,7 @@ fn menu_history_opens_picker() { #[test] fn menu_hotlist_opens_picker() { let tmp = tempfile::tempdir().unwrap(); - let mut state = AppState { - mode: AppMode::Menu, - ui: lc::app::types::UiState { - menu_selected: 2, - menu_item_selected: 6, - ..Default::default() - }, - ..Default::default() - }; + let mut state = menu_state(COMMAND_HOTLIST); state.hotlist_push(tmp.path().to_path_buf()); dispatch_menu(&mut state, KeyCode::Enter); @@ -153,15 +197,7 @@ fn menu_hotlist_opens_picker() { #[test] fn menu_sort_preserves_current_entry_focus() { - let mut state = AppState { - mode: AppMode::Menu, - ui: lc::app::types::UiState { - menu_selected: 0, - menu_item_selected: 1, - ..Default::default() - }, - ..Default::default() - }; + let mut state = menu_state(LEFT_SORT_ORDER); state .left_panel .set_entries(vec![entry("zeta.txt").build(), entry("alpha.txt").build()]); @@ -211,15 +247,7 @@ fn menu_sort_preserves_current_entry_focus() { #[test] fn menu_reset_filter_preserves_current_entry_focus() { - let mut state = AppState { - mode: AppMode::Menu, - ui: lc::app::types::UiState { - menu_selected: 0, - menu_item_selected: 4, - ..Default::default() - }, - ..Default::default() - }; + let mut state = menu_state(LEFT_RESET_FILTER); state .left_panel .set_entries(vec![entry("alpha.txt").build(), entry("beta.txt").build()]); @@ -240,16 +268,14 @@ fn menu_reset_filter_preserves_current_entry_focus() { ); } +/// `99` is a deliberately out-of-range item index: no menu has that many items, +/// so `menu_action_at` returns `None` and the action runner must fall back to +/// `Normal` instead of panicking or indexing past the actions slice. +const ITEM_OUT_OF_RANGE: usize = 99; + #[test] fn run_selected_menu_action_fallback_to_normal() { - let mut state = AppState { - mode: AppMode::Menu, - ui: lc::app::types::UiState { - menu_item_selected: 99, - ..Default::default() - }, - ..Default::default() - }; + let mut state = menu_state((0, ITEM_OUT_OF_RANGE)); run_menu_action(&mut state); @@ -258,16 +284,7 @@ fn run_selected_menu_action_fallback_to_normal() { #[test] fn menu_command_line_clears_stale_prev_mode() { - let mut state = AppState { - mode: AppMode::Menu, - prev_mode: Some(AppMode::Search), - ui: lc::app::types::UiState { - menu_selected: 2, - menu_item_selected: 7, - ..Default::default() - }, - ..Default::default() - }; + let mut state = menu_state_from(COMMAND_COMMAND_LINE, AppMode::Search); run_menu_action(&mut state); @@ -277,16 +294,7 @@ fn menu_command_line_clears_stale_prev_mode() { #[test] fn menu_right_panel_sort_changes_right_panel() { - let mut state = AppState { - mode: AppMode::Menu, - ui: lc::app::types::UiState { - menu_selected: 4, - menu_item_selected: 1, - ..Default::default() - }, - active_panel: ActivePanel::Left, - ..Default::default() - }; + let mut state = menu_state(RIGHT_SORT_ORDER); state .right_panel .set_sort_mode(lc::app::types::SortMode::new( @@ -309,16 +317,7 @@ fn menu_right_panel_sort_changes_right_panel() { #[test] fn menu_right_panel_filter_applies_to_right_panel() { - let mut state = AppState { - mode: AppMode::Menu, - ui: lc::app::types::UiState { - menu_selected: 4, - menu_item_selected: 2, - ..Default::default() - }, - active_panel: ActivePanel::Left, - ..Default::default() - }; + let mut state = menu_state(RIGHT_FILTER); dispatch_menu(&mut state, KeyCode::Enter); @@ -333,16 +332,7 @@ fn menu_right_panel_filter_applies_to_right_panel() { #[test] fn menu_right_panel_listing_mode_toggles_right() { - let mut state = AppState { - mode: AppMode::Menu, - ui: lc::app::types::UiState { - menu_selected: 4, - menu_item_selected: 0, - ..Default::default() - }, - active_panel: ActivePanel::Left, - ..Default::default() - }; + let mut state = menu_state(RIGHT_LISTING_MODE); let initial_mode = state.right_panel.listing_mode(); run_menu_action(&mut state); @@ -353,16 +343,7 @@ fn menu_right_panel_listing_mode_toggles_right() { #[test] fn menu_right_panel_refresh_refreshes_right() { let temp_dir = tempfile::tempdir().unwrap(); - let mut state = AppState { - mode: AppMode::Menu, - ui: lc::app::types::UiState { - menu_selected: 4, - menu_item_selected: 3, - ..Default::default() - }, - active_panel: ActivePanel::Left, - ..Default::default() - }; + let mut state = menu_state(RIGHT_REFRESH); state.right_panel.set_path(temp_dir.path().to_path_buf()); std::fs::write(temp_dir.path().join("test.txt"), "content").unwrap(); @@ -379,16 +360,7 @@ fn menu_right_panel_refresh_refreshes_right() { #[test] fn menu_cancel_from_search_restores_search_mode() { - let mut state = AppState { - mode: AppMode::Menu, - prev_mode: Some(AppMode::Search), - ui: lc::app::types::UiState { - menu_selected: 1, - menu_item_selected: 0, - ..Default::default() - }, - ..Default::default() - }; + let mut state = menu_state_from(FILE_USER_MENU, AppMode::Search); dispatch_menu(&mut state, KeyCode::Esc); @@ -398,16 +370,7 @@ fn menu_cancel_from_search_restores_search_mode() { #[test] fn menu_cancel_from_normal_returns_to_normal() { - let mut state = AppState { - mode: AppMode::Menu, - prev_mode: Some(AppMode::Normal), - ui: lc::app::types::UiState { - menu_selected: 1, - menu_item_selected: 0, - ..Default::default() - }, - ..Default::default() - }; + let mut state = menu_state_from(FILE_USER_MENU, AppMode::Normal); dispatch_menu(&mut state, KeyCode::Esc); @@ -417,16 +380,7 @@ fn menu_cancel_from_normal_returns_to_normal() { #[test] fn menu_cancel_with_no_prev_mode_defaults_to_normal() { - let mut state = AppState { - mode: AppMode::Menu, - prev_mode: None, - ui: lc::app::types::UiState { - menu_selected: 1, - menu_item_selected: 0, - ..Default::default() - }, - ..Default::default() - }; + let mut state = menu_state(FILE_USER_MENU); dispatch_menu(&mut state, KeyCode::Esc); @@ -436,16 +390,7 @@ fn menu_cancel_with_no_prev_mode_defaults_to_normal() { #[test] fn menu_cancel_with_f9_restores_prev_mode() { - let mut state = AppState { - mode: AppMode::Menu, - prev_mode: Some(AppMode::Viewing), - ui: lc::app::types::UiState { - menu_selected: 1, - menu_item_selected: 0, - ..Default::default() - }, - ..Default::default() - }; + let mut state = menu_state_from(FILE_USER_MENU, AppMode::Viewing); dispatch_menu(&mut state, KeyCode::F(9)); @@ -472,8 +417,7 @@ fn menu_rename_collision_shows_error_message() { state.left_panel.cursor = 0; state.active_panel = ActivePanel::Left; state.mode = AppMode::Menu; - state.ui.menu_selected = 1; - state.ui.menu_item_selected = 7; + (state.ui.menu_selected, state.ui.menu_item_selected) = FILE_RENAME; dispatch_menu(&mut state, KeyCode::Enter); @@ -482,10 +426,8 @@ fn menu_rename_collision_shows_error_message() { .dialog_input .set_text_at_end("existing.txt".to_string()); - crate::input::dialogs::handle_dialog( + dialog_key( &mut state, - &mut None, - &mut None, KeyCode::Enter, ratatui::layout::Size::new(80, TEST_HEIGHT), ); diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 7142d39..9366a16 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -2,34 +2,51 @@ //! Integration tests for Libre Commander. //! -//! Covers `AppState` harness, key bindings, search mode, dialogs, file viewer, -//! file operations (copy/move/delete/overwrite), pickers, menus, and selection. +//! These run as a separate test crate against the public `lc` API plus a small +//! amount of `cfg(test)` glue, driving real `AppState` through the input +//! dispatch and rendering paths. Each submodule owns one behavioural area: +//! +//! - `helpers` — shared harness (`dispatch_test_event`, `dispatch_key`, +//! `dialog_key`, `TestEntry`, buffer assertions). Other modules build on it. +//! - `compare` — `compare_directories` op: summary counts and unique-entry marking. +//! - `dialogs` — dialog input handling and rendered-buffer assertions. +//! - `history` — command-line history: dedup, capacity cap, picker load/cancel. +//! - `keybinds` — keymap config parsing and key-to-action resolution. +//! - `keyevents` — raw `crossterm` event dispatch (resize/focus/mouse/key-kind). +//! - `menu` — F9 menu bar: navigation and menu-action dispatch. +//! - `misc` — environment/XDG path resolution and other cross-cutting cases. +//! - `overwrite` — copy/move overwrite confirmation flow. +//! - `pickers` — hotlist and history pickers: add/dedup/delete/wrap navigation. +//! - `search` — incremental search mode. +//! - `selection` — mark/unmark of entries and shift-select movement. +//! - `user_menu` — user-defined (config-driven) menu entries. +//! - `viewer` — internal file viewer and image-preview loaders. -// File comparison tests +// `compare_directories` op: summary counts and unique-entry selection marking. mod compare; -// Dialog input and rendering tests +// Dialog input handling plus rendered-buffer assertions. mod dialogs; -// Shared test helpers and AppState harness +// Shared test harness: dispatch helpers, `TestEntry` builder, buffer utilities. mod helpers; -// Directory history navigation tests +// Command history: dedup, capacity cap, and history-picker load/cancel. mod history; -// Key binding configuration tests +// Keymap config parsing and key-to-action resolution. mod keybinds; -// Raw key event dispatch tests +// Raw crossterm event dispatch: resize, focus, mouse, and key-event kinds. mod keyevents; -// Menu bar tests +// F9 menu bar: navigation and menu-action dispatch. mod menu; -// Miscellaneous integration tests +// Cross-cutting cases: environment/XDG path resolution and other one-offs. mod misc; -// File overwrite confirmation tests +// Copy/move overwrite confirmation flow. mod overwrite; -// File/folder picker tests +// Hotlist and history pickers: add, dedup, delete, and wrap-around navigation. mod pickers; -// Search mode tests +// Incremental search mode. mod search; -// File selection (mark/unmark) tests +// Entry mark/unmark and shift-select cursor movement. mod selection; -// User-defined menu tests +// User-defined (config-driven) menu entries. mod user_menu; -// Internal viewer tests +// Internal file viewer and image-preview loader behaviour. mod viewer; diff --git a/src/tests/overwrite.rs b/src/tests/overwrite.rs index f778468..55e574a 100644 --- a/src/tests/overwrite.rs +++ b/src/tests/overwrite.rs @@ -516,13 +516,20 @@ fn archive_extract_enter_with_conflict_shows_overwrite_dialog_without_starting_a }; let mut running_job = None; - dialogs::handle_dialog( - &mut state, - &mut None, - &mut running_job, - KeyCode::Enter, - Size::new(80, 24), - ); + { + let mut viewer_state = None; + let mut viewer_loader = None; + let mut image_preview_loader = None; + let mut ctx = crate::input::EventContext { + state: &mut state, + viewer_state: &mut viewer_state, + viewer_loader: &mut viewer_loader, + image_preview_loader: &mut image_preview_loader, + running_job: &mut running_job, + term_size: Size::new(80, 24), + }; + dialogs::handle_dialog(&mut ctx, KeyCode::Enter); + } assert!(matches!( state.mode, diff --git a/src/tests/pickers.rs b/src/tests/pickers.rs index 712a349..9a67d07 100644 --- a/src/tests/pickers.rs +++ b/src/tests/pickers.rs @@ -73,8 +73,10 @@ fn hotlist_picker_delete_middle_and_last() { assert_eq!(state.ui.picker_selected, 0); } +// Up/Down do NOT wrap; on empty and single-item lists the cursor stays clamped +// at 0 in both directions (no wrap-around to the other end). #[test] -fn picker_wrap_empty_and_single() { +fn picker_navigation_clamps_at_bounds_empty_and_single() { let mut empty = picker_at(PickerKind::History, 0); pickers::handle_list_picker(&mut empty, KeyCode::Up); assert_eq!(empty.ui.picker_selected, 0); @@ -152,30 +154,146 @@ fn archive_menu_picker_esc_returns_normal() { assert_eq!(state.mode, AppMode::Normal); } +/// Number of options the `ArchiveMenu` picker exposes, derived at runtime +/// instead of hard-coded. The option list lives in a private `const ITEMS` +/// inside the production handler (and a twin in `render.rs`); neither is +/// reachable from tests. Rather than baking in a magic count (brittle the +/// moment an option is added/removed), we lean on the navigation contract: +/// `End` clamps the cursor to the last index, so `last_index + 1` is the +/// number of options. Adding a third archive option makes this return 3 +/// automatically with no test edits. +fn archive_menu_option_count() -> usize { + let mut state = picker_at(PickerKind::ArchiveMenu, 0); + pickers::handle_list_picker(&mut state, KeyCode::End); + state.ui.picker_selected + 1 +} + #[test] fn archive_menu_picker_navigate_bounds() { + let count = archive_menu_option_count(); + assert!(count >= 2, "expected at least Extract + Create options"); + let last = count - 1; let mut state = picker_at(PickerKind::ArchiveMenu, 0); + // Walk Down past the end: cursor advances one per key, then clamps at `last`. + for expected in 1..=last { + pickers::handle_list_picker(&mut state, KeyCode::Down); + assert_eq!(state.ui.picker_selected, expected); + } pickers::handle_list_picker(&mut state, KeyCode::Down); - assert_eq!(state.ui.picker_selected, 1); - - pickers::handle_list_picker(&mut state, KeyCode::Down); - assert_eq!(state.ui.picker_selected, 1); - - pickers::handle_list_picker(&mut state, KeyCode::Up); - assert_eq!(state.ui.picker_selected, 0); + assert_eq!(state.ui.picker_selected, last, "Down clamps at last index"); + // Walk Up past the start: cursor retreats one per key, then clamps at 0. + for expected in (0..last).rev() { + pickers::handle_list_picker(&mut state, KeyCode::Up); + assert_eq!(state.ui.picker_selected, expected); + } pickers::handle_list_picker(&mut state, KeyCode::Up); - assert_eq!(state.ui.picker_selected, 0); + assert_eq!(state.ui.picker_selected, 0, "Up clamps at first index"); } #[test] fn archive_menu_picker_home_end() { + let last = archive_menu_option_count() - 1; let mut state = picker_at(PickerKind::ArchiveMenu, 0); pickers::handle_list_picker(&mut state, KeyCode::End); - assert_eq!(state.ui.picker_selected, 1); + assert_eq!(state.ui.picker_selected, last); pickers::handle_list_picker(&mut state, KeyCode::Home); assert_eq!(state.ui.picker_selected, 0); } + +// Happy path: Enter on a hotlist picker performs the real selection action, +// not merely closing the picker. The selected entry (index 1 of two real +// directories) drives `navigate_to_hotlist`, which changes the active panel's +// path and reports the `cd to ...` status. Exercising two distinct temp dirs +// proves the cursor index actually selects the right entry. +#[test] +fn hotlist_picker_enter_navigates_to_selected_directory() { + let dir0 = tempfile::tempdir().unwrap(); + let dir1 = tempfile::tempdir().unwrap(); + let path0 = dir0.path().to_path_buf(); + let path1 = dir1.path().to_path_buf(); + + let mut state = AppState { + left_panel: PanelState::new(path0.clone()), + mode: AppMode::ListPicker(PickerKind::Hotlist), + ..Default::default() + }; + state.ui.directory_hotlist = vec![path0, path1.clone()]; + // Cursor on the second entry: Enter must navigate there, not to entry 0. + state.ui.picker_selected = 1; + + pickers::handle_list_picker(&mut state, KeyCode::Enter); + + assert_eq!(state.mode, AppMode::Normal, "Enter closes the picker"); + assert_eq!( + state.active_panel().path(), + path1.as_path(), + "active panel navigated to the selected hotlist entry" + ); + assert_eq!( + state.ui.status_message, + Some(format!("cd to {}", path1.display())), + "selection triggered the navigation action" + ); +} + +// PageUp/PageDown are not navigation keys for list pickers: `handle_nav_key` +// only maps Up/Down/Home/End, so these fall through as no-ops and leave the +// cursor untouched (they neither page nor clamp). This pins that contract so a +// future addition of paging would be a deliberate, test-visible change. +#[test] +fn picker_page_up_down_are_noops() { + let mut state = with_history(picker_at(PickerKind::History, 1), &["a", "b", "c"]); + + pickers::handle_list_picker(&mut state, KeyCode::PageDown); + assert_eq!(state.ui.picker_selected, 1, "PageDown does not move cursor"); + + pickers::handle_list_picker(&mut state, KeyCode::PageUp); + assert_eq!(state.ui.picker_selected, 1, "PageUp does not move cursor"); +} + +// A printable character that is not a picker hotkey is a no-op: non-filtering +// pickers (History here) ignore typed chars entirely — no cursor move, no mode +// change, no status. Only Hotlist's 'a'/'d' are meaningful hotkeys elsewhere. +#[test] +fn picker_typed_char_is_noop_when_not_a_hotkey() { + let mut state = with_history(picker_at(PickerKind::History, 1), &["a", "b", "c"]); + + pickers::handle_list_picker(&mut state, KeyCode::Char('x')); + + assert_eq!(state.ui.picker_selected, 1, "char does not move cursor"); + assert_eq!( + state.mode, + AppMode::ListPicker(PickerKind::History), + "char does not close the picker" + ); + assert_eq!(state.ui.status_message, None, "char produces no status"); +} + +// In the hotlist picker, 'a'/'d' are hotkeys but any other char is inert: it +// must not add/remove entries, move the cursor, or close the picker. +#[test] +fn hotlist_picker_unmapped_char_is_noop() { + let mut state = hotlist_state(&["/a", "/b", "/c"]); + state.ui.picker_selected = 1; + + pickers::handle_list_picker(&mut state, KeyCode::Char('z')); + + assert_eq!( + state.hotlist().len(), + 3, + "unmapped char does not mutate list" + ); + assert_eq!( + state.ui.picker_selected, 1, + "unmapped char does not move cursor" + ); + assert_eq!( + state.mode, + AppMode::ListPicker(PickerKind::Hotlist), + "unmapped char does not close the picker" + ); +} diff --git a/src/tests/user_menu.rs b/src/tests/user_menu.rs index deca28e..908051d 100644 --- a/src/tests/user_menu.rs +++ b/src/tests/user_menu.rs @@ -8,7 +8,6 @@ use lc::app; use lc::app::types::PickerKind; use lc::app::types::{AppMode, AppState, UiState}; use lc::app::user_menu::MenuSource; -use lc::ui::viewer; use std::path::Path; const FILE_MENU_INDEX: usize = 1; @@ -74,10 +73,6 @@ fn single_menu_entry() -> Vec { }] } -fn test_viewer_refs() -> (Option, Option) { - (None, None) -} - /// Build an [`AppState`] in `mode` with the given user-menu `entries` already /// loaded. Collapses the repeated `AppState { mode, ui: UiState { .. } }` /// literal used by the list-picker tests below. @@ -168,16 +163,19 @@ fn user_menu_file_menu_no_menu_file_shows_error() { let tmp = tempfile::tempdir().unwrap(); let mut terminal = test_terminal(); let mut state = test_menu_state(&tmp); - let (mut no_viewer, mut no_loader) = test_viewer_refs(); - - handle_menu_mode( - &mut state, - &mut no_viewer, - &mut no_loader, - KeyCode::Enter, - 24, - &mut terminal, - ); + let mut no_viewer = None; + let mut no_loader = None; + let mut no_image = None; + let mut no_job = None; + let mut ctx = crate::input::EventContext { + state: &mut state, + viewer_state: &mut no_viewer, + viewer_loader: &mut no_loader, + image_preview_loader: &mut no_image, + running_job: &mut no_job, + term_size: ratatui::layout::Size::new(80, 24), + }; + handle_menu_mode(&mut ctx, KeyCode::Enter, &mut terminal); assert!(matches!( state.mode, @@ -191,16 +189,19 @@ fn user_menu_file_menu_with_entries_opens_picker() { create_menu_file(tmp.path()); let mut terminal = test_terminal(); let mut state = test_menu_state(&tmp); - let (mut no_viewer, mut no_loader) = test_viewer_refs(); - - handle_menu_mode( - &mut state, - &mut no_viewer, - &mut no_loader, - KeyCode::Enter, - 24, - &mut terminal, - ); + let mut no_viewer = None; + let mut no_loader = None; + let mut no_image = None; + let mut no_job = None; + let mut ctx = crate::input::EventContext { + state: &mut state, + viewer_state: &mut no_viewer, + viewer_loader: &mut no_loader, + image_preview_loader: &mut no_image, + running_job: &mut no_job, + term_size: ratatui::layout::Size::new(80, 24), + }; + handle_menu_mode(&mut ctx, KeyCode::Enter, &mut terminal); assert_eq!(state.mode, AppMode::ListPicker(PickerKind::UserMenu)); assert_eq!(state.ui.picker_selected, 0); @@ -221,17 +222,19 @@ fn f2_loads_user_menu_file_with_entries() { let mut terminal = test_terminal(); let mut state = AppState::default(); state.left_panel.set_path(tmp.path().to_path_buf()); - let (mut no_viewer, mut no_loader) = test_viewer_refs(); - - handle_normal_mode( - &mut state, - &mut no_viewer, - &mut no_loader, - KeyCode::F(2), - KeyModifiers::NONE, - 24, - &mut terminal, - ); + let mut no_viewer = None; + let mut no_loader = None; + let mut no_image = None; + let mut no_job = None; + let mut ctx = crate::input::EventContext { + state: &mut state, + viewer_state: &mut no_viewer, + viewer_loader: &mut no_loader, + image_preview_loader: &mut no_image, + running_job: &mut no_job, + term_size: ratatui::layout::Size::new(80, 24), + }; + handle_normal_mode(&mut ctx, KeyCode::F(2), KeyModifiers::NONE, &mut terminal); assert_eq!(state.mode, AppMode::ListPicker(PickerKind::UserMenu)); assert_eq!(state.ui.user_menu_entries.len(), 2); @@ -250,17 +253,19 @@ fn f2_no_user_menu_file_shows_error() { let mut terminal = test_terminal(); let mut state = AppState::default(); state.left_panel.set_path(tmp.path().to_path_buf()); - let (mut no_viewer, mut no_loader) = test_viewer_refs(); - - handle_normal_mode( - &mut state, - &mut no_viewer, - &mut no_loader, - KeyCode::F(2), - KeyModifiers::NONE, - 24, - &mut terminal, - ); + let mut no_viewer = None; + let mut no_loader = None; + let mut no_image = None; + let mut no_job = None; + let mut ctx = crate::input::EventContext { + state: &mut state, + viewer_state: &mut no_viewer, + viewer_loader: &mut no_loader, + image_preview_loader: &mut no_image, + running_job: &mut no_job, + term_size: ratatui::layout::Size::new(80, 24), + }; + handle_normal_mode(&mut ctx, KeyCode::F(2), KeyModifiers::NONE, &mut terminal); assert!(matches!( state.mode, diff --git a/src/tests/viewer.rs b/src/tests/viewer.rs index 72c241c..60c5165 100644 --- a/src/tests/viewer.rs +++ b/src/tests/viewer.rs @@ -162,15 +162,21 @@ fn viewer_search_esc_keeps_viewer_previous_mode() { ..Default::default() }; let mut viewer: Option = None; + let mut viewer_loader = None; + let mut image_preview_loader = None; let mut job: Option = None; - dialogs::handle_dialog( - &mut state, - &mut viewer, - &mut job, - KeyCode::Esc, - TEST_VIEWPORT, - ); + { + let mut ctx = crate::input::EventContext { + state: &mut state, + viewer_state: &mut viewer, + viewer_loader: &mut viewer_loader, + image_preview_loader: &mut image_preview_loader, + running_job: &mut job, + term_size: TEST_VIEWPORT, + }; + dialogs::handle_dialog(&mut ctx, KeyCode::Esc); + } assert!(matches!(state.mode, AppMode::Viewing)); assert_eq!(state.prev_mode, Some(AppMode::Normal)); @@ -248,15 +254,19 @@ fn viewer_close_via_escape() { }; let mut viewer_loader: Option = None; let mut image_preview_loader: Option = None; + let mut job = None; - crate::input::mode_dispatch::handle_viewer_mode( - &mut state, - &mut viewer_state, - &mut viewer_loader, - &mut image_preview_loader, - KeyCode::Esc, - TEST_VIEWPORT, - ); + { + let mut ctx = crate::input::EventContext { + state: &mut state, + viewer_state: &mut viewer_state, + viewer_loader: &mut viewer_loader, + image_preview_loader: &mut image_preview_loader, + running_job: &mut job, + term_size: TEST_VIEWPORT, + }; + crate::input::mode_dispatch::handle_viewer_mode(&mut ctx, KeyCode::Esc); + } assert!(matches!(state.mode, AppMode::Normal)); assert!(viewer_state.is_none()); From 16914e5e53142328f90d9e04ce32158480167abe Mon Sep 17 00:00:00 2001 From: Leszek Date: Mon, 15 Jun 2026 00:43:10 +0200 Subject: [PATCH 2/3] fix(input): archive Left/Right no longer moves text cursor; trim input-action paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From external code review: - archive dialogs: Left/Right toggled OK/Cancel AND moved the dest-path text cursor — a regression from the PR8 extract/create handler merge (the old handlers returned early on Left/Right). Add ArchiveNav::Handled so the toggle no longer falls through to apply_dialog_text_edit. - handle_input_action: trim create-directory/rename/chmod/filter/quick-cd input so pasted leading/trailing whitespace no longer creates badly-named entries. Viewer search and find-file keep raw input (spaces are significant there, e.g. searching indented text). Regression tests added for both. Gate green: fmt, clippy -D warnings, 1227 tests, release build. --- src/input/dialogs.rs | 28 +++++++++++++++----- src/tests/keyevents.rs | 33 ++++++++++++++++++++++++ src/tests/overwrite.rs | 58 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 7 deletions(-) diff --git a/src/input/dialogs.rs b/src/input/dialogs.rs index 02df87e..c7b8528 100644 --- a/src/input/dialogs.rs +++ b/src/input/dialogs.rs @@ -388,17 +388,23 @@ fn handle_input_action( action: &InputAction, terminal_height: u16, ) { + // Raw input is preserved verbatim for the search actions (ViewerSearch, + // FindFile) where leading/trailing whitespace is meaningful (e.g. searching + // indented code like ` fn`). All filesystem-mutating / navigation actions + // trim it so pasted whitespace cannot create entries named `" foo "` and so + // whitespace-only input fails the non-empty validation as empty. let input = state.input.dialog_input.text().to_owned(); + let trimmed = input.trim(); let outcome = match action { InputAction::ViewerSearch => { input_action_viewer_search(state, viewer_state, &input, terminal_height) } - InputAction::CreateDirectory => input_action_create_directory(state, &input), - InputAction::Rename => input_action_rename(state, &input), - InputAction::Chmod => input_action_chmod(state, &input), - InputAction::Filter => input_action_filter(state, input, terminal_height), + InputAction::CreateDirectory => input_action_create_directory(state, trimmed), + InputAction::Rename => input_action_rename(state, trimmed), + InputAction::Chmod => input_action_chmod(state, trimmed), + InputAction::Filter => input_action_filter(state, trimmed.to_owned(), terminal_height), InputAction::QuickCd => { - handle_quick_cd(state, &input); + handle_quick_cd(state, trimmed); InputOutcome::ResetWithRefresh } InputAction::FindFile => { @@ -542,7 +548,12 @@ enum ArchiveNav { Commit, /// User cancelled (Esc, Cancel button) — dialog already dismissed. Dismissed, - /// A non-committing key (toggle/no-op) was handled; fall through to text edit. + /// Key fully handled (e.g. OK/Cancel toggle) — do NOT fall through to text + /// edit. Prevents Left/Right from both toggling the button selection and + /// moving the destination text cursor (double action). + Handled, + /// A non-committing key (no-op for nav) was not consumed; fall through to + /// text edit (Char/Backspace/Delete/Home/End). Continue, } @@ -559,7 +570,7 @@ fn archive_dialog_nav(state: &mut AppState, key: KeyCode) -> ArchiveNav { } else { 0 }; - ArchiveNav::Continue + ArchiveNav::Handled } KeyCode::Enter if state.input.dialog_selection == 1 => { dismiss_dialog(state); @@ -648,6 +659,9 @@ fn handle_archive_dialog( } return; } + // Key already consumed by nav (OK/Cancel toggle); must NOT also run text + // edit, otherwise Left/Right would move the dest-path cursor too. + ArchiveNav::Handled => return, ArchiveNav::Continue => {} } if let Some(dest_input) = active_archive_dest_input(state) { diff --git a/src/tests/keyevents.rs b/src/tests/keyevents.rs index 15d9629..ff6a061 100644 --- a/src/tests/keyevents.rs +++ b/src/tests/keyevents.rs @@ -144,6 +144,39 @@ fn key_repeat_text_edit_updates_input_dialog() { assert_eq!(state.input.dialog_input.cursor(), 1); } +#[test] +fn create_directory_trims_surrounding_whitespace() { + // Leading/trailing whitespace (e.g. from a paste) must be trimmed before the + // directory is created, so `" newdir "` resolves to `newdir`, not a dir + // named with literal spaces. + let (tmp, mut state) = panel_with_files(&[]); + state.mode = AppMode::Dialog(DialogKind::Input { + prompt: "Create directory:".to_string(), + action: InputAction::CreateDirectory, + }); + state.input.dialog_input = { + let mut ti = TextInput::new(); + ti.set_text(" newdir ".to_string()); + ti + }; + let mut terminal = test_terminal(); + let key = KeyEvent::new_with_kind(KeyCode::Enter, KeyModifiers::NONE, KeyEventKind::Press); + + let DispatchResult { handled, .. } = + dispatch_test_event(&mut state, &mut terminal, &Event::Key(key)); + + assert_eq!(handled, Ok(true)); + assert!( + tmp.path().join("newdir").is_dir(), + "trimmed `newdir` directory must be created" + ); + assert!( + !tmp.path().join(" newdir ").exists(), + "untrimmed directory name must NOT be created" + ); + assert!(matches!(state.mode, AppMode::Normal)); +} + #[test] fn key_repeat_destructive_is_ignored() { let (tmp, mut state) = panel_with_files(&["victim.txt"]); diff --git a/src/tests/overwrite.rs b/src/tests/overwrite.rs index 55e574a..4e07584 100644 --- a/src/tests/overwrite.rs +++ b/src/tests/overwrite.rs @@ -545,6 +545,64 @@ fn archive_extract_enter_with_conflict_shows_overwrite_dialog_without_starting_a )); } +// Regression: Left/Right in an archive dialog must ONLY toggle the OK/Cancel +// selection — it must NOT also fall through to the destination text editor and +// move the text cursor (a double action). See `ArchiveNav::Handled`. +#[test] +fn archive_extract_left_toggles_selection_without_moving_text_cursor() { + let tmp = tempfile::tempdir().unwrap(); + let archive = create_test_tar_gz(tmp.path(), &["file1.txt"]); + + let mut dest_input = TextInput::new(); + dest_input.set_text_at_end("abc".to_string()); + let cursor_before = dest_input.cursor(); + assert_eq!( + cursor_before, 3, + "precondition: cursor sits at end of \"abc\"" + ); + + let mut state = AppState { + mode: AppMode::Dialog(DialogKind::ArchiveExtract(Box::new( + ArchiveExtractDetails { + source: archive, + entries: vec![], + dest_input, + }, + ))), + input: InputState { + dialog_selection: 0, + ..Default::default() + }, + ..Default::default() + }; + let mut running_job = None; + + { + let mut viewer_state = None; + let mut viewer_loader = None; + let mut image_preview_loader = None; + let mut ctx = crate::input::EventContext { + state: &mut state, + viewer_state: &mut viewer_state, + viewer_loader: &mut viewer_loader, + image_preview_loader: &mut image_preview_loader, + running_job: &mut running_job, + term_size: Size::new(80, 24), + }; + dialogs::handle_dialog(&mut ctx, KeyCode::Left); + } + + // Selection toggled (0 -> 1): Left/Right was consumed as button navigation. + assert_eq!(state.input.dialog_selection, 1); + + // The destination text editor must be untouched: same text, same cursor. + let AppMode::Dialog(DialogKind::ArchiveExtract(details)) = &state.mode else { + panic!("dialog should still be ArchiveExtract"); + }; + assert_eq!(details.dest_input.text(), "abc"); + assert_eq!(details.dest_input.cursor(), cursor_before); +} + #[test] fn check_overwrite_pending_action_none_returns_none() { let state = AppState { From 81e54377a2a95515b2a7d3b17fc9154b685fb82b Mon Sep 17 00:00:00 2001 From: Leszek Date: Mon, 15 Jun 2026 00:53:32 +0200 Subject: [PATCH 3/3] fix(ui): cache function-bar cells in thread_local, not a static OnceLock ratatui-widgets 0.3.1 added a non-Sync Shadow (Arc) to Block, so Paragraph is no longer Sync and `static CELLS: OnceLock<[Paragraph; 10]>` failed to compile (E0277) on any non --locked build. Rendering is single-threaded, so move the once-built cell cache into a thread_local!, which does not require Sync. Builds against both ratatui-widgets 0.3.0 and 0.3.1; render behavior unchanged. --- src/ui/panels/mod.rs | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/src/ui/panels/mod.rs b/src/ui/panels/mod.rs index b24918d..66a7dd4 100644 --- a/src/ui/panels/mod.rs +++ b/src/ui/panels/mod.rs @@ -7,7 +7,6 @@ use ratatui::{ }; use std::borrow::Cow; use std::fmt::Write; -use std::sync::OnceLock; use unicode_width::UnicodeWidthStr; use super::theme::{ColorCtx, ColorPalette, DEFAULT_COLORS, IconTheme, Theme}; @@ -596,15 +595,25 @@ fn make_function_bar_cell(i: usize, key_style: Style, label_style: Style) -> Par Paragraph::new(line).block(Block::default().padding(Padding::new(1, 1, 0, 0))) } -/// The 10 function-bar cells for the built-in palette, built once. The bar is -/// static text and (for the default theme) static styling, so it is rendered by -/// reference instead of rebuilt every frame. -fn default_function_bar_cells() -> &'static [Paragraph<'static>; 10] { - static CELLS: OnceLock<[Paragraph<'static>; 10]> = OnceLock::new(); - CELLS.get_or_init(|| { - let (key_style, label_style) = function_bar_styles(&DEFAULT_COLORS); - std::array::from_fn(|i| make_function_bar_cell(i, key_style, label_style)) - }) +/// Render the default-palette function-bar cells, built once per render thread. +/// The bar is static text and (for the default theme) static styling, so the 10 +/// cells are cached and rendered by reference instead of rebuilt every frame. +/// `Paragraph` is no longer `Sync` (ratatui-widgets >=0.3.1 put a non-`Sync` +/// `Shadow` in `Block`), so the cache lives in a `thread_local!` instead of a +/// `static OnceLock`. Rendering is single-threaded, so this is equivalent to the +/// previous process-wide cache. +fn render_default_function_bar(f: &mut Frame, chunks: &[Rect]) { + thread_local! { + static CELLS: [Paragraph<'static>; 10] = { + let (key_style, label_style) = function_bar_styles(&DEFAULT_COLORS); + std::array::from_fn(|i| make_function_bar_cell(i, key_style, label_style)) + }; + } + CELLS.with(|cells| { + for (cell, chunk) in cells.iter().zip(chunks.iter()) { + f.render_widget(cell, *chunk); + } + }); } pub fn render_function_bar_with_colors(f: &mut Frame, area: Rect, colors: &ColorPalette) { @@ -619,10 +628,7 @@ pub fn render_function_bar_with_colors(f: &mut Frame, area: Rect, colors: &Color if colors.function_bar_fg == DEFAULT_COLORS.function_bar_fg && colors.function_bar_bg == DEFAULT_COLORS.function_bar_bg { - let cells = default_function_bar_cells(); - for (cell, chunk) in cells.iter().zip(chunks.iter()) { - f.render_widget(cell, *chunk); - } + render_default_function_bar(f, &chunks); return; }