From f1412b4f8cf4c0e7de808d5ed5e970ff0840c157 Mon Sep 17 00:00:00 2001 From: Christoffer Martinsson Date: Thu, 11 Dec 2025 15:16:57 +0100 Subject: [PATCH] Refactor to eliminate keyboard/mouse handler disconnects Extract duplicate logic into shared action functions to ensure consistent behavior between keyboard and mouse interactions: - action_remove_from_playlist: Unified playlist removal logic - action_play_from_playlist: Unified playlist playback with optional pause state preservation - handle_context_menu_action: Unified context menu execution Fixes: - Remove from playlist now checks index before removal (was broken in keyboard 'd' handler) - Mouse double-click on playlist now preserves pause state - Context menu handling no longer duplicated across 400+ lines All keyboard and mouse actions now use identical code paths, eliminating state bugs from inconsistent implementations. --- Cargo.toml | 2 +- src/main.rs | 278 +++++++++++++++++++--------------------------------- 2 files changed, 104 insertions(+), 176 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index fafaac1..70af2f6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cm-player" -version = "0.1.18" +version = "0.1.19" edition = "2021" [dependencies] diff --git a/src/main.rs b/src/main.rs index 406da7e..90bb2bc 100644 --- a/src/main.rs +++ b/src/main.rs @@ -144,6 +144,102 @@ fn action_stop(state: &mut AppState, player: &mut player::Player) -> Result<()> Ok(()) } +fn action_remove_from_playlist(state: &mut AppState, player: &mut player::Player) -> Result<()> { + let was_playing_removed = state.playlist_index == state.selected_playlist_index; + state.remove_selected_playlist_item(); + + if state.playlist.is_empty() { + state.player_state = PlayerState::Stopped; + state.current_file = None; + player.stop()?; + } else if was_playing_removed && state.player_state == PlayerState::Playing { + state.current_file = Some(state.playlist[state.playlist_index].clone()); + if let Some(ref path) = state.current_file { + player.play(path)?; + player.update_metadata(); + } + } + Ok(()) +} + +fn action_play_from_playlist(state: &mut AppState, player: &mut player::Player, preserve_pause: bool) -> Result<()> { + state.playlist_index = state.selected_playlist_index; + state.current_file = Some(state.playlist[state.playlist_index].clone()); + + if preserve_pause { + match state.player_state { + PlayerState::Playing => { + if let Some(ref path) = state.current_file { + player.play(path)?; + player.update_metadata(); + tracing::info!("Jumped to track: {:?}", path); + } + } + PlayerState::Paused => { + if let Some(ref path) = state.current_file { + player.play(path)?; + player.update_metadata(); + player.pause()?; + tracing::info!("Jumped to track (paused): {:?}", path); + } + } + PlayerState::Stopped => { + state.player_state = PlayerState::Playing; + if let Some(ref path) = state.current_file { + player.play(path)?; + player.update_metadata(); + tracing::info!("Started playing track: {:?}", path); + } + } + } + } else { + state.player_state = PlayerState::Playing; + if let Some(ref path) = state.current_file { + player.play(path)?; + player.update_metadata(); + tracing::info!("Playing from playlist: {:?}", path); + } + } + Ok(()) +} + +fn handle_context_menu_action(menu_type: state::ContextMenuType, selected: usize, state: &mut AppState, player: &mut player::Player) -> Result<()> { + match menu_type { + state::ContextMenuType::FilePanel => { + match selected { + 0 => action_play_selection(state, player)?, + 1 => state.add_to_playlist(), + _ => {} + } + } + state::ContextMenuType::Playlist => { + match selected { + 0 => action_remove_from_playlist(state, player)?, + 1 => { + state.shuffle_playlist(); + tracing::info!("Playlist randomised from context menu"); + } + _ => {} + } + } + state::ContextMenuType::TitleBar => { + match selected { + 0 => action_stop(state, player)?, + 1 => { + state.cycle_play_mode(); + tracing::info!("Play mode: {:?}", state.play_mode); + } + 2 => { + state.show_refresh_confirm = true; + tracing::info!("Refresh requested from context menu"); + } + _ => {} + } + } + } + Ok(()) +} + async fn run_app( terminal: &mut Terminal, state: &mut AppState, @@ -361,64 +457,7 @@ async fn handle_key_event(terminal: &mut Terminal< let menu_type = menu.menu_type; let selected = menu.selected_index; state.context_menu = None; - - match menu_type { - ContextMenuType::FilePanel => { - match selected { - 0 => action_play_selection(state, player)?, - 1 => state.add_to_playlist(), - _ => {} - } - } - ContextMenuType::Playlist => { - match selected { - 0 => { - // Remove - let was_playing_removed = state.playlist_index == state.selected_playlist_index; - state.remove_selected_playlist_item(); - - // Handle edge cases after removal - if state.playlist.is_empty() { - state.player_state = PlayerState::Stopped; - state.current_file = None; - player.stop()?; - } else if was_playing_removed && state.player_state == PlayerState::Playing { - // Removed currently playing track, start new one at same index - state.current_file = Some(state.playlist[state.playlist_index].clone()); - if let Some(ref path) = state.current_file { - player.play(path)?; - player.update_metadata(); - } - } - } - 1 => { - // Randomise - state.shuffle_playlist(); - tracing::info!("Playlist randomised from context menu"); - } - _ => {} - } - } - ContextMenuType::TitleBar => { - match selected { - 0 => { - // Stop - action_stop(state, player)?; - } - 1 => { - // Toggle Loop - state.cycle_play_mode(); - tracing::info!("Play mode: {:?}", state.play_mode); - } - 2 => { - // Refresh - state.show_refresh_confirm = true; - tracing::info!("Refresh requested from context menu"); - } - _ => {} - } - } - } + handle_context_menu_action(menu_type, selected, state, player)?; } KeyCode::Esc => { state.context_menu = None; @@ -604,39 +643,13 @@ async fn handle_key_event(terminal: &mut Terminal< } (KeyCode::Char('d'), _) => { if state.focus_playlist { - // Remove selected track from playlist - state.remove_selected_playlist_item(); - // If removed currently playing track, handle it - if state.playlist.is_empty() { - state.player_state = PlayerState::Stopped; - state.current_file = None; - player.stop()?; - } else if state.playlist_index == state.selected_playlist_index { - // Removed currently playing track, play next one - if state.playlist_index < state.playlist.len() { - state.current_file = Some(state.playlist[state.playlist_index].clone()); - if state.player_state == PlayerState::Playing { - if let Some(ref path) = state.current_file { - player.play(path)?; - player.update_metadata(); - } - } - } - } + action_remove_from_playlist(state, player)?; } } (KeyCode::Enter, _) => { if state.focus_playlist { - // Play selected track from playlist if state.selected_playlist_index < state.playlist.len() { - state.playlist_index = state.selected_playlist_index; - state.current_file = Some(state.playlist[state.playlist_index].clone()); - state.player_state = PlayerState::Playing; - if let Some(ref path) = state.current_file { - player.play(path)?; - player.update_metadata(); - tracing::info!("Playing from playlist: {:?}", path); - } + action_play_from_playlist(state, player, false)?; } } else { action_play_selection(state, player)?; @@ -758,63 +771,7 @@ fn handle_mouse_event(state: &mut AppState, mouse: MouseEvent, title_bar_area: r let menu_type = menu.menu_type; let selected = relative_y; state.context_menu = None; - - match menu_type { - ContextMenuType::FilePanel => { - match selected { - 0 => action_play_selection(state, player)?, - 1 => state.add_to_playlist(), - _ => {} - } - } - ContextMenuType::Playlist => { - match selected { - 0 => { - // Remove - let was_playing_removed = state.playlist_index == state.selected_playlist_index; - state.remove_selected_playlist_item(); - - // Handle edge cases after removal - if state.playlist.is_empty() { - state.player_state = PlayerState::Stopped; - state.current_file = None; - player.stop()?; - } else if was_playing_removed && state.player_state == PlayerState::Playing { - state.current_file = Some(state.playlist[state.playlist_index].clone()); - if let Some(ref path) = state.current_file { - player.play(path)?; - player.update_metadata(); - } - } - } - 1 => { - // Randomise - state.shuffle_playlist(); - tracing::info!("Playlist randomised from context menu (mouse)"); - } - _ => {} - } - } - ContextMenuType::TitleBar => { - match selected { - 0 => { - // Stop - action_stop(state, player)?; - } - 1 => { - // Toggle Loop - state.cycle_play_mode(); - tracing::info!("Play mode: {:?} (mouse)", state.play_mode); - } - 2 => { - // Refresh - state.show_refresh_confirm = true; - tracing::info!("Refresh requested from context menu (mouse)"); - } - _ => {} - } - } - } + handle_context_menu_action(menu_type, selected, state, player)?; } return Ok(()); } else { @@ -994,38 +951,9 @@ fn handle_mouse_event(state: &mut AppState, mouse: MouseEvent, title_bar_area: r }; if is_double_click { - // Double click = play the track - state.playlist_index = actual_track; - state.current_file = Some(state.playlist[state.playlist_index].clone()); - - match state.player_state { - PlayerState::Playing => { - // Keep playing - if let Some(ref path) = state.current_file { - player.play(path)?; - player.update_metadata(); - tracing::info!("Jumped to track: {:?}", path); - } - } - PlayerState::Paused => { - // Load but stay paused - if let Some(ref path) = state.current_file { - player.play(path)?; - player.update_metadata(); - player.pause()?; - tracing::info!("Jumped to track (paused): {:?}", path); - } - } - PlayerState::Stopped => { - // Start playing from clicked track - state.player_state = PlayerState::Playing; - if let Some(ref path) = state.current_file { - player.play(path)?; - player.update_metadata(); - tracing::info!("Started playing track: {:?}", path); - } - } - } + // Double click = play the track (preserve pause state) + state.selected_playlist_index = actual_track; + action_play_from_playlist(state, player, true)?; // Reset click tracking after action state.last_click_time = None; state.last_click_index = None;