Play button for album and playlist #561
Conversation
…ing between playlists
xou816
force-pushed
the
play_button_for_album_and_playlist
branch
from
5331616
to
90e9a2a
Compare
|
|
||
| fn autoscroll_to_playing(&self) -> bool { | ||
| true | ||
| } | ||
|
|
||
| fn is_selection_enabled(&self) -> bool { | ||
| self.selection() | ||
| .map(|s| s.is_selection_enabled()) | ||
| .unwrap_or(false) | ||
| } | ||
|
|
||
| fn song_state(&self, id: &str) -> SongState { | ||
| let is_playing = self.current_song_id().map(|s| s.eq(id)).unwrap_or(false); | ||
| let is_selected = self | ||
| .selection() | ||
| .map(|s| s.is_song_selected(id)) | ||
| .unwrap_or(false); | ||
| SongState { | ||
| is_selected, | ||
| is_playing, | ||
| } | ||
| } | ||
|
|
||
| fn toggle_select(&self, id: &str) { | ||
| if let Some(selection) = self.selection() { | ||
| if selection.is_song_selected(id) { | ||
| self.deselect_song(id); | ||
| } else { | ||
| self.select_song(id); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
i think these are not need/already part of the trait?
| fn playlist_song_ids(&self) -> Option<Vec<String>> { | ||
| None | ||
| } | ||
|
|
||
| fn playlist_is_playing(&self) -> bool { | ||
| false | ||
| } | ||
|
|
There was a problem hiding this comment.
I get what you were trying to do -- but we don´t need these in the trait. code expecting a PlaylistModel will not need these methods
| None | ||
| } | ||
|
|
||
| fn playlist_is_playing(&self) -> bool { |
There was a problem hiding this comment.
there is a simpler way to implement this logic : you can look in the PlaybackState for source, you might have to add an accessor there but thats fine :)
| @@ -95,6 +95,19 @@ | |||
| </child> | |||
| </object> | |||
| </child> | |||
| <child> | |||
There was a problem hiding this comment.
I think we should have some box around the buttons if we're goign to have more than one -- see how it looks on mobile
|
Thanks for your review! I currently have exams, but once they are done in a few weeks, I will come back to this project and implement your change requests on all my PR's :) |
|
No worries at all, that gives me the time to go through the other PRs as well :) |
|
(just a heads up, there'll probably be a couple of merge conflicts, and more coming up -- sorry for the inconvenience, feel free to reach out if needed) |
|
I have some free time. If its alright @xou816 and @SarahRemus, could I try and take what I see here and create a new PR for the current development branch since this has gone a little inactive? |
|
Sure yeah! |
|
I don't think shuffle should be disabled when playing a playlist. In the official Spotify client, if shuffle is enabled the playlist is started from a random song and shuffle stays on. |
|
Closing since this was superseeded by #662 |
This PR fixes issue #493.
Implementation details:
I implemented a play button for albums and playlists, similar to the one in Spotify.
The button is shown in the header bar next to the starring button.