Implement UI for creating new playlists #234
Conversation
nuugen
force-pushed
the
create-playlist-ui
branch
from
a238a01
to
77c08d0
Compare
There was a problem hiding this comment.
Looking good. Thanks for the PR and taking time to implement this feature. A few suggestions.
The desc field looks to be broken upstream in rspotify - the new playlist does not seem to have this informatino attached. Both of these limitations are also observed in the CLI implementation
Can you elaborate by "the new playlist does not seem to have this information attached"? desc seems to work fine for me. I can see the description of new playlist created by spotify_player in the official Spotify app.
once there are more instances of the pop-up with input prompt (similar to both Search and PlaylistCreate here), it should be abstracted.
nice to have for sure but no need to implement one in this PR.
| desc, | ||
| } => { | ||
| let user_id = state.data.read().user_data.user.to_owned().unwrap().id; | ||
| let _ = self |
There was a problem hiding this comment.
can have a info log here saying a new playlist with ... id was successfully created.
| let _ = self | ||
| .create_new_playlist( | ||
| state, | ||
| user_id.clone(), |
| Playlist { | ||
| id: resp.id.to_owned(), | ||
| collaborative: collab, | ||
| name: playlist_name.to_string(), | ||
| owner: ("".into(), user_id), | ||
| }, |
There was a problem hiding this comment.
should use resp.into() instead. state::Playlist does implement the From<rspotify_model::FullPlaylist> trait.
| @@ -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", | |||
There was a problem hiding this comment.
| Self::CreatePlaylist => "create new playlist", | |
| Self::CreatePlaylist => "create a new playlist", |
There was a problem hiding this comment.
can you update https://github.com/aome510/spotify-player#commands as well?
| .split(chunks[1]); | ||
|
|
||
| let name_input = construct_and_render_block( | ||
| "Enter Name for New Playlist:", |
There was a problem hiding this comment.
| "Enter Name for New Playlist:", | |
| "Enter Name for New Playlist", |
| if !name.is_empty() { | ||
| name.pop().unwrap(); | ||
| } |
There was a problem hiding this comment.
| if !name.is_empty() { | |
| name.pop().unwrap(); | |
| } | |
| name.pop(); |
no need to check is_empty then unwrap
| if !desc.is_empty() { | ||
| desc.pop().unwrap(); | ||
| } |
There was a problem hiding this comment.
| if !desc.is_empty() { | |
| desc.pop().unwrap(); | |
| } | |
| desc.pop(); |
| } | ||
| return Ok(true); | ||
| } | ||
| crossterm::event::KeyCode::Tab => { |
There was a problem hiding this comment.
| crossterm::event::KeyCode::Tab => { | |
| crossterm::event::KeyCode::Tab | crossterm::event::KeyCode::BackTab => { |
can also handle shift+Tab
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
just return Ok(false) in this case.
| Command::CreatePlaylist => { | ||
| ui.popup = Some(PopupState::PlaylistCreate { | ||
| name: "".into(), | ||
| desc: "".into(), | ||
| current_field: PlaylistCreateCurrentField::Name, | ||
| }); | ||
| } |
There was a problem hiding this comment.
shouldn't handle the create playlist command here.
I just tested this again today, and the description is now transmitted properly. On the day I was implementing this feature, that was not the case - neither through the popup or the CLI. Weird. |
|
Thanks for the great suggestions, will work through them when time permits. |
|
This feature looks great ✨ |
|
I'm still waiting for updates from @nuugen. I can take over if time permits. |
|
Hi, I noticed that this PR is stuck for some time now. May I finish this implementation as this feature would be really useful? |
|
Hi, sorry all for the hold-up. @m-torhan, please, go ahead with finishing this feature. Thanks! |
|
Close in favour of #365 |
This attempts to implement a rudimentary UI flow for creating a new playlist.
Resolves #87.
Usage:
While focused on the playlist window, pressing
Nwill show a pop-up prompting user to enter the name and the description of the new playlist to be created. PressingTabhere will switch focus between the two fields.Limitation:
For simplicity, the
publicandcollabstatuses will be omitted.TheThis seems to be a misobservation.descfield looks to be broken upstream inrspotify- the new playlist does not seem to have this informatino attached. Both of these limitations are also observed in the CLI implementation (#222).I'm also relatively new to Rust, so I mainly focused on submitting a purely functioning implementation, without much regard to the DRYness of code - once there are more instances of the pop-up with input prompt (similar to both
SearchandPlaylistCreatehere), it should be abstracted.Suggestions and critics are of course more than welcome and would help me a lot getting started in my Rust journey.