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

Add play button to album and playlist view #662

Merged
merged 12 commits into from
Jun 7, 2023

Conversation

Zaedus
Copy link
Contributor

@Zaedus Zaedus commented Jun 5, 2023

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
Copy link
Contributor Author

Zaedus commented Jun 6, 2023

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
Copy link
Contributor Author

Zaedus commented Jun 6, 2023

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
Copy link
Owner

xou816 commented Jun 6, 2023

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

src/app/components/details/album_header.rs Outdated Show resolved Hide resolved
src/app/components/details/details_model.rs Outdated Show resolved Hide resolved
src/app/components/playlist/playlist.rs Outdated Show resolved Hide resolved
@xou816
Copy link
Owner

xou816 commented Jun 6, 2023

Thanks for you work :)

@Zaedus
Copy link
Contributor Author

Zaedus commented Jun 6, 2023

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
Copy link
Owner

xou816 commented Jun 6, 2023

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 😁 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
Copy link
Owner

xou816 commented Jun 6, 2023

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

@Zaedus
Copy link
Contributor Author

Zaedus commented Jun 7, 2023

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
Copy link
Owner

xou816 commented Jun 7, 2023

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 : #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)

@Zaedus
Copy link
Contributor Author

Zaedus commented Jun 7, 2023

Oh and have a look at this suggestion if you don't mind it will be a little clearer I believe : #662 (comment)

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
Copy link
Owner

xou816 commented Jun 7, 2023

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

@xou816
Copy link
Owner

xou816 commented Jun 7, 2023

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

@xou816 xou816 self-requested a review June 7, 2023 21:38
@xou816 xou816 merged commit 5361d5d into xou816:development Jun 7, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants