-
Notifications
You must be signed in to change notification settings - Fork 119
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
Add play button to album and playlist view #662
Conversation
Note for the changes I made to fix the centering: I removed the css that handled margins and added the margins back in the blueprint. I then removed the margins in the |
Oh you're right I forgot about this centering issue! Thanks for fixing it :) |
Thanks for you work :) |
Is the DetailsModel only for albums? I'm a little confused just by name naming because there is a PlaylistDetailsModel as well. Also, the original PR wanted to add a play button to the album and playlist view, but only the album view was implemented afaik. I will implement one for the playlist view for the sake of consistency, but let me know if that's not needed. |
the poor naming is my fault 😁 DetailsModel is only for albums, that's right!
That's fine by me as long as you are okay with it too! :) |
(oh and don't forget to check the CI on github) |
Oh yeah that one's on me! Thankfully the compiler is helpful, this should work: --- a/src/api/api_models.rs
+++ b/src/api/api_models.rs
@@ -543,7 +543,7 @@ impl TryFrom<Album> for SongBatch {
type Error = ();
fn try_from(mut album: Album) -> Result<Self, Self::Error> {
- let tracks = std::mem::replace(&mut album.tracks, None).ok_or(())?;
+ let tracks = album.tracks.take().ok_or(())?;
Ok((tracks, &album).into())
}
}
I think it's fine for now, if there are subtle design issues we can always address them later! Oh and have a look at this suggestion if you don't mind it will be a little clearer I believe : #662 (comment) Or even just : fn album_is_playing(&self) -> bool {
matches!(
self.state().playback.current_source(),
Some(SongsSource::Album(ref id)) if id == &self.id)
} (and similarly for plyalists) |
I did and was honestly shocked that it could be simplified that much haha (Look here at this commit) Did you also want me to also try getting the name for the album variation to be "album_is_playing"? |
Oh my bad, didn´t look at the right diff, great :) no worries about the naming! |
LGTM, let's merge this :) thanks for your work!! |
Almost all of this code is ported from PR #561 as of right now. I've made some changes based on @xou816's comments in the previous PR, but as I am not familiar with the codebase yet, I haven't addressed all of his comments. I plan to get some guidance on how to go about improving this. Please bear with me since I am new to this project and relatively new to working with gtk-rs.