From 3e0cef09028e1b909fbabbef7f4fc3ce0e76ec88 Mon Sep 17 00:00:00 2001 From: yokko Date: Tue, 7 Apr 2026 11:13:24 +0800 Subject: [PATCH 1/5] feat: added confirmation popups on every destructive action --- spotify_player/src/event/mod.rs | 44 ++++++++++++++++------------ spotify_player/src/event/popup.rs | 44 +++++++++++++++++++++++++++- spotify_player/src/state/ui/popup.rs | 21 ++++++++++++- spotify_player/src/ui/popup.rs | 16 ++++++++++ 4 files changed, 104 insertions(+), 21 deletions(-) diff --git a/spotify_player/src/event/mod.rs b/spotify_player/src/event/mod.rs index f3861e2b..2af84a4a 100644 --- a/spotify_player/src/event/mod.rs +++ b/spotify_player/src/event/mod.rs @@ -8,12 +8,12 @@ use crate::{ key::{Key, KeySequence}, state::{ ActionListItem, Album, AlbumId, Artist, ArtistFocusState, ArtistId, ArtistPopupAction, - BrowsePageUIState, Context, ContextId, ContextPageType, ContextPageUIState, DataReadGuard, - Focusable, Id, Item, ItemId, LibraryFocusState, LibraryPageUIState, PageState, PageType, - PlayableId, Playback, PlaylistCreateCurrentField, PlaylistFolderItem, PlaylistId, - PlaylistPopupAction, PopupState, SearchFocusState, SearchPageUIState, SharedState, ShowId, - Track, TrackId, TrackOrder, TracksId, UIStateGuard, USER_LIKED_TRACKS_ID, - USER_RECENTLY_PLAYED_TRACKS_ID, USER_TOP_TRACKS_ID, + BrowsePageUIState, ConfirmableAction, Context, ContextId, ContextPageType, + ContextPageUIState, DataReadGuard, Focusable, Id, Item, ItemId, LibraryFocusState, + LibraryPageUIState, PageState, PageType, PlayableId, Playback, PlaylistCreateCurrentField, + PlaylistFolderItem, PlaylistId, PlaylistPopupAction, PopupState, SearchFocusState, + SearchPageUIState, SharedState, ShowId, Track, TrackId, TrackOrder, TracksId, UIStateGuard, + USER_LIKED_TRACKS_ID, USER_RECENTLY_PLAYED_TRACKS_ID, USER_TOP_TRACKS_ID, }, ui::{single_line_input::LineInput, Orientation}, utils::parse_uri, @@ -289,12 +289,14 @@ pub fn handle_action_in_context( .. } = ui.current_page() { - client_pub.send(ClientRequest::DeleteTrackFromPlaylist( - playlist_id.clone_static(), - track.id, - ))?; + ui.popup = Some(PopupState::ConfirmAction { + message: "Are you sure?".to_string(), + action: ConfirmableAction::DeleteTrackFromPlaylist { + playlist_id: playlist_id.clone_static(), + track_id: track.id, + }, + }); } - ui.popup = None; Ok(true) } _ => Ok(false), @@ -318,8 +320,10 @@ pub fn handle_action_in_context( Ok(true) } Action::DeleteFromLibrary => { - client_pub.send(ClientRequest::DeleteFromLibrary(ItemId::Album(album.id)))?; - ui.popup = None; + ui.popup = Some(PopupState::ConfirmAction { + message: "Are you sure?".to_string(), + action: ConfirmableAction::DeleteFromLibrary(ItemId::Album(album.id)), + }); Ok(true) } Action::CopyLink => { @@ -376,10 +380,10 @@ pub fn handle_action_in_context( Ok(true) } Action::DeleteFromLibrary => { - client_pub.send(ClientRequest::DeleteFromLibrary(ItemId::Playlist( - playlist.id, - )))?; - ui.popup = None; + ui.popup = Some(PopupState::ConfirmAction { + message: "Are you sure?".to_string(), + action: ConfirmableAction::DeleteFromLibrary(ItemId::Playlist(playlist.id)), + }); Ok(true) } _ => Ok(false), @@ -397,8 +401,10 @@ pub fn handle_action_in_context( Ok(true) } Action::DeleteFromLibrary => { - client_pub.send(ClientRequest::DeleteFromLibrary(ItemId::Show(show.id)))?; - ui.popup = None; + ui.popup = Some(PopupState::ConfirmAction { + message: "Are you sure?".to_string(), + action: ConfirmableAction::DeleteFromLibrary(ItemId::Show(show.id)), + }); Ok(true) } _ => Ok(false), diff --git a/spotify_player/src/event/popup.rs b/spotify_player/src/event/popup.rs index 7b7c8521..4c7f971c 100644 --- a/spotify_player/src/event/popup.rs +++ b/spotify_player/src/event/popup.rs @@ -1,5 +1,7 @@ use super::*; -use crate::{command::construct_artist_actions, utils::filtered_items_from_query}; +use crate::{ + command::construct_artist_actions, state::ConfirmableAction, utils::filtered_items_from_query, +}; use anyhow::Context; pub fn handle_key_sequence_for_popup( @@ -30,6 +32,14 @@ pub fn handle_key_sequence_for_popup( return Ok(true); } } + PopupState::ConfirmAction { action, .. } => { + return handle_key_sequence_for_confirm_popup( + key_sequence, + client_pub, + ui, + action.clone(), + ); + } _ => {} } @@ -41,6 +51,9 @@ pub fn handle_key_sequence_for_popup( }; match ui.popup.as_ref().context("empty popup")? { + PopupState::ConfirmAction { .. } => { + anyhow::bail!("confirm action should be handler before") + } PopupState::Search { .. } => anyhow::bail!("search popup should be handled before"), PopupState::PlaylistCreate { .. } => { anyhow::bail!("create playlist popup should be handled before") @@ -600,3 +613,32 @@ fn handle_key_sequence_for_playlist_search_popup( false } + +fn handle_key_sequence_for_confirm_popup( + key_sequence: &KeySequence, + client_pub: &flume::Sender, + ui: &mut UIStateGuard, + action: ConfirmableAction, +) -> Result { + if matches!( + key_sequence.keys.as_slice(), + [Key::None(crossterm::event::KeyCode::Char('y'))] + ) { + match action { + ConfirmableAction::DeleteTrackFromPlaylist { + playlist_id, + track_id, + } => { + client_pub.send(ClientRequest::DeleteTrackFromPlaylist( + playlist_id, + track_id, + ))?; + } + ConfirmableAction::DeleteFromLibrary(item_id) => { + client_pub.send(ClientRequest::DeleteFromLibrary(item_id))?; + } + } + } + ui.popup = None; + Ok(true) +} diff --git a/spotify_player/src/state/ui/popup.rs b/spotify_player/src/state/ui/popup.rs index a69415ab..0c7931a5 100644 --- a/spotify_player/src/state/ui/popup.rs +++ b/spotify_player/src/state/ui/popup.rs @@ -1,9 +1,13 @@ use crate::{ command, - state::model::{Album, Artist, Episode, EpisodeId, Playlist, Show, Track, TrackId}, + state::{ + model::{Album, Artist, Episode, EpisodeId, Playlist, Show, Track, TrackId}, + ItemId, + }, ui::single_line_input::LineInput, }; use ratatui::widgets::ListState; +use rspotify::model::PlaylistId; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum PlaylistCreateCurrentField { @@ -28,6 +32,19 @@ pub enum PopupState { desc: LineInput, current_field: PlaylistCreateCurrentField, }, + ConfirmAction { + message: String, + action: ConfirmableAction, + }, +} + +#[derive(Debug, Clone)] +pub enum ConfirmableAction { + DeleteTrackFromPlaylist { + playlist_id: PlaylistId<'static>, + track_id: TrackId<'static>, + }, + DeleteFromLibrary(ItemId), } #[derive(Debug, Clone)] @@ -78,6 +95,7 @@ impl PopupState { | Self::ThemeList(.., list_state) | Self::ActionList(.., list_state) => Some(list_state), Self::Search { .. } | Self::PlaylistCreate { .. } => None, + Self::ConfirmAction { .. } => None, } } @@ -92,6 +110,7 @@ impl PopupState { | Self::ThemeList(.., list_state) | Self::ActionList(.., list_state) => Some(list_state), Self::Search { .. } | Self::PlaylistCreate { .. } => None, + Self::ConfirmAction { .. } => None, } } diff --git a/spotify_player/src/ui/popup.rs b/spotify_player/src/ui/popup.rs index a133ef65..46241079 100644 --- a/spotify_player/src/ui/popup.rs +++ b/spotify_player/src/ui/popup.rs @@ -196,6 +196,22 @@ pub fn render_popup( let rect = render_list_popup(frame, rect, "Artists", items, 5, ui); (rect, false) } + PopupState::ConfirmAction { message, .. } => { + let chunks = + Layout::vertical([Constraint::Fill(0), Constraint::Length(3)]).split(rect); + + let confirm_rect = construct_and_render_block( + "Confirm", + &ui.theme, + Borders::ALL, + frame, + chunks[1], + ); + + frame.render_widget(Paragraph::new(format!("{message} (y/n)")), confirm_rect); + + (chunks[0], true) + } }, } } From 8e4d255d7cf45ea38a2cab54b90a812e75f412de Mon Sep 17 00:00:00 2001 From: yokko Date: Tue, 7 Apr 2026 12:03:13 +0800 Subject: [PATCH 2/5] fix: fixed clippy warns --- spotify_player/src/state/ui/popup.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/spotify_player/src/state/ui/popup.rs b/spotify_player/src/state/ui/popup.rs index 0c7931a5..806e8421 100644 --- a/spotify_player/src/state/ui/popup.rs +++ b/spotify_player/src/state/ui/popup.rs @@ -94,8 +94,7 @@ impl PopupState { | Self::ArtistList(.., list_state) | Self::ThemeList(.., list_state) | Self::ActionList(.., list_state) => Some(list_state), - Self::Search { .. } | Self::PlaylistCreate { .. } => None, - Self::ConfirmAction { .. } => None, + Self::Search { .. } | Self::PlaylistCreate { .. } | Self::ConfirmAction { .. } => None, } } @@ -109,8 +108,7 @@ impl PopupState { | Self::ArtistList(.., list_state) | Self::ThemeList(.., list_state) | Self::ActionList(.., list_state) => Some(list_state), - Self::Search { .. } | Self::PlaylistCreate { .. } => None, - Self::ConfirmAction { .. } => None, + Self::Search { .. } | Self::PlaylistCreate { .. } | Self::ConfirmAction { .. } => None, } } From 27145866c463d43cee67a5e52ce31c7048bfe484 Mon Sep 17 00:00:00 2001 From: yokko Date: Wed, 8 Apr 2026 09:49:47 +0800 Subject: [PATCH 3/5] fix: changed to more detailed labels --- spotify_player/src/event/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spotify_player/src/event/mod.rs b/spotify_player/src/event/mod.rs index 2af84a4a..a160cd81 100644 --- a/spotify_player/src/event/mod.rs +++ b/spotify_player/src/event/mod.rs @@ -290,7 +290,7 @@ pub fn handle_action_in_context( } = ui.current_page() { ui.popup = Some(PopupState::ConfirmAction { - message: "Are you sure?".to_string(), + message: "Delete this track?".to_string(), action: ConfirmableAction::DeleteTrackFromPlaylist { playlist_id: playlist_id.clone_static(), track_id: track.id, @@ -321,7 +321,7 @@ pub fn handle_action_in_context( } Action::DeleteFromLibrary => { ui.popup = Some(PopupState::ConfirmAction { - message: "Are you sure?".to_string(), + message: "Delete this album?".to_string(), action: ConfirmableAction::DeleteFromLibrary(ItemId::Album(album.id)), }); Ok(true) @@ -381,7 +381,7 @@ pub fn handle_action_in_context( } Action::DeleteFromLibrary => { ui.popup = Some(PopupState::ConfirmAction { - message: "Are you sure?".to_string(), + message: "Delete this playlist?".to_string(), action: ConfirmableAction::DeleteFromLibrary(ItemId::Playlist(playlist.id)), }); Ok(true) @@ -402,7 +402,7 @@ pub fn handle_action_in_context( } Action::DeleteFromLibrary => { ui.popup = Some(PopupState::ConfirmAction { - message: "Are you sure?".to_string(), + message: "Delete this show?".to_string(), action: ConfirmableAction::DeleteFromLibrary(ItemId::Show(show.id)), }); Ok(true) From 8646640581a2b161e963ab2af1890bcd97db2ee8 Mon Sep 17 00:00:00 2001 From: yokko Date: Thu, 9 Apr 2026 09:46:27 +0800 Subject: [PATCH 4/5] fix: more detailed confirmation messages (include item name) --- spotify_player/src/event/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spotify_player/src/event/mod.rs b/spotify_player/src/event/mod.rs index a160cd81..ab0b06ed 100644 --- a/spotify_player/src/event/mod.rs +++ b/spotify_player/src/event/mod.rs @@ -290,7 +290,7 @@ pub fn handle_action_in_context( } = ui.current_page() { ui.popup = Some(PopupState::ConfirmAction { - message: "Delete this track?".to_string(), + message: format!("Delete {}?", track.name), action: ConfirmableAction::DeleteTrackFromPlaylist { playlist_id: playlist_id.clone_static(), track_id: track.id, @@ -321,7 +321,7 @@ pub fn handle_action_in_context( } Action::DeleteFromLibrary => { ui.popup = Some(PopupState::ConfirmAction { - message: "Delete this album?".to_string(), + message: format!("Delete {}?", album.name), action: ConfirmableAction::DeleteFromLibrary(ItemId::Album(album.id)), }); Ok(true) @@ -381,7 +381,7 @@ pub fn handle_action_in_context( } Action::DeleteFromLibrary => { ui.popup = Some(PopupState::ConfirmAction { - message: "Delete this playlist?".to_string(), + message: format!("Delete {}?", playlist.name), action: ConfirmableAction::DeleteFromLibrary(ItemId::Playlist(playlist.id)), }); Ok(true) @@ -402,7 +402,7 @@ pub fn handle_action_in_context( } Action::DeleteFromLibrary => { ui.popup = Some(PopupState::ConfirmAction { - message: "Delete this show?".to_string(), + message: format!("Delete {}?", show.name), action: ConfirmableAction::DeleteFromLibrary(ItemId::Show(show.id)), }); Ok(true) From b1cd9560ddab95b7ef2947ae413c1bd24d0fceed Mon Sep 17 00:00:00 2001 From: Thang Pham Date: Fri, 10 Apr 2026 09:45:28 -0400 Subject: [PATCH 5/5] cleanup --- spotify_player/src/event/mod.rs | 8 ++++---- spotify_player/src/event/popup.rs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/spotify_player/src/event/mod.rs b/spotify_player/src/event/mod.rs index ab0b06ed..660453d3 100644 --- a/spotify_player/src/event/mod.rs +++ b/spotify_player/src/event/mod.rs @@ -290,7 +290,7 @@ pub fn handle_action_in_context( } = ui.current_page() { ui.popup = Some(PopupState::ConfirmAction { - message: format!("Delete {}?", track.name), + message: format!("Delete {} from this playlist?", track.name), action: ConfirmableAction::DeleteTrackFromPlaylist { playlist_id: playlist_id.clone_static(), track_id: track.id, @@ -321,7 +321,7 @@ pub fn handle_action_in_context( } Action::DeleteFromLibrary => { ui.popup = Some(PopupState::ConfirmAction { - message: format!("Delete {}?", album.name), + message: format!("Delete {} from your library?", album.name), action: ConfirmableAction::DeleteFromLibrary(ItemId::Album(album.id)), }); Ok(true) @@ -381,7 +381,7 @@ pub fn handle_action_in_context( } Action::DeleteFromLibrary => { ui.popup = Some(PopupState::ConfirmAction { - message: format!("Delete {}?", playlist.name), + message: format!("Delete {} from your library?", playlist.name), action: ConfirmableAction::DeleteFromLibrary(ItemId::Playlist(playlist.id)), }); Ok(true) @@ -402,7 +402,7 @@ pub fn handle_action_in_context( } Action::DeleteFromLibrary => { ui.popup = Some(PopupState::ConfirmAction { - message: format!("Delete {}?", show.name), + message: format!("Delete {} from your library?", show.name), action: ConfirmableAction::DeleteFromLibrary(ItemId::Show(show.id)), }); Ok(true) diff --git a/spotify_player/src/event/popup.rs b/spotify_player/src/event/popup.rs index 4c7f971c..55540785 100644 --- a/spotify_player/src/event/popup.rs +++ b/spotify_player/src/event/popup.rs @@ -52,7 +52,7 @@ pub fn handle_key_sequence_for_popup( match ui.popup.as_ref().context("empty popup")? { PopupState::ConfirmAction { .. } => { - anyhow::bail!("confirm action should be handler before") + anyhow::bail!("confirm action should be handled before") } PopupState::Search { .. } => anyhow::bail!("search popup should be handled before"), PopupState::PlaylistCreate { .. } => {