From d53542afa69976da12c6469ebd2195f57dbc486c Mon Sep 17 00:00:00 2001 From: Christoffer Martinsson Date: Fri, 12 Dec 2025 16:19:56 +0100 Subject: [PATCH] Eliminate code duplication with unified action functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Create action functions for stop, volume, and seek operations and use them consistently across keyboard handlers, mouse handlers, and API handlers. This eliminates duplicate logic and ensures consistent behavior across all input methods. Also fixes stop command triggering auto-advance by setting the skip_position_update flag to prevent the Playing→Stopped transition from being interpreted as a natural track ending. --- Cargo.toml | 2 +- src/main.rs | 102 +++++++++++++++++++++++++--------------------------- 2 files changed, 50 insertions(+), 54 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 4a136cb..e92b322 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cm-player" -version = "0.1.32" +version = "0.1.33" edition = "2021" [dependencies] diff --git a/src/main.rs b/src/main.rs index 9efd20d..c9a5cbd 100644 --- a/src/main.rs +++ b/src/main.rs @@ -262,14 +262,44 @@ fn action_toggle_play_pause(state: &mut AppState, player: &mut player::Player) - Ok(()) } -fn action_stop(state: &mut AppState, player: &mut player::Player) -> Result<()> { +fn action_stop(state: &mut AppState, player: &mut player::Player, skip_position_update: &mut bool) -> Result<()> { player.stop()?; state.current_position = 0.0; state.current_duration = 0.0; + *skip_position_update = true; // Prevent auto-advance on manual stop tracing::info!("Stopped"); Ok(()) } +fn action_volume_up(state: &mut AppState, player: &mut player::Player) -> Result<()> { + state.volume = (state.volume + 5).min(100); + player.set_volume(state.volume)?; + tracing::info!("Volume: {}%", state.volume); + Ok(()) +} + +fn action_volume_down(state: &mut AppState, player: &mut player::Player) -> Result<()> { + state.volume = (state.volume - 5).max(0); + player.set_volume(state.volume)?; + tracing::info!("Volume: {}%", state.volume); + Ok(()) +} + +fn action_volume_set(state: &mut AppState, player: &mut player::Player, volume: i64) -> Result<()> { + state.volume = volume.clamp(0, 100); + player.set_volume(state.volume)?; + tracing::info!("Volume: {}%", state.volume); + Ok(()) +} + +fn action_seek(player: &mut player::Player, seconds: f64) -> Result<()> { + if player.get_player_state() != Some(PlayerState::Stopped) { + player.seek(seconds)?; + tracing::info!("Seek {}s", seconds); + } + 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; let was_playing = player.get_player_state() == Some(PlayerState::Playing); @@ -415,7 +445,7 @@ fn handle_context_menu_action(menu_type: state::ContextMenuType, selected: usize } state::ContextMenuType::TitleBar => { match selected { - 0 => action_stop(state, player)?, + 0 => action_stop(state, player, skip_position_update)?, 1 => { state.cycle_play_mode(); tracing::info!("Play mode: {:?}", state.play_mode); @@ -453,26 +483,11 @@ fn run_app( tracing::debug!("Processing API command: {:?}", cmd); match cmd { api::ApiCommand::PlayPause => { - if let Some(player_state) = player.get_player_state() { - match player_state { - PlayerState::Stopped => { - // Play current file or first in playlist - if state.current_file.is_none() && !state.playlist.is_empty() { - state.current_file = Some(state.playlist[0].clone()); - } - if let Some(ref file) = state.current_file { - player.play(file)?; - } - } - PlayerState::Playing => player.pause()?, - PlayerState::Paused => player.resume()?, - } - state_changed = true; - } + action_toggle_play_pause(state, player)?; + state_changed = true; } api::ApiCommand::Stop => { - player.stop()?; - state.current_file = None; + action_stop(state, player, &mut skip_position_update)?; state_changed = true; } api::ApiCommand::Next => { @@ -484,26 +499,23 @@ fn run_app( state_changed = true; } api::ApiCommand::VolumeUp => { - state.volume = (state.volume + 5).min(100); - player.set_volume(state.volume)?; + action_volume_up(state, player)?; state_changed = true; } api::ApiCommand::VolumeDown => { - state.volume = (state.volume - 5).max(0); - player.set_volume(state.volume)?; + action_volume_down(state, player)?; state_changed = true; } api::ApiCommand::VolumeSet { volume } => { - state.volume = volume.clamp(0, 100); - player.set_volume(state.volume)?; + action_volume_set(state, player, volume)?; state_changed = true; } api::ApiCommand::SeekForward { seconds } => { - player.seek(seconds)?; + action_seek(player, seconds)?; state_changed = true; } api::ApiCommand::SeekBackward { seconds } => { - player.seek(-seconds)?; + action_seek(player, -seconds)?; state_changed = true; } api::ApiCommand::GetStatus => { @@ -554,8 +566,10 @@ fn run_app( // Check if track ended and play next // When MPV finishes playing a file, it goes to idle (Stopped state) // Detect Playing → Stopped transition = track ended, play next + // But skip this check if we just manually stopped (skip_position_update flag) if previous_player_state == Some(PlayerState::Playing) && player_state == PlayerState::Stopped + && !skip_position_update { let should_continue = state.play_next(); // play_next() returns true if should continue playing, false if should stop @@ -894,7 +908,7 @@ fn handle_key_event(terminal: &mut Terminal, st } } (KeyCode::Char('s'), _) => { - action_stop(state, player)?; + action_stop(state, player, skip_position_update)?; } (KeyCode::Char('m'), _) => { state.cycle_play_mode(); @@ -908,28 +922,16 @@ fn handle_key_event(terminal: &mut Terminal, st action_toggle_play_pause(state, player)?; } (KeyCode::Char('H'), KeyModifiers::SHIFT) => { - if player.get_player_state() != Some(PlayerState::Stopped) { - player.seek(-10.0)?; - tracing::info!("Seek backward 10s"); - } + action_seek(player, -10.0)?; } (KeyCode::Char('L'), KeyModifiers::SHIFT) => { - if player.get_player_state() != Some(PlayerState::Stopped) { - player.seek(10.0)?; - tracing::info!("Seek forward 10s"); - } + action_seek(player, 10.0)?; } (KeyCode::Char('+'), _) | (KeyCode::Char('='), _) => { - let new_volume = (state.volume + 5).min(100); - state.volume = new_volume; - player.set_volume(new_volume)?; - tracing::info!("Volume: {}%", new_volume); + action_volume_up(state, player)?; } (KeyCode::Char('-'), _) => { - let new_volume = (state.volume - 5).max(0); - state.volume = new_volume; - player.set_volume(new_volume)?; - tracing::info!("Volume: {}%", new_volume); + action_volume_down(state, player)?; } (KeyCode::Char('r'), _) => { state.show_refresh_confirm = true; @@ -1032,10 +1034,7 @@ fn handle_mouse_event(state: &mut AppState, mouse: MouseEvent, title_bar_area: r && y < title_bar_area.y + title_bar_area.height { // Scroll on title bar = decrease volume - let new_volume = (state.volume - 5).max(0); - state.volume = new_volume; - player.set_volume(new_volume)?; - tracing::info!("Volume: {}%", new_volume); + action_volume_down(state, player)?; } else if x >= playlist_area.x && x < playlist_area.x + playlist_area.width && y >= playlist_area.y @@ -1058,10 +1057,7 @@ fn handle_mouse_event(state: &mut AppState, mouse: MouseEvent, title_bar_area: r && y < title_bar_area.y + title_bar_area.height { // Scroll on title bar = increase volume - let new_volume = (state.volume + 5).min(100); - state.volume = new_volume; - player.set_volume(new_volume)?; - tracing::info!("Volume: {}%", new_volume); + action_volume_up(state, player)?; } else if x >= playlist_area.x && x < playlist_area.x + playlist_area.width && y >= playlist_area.y