Add album year to the list & the details view #391
Conversation
|
Hey, thanks a lot for your work! :) this looks promising!
Yeah, sorry about that :( I'm absolutely open to this being changed though! Two possibilities off the top of my head: either we keep going with this model being shared but adapt the widget so that things such as the release date are As for search results or playlists, well if we don't have the date, don't sweat it, we won't be able to show it and it is fine. This too could be solved by having the release date be an Option in that AlbumModel I think! |
6d97dc6
to
d0d58c9
Compare
0ca7e45
to
698aa01
Compare
|
Thanks for the advice! Now I have wrapped |
0a99410
to
a1f805b
Compare
src/app/components/album/album.css
Outdated
| @@ -35,11 +35,15 @@ leaflet.folded .album .album__cover { | |||
| font-size: 14px; | |||
| } | |||
|
|
|||
| .album label.album__year { | |||
| font-size: 14px; | |||
There was a problem hiding this comment.
We really shouldn't hardcode font-sizes like that. Not a blocker for this particular PR since it's used elsewhere, but eventually, we need to remove these.
There was a problem hiding this comment.
Agreed. GTK offers @define-color to reduce hard-coded colors, but it doesn't for other types. Neither var() seems to be unavailable for GTK so without preprocessors would that be difficult?
There was a problem hiding this comment.
rem seems to do the trick:
src/app/models/gtypes/album_model.rs
Outdated
| @@ -85,6 +98,7 @@ mod imp { | |||
| Self { | |||
| album: RefCell::new(None), | |||
| artist: RefCell::new(None), | |||
| year: RefCell::new(0), | |||
There was a problem hiding this comment.
I feel like this could definitely be a RefCell<Option>, this would avoid all the unwrapping and awkward comparison to 0 you have to deal with!
There was a problem hiding this comment.
I was still confused how GTK deals with optional values. Fixed in the latest commit. :)
|
All good to me :) looks like you've got a few conflicts to fix but then we're good to merge! |
|
Resolved conflicts :) |
|
Perfect, great work, merging this. thanks! |
Fixes #137
It looks like this:
and at the details view:
The GTK part wasn't that hard, but the problem is that not only albums but also playlists makes use of
AlbumthroughTrackItem. The album widget requiresAlbumModel,AlbumModelrequiresAlbumDescription, andAlbumDescriptionrequiresAlbum.I tried moving
release_dateinAlbumReleaseDetailstoAlbumDescription(in commitdc71a29) and it worked fine for albums. However, since playlists do not have their release dates, the program fails to parse them. I could just letrelease_datestay inAlbumReleaseDetailsand use it, but inSearchResults::update_results, for example, we do not have access to anAlbumReleaseDetails.The commits are messed up... I will squash them later.