Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement UI for creating new playlists #234

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 14 additions & 31 deletions spotify_player/src/cli/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -375,45 +375,28 @@ async fn handle_playlist_request(
collab,
description,
} => {
let user = state.data.read().user_data.user.to_owned().unwrap();
let id = user.id;

let resp = client
.spotify
.user_playlist_create(
id,
let user_id = state.data.read().user_data.user.to_owned().unwrap().id;
let playlist_id = client
.create_new_playlist(
state,
user_id.clone(),
name.as_str(),
Some(public),
Some(collab),
Some(description.as_str()),
public,
collab,
description.as_str(),
)
.await?;
Ok(format!(
"Playlist '{}' with id '{}' was created.",
resp.name, resp.id
name,
playlist_id.id()
))
}
PlaylistCommand::Delete { id } => {
let user = state.data.read().user_data.user.to_owned().unwrap();
let uid = user.id;

let following = client
.spotify
.playlist_check_follow(id.to_owned(), &[uid])
.await
.context(format!("Could not find playlist '{}'", id.id()))?
.pop()
.unwrap();

// Won't delete if not following
if following {
client.spotify.playlist_unfollow(id.to_owned()).await?;
Ok(format!("Playlist '{id}' was deleted/unfollowed"))
} else {
Ok(format!(
"Playlist '{id}' was not followed by the user, nothing to be done.",
))
}
client
.delete_from_library(state, crate::state::ItemId::Playlist(id.to_owned()))
.await?;
Ok(format!("Playlist with id '{}' was deleted.", id.id()))
}
PlaylistCommand::List => {
let resp = client.current_user_playlists().await?;
Expand Down
49 changes: 49 additions & 0 deletions spotify_player/src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,24 @@ impl Client {
)
.await?;
}
ClientRequest::CreatePlaylist {
playlist_name,
public,
collab,
desc,
} => {
let user_id = state.data.read().user_data.user.to_owned().unwrap().id;
let _ = self
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can have a info log here saying a new playlist with ... id was successfully created.

.create_new_playlist(
state,
user_id.clone(),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant clone?

playlist_name.as_str(),
public,
collab,
desc.as_str(),
)
.await?;
}
};

tracing::info!(
Expand Down Expand Up @@ -1282,6 +1300,37 @@ impl Client {
Ok(())
}

pub async fn create_new_playlist(
&self,
state: &SharedState,
user_id: UserId<'static>,
playlist_name: &str,
public: bool,
collab: bool,
desc: &str,
) -> Result<PlaylistId> {
let resp = self
.spotify
.user_playlist_create(
user_id.to_owned(),
playlist_name,
Some(public),
Some(collab),
Some(desc),
)
.await?;
state.data.write().user_data.playlists.insert(
0,
Playlist {
id: resp.id.to_owned(),
collaborative: collab,
name: playlist_name.to_string(),
owner: ("".into(), user_id),
},
Comment on lines +1324 to +1329
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should use resp.into() instead. state::Playlist does implement the From<rspotify_model::FullPlaylist> trait.

);
Ok(resp.id)
}

#[cfg(feature = "notify")]
fn notify_new_track(
track: rspotify_model::FullTrack,
Expand Down
4 changes: 4 additions & 0 deletions spotify_player/src/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ pub enum Command {

MovePlaylistItemUp,
MovePlaylistItemDown,

CreatePlaylist,
}

#[derive(Debug, Copy, Clone)]
Expand Down Expand Up @@ -204,6 +206,8 @@ impl Command {
Self::ReverseTrackOrder => "reverse the order of the track table (if any)",
Self::MovePlaylistItemUp => "move playlist item up one position",
Self::MovePlaylistItemDown => "move playlist item down one position",

Self::CreatePlaylist => "create new playlist",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Self::CreatePlaylist => "create new playlist",
Self::CreatePlaylist => "create a new playlist",

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
}
}
4 changes: 4 additions & 0 deletions spotify_player/src/config/keymap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,10 @@ impl Default for KeymapConfig {
key_sequence: "C-j".into(),
command: Command::MovePlaylistItemDown,
},
Keymap {
key_sequence: "N".into(),
command: Command::CreatePlaylist,
},
],
}
}
Expand Down
6 changes: 6 additions & 0 deletions spotify_player/src/event/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ pub enum ClientRequest {
},
#[cfg(feature = "streaming")]
NewStreamingConnection,
CreatePlaylist {
playlist_name: String,
public: bool,
collab: bool,
desc: String,
},
}

