From 6d8a7ed526f8818133244a536ae24972294bebcb Mon Sep 17 00:00:00 2001 From: lalvarezt Date: Thu, 24 Jul 2025 10:52:36 +0200 Subject: [PATCH] refactor: rework help panel screen to generate dynamically and based on global/channel keybindings --- benches/main/ui.rs | 2 + television/app.rs | 3 + television/config/keybindings.rs | 197 ++++++++++++++++++++++++-- television/config/mod.rs | 236 +++++++++++++++++++++++++++++-- television/draw.rs | 4 +- television/event.rs | 4 +- television/main.rs | 131 +---------------- television/screen/help_panel.rs | 213 +++++++++++++++++++++------- television/screen/keybindings.rs | 186 +++--------------------- television/screen/layout.rs | 10 +- television/television.rs | 14 +- tests/app.rs | 2 + 12 files changed, 623 insertions(+), 379 deletions(-) diff --git a/benches/main/ui.rs b/benches/main/ui.rs index be1f46d..984a623 100644 --- a/benches/main/ui.rs +++ b/benches/main/ui.rs @@ -16,6 +16,7 @@ use television::{ action::Action, cable::Cable, channels::entry::{Entry, into_ranges}, + cli::PostProcessedCli, config::{Config, ConfigEnv}, screen::{colors::ResultsColorscheme, result_item::build_results_list}, television::Television, @@ -490,6 +491,7 @@ pub fn draw(c: &mut Criterion) { Some(50), false, cable.clone(), + &PostProcessedCli::default(), ); tv.find("television"); for _ in 0..5 { diff --git a/television/app.rs b/television/app.rs index 858c039..ee6c61c 100644 --- a/television/app.rs +++ b/television/app.rs @@ -2,6 +2,7 @@ use crate::{ action::Action, cable::Cable, channels::{entry::Entry, prototypes::ChannelPrototype}, + cli::PostProcessedCli, config::{Config, DEFAULT_PREVIEW_SIZE, default_tick_rate}, event::{Event, EventLoop, InputEvent, Key, MouseInputEvent}, history::History, @@ -181,6 +182,7 @@ impl App { input: Option, options: AppOptions, cable_channels: Cable, + cli_args: &PostProcessedCli, ) -> Self { let (action_tx, action_rx) = mpsc::unbounded_channel(); let (render_tx, render_rx) = mpsc::unbounded_channel(); @@ -198,6 +200,7 @@ impl App { options.preview_size, options.exact, cable_channels, + cli_args, ); // Create input map from the merged config that includes both key and event bindings diff --git a/television/config/keybindings.rs b/television/config/keybindings.rs index 2252973..4ed464c 100644 --- a/television/config/keybindings.rs +++ b/television/config/keybindings.rs @@ -3,7 +3,7 @@ use crate::{ event::{Key, convert_raw_event_to_key}, }; use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; -use rustc_hash::FxHashMap; +use rustc_hash::{FxHashMap, FxHashSet}; use serde::{Deserialize, Serialize}; use std::fmt::Display; use std::hash::Hash; @@ -279,21 +279,119 @@ where } } +/// Represents the source type of keybindings being merged. +/// +/// # Variants +/// +/// * `Global` - Bindings from user configuration files or default settings +/// * `Channel` - Bindings specific to individual channels/data sources +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum BindingSource { + Global, + Channel, +} + +impl BindingSource { + /// Returns the string representation of the binding source. + pub fn as_str(&self) -> &'static str { + match self { + BindingSource::Global => "global", + BindingSource::Channel => "channel", + } + } +} + +/// Tracks source information for merged keybindings. +#[derive(Debug, Clone, Default, PartialEq)] +pub struct KeybindingSource { + /// Keys that come from channel-specific configuration + pub channel_keys: FxHashSet, + /// Keys that come from global configuration + pub global_keys: FxHashSet, +} + +impl KeybindingSource { + /// Check if a key comes from channel configuration + pub fn is_channel_key(&self, key: &Key) -> bool { + self.channel_keys.contains(key) + } + + /// Check if a key comes from global configuration + pub fn is_global_key(&self, key: &Key) -> bool { + self.global_keys.contains(key) + } + + /// Add a key as coming from channel configuration + pub fn add_channel_key(&mut self, key: Key) { + self.channel_keys.insert(key); + } + + /// Add a key as coming from global configuration + pub fn add_global_key(&mut self, key: Key) { + self.global_keys.insert(key); + } + + /// Check if there are any tracked keybindings + pub fn has_any_keys(&self) -> bool { + !self.channel_keys.is_empty() || !self.global_keys.is_empty() + } + + /// Check if there are any channel-specific keybindings + pub fn has_channel_keys(&self) -> bool { + !self.channel_keys.is_empty() + } + + /// Check if there are any global keybindings + pub fn has_global_keys(&self) -> bool { + !self.global_keys.is_empty() + } + + /// Get the total count of tracked keys + pub fn total_keys(&self) -> usize { + self.channel_keys.len() + self.global_keys.len() + } + + /// Clear all tracking information + pub fn clear(&mut self) { + self.channel_keys.clear(); + self.global_keys.clear(); + } +} + +impl Hash for KeybindingSource { + fn hash(&self, state: &mut H) { + let mut channel_sorted: Vec<_> = self.channel_keys.iter().collect(); + let mut global_sorted: Vec<_> = self.global_keys.iter().collect(); + + channel_sorted.sort(); // Key must implement Ord + global_sorted.sort(); + + for key in channel_sorted { + key.hash(state); + } + for key in global_sorted { + key.hash(state); + } + } +} + /// Merges two binding collections, with new bindings taking precedence. +/// Keeps track of the source information. /// /// # Arguments /// /// * `bindings` - The base bindings collection (will be consumed) /// * `new_bindings` - The new bindings to merge in (higher precedence) +/// * `source` - The source type of the new bindings being merged /// /// # Returns /// -/// A new `Bindings` collection with merged key mappings. +/// A tuple of (`merged_bindings`, `source_tracking`) /// /// # Examples /// /// ```rust -/// use television::config::keybindings::{KeyBindings, merge_bindings}; +/// use television::config::keybindings::{KeyBindings, merge_bindings, BindingSource}; /// use television::event::Key; /// use television::action::Action; /// @@ -307,23 +405,88 @@ where /// (Key::Tab, Action::ToggleSelectionDown), // Add new binding /// ]); /// -/// let merged = merge_bindings(base, &custom); -/// assert_eq!(merged.get(&Key::Enter), Some(&Action::ConfirmSelection.into())); -/// assert_eq!(merged.get(&Key::Esc), Some(&Action::NoOp.into())); -/// assert_eq!(merged.get(&Key::Tab), Some(&Action::ToggleSelectionDown.into())); +/// let (merged, source) = merge_bindings(base, &custom, BindingSource::Channel); +/// assert!(source.is_channel_key(&Key::Esc)); +/// assert!(source.is_channel_key(&Key::Tab)); +/// assert!(source.is_global_key(&Key::Enter)); /// ``` pub fn merge_bindings( mut bindings: Bindings, new_bindings: &Bindings, -) -> Bindings + source: BindingSource, +) -> (Bindings, KeybindingSource) where K: Display + FromStr + Clone + Eq + Hash, K::Err: Display, { - for (key, actions) in &new_bindings.bindings { - bindings.bindings.insert(key.clone(), actions.clone()); + use tracing::{debug, trace}; + + let mut keybinding_source = KeybindingSource::default(); + + debug!( + "Starting merge_bindings: {} existing bindings, {} new {} bindings", + bindings.bindings.len(), + new_bindings.bindings.len(), + source.as_str() + ); + + // Mark existing keys as global + for key in bindings.bindings.keys() { + if let Ok(tv_key) = Key::from_str(&key.to_string()) { + keybinding_source.add_global_key(tv_key); + trace!("Marked existing key '{}' as global", key); + } } - bindings + + // Merge new bindings and mark them based on source type + let mut merged_count = 0; + let mut override_count = 0; + + for (key, actions) in &new_bindings.bindings { + let was_existing = bindings.bindings.contains_key(key); + bindings.bindings.insert(key.clone(), actions.clone()); + merged_count += 1; + + if let Ok(tv_key) = Key::from_str(&key.to_string()) { + match source { + BindingSource::Channel => { + keybinding_source.add_channel_key(tv_key); + // Remove from global keys if it was there (override) + if keybinding_source.global_keys.remove(&tv_key) { + override_count += 1; + trace!( + "Channel key '{}' overrode global binding", + key + ); + } else { + trace!("Added new channel key '{}'", key); + } + } + BindingSource::Global => { + // New bindings are global - add to global keys + keybinding_source.add_global_key(tv_key); + if was_existing { + trace!( + "Global key '{}' overrode existing binding", + key + ); + } else { + trace!("Added new global key '{}'", key); + } + } + } + } + } + + debug!( + "Merge completed: {} keys merged ({} overrides), final tracking: {} global, {} channel", + merged_count, + override_count, + keybinding_source.global_keys.len(), + keybinding_source.channel_keys.len() + ); + + (bindings, keybinding_source) } impl Default for Bindings { @@ -1012,7 +1175,11 @@ mod tests { (Key::PageDown, Action::SelectNextPage), ]); - let merged = merge_bindings(base_keybindings, &custom_keybindings); + let (merged, _) = merge_bindings( + base_keybindings, + &custom_keybindings, + BindingSource::Global, + ); // Should contain both base and custom keybindings assert!(merged.bindings.contains_key(&Key::Esc)); @@ -1123,7 +1290,11 @@ mod tests { bindings: custom_bindings, }; - let merged = merge_bindings(base_keybindings, &custom_keybindings); + let (merged, _) = merge_bindings( + base_keybindings, + &custom_keybindings, + BindingSource::Global, + ); // Custom multiple actions should be present assert_eq!( diff --git a/television/config/mod.rs b/television/config/mod.rs index f1e4a6e..4893927 100644 --- a/television/config/mod.rs +++ b/television/config/mod.rs @@ -1,6 +1,7 @@ use crate::{ cable::CABLE_DIR_NAME, - channels::prototypes::{DEFAULT_PROTOTYPE_NAME, UiSpec}, + channels::prototypes::{DEFAULT_PROTOTYPE_NAME, Template, UiSpec}, + cli::PostProcessedCli, history::DEFAULT_HISTORY_SIZE, }; use anyhow::{Context, Result}; @@ -11,10 +12,14 @@ use std::{ env, hash::Hash, path::{Path, PathBuf}, + str::FromStr, }; use tracing::{debug, warn}; -pub use keybindings::{EventBindings, EventType, KeyBindings, merge_bindings}; +pub use keybindings::{ + BindingSource, EventBindings, EventType, KeyBindings, KeybindingSource, + merge_bindings, +}; pub use themes::Theme; pub use ui::UiConfig; @@ -90,6 +95,9 @@ pub struct Config { /// Shell integration configuration #[serde(default)] pub shell_integration: ShellIntegrationConfig, + /// Keybinding source tracking (not serialized) + #[serde(skip)] + pub keybinding_source: KeybindingSource, } const PROJECT_NAME: &str = "television"; @@ -221,13 +229,19 @@ impl Config { merged_keybindings.extend(new.shell_integration.keybindings.clone()); new.shell_integration.keybindings = merged_keybindings; - // merge keybindings with default keybindings - let keybindings = - merge_bindings(default.keybindings.clone(), &new.keybindings); + let (keybindings, keybinding_source) = merge_bindings( + default.keybindings.clone(), + &new.keybindings, + BindingSource::Global, + ); new.keybindings = keybindings; + new.keybinding_source = keybinding_source; - // merge event bindings with default event bindings - let events = merge_bindings(default.events.clone(), &new.events); + let (events, _) = merge_bindings( + default.events.clone(), + &new.events, + BindingSource::Global, + ); new.events = events; Config { @@ -236,15 +250,217 @@ impl Config { events: new.events, ui: new.ui, shell_integration: new.shell_integration, + keybinding_source: new.keybinding_source, } } - pub fn merge_keybindings(&mut self, other: &KeyBindings) { - self.keybindings = merge_bindings(self.keybindings.clone(), other); + pub fn merge_channel_keybindings(&mut self, other: &KeyBindings) { + let (merged, source) = merge_bindings( + self.keybindings.clone(), + other, + BindingSource::Channel, + ); + self.keybindings = merged; + for key in &source.channel_keys { + self.keybinding_source.add_channel_key(*key); + self.keybinding_source.global_keys.remove(key); + } + } + + /// Apply CLI keybinding overrides while preserving source categorization. + /// + /// CLI overrides preserve the original source of the key: + /// - If overriding a global key → keep it in global list + /// - If overriding a channel key → keep it in channel list + /// - If adding a new key → add to global list (CLI is user's global preference) + pub fn apply_cli_keybinding_overrides( + &mut self, + cli_keybindings: &KeyBindings, + ) { + use tracing::{debug, trace}; + + debug!( + "Applying {} CLI keybinding overrides", + cli_keybindings.len() + ); + + let mut override_count = 0; + let mut new_key_count = 0; + let mut preserved_global = 0; + let mut preserved_channel = 0; + + for (key, actions) in &cli_keybindings.bindings { + let was_existing = self.keybindings.contains_key(key); + + // Update the keybinding + self.keybindings.insert(*key, actions.clone()); + + if let Ok(tv_key) = crate::event::Key::from_str(&key.to_string()) { + if was_existing { + override_count += 1; + // Preserve existing source categorization + if self.keybinding_source.is_global_key(&tv_key) { + // Key was already global, keep it global + preserved_global += 1; + trace!("CLI override preserved global key: '{}'", key); + } else if self.keybinding_source.is_channel_key(&tv_key) { + // Key was channel-specific, keep it channel-specific + preserved_channel += 1; + trace!( + "CLI override preserved channel key: '{}'", + key + ); + } else { + // Key exists in keybindings but not tracked in source - treat as global + self.keybinding_source.add_global_key(tv_key); + trace!( + "CLI override added untracked existing key '{}' to global", + key + ); + } + } else { + // New key from CLI - add to global (CLI is user's global preference) + new_key_count += 1; + self.keybinding_source.add_global_key(tv_key); + trace!("CLI override added new global key: '{}'", key); + } + } + } + + debug!( + "CLI override completed: {} overrides ({} preserved global, {} preserved channel), {} new keys", + override_count, preserved_global, preserved_channel, new_key_count + ); } pub fn merge_event_bindings(&mut self, other: &EventBindings) { - self.events = merge_bindings(self.events.clone(), other); + let (merged, _) = + merge_bindings(self.events.clone(), other, BindingSource::Global); + self.events = merged; + } + + /// Apply CLI overrides to this config + pub fn apply_cli_overrides(&mut self, args: &PostProcessedCli) { + use crate::{ + action::Action, features::FeatureFlags, + screen::keybindings::remove_action_bindings, + }; + use tracing::debug; + + debug!("Applying CLI overrides to config after channel merging"); + + if let Some(cable_dir) = &args.cable_dir { + self.application.cable_dir.clone_from(cable_dir); + } + if let Some(tick_rate) = args.tick_rate { + self.application.tick_rate = tick_rate; + } + if args.global_history { + self.application.global_history = true; + } + // Handle preview panel flags + if args.no_preview { + self.ui.features.disable(FeatureFlags::PreviewPanel); + remove_action_bindings( + &mut self.keybindings, + &Action::TogglePreview.into(), + ); + } else if args.hide_preview { + self.ui.features.hide(FeatureFlags::PreviewPanel); + } else if args.show_preview { + self.ui.features.enable(FeatureFlags::PreviewPanel); + } + + if let Some(ps) = args.preview_size { + self.ui.preview_panel.size = ps; + } + + // Handle status bar flags + if args.no_status_bar { + self.ui.features.disable(FeatureFlags::StatusBar); + remove_action_bindings( + &mut self.keybindings, + &Action::ToggleStatusBar.into(), + ); + } else if args.hide_status_bar { + self.ui.features.hide(FeatureFlags::StatusBar); + } else if args.show_status_bar { + self.ui.features.enable(FeatureFlags::StatusBar); + } + + // Handle remote control flags + if args.no_remote { + self.ui.features.disable(FeatureFlags::RemoteControl); + remove_action_bindings( + &mut self.keybindings, + &Action::ToggleRemoteControl.into(), + ); + } else if args.hide_remote { + self.ui.features.hide(FeatureFlags::RemoteControl); + } else if args.show_remote { + self.ui.features.enable(FeatureFlags::RemoteControl); + } + + // Handle help panel flags + if args.no_help_panel { + self.ui.features.disable(FeatureFlags::HelpPanel); + remove_action_bindings( + &mut self.keybindings, + &Action::ToggleHelp.into(), + ); + } else if args.hide_help_panel { + self.ui.features.hide(FeatureFlags::HelpPanel); + } else if args.show_help_panel { + self.ui.features.enable(FeatureFlags::HelpPanel); + } + + // Apply CLI keybinding overrides with proper source tracking + if let Some(keybindings) = &args.keybindings { + self.apply_cli_keybinding_overrides(keybindings); + } + + self.ui.ui_scale = args.ui_scale.unwrap_or(self.ui.ui_scale); + if let Some(input_header) = &args.input_header { + if let Ok(t) = Template::parse(input_header) { + self.ui.input_bar.header = Some(t); + } + } + if let Some(input_prompt) = &args.input_prompt { + self.ui.input_bar.prompt.clone_from(input_prompt); + } + if let Some(preview_header) = &args.preview_header { + if let Ok(t) = Template::parse(preview_header) { + self.ui.preview_panel.header = Some(t); + } + } + if let Some(preview_footer) = &args.preview_footer { + if let Ok(t) = + crate::channels::prototypes::Template::parse(preview_footer) + { + self.ui.preview_panel.footer = Some(t); + } + } + if let Some(layout) = args.layout { + self.ui.orientation = layout; + } + if let Some(input_border) = args.input_border { + self.ui.input_bar.border_type = input_border; + } + if let Some(preview_border) = args.preview_border { + self.ui.preview_panel.border_type = preview_border; + } + if let Some(results_border) = args.results_border { + self.ui.results_panel.border_type = results_border; + } + if let Some(input_padding) = args.input_padding { + self.ui.input_bar.padding = input_padding; + } + if let Some(preview_padding) = args.preview_padding { + self.ui.preview_panel.padding = preview_padding; + } + if let Some(results_padding) = args.results_padding { + self.ui.results_panel.padding = results_padding; + } } pub fn apply_prototype_ui_spec(&mut self, ui_spec: &UiSpec) { diff --git a/television/draw.rs b/television/draw.rs index 5dddf08..8fca762 100644 --- a/television/draw.rs +++ b/television/draw.rs @@ -179,7 +179,7 @@ pub fn draw(ctx: Box, f: &mut Frame<'_>, area: Rect) -> Result { &ctx.config.ui, show_remote, ctx.tv_state.preview_state.enabled, - Some(&ctx.config.keybindings), + Some(&ctx.config), ctx.tv_state.mode, &ctx.colorscheme, ); @@ -247,7 +247,7 @@ pub fn draw(ctx: Box, f: &mut Frame<'_>, area: Rect) -> Result { draw_help_panel( f, help_area, - &ctx.config.keybindings, + &ctx.config, ctx.tv_state.mode, &ctx.colorscheme, ); diff --git a/television/event.rs b/television/event.rs index 49c79fc..7af748c 100644 --- a/television/event.rs +++ b/television/event.rs @@ -28,7 +28,9 @@ pub enum Event { Tick, } -#[derive(Debug, Clone, Copy, Serialize, PartialEq, PartialOrd, Eq, Hash)] +#[derive( + Debug, Clone, Copy, Serialize, PartialEq, PartialOrd, Eq, Hash, Ord, +)] pub enum Key { Backspace, Enter, diff --git a/television/main.rs b/television/main.rs index dce1a19..a4cc1bd 100644 --- a/television/main.rs +++ b/television/main.rs @@ -5,23 +5,20 @@ use std::io::{BufWriter, IsTerminal, Write, stdout}; use std::path::PathBuf; use std::process::exit; use television::{ - action::Action, app::{App, AppOptions}, cable::{Cable, cable_empty_exit, load_cable}, channels::prototypes::{ ChannelPrototype, CommandSpec, PreviewSpec, Template, UiSpec, }, - cli::post_process, cli::{ PostProcessedCli, args::{Cli, Command}, - guess_channel_from_prompt, list_channels, + guess_channel_from_prompt, list_channels, post_process, }, - config::{Config, ConfigEnv, merge_bindings, ui::InputBarConfig}, + config::{Config, ConfigEnv, ui::InputBarConfig}, errors::os_error_exit, features::FeatureFlags, gh::update_local_channels, - screen::keybindings::remove_action_bindings, television::Mode, utils::clipboard::CLIPBOARD, utils::{ @@ -56,7 +53,7 @@ async fn main() -> Result<()> { // override configuration values with provided CLI arguments debug!("Applying CLI overrides..."); - apply_cli_overrides(&args, &mut config); + config.apply_cli_overrides(&args); // handle subcommands debug!("Handling subcommands..."); @@ -104,6 +101,7 @@ async fn main() -> Result<()> { args.input.clone(), options, cable, + &args, ); // If the user requested to show the remote control on startup, switch the @@ -135,121 +133,6 @@ async fn main() -> Result<()> { exit(0); } -/// Apply overrides from the CLI arguments to the configuration. -/// -/// This function mutates the configuration in place. -fn apply_cli_overrides(args: &PostProcessedCli, config: &mut Config) { - if let Some(cable_dir) = &args.cable_dir { - config.application.cable_dir.clone_from(cable_dir); - } - if let Some(tick_rate) = args.tick_rate { - config.application.tick_rate = tick_rate; - } - if args.global_history { - config.application.global_history = true; - } - // Handle preview panel flags - if args.no_preview { - config.ui.features.disable(FeatureFlags::PreviewPanel); - remove_action_bindings( - &mut config.keybindings, - &Action::TogglePreview.into(), - ); - } else if args.hide_preview { - config.ui.features.hide(FeatureFlags::PreviewPanel); - } else if args.show_preview { - config.ui.features.enable(FeatureFlags::PreviewPanel); - } - - if let Some(ps) = args.preview_size { - config.ui.preview_panel.size = ps; - } - - // Handle status bar flags - if args.no_status_bar { - config.ui.features.disable(FeatureFlags::StatusBar); - remove_action_bindings( - &mut config.keybindings, - &Action::ToggleStatusBar.into(), - ); - } else if args.hide_status_bar { - config.ui.features.hide(FeatureFlags::StatusBar); - } else if args.show_status_bar { - config.ui.features.enable(FeatureFlags::StatusBar); - } - - // Handle remote control flags - if args.no_remote { - config.ui.features.disable(FeatureFlags::RemoteControl); - remove_action_bindings( - &mut config.keybindings, - &Action::ToggleRemoteControl.into(), - ); - } else if args.hide_remote { - config.ui.features.hide(FeatureFlags::RemoteControl); - } else if args.show_remote { - config.ui.features.enable(FeatureFlags::RemoteControl); - } - - // Handle help panel flags - if args.no_help_panel { - config.ui.features.disable(FeatureFlags::HelpPanel); - remove_action_bindings( - &mut config.keybindings, - &Action::ToggleHelp.into(), - ); - } else if args.hide_help_panel { - config.ui.features.hide(FeatureFlags::HelpPanel); - } else if args.show_help_panel { - config.ui.features.enable(FeatureFlags::HelpPanel); - } - - if let Some(keybindings) = &args.keybindings { - config.keybindings = - merge_bindings(config.keybindings.clone(), keybindings); - } - config.ui.ui_scale = args.ui_scale.unwrap_or(config.ui.ui_scale); - if let Some(input_header) = &args.input_header { - if let Ok(t) = Template::parse(input_header) { - config.ui.input_bar.header = Some(t); - } - } - if let Some(input_prompt) = &args.input_prompt { - config.ui.input_bar.prompt.clone_from(input_prompt); - } - if let Some(preview_header) = &args.preview_header { - if let Ok(t) = Template::parse(preview_header) { - config.ui.preview_panel.header = Some(t); - } - } - if let Some(preview_footer) = &args.preview_footer { - if let Ok(t) = Template::parse(preview_footer) { - config.ui.preview_panel.footer = Some(t); - } - } - if let Some(layout) = args.layout { - config.ui.orientation = layout; - } - if let Some(input_border) = args.input_border { - config.ui.input_bar.border_type = input_border; - } - if let Some(preview_border) = args.preview_border { - config.ui.preview_panel.border_type = preview_border; - } - if let Some(results_border) = args.results_border { - config.ui.results_panel.border_type = results_border; - } - if let Some(input_padding) = args.input_padding { - config.ui.input_bar.padding = input_padding; - } - if let Some(preview_padding) = args.preview_padding { - config.ui.preview_panel.padding = preview_padding; - } - if let Some(results_padding) = args.results_padding { - config.ui.results_panel.padding = results_padding; - } -} - pub fn set_current_dir(path: &PathBuf) -> Result<()> { env::set_current_dir(path)?; Ok(()) @@ -821,7 +704,7 @@ mod tests { results_padding: Some(Padding::new(9, 10, 11, 12)), ..Default::default() }; - apply_cli_overrides(&args, &mut config); + config.apply_cli_overrides(&args); assert_eq!(config.application.tick_rate, 100_f64); assert!(!config.ui.features.is_enabled(FeatureFlags::PreviewPanel)); @@ -959,7 +842,7 @@ mod tests { ui_scale: Some(90), ..Default::default() }; - apply_cli_overrides(&args, &mut config); + config.apply_cli_overrides(&args); assert_eq!(config.ui.ui_scale, 90); @@ -967,7 +850,7 @@ mod tests { let mut config = Config::default(); config.ui.ui_scale = 70; let args = PostProcessedCli::default(); - apply_cli_overrides(&args, &mut config); + config.apply_cli_overrides(&args); assert_eq!(config.ui.ui_scale, 70); } diff --git a/television/screen/help_panel.rs b/television/screen/help_panel.rs index 0165c31..8f8db39 100644 --- a/television/screen/help_panel.rs +++ b/television/screen/help_panel.rs @@ -1,7 +1,8 @@ use crate::{ - config::KeyBindings, + action::Action, + config::{Config, KeyBindings}, + event::Key, screen::colors::Colorscheme, - screen::keybindings::{ActionMapping, find_keys_for_single_action}, television::Mode, }; use ratatui::{ @@ -19,7 +20,7 @@ const MIN_PANEL_HEIGHT: u16 = 5; pub fn draw_help_panel( f: &mut Frame<'_>, area: Rect, - keybindings: &KeyBindings, + config: &Config, tv_mode: Mode, colorscheme: &Colorscheme, ) { @@ -28,7 +29,7 @@ pub fn draw_help_panel( } // Generate content - let content = generate_help_content(keybindings, tv_mode, colorscheme); + let content = generate_help_content(config, tv_mode, colorscheme); // Clear the area first to create the floating effect f.render_widget(Clear, area); @@ -52,37 +53,116 @@ pub fn draw_help_panel( f.render_widget(paragraph, area); } -/// Adds keybinding lines for action mappings to the given lines vector -fn add_keybinding_lines_for_mappings( +/// Adds keybinding lines for specific keys to the given lines vector +fn add_keybinding_lines_for_keys( lines: &mut Vec>, keybindings: &KeyBindings, - mappings: &[ActionMapping], + keys: impl Iterator, mode: Mode, colorscheme: &Colorscheme, + category_name: &str, ) { - for mapping in mappings { - for (action, description) in &mapping.actions { - let keys = find_keys_for_single_action(keybindings, action); - for key in keys { - lines.push(create_compact_keybinding_line( - &key, - description, - mode, - colorscheme, - )); + use tracing::trace; + + // Collect all valid keybinding entries + let mut entries: Vec<(String, String)> = Vec::new(); + let mut filtered_count = 0; + + for key in keys { + if let Some(actions) = keybindings.get(&key) { + for action in actions.as_slice() { + // Filter out NoOp actions (unbound keys) + if matches!(action, Action::NoOp) { + trace!( + "Filtering out NoOp (unboud keys) action for key '{}' in {} category", + key, category_name + ); + filtered_count += 1; + continue; + } + + let description = get_action_description(action); + let key_string = key.to_string(); + entries.push((description.clone(), key_string.clone())); + trace!( + "Added keybinding: {} -> {} ({})", + key_string, description, category_name + ); } } } + + // Sort entries alphabetically by description + entries.sort_by(|a, b| a.0.cmp(&b.0)); + trace!( + "Sorted {} keybindings for {} category (filtered {} NoOp entries)", + entries.len(), + category_name, + filtered_count + ); + + // Create lines from sorted entries + for (description, key_string) in entries { + lines.push(create_compact_keybinding_line( + &key_string, + &description, + mode, + colorscheme, + )); + } } -/// Generates the help content organized into global and mode-specific groups +/// Get human-readable description for an action +fn get_action_description(action: &Action) -> String { + match action { + Action::Quit => "Quit".to_string(), + Action::TogglePreview => "Toggle preview".to_string(), + Action::ToggleHelp => "Toggle help".to_string(), + Action::ToggleStatusBar => "Toggle status bar".to_string(), + Action::ToggleRemoteControl => "Toggle remote control".to_string(), + Action::SelectPrevEntry => "Navigate up".to_string(), + Action::SelectNextEntry => "Navigate down".to_string(), + Action::SelectPrevPage => "Page up".to_string(), + Action::SelectNextPage => "Page down".to_string(), + Action::SelectPrevHistory => "Previous history".to_string(), + Action::SelectNextHistory => "Next history".to_string(), + Action::ScrollPreviewHalfPageUp => "Preview scroll up".to_string(), + Action::ScrollPreviewHalfPageDown => "Preview scroll down".to_string(), + Action::ConfirmSelection => "Select entry".to_string(), + Action::ToggleSelectionDown => "Toggle selection down".to_string(), + Action::ToggleSelectionUp => "Toggle selection up".to_string(), + Action::CopyEntryToClipboard => "Copy to clipboard".to_string(), + Action::CycleSources => "Cycle sources".to_string(), + Action::ReloadSource => "Reload source".to_string(), + Action::DeletePrevChar => "Delete previous char".to_string(), + Action::DeletePrevWord => "Delete previous word".to_string(), + Action::DeleteNextChar => "Delete next char".to_string(), + Action::DeleteLine => "Delete line".to_string(), + Action::GoToPrevChar => "Move cursor left".to_string(), + Action::GoToNextChar => "Move cursor right".to_string(), + Action::GoToInputStart => "Move to start".to_string(), + Action::GoToInputEnd => "Move to end".to_string(), + _ => action.to_string(), + } +} + +/// Generates the help content organized into global and channel-specific groups fn generate_help_content( - keybindings: &KeyBindings, + config: &Config, mode: Mode, colorscheme: &Colorscheme, ) -> Vec> { + use tracing::debug; + let mut lines = Vec::new(); + debug!("Generating help content for mode: {:?}", mode); + debug!( + "Keybinding source tracking - Global keys: {}, Channel keys: {}", + config.keybinding_source.global_keys.len(), + config.keybinding_source.channel_keys.len() + ); + // Global keybindings section header lines.push(Line::from(vec![Span::styled( "Global", @@ -92,53 +172,68 @@ fn generate_help_content( .underlined(), )])); - // Global actions using centralized system - let global_mappings = ActionMapping::global_actions(); - add_keybinding_lines_for_mappings( + // Global keybindings - all keys that are NOT from channel configs + let global_keys = config.keybinding_source.global_keys.iter().copied(); + add_keybinding_lines_for_keys( &mut lines, - keybindings, - &global_mappings, + &config.keybindings, + global_keys, mode, colorscheme, + "Global", ); - // Add spacing between Global and mode-specific sections - lines.push(Line::from("")); + // Add spacing between Global and channel-specific sections only if we have channel keys + if !config.keybinding_source.channel_keys.is_empty() { + lines.push(Line::from("")); + } - // Mode-specific keybindings section header + // Channel-specific keybindings section header let mode_name = match mode { Mode::Channel => "Channel", Mode::RemoteControl => "Remote", }; - lines.push(Line::from(vec![Span::styled( - mode_name, - Style::default() - .fg(colorscheme.help.metadata_field_name_fg) - .bold() - .underlined(), - )])); + // Only show channel section if there are channel-specific keybindings + if config.keybinding_source.has_channel_keys() { + lines.push(Line::from(vec![Span::styled( + mode_name, + Style::default() + .fg(colorscheme.help.metadata_field_name_fg) + .bold() + .underlined(), + )])); - // Navigation actions (common to both modes) using centralized system - let nav_mappings = ActionMapping::navigation_actions(); - add_keybinding_lines_for_mappings( - &mut lines, - keybindings, - &nav_mappings, - mode, - colorscheme, - ); + // Channel-specific keybindings - only keys from channel configs + let channel_keys = + config.keybinding_source.channel_keys.iter().copied(); + add_keybinding_lines_for_keys( + &mut lines, + &config.keybindings, + channel_keys, + mode, + colorscheme, + mode_name, + ); + } else { + debug!( + "No channel-specific keybindings found, skipping channel section for mode: {:?}", + mode + ); + } - // Mode-specific actions using centralized system - let mode_mappings = ActionMapping::mode_specific_actions(mode); - add_keybinding_lines_for_mappings( - &mut lines, - keybindings, - &mode_mappings, - mode, - colorscheme, - ); + // Handle edge case where no keybindings are found at all + if !config.keybinding_source.has_any_keys() { + debug!("Warning: No keybindings found in source tracking!"); + lines.push(Line::from(vec![Span::styled( + "No keybindings configured", + Style::default() + .fg(colorscheme.help.metadata_field_name_fg) + .italic(), + )])); + } + debug!("Generated help content with {} total lines", lines.len()); lines } @@ -168,12 +263,14 @@ fn create_compact_keybinding_line( /// Calculates the required dimensions for the help panel based on content #[allow(clippy::cast_possible_truncation)] pub fn calculate_help_panel_size( - keybindings: &KeyBindings, + config: &Config, mode: Mode, colorscheme: &Colorscheme, ) -> (u16, u16) { + use tracing::trace; + // Generate content to count items and calculate width - let content = generate_help_content(keybindings, mode, colorscheme); + let content = generate_help_content(config, mode, colorscheme); // Calculate required width based on actual content let max_content_width = content @@ -188,5 +285,13 @@ pub fn calculate_help_panel_size( let required_width = (max_content_width + 3).max(25) as u16; let required_height = (content.len() + 2).max(8) as u16; + trace!( + "Help panel size calculation: {} lines, max width {}, final dimensions {}x{}", + content.len(), + max_content_width, + required_width, + required_height + ); + (required_width, required_height) } diff --git a/television/screen/keybindings.rs b/television/screen/keybindings.rs index 9831609..3f0db33 100644 --- a/television/screen/keybindings.rs +++ b/television/screen/keybindings.rs @@ -1,175 +1,9 @@ use crate::{ action::{Action, Actions}, config::KeyBindings, - television::Mode, }; -use std::fmt::Display; -/// Centralized action descriptions to avoid duplication between keybinding panel and help bar -#[derive(Debug, Clone, PartialEq, Eq, Hash)] -pub enum ActionCategory { - // Global actions - Quit, - ToggleFeature, - - // Navigation actions (common to both modes) - ResultsNavigation, - PreviewNavigation, - - // Selection actions - SelectEntry, - ToggleSelection, - - // Channel-specific actions - CopyEntryToClipboard, - ToggleRemoteControl, - CycleSources, - ReloadSource, -} - -impl Display for ActionCategory { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let description = match self { - ActionCategory::Quit => "Quit", - ActionCategory::ToggleFeature => "Toggle features", - ActionCategory::ResultsNavigation => "Results navigation", - ActionCategory::PreviewNavigation => "Preview navigation", - ActionCategory::SelectEntry => "Select entry", - ActionCategory::ToggleSelection => "Toggle selection", - ActionCategory::CopyEntryToClipboard => "Copy entry to clipboard", - ActionCategory::ToggleRemoteControl => "Toggle Remote control", - ActionCategory::CycleSources => "Cycle through sources", - ActionCategory::ReloadSource => "Reload source", - }; - write!(f, "{description}") - } -} - -/// Defines what actions belong to each category and their individual descriptions -pub struct ActionMapping { - pub category: ActionCategory, - pub actions: Vec<(Action, &'static str)>, -} - -impl ActionMapping { - /// Get all action mappings for global actions - pub fn global_actions() -> Vec { - vec![ - ActionMapping { - category: ActionCategory::Quit, - actions: vec![(Action::Quit, "Quit")], - }, - ActionMapping { - category: ActionCategory::ToggleFeature, - actions: vec![ - (Action::TogglePreview, "Toggle preview"), - (Action::ToggleHelp, "Toggle help"), - (Action::ToggleStatusBar, "Toggle status bar"), - ], - }, - ] - } - - /// Get all action mappings for navigation actions (common to both modes) - pub fn navigation_actions() -> Vec { - vec![ - ActionMapping { - category: ActionCategory::ResultsNavigation, - actions: vec![ - (Action::SelectPrevEntry, "Navigate up"), - (Action::SelectNextEntry, "Navigate down"), - (Action::SelectPrevPage, "Page up"), - (Action::SelectNextPage, "Page down"), - ], - }, - ActionMapping { - category: ActionCategory::PreviewNavigation, - actions: vec![ - (Action::ScrollPreviewHalfPageUp, "Preview scroll up"), - (Action::ScrollPreviewHalfPageDown, "Preview scroll down"), - ], - }, - ] - } - - /// Get mode-specific action mappings - pub fn mode_specific_actions(mode: Mode) -> Vec { - match mode { - Mode::Channel => vec![ - ActionMapping { - category: ActionCategory::SelectEntry, - actions: vec![ - (Action::ConfirmSelection, "Select entry"), - (Action::ToggleSelectionDown, "Toggle selection down"), - (Action::ToggleSelectionUp, "Toggle selection up"), - ], - }, - ActionMapping { - category: ActionCategory::CopyEntryToClipboard, - actions: vec![( - Action::CopyEntryToClipboard, - "Copy to clipboard", - )], - }, - ActionMapping { - category: ActionCategory::ToggleRemoteControl, - actions: vec![( - Action::ToggleRemoteControl, - "Remote Control", - )], - }, - ActionMapping { - category: ActionCategory::CycleSources, - actions: vec![(Action::CycleSources, "Cycle sources")], - }, - ActionMapping { - category: ActionCategory::ReloadSource, - actions: vec![(Action::ReloadSource, "Reload source")], - }, - ], - Mode::RemoteControl => vec![ - ActionMapping { - category: ActionCategory::SelectEntry, - actions: vec![(Action::ConfirmSelection, "Select entry")], - }, - ActionMapping { - category: ActionCategory::ToggleRemoteControl, - actions: vec![( - Action::ToggleRemoteControl, - "Back to Channel", - )], - }, - ], - } - } - - /// Get all actions for a specific category, flattened for help bar usage - pub fn actions_for_category(&self) -> &[Action] { - // This is a bit of a hack to return just the Action part of the tuples - // We'll need to handle this differently in the help bar system - &[] - } -} - -/// Extract keys for a single action from the new Key->Action keybindings format -pub fn find_keys_for_action( - keybindings: &KeyBindings, - target_action: &Actions, -) -> Vec { - keybindings - .bindings - .iter() - .filter_map(|(key, action)| { - if action == target_action { - Some(key.to_string()) - } else { - None - } - }) - .collect() -} - -/// Extract keys for a single action (convenience function) +/// Extract keys for a single action from the keybindings format pub fn find_keys_for_single_action( keybindings: &KeyBindings, target_action: &Action, @@ -199,6 +33,24 @@ pub fn extract_keys_for_actions( .collect() } +/// Extract keys for a single action from the new Key->Action keybindings format +pub fn find_keys_for_action( + keybindings: &KeyBindings, + target_action: &Actions, +) -> Vec { + keybindings + .bindings + .iter() + .filter_map(|(key, action)| { + if action == target_action { + Some(key.to_string()) + } else { + None + } + }) + .collect() +} + /// Remove all keybindings for a specific action from `KeyBindings` pub fn remove_action_bindings( keybindings: &mut KeyBindings, diff --git a/television/screen/layout.rs b/television/screen/layout.rs index e2a9010..6b7439b 100644 --- a/television/screen/layout.rs +++ b/television/screen/layout.rs @@ -1,5 +1,5 @@ use crate::{ - config::{KeyBindings, UiConfig}, + config::{Config, UiConfig}, features::FeatureFlags, screen::{ colors::Colorscheme, help_panel::calculate_help_panel_size, @@ -137,7 +137,7 @@ impl Layout { ui_config: &UiConfig, show_remote: bool, show_preview: bool, - keybindings: Option<&KeyBindings>, + config: Option<&Config>, mode: Mode, colorscheme: &Colorscheme, ) -> Self { @@ -401,12 +401,12 @@ impl Layout { area }; - if let Some(kb) = keybindings { + if let Some(cfg) = config { let (width, height) = - calculate_help_panel_size(kb, mode, colorscheme); + calculate_help_panel_size(cfg, mode, colorscheme); Some(bottom_right_rect(width, height, hp_area)) } else { - // Fallback to reasonable default if keybindings not available + // Fallback to reasonable default if config not available Some(bottom_right_rect(45, 25, hp_area)) } } else { diff --git a/television/television.rs b/television/television.rs index 4809324..97a2534 100644 --- a/television/television.rs +++ b/television/television.rs @@ -7,6 +7,7 @@ use crate::{ prototypes::ChannelPrototype, remote_control::{CableEntry, RemoteControl}, }, + cli::PostProcessedCli, config::{Config, Theme}, draw::{ChannelState, Ctx, TvState}, errors::os_error_exit, @@ -99,14 +100,15 @@ impl Television { preview_size: Option, exact: bool, cable_channels: Cable, + cli_args: &PostProcessedCli, ) -> Self { let mut config = Self::merge_base_config_with_prototype_specs( &base_config, &channel_prototype, ); - // Apply CLI overrides after prototype merging to ensure they take precedence - Self::apply_cli_overrides(&mut config, no_preview, preview_size); + // Apply ALL CLI overrides (including keybindings) after channel merging + config.apply_cli_overrides(cli_args); debug!("Merged config: {:?}", config); @@ -201,7 +203,7 @@ impl Television { let mut config = base_config.clone(); // keybindings if let Some(keybindings) = &channel_prototype.keybindings { - config.merge_keybindings(&keybindings.bindings); + config.merge_channel_keybindings(&keybindings.bindings); } // ui if let Some(ui_spec) = &channel_prototype.ui { @@ -944,6 +946,8 @@ mod test { #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_cli_overrides() { + use crate::cli::PostProcessedCli; + let config = crate::config::Config::default(); let prototype = crate::channels::prototypes::ChannelPrototype::new( "test", "echo 1", @@ -958,6 +962,7 @@ mod test { Some(50), true, Cable::from_prototypes(vec![]), + &PostProcessedCli::default(), ); assert_eq!(tv.matching_mode, MatchingMode::Substring); @@ -967,6 +972,8 @@ mod test { #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_channel_keybindings_take_precedence() { + use crate::cli::PostProcessedCli; + let mut config = crate::config::Config::default(); config .keybindings @@ -997,6 +1004,7 @@ mod test { Some(50), true, Cable::from_prototypes(vec![]), + &PostProcessedCli::default(), ); assert_eq!( diff --git a/tests/app.rs b/tests/app.rs index 51b1da0..0d8d933 100644 --- a/tests/app.rs +++ b/tests/app.rs @@ -9,6 +9,7 @@ use television::{ app::{App, AppOptions}, cable::Cable, channels::prototypes::ChannelPrototype, + cli::PostProcessedCli, config::default_config_from_file, }; use tokio::{task::JoinHandle, time::timeout}; @@ -69,6 +70,7 @@ fn setup_app( ChannelPrototype::new("dirs", "find . -type d"), ChannelPrototype::new("env", "printenv"), ]), + &PostProcessedCli::default(), ); // retrieve the app's action channel handle in order to send a quit action