xou816 / spot

Native Spotify client for the GNOME desktop
MIT License
2.25k stars 119 forks source link

Add play button to album and playlist view #662

Closed Zaedus closed 1 year ago

Zaedus commented 1 year ago

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.

Screenshot from 2023-06-05 18-36-37

Zaedus commented 1 year ago

A little update: while trying to get the mobile view to look right, I think I stumbled across a problem with the text alignment? I was trying to center the buttons and they looked off, but the buttons are correctly centered. Not sure if this is just a remnant of altering the AlbumHeader for mobile view or an intentional choice because of wonky centering problems, but I'm going to try and fix that in this PR as well if that's alright. image image

Zaedus commented 1 year ago

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 set_centered function so everything lines up nicely. Margins set via properties on a gobject are different than margins set via css so I'd rather not mix the two in this case.

xou816 commented 1 year ago

Oh you're right I forgot about this centering issue! Thanks for fixing it :)

xou816 commented 1 year ago

Thanks for you work :)

Zaedus commented 1 year ago

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.

xou816 commented 1 year ago

Is the DetailsModel only for albums? I'm a little confused just by name naming because there is a PlaylistDetailsModel as well.

the poor naming is my fault :grin: DetailsModel is only for albums, that's right!

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.

That's fine by me as long as you are okay with it too! :)

xou816 commented 1 year ago

(oh and don't forget to check the CI on github)

Zaedus commented 1 year ago

Two orders of business. Firstly, I've fixed up basically all of the CI problems on my end, although there is a lingering one which isn't caused by my PR and I don't know how to solve anyways haha

Screenshot from 2023-06-07 00-16-05

Secondly, the look of the playlist view button is a little out of place on mobile. On the album view, it has it's friend, the like button, so it makes more sense to give both of them a whole row to sit on, but when its by itself it doesn't look quite right. If you know anybody that can help or if you have any ideas, I can try to make this a little bit more aesthetically appealing. image

xou816 commented 1 year ago

Two orders of business. Firstly, I've fixed up basically all of the CI problems on my end, although there is a lingering one which isn't caused by my PR and I don't know how to solve anyways haha

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())
     }
 }

Secondly, the look of the playlist view button is a little out of place on mobile.

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 : https://github.com/xou816/spot/pull/662#discussion_r1220343970

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)

Zaedus commented 1 year ago

Oh and have a look at this suggestion if you don't mind it will be a little clearer I believe : https://github.com/xou816/spot/pull/662#discussion_r1220343970

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"?

xou816 commented 1 year ago

Oh my bad, didn´t look at the right diff, great :) no worries about the naming!

xou816 commented 1 year ago

LGTM, let's merge this :) thanks for your work!!