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

Show song artists on album screen (and more) #287

Merged
merged 13 commits into from Jul 25, 2022

Conversation

rom4nik
Copy link
Contributor

@rom4nik rom4nik commented Jul 23, 2022

Fixes #265. By default song artists are hidden if their set matches set of album's artists, but it's configurable (and persists across app restarts, thanks for hint in the other PR about that!).

As discussed in that issue, this PR also reorganizes SongListTile a bit and replaces stars in favorite buttons with hearts. I also added the now a bit more portable favorite button to artist, album and playlist screens.

Album screen Settings
Screenshot_20220723-031707_Finamp Screenshot_20220723-024024_Finamp

@jmshrv
Copy link
Owner

jmshrv commented Jul 24, 2022

Just pushed a commit that puts the download indicator in a WidgetSpan and aligns it to the text. This looks a lot better IMO, do you agree?

Simulator Screen Shot - iPhone 13 - 2022-07-24 at 13 28 46

@jmshrv
Copy link
Owner

jmshrv commented Jul 24, 2022

The alignment is still a bit off, but I could have a look in a bit

@rom4nik
Copy link
Contributor Author

rom4nik commented Jul 24, 2022

It's probably because SVG of the icon itself has padding:

obraz

obraz

Also the indicator looks a bit tiny now imo, but at least it's indeed nicely aligned to text in the same line.

@jmshrv
Copy link
Owner

jmshrv commented Jul 24, 2022

Yeah, it is a bit small. We could just put it back at the side. The Material icons don't really have a good icon for this use case. download_for_offline is pretty close to what Spotify has, but it doesn't look great in Finamp due to how small the download is.

@rom4nik
Copy link
Contributor Author

rom4nik commented Jul 24, 2022

Doesn't look much smaller than in Spotify imo, I'd just suggest aligning it to top, since song length won't have characters like y next to the indicator. Size will depend on DPI anyway.

obraz

As for moving it back at the side, I'd say favorite indicator would be better suited there rather than download indicator. In an album you could have all songs downloaded (so you don't need a big sign on each song that it's present on device), but only some of them favorited.

@jmshrv
Copy link
Owner

jmshrv commented Jul 25, 2022

Adding 3 to bodyText2's font size makes the icon the same height as the paragraph (17), at least on iOS. Will test on Android in a bit. I've opened an issue for Flutter to see if it'll be possible for WidgetSpan's child to be automatically constrained to the height of the paragraph: flutter/flutter#108294

Screenshot 2022-07-25 at 15 56 37

@jmshrv
Copy link
Owner

jmshrv commented Jul 25, 2022

It's pretty much perfect with the offset you had :)

Screenshot 2022-07-25 at 16 35 07

@rom4nik
Copy link
Contributor Author

rom4nik commented Jul 25, 2022

Looks good to me too! The bottom bar of the icon is a liiiitle bit below text, but I didn't notice it until I zoomed in, so that's fine by me.

IMG_20220725_193150

@jmshrv
Copy link
Owner

jmshrv commented Jul 25, 2022

That alignment is probably the best we can get, the code looks good so I'm going to merge this

@jmshrv jmshrv merged commit 6c372eb into jmshrv:main Jul 25, 2022
@rom4nik rom4nik deleted the songlisttile-trackartists branch July 25, 2022 21:31
jmshrv added a commit that referenced this pull request Jul 27, 2022
* SongListTile track artists WIP

* ignore artist order

* Offset DownloadedIndicator to align with track number, use ellipsis overflow for artists

* Pass now played item to FavoriteButton

* Use default color for FavoriteButton if not favorited

* Add settings option

* Replace stars with hearts in favorite buttons

* Add favorite button to album and artist screen

* Don't show favorite button for genres

* Put download indicator in TextSpan, specify size

* Align top, add 3 to widget size

* Add offset

* Remove redundant initState/dispose

Co-authored-by: UnicornsOnLSD <44349936+UnicornsOnLSD@users.noreply.github.com>
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.

Feature Request: Show Track Artist next to Play Time in Album View
2 participants