/// starts a terminal event handler (key pressed, mouse clicked, etc)
Expand Down
104 changes: 103 additions & 1 deletion spotify_player/src/event/popup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,16 @@ pub fn handle_key_sequence_for_popup(
// handle popups that need reading the raw key sequence instead of the matched command
match popup {
PopupState::Search { .. } => {
// NOTE: the `drop(ui)` is important as the handle function for search
// NOTE: the `drop(ui)` is important as the handle function for the popup
// re-acquire the UI lock, so we need to drop the current UI lock to avoid a deadlock.
drop(ui);
return handle_key_sequence_for_search_popup(key_sequence, client_pub, state);
}
PopupState::PlaylistCreate { .. } => {
// NOTE: same here.
drop(ui);
return handle_key_sequence_for_create_playlist_popup(key_sequence, client_pub, state);
}
PopupState::ActionList(item, ..) => {
return handle_key_sequence_for_action_list_popup(
item.n_actions(),
Expand All @@ -49,6 +54,9 @@ pub fn handle_key_sequence_for_popup(

match popup {
PopupState::Search { .. } => anyhow::bail!("search popup should be handled before"),
PopupState::PlaylistCreate { .. } => {
anyhow::bail!("create playlist popup should be handled before")
}
PopupState::ActionList(..) => {
anyhow::bail!("action list popup should be handled before")
}
Expand Down Expand Up @@ -259,6 +267,100 @@ fn handle_command_for_queue_popup(
Ok(true)
}

fn handle_key_sequence_for_create_playlist_popup(
key_sequence: &KeySequence,
client_pub: &flume::Sender<ClientRequest>,
state: &SharedState,
) -> Result<bool> {
{
let mut ui = state.ui.lock();
let (name, desc, current_field) = match ui.popup {
Some(PopupState::PlaylistCreate {
ref mut name,
ref mut desc,
ref mut current_field,
}) => (name, desc, current_field),
_ => return Ok(false),
};
if key_sequence.keys.len() == 1 {
if let Key::None(c) = key_sequence.keys[0] {
match c {
crossterm::event::KeyCode::Char(c) => {
match &current_field {
PlaylistCreateCurrentField::Name => {
name.push(c);
}
PlaylistCreateCurrentField::Desc => {
desc.push(c);
}
}
return Ok(true);
}
crossterm::event::KeyCode::Backspace => {
match &current_field {
PlaylistCreateCurrentField::Name => {
if !name.is_empty() {
name.pop().unwrap();
}
Comment on lines +302 to +304
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if !name.is_empty() {
name.pop().unwrap();
}
name.pop();

no need to check is_empty then unwrap

}
PlaylistCreateCurrentField::Desc => {
if !desc.is_empty() {
desc.pop().unwrap();
}
Comment on lines +307 to +309
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if !desc.is_empty() {
desc.pop().unwrap();
}
desc.pop();

}
}
return Ok(true);
}
crossterm::event::KeyCode::Tab => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
crossterm::event::KeyCode::Tab => {
crossterm::event::KeyCode::Tab | crossterm::event::KeyCode::BackTab => {

can also handle shift+Tab

*current_field = match &current_field {
PlaylistCreateCurrentField::Name => PlaylistCreateCurrentField::Desc,
PlaylistCreateCurrentField::Desc => PlaylistCreateCurrentField::Name,
};
return Ok(true);
}
crossterm::event::KeyCode::Enter => {
client_pub.send(ClientRequest::CreatePlaylist {
playlist_name: name.to_owned(),
public: false,
collab: false,
desc: desc.to_owned(),
})?;
ui.popup = None;
return Ok(true);
}
_ => {}
}
}
}
}

let command = state
.keymap_config
.find_command_from_key_sequence(key_sequence);
if let Some(Command::ClosePopup) = command {
state.ui.lock().popup = None;
return Ok(true);
}

let page_type = state.ui.lock().current_page().page_type();
match page_type {
PageType::Library => page::handle_key_sequence_for_library_page(key_sequence, state),
PageType::Search => {
page::handle_key_sequence_for_search_page(key_sequence, client_pub, state)
}
PageType::Context => {
page::handle_key_sequence_for_context_page(key_sequence, client_pub, state)
}
PageType::Browse => {
page::handle_key_sequence_for_browse_page(key_sequence, client_pub, state)
}
#[cfg(feature = "lyric-finder")]
PageType::Lyric => {
page::handle_key_sequence_for_lyric_page(key_sequence, client_pub, state)
}
}
Comment on lines +345 to +361
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to do this for the create playlist popup. Search popup is special because the focus is not placed on it when the popup is created. It's expected to use along with the focused window for interactive searching.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just return Ok(false) in this case.

}

/// handles a key sequence for a context search popup
fn handle_key_sequence_for_search_popup(
key_sequence: &KeySequence,
Expand Down
7 changes: 7 additions & 0 deletions spotify_player/src/event/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,13 @@ pub fn handle_command_for_playlist_list_window(
new_list_state(),
));
}
Command::CreatePlaylist => {
ui.popup = Some(PopupState::PlaylistCreate {
name: "".into(),
desc: "".into(),
current_field: PlaylistCreateCurrentField::Name,
});
}
Comment on lines +426 to +432
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't handle the create playlist command here.

_ => return Ok(false),
}
Ok(true)
Expand Down
33 changes: 28 additions & 5 deletions spotify_player/src/state/ui/popup.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,35 @@
use crate::{command, state::model::*};
use tui::widgets::ListState;

#[derive(Debug)]
pub enum PlaylistCreateCurrentField {
Name,
Desc,
}

#[derive(Debug)]
pub enum PopupState {
CommandHelp { scroll_offset: usize },
Search { query: String },
CommandHelp {
scroll_offset: usize,
},
Search {
query: String,
},
UserPlaylistList(PlaylistPopupAction, ListState),
UserFollowedArtistList(ListState),
UserSavedAlbumList(ListState),
DeviceList(ListState),
ArtistList(ArtistPopupAction, Vec<Artist>, ListState),
ThemeList(Vec<crate::config::Theme>, ListState),
ActionList(ActionListItem, ListState),
Queue { scroll_offset: usize },
Queue {
scroll_offset: usize,
},
PlaylistCreate {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to have a focus state for PlaylistCreate so that user can know which component is in focus. See the search page implementation and its UI for reference.

name: String,
desc: String,
current_field: PlaylistCreateCurrentField,
},
}

#[derive(Debug, Clone)]
Expand Down Expand Up @@ -48,7 +65,10 @@ impl PopupState {
Self::ArtistList(.., list_state) => Some(list_state),
Self::ThemeList(.., list_state) => Some(list_state),
Self::ActionList(.., list_state) => Some(list_state),
Self::CommandHelp { .. } | Self::Search { .. } | Self::Queue { .. } => None,
Self::CommandHelp { .. }
| Self::Search { .. }
| Self::Queue { .. }
| Self::PlaylistCreate { .. } => None,
}
}

Expand All @@ -62,7 +82,10 @@ impl PopupState {
Self::ArtistList(.., list_state) => Some(list_state),
Self::ThemeList(.., list_state) => Some(list_state),
Self::ActionList(.., list_state) => Some(list_state),
Self::CommandHelp { .. } | Self::Search { .. } | Self::Queue { .. } => None,
Self::CommandHelp { .. }
| Self::Search { .. }
| Self::Queue { .. }
| Self::PlaylistCreate { .. } => None,
}
}

Expand Down
Loading
Loading