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

✨ Redesign track list tile #829

Merged
merged 47 commits into from
Nov 11, 2024
Merged

✨ Redesign track list tile #829

merged 47 commits into from
Nov 11, 2024

Conversation

Chaphasilor
Copy link
Collaborator

@Chaphasilor Chaphasilor commented Jul 31, 2024

Finally got around to redesign the track/song list tile to make it consistent, fit two lines of text and use a track-based accent color.
There's now also a favorite indicator / playlist selector button, just like in the design.

Some features, like the download indicator, are still missing. But those require some other changes and a finalized concept anyway, so I left them out for now.
I also updated the dismiss indicator and added text that describes what happens when dismissed, and allocated a dedicated space for the fast scroller instead of overlaying it

If this redesign doesn't have at least feature parity (all features that the old design had), please let me know!

TODO

  • Fix contrast of heart icon in light mode
  • Update queue list tiles
  • Add support for more specific display options (like track numbers on albums)
    • Add setting to show track covers on album screen (default false)
  • Update "Previous Tracks" tiles
  • Fix theme errors on artist screen (top tracks)
  • Make theme transition less janky? (help please!)
  • Fix render bug with brightness change
  • Show download and lyrics indicators
  • Disable dismissible on tab view
  • Slightly increase title height to not cut off characters
old screenshots

@Chaphasilor Chaphasilor added design Design changes redesign-beta Issues related to the beta/redsigned version of Finamp labels Jul 31, 2024
@Chaphasilor Chaphasilor requested a review from jmshrv July 31, 2024 14:18
@Komodo5197
Copy link

I played with this and noticed a couple issues:

  • The add to playlist menu is using the currently playing image/theme instead of the clicked song.
  • The fast scroller spacing has issues. Going into split-screen mode or scaling the screen really tall can both make the scroller wide enough to stick out past the fixed space given to it and infringe on the song tiles.
  • I'm not really sure why, but there seems to be performance issues. Scrolling deep into the song list results in extreme lag, which does not occur with the current track list.
  • There's not enough contrast between the song title and the artist subtitle. This makes it harder to scan through titles compared to the old design or the album list screen. This is especially annoying in albums with only one artist, where this info is useless.
  • The currently playing visualizer bars are way less visible now that they use the theme color and are placed on top of and in line with the album images. This is fine for the tracks list, where the the theming makes the currently playing song stand out, but will probably be more of an issue on the album screen, because eventually that screen is supposed to be fully themed.
  • This isn't an issue per-se, but I don't particularly like the aesthetics of having songs in little pills all the time.

@Chaphasilor
Copy link
Collaborator Author

Hey, thanks for giving it a try!

  1. I can't seem to repro that. What are the exact steps? Here's how it works for me:
    https://github.com/user-attachments/assets/176aa662-a995-4d97-9439-f6116b0df31c

  2. I'm honestly not surprised the fast scroller has issues. It also is unusable in landscape mode, but I think that has always been the case. I'll see if I can make it more consistent.

  3. I modified the condition that checks if the rendered track is the currently playing track. Maybe that introduced issues? I also noticed the (regular) menu lags a bit befor opening.

  4. I could decrease the font size for the artist again, but didn't think it was such a big deal. Do you have the setting enabled that only shows artists in the album lists if they're different from the album artist?

  5. I mean the visualizer isn't super important anyway, and I'd much prefer to save some horizontal space. But even in the redesigned album screen, the tracks will be grey except for the currently playing track, so I think it should be obvious enough?

  6. You're not the first to mention that. Is it just for aesthetic reasons or would you also prefer a more "compact" layout?

Also, the original plan was to add a download indicator on each track item. I'm not sure if I'll still do that, but either way it might be comfortable to have a provider for getting and updating the download status of items, including albums, etc. Similar to what you added for the favorite status.
If you have the time and feel like it, I'd be glad if you could make a small PR that adds such a download state provider. Right now this is handled by the download button (which already uses a provider for reading), but there might be cases where that specific download button isn't suitable.

If you feel like it would be unnecessary or a bad idea, please let me know!

@Chaphasilor
Copy link
Collaborator Author

Okay, I think I figured some of the issues out. I didn't notice that I still had two gesture detected that did the same thing, with only one being properly connected to the menu theme calculation. That was probably the reason why you saw the wrong theme being used, a simple race condition. Probably also the reason for the laggy menu opening, and might've otherwise negatively impacted performance.
I also made of the widget stateless, since we didn't need two stateful widgets with different keepalive mixins. From my limited testing it seems like performance is much better now, could you please confirm that?

Font size if update and icon contrast is also fixed.

So the only major issue left should be the fast scroller.

@jmshrv
Copy link
Owner

jmshrv commented Aug 2, 2024

ooo nice :)

only visual suggestion I can think of is that having a like button everywhere seems a bit overkill, but does make sense when looking at an album/maybe searching

Copy link
Owner

@jmshrv jmshrv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good :D

fontSize: 16,
height: 1.0),
overflow: TextOverflow.ellipsis,
maxLines: 2,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would maxLines: 1 maybe look better here? Songs with long titles look a bit "compressed" right now (or maybe the line height is wrong? I prefer to use provided fonts like "headline" instead of defining them myself. I'm pretty sure you can dig around and find what ListTile uses)

Screenshot of Infinity on High, which includes a lot of long titles

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was aiming to fit two rows onto a tile instead of just one. I agree that it looks off. I'll think about it...

@jmshrv
Copy link
Owner

jmshrv commented Aug 2, 2024

As for the card design, I think it's a bit busy, but I can't really think of an alternative idea right now

@Komodo5197
Copy link

  1. I'm talking about pressing the new favorite/add to playlist button on the right of the tracks. The song menu is correctly themed, but pressing that isn't.
  2. The performance does seem to have been fixed.
  3. The smaller artist font is slightly better. I do have the setting to hide matching artists enabled, it seems like it isn't working?
  4. Horizontal space is important. If the tracks are going to stay grey, it's probably fine I guess? If we're relying on theming, I'd like the background color to be a bit less subtle.
  5. The density seems about the same as before, and I think it's fine. The album covers may even be bigger at the same density? This is mostly aesthetics. Honestly, I liked the way the actual item lists looked before well enough, with no separators at all, and my instinct would be to keep it like that, matching the background color so that you only see the card when highlighting or on the currently playing track.

Regarding a downloads provider, I'm not sure there's really anything I can do there. As you noted, reading is already cleanly exposed with the download status provider, so that only leaves updating. The guts of that are already packed into downloadsService.deleteDownload(stub: item) and DownloadDialog.show(context, item, viewId), so the remaining logic is pretty tied into the exact UI. Any changes that match the downloadbutton UI that exactly would probably be better off as more parameters in that class.

Also, I noticed that if one song is playing, and then you play a new one with the different theme, the new song has the old theme for a second during the transition. I think we probably shouldn't animate theme transitions at all for the song tiles, because unlike the player screen components each tile only ever has one theme, it's own.

@Chaphasilor
Copy link
Collaborator Author

@Komodo5197 sorry for not getting back earlier :)

  1. Ah, I see. It's not the theme that's wrong though, it's the actual track. The currently playing track is used instead of the selected track. I'll fix it.
  2. Nice!
  3. You're right, I didn't add that functionality back in yet 😅
  4. I'll see if I can increase the contrast in a nice way, either for the visualizer or the background/accent color. Don't want things to be too saturated though
  5. Let me think of a design that isn't too busy but stays consistent with the queue :)

Thanks for your thoughts about the provider stuff, I'll see how best to integrate it then.
I also noticed the weird transition. My issue was that the accent color is based on the current album cover (of the currently playing track, in full resolution). That's the only cover we calculate the accent color for, and it takes a bit before the cover has loaded. This means the old color would be shown even without the transition. I'll definitely try it out though. But I don't really have an idea how to instantly get the accent color for the currently playing track, at least not using the full-resolution cover (which is important because lower-res covers might end up producing different accent colors). Do you have any ideas?

@Chaphasilor
Copy link
Collaborator Author

Now that I think about it, we should already be pre-caching the full resolution covers of upcoming tracks, so it is possible to get the full-resolution cover right-away 🙈

@Uno-Muno
Copy link

Uno-Muno commented Oct 2, 2024

Gonna chip in some designs.

I like tiles, but after thinking about them for a week I see more and more problems with them. Although they do have some pros too.

  • They take extra space leaving less room for the content,
  • They add more clutter as they draw more lines,
  • They limit scaling vertically (for example when we want to use multiple lines and keep the padding same, then we would have to increase the size of the image as well, but that's not a good option),
  • On the bright side they do make drag-and-drop look and feel really nice.

So I went back to the roots and played around with some no background tiles :)
Group 26
Without background the album/artist/song title can easily be 3 lines or even 4 but that's not very nice (and is far fetched anyways).
The overflow menu can be omitted and replaced by a gesture/long press leaving more room for other stuff, but personally I like visual indication, although the target audience is quite tech savvy and can probably figure it out.
In the song tiles I wasn't too sure about how to wrap multiple elements (the artist and the album name) so I took inspiration from Spotify and added marquee/slider thingy. There are probably better solutions to this.

I didn't pay any attention to the dressing, just the songs, albums and artists.

@Chaphasilor
Copy link
Collaborator Author

@UnnKrigul1 thanks for the input!
I'm willing to let go of tiles for most stuff, it really seem to be too much of a hassle to get right.
We could think about reserving the tiled look only for the currently playing track, like in your non-tiled queue mockup. This way we could have a clear and obvious visual indication, we could have consistency in how the current track is shown (possibly even showing the progress of a track in the track list too), but would make better use of space for all other items.

I'll make some changes and upload some new screenshots soon :)

@Chaphasilor
Copy link
Collaborator Author

I've made a few changes now and updated the screenshots in the description. Let me know what you think!
If you approve, I could move ahead and apply the same treatment to the queue list tiles.

@Uno-Muno
Copy link

Uno-Muno commented Oct 9, 2024

Looking nice! I would decrease the margin between upper and lower text row to like 4px, but otherwise perfect. Thanks!

@Chaphasilor
Copy link
Collaborator Author

@Uno-Muno I've tried, but afaik the only way to do that is to limit the available height or add paddings, both which reduce the height available to the text. I can't make the tile easily grow to be taller, and if I use paddings then the second line of long titles will be cut off.
James mentioned two-line titles looking weird early on, so maybe we should just go back to limiting the title to a single line? But honestly, for practical reasons I'm against that, and I'd still prefer practicality over aesthetics....

- doesn't work well with new track list tile
- other apps also show the artist always (Spotify, YT Music)
- uses Finamp's blue as a fallback until the correct accent color has been calculated
- avoids pulsing/flashing effect on theme change
- also adjusted download indicator icons and size
@Chaphasilor
Copy link
Collaborator Author

@Komodo5197 I've increased font size of the track title and reduced the opacity of the artist name, is this better?

Also, any other comments before I merge this?

@Komodo5197
Copy link

Okay, I tried it out again:

  • The artist fade is better, but I'd really prefer it to be as faded as you had the matching artists, or have the albums on the track list now, but if you feel they need to be more prominent then that I guess I can't really argue otherwise.
  • I have an album that's missing track numbers, and all the indexes are showing up as -1 instead of being hidden.
  • I'm getting a weird fade effect where the artist line believes it is overflowing the list tile vertically on all tiles. This is specifically the windows build that I'm testing, I don't know if this is universal.
  • The artists field is still showing up empty on my album without artists. It looks like the artists field gets returned as [], not null, breaking your fallback logic.
  • I've noticed a possible performance optimization - album_screen_content uses a SliverList when it could be using a SliverFixedExtentList due to our fixed height tracks.

@Chaphasilor
Copy link
Collaborator Author

  1. I feel like the artist is imporant for the tracks tab or playlists (where there are often similar titles and the artist can be used to find the desired track), but on the album page it's probably not that important.
    Maybe there could be a setting to reduce the prominence of artists further like you want to, and maybe only for albums? Either way, this is something we can iterate on in future updates, when we also received some feedback from other users. Nothing is set in stone yet :)
  2. I didn't know indices could be missing. I'll try to handle it better. You would want the indices and covers to be hidden for those tracks?
  3. That's probably because of overflow: fade and maxLines: 1. I checked that on my phone and didn't notice any overflows, but I'll try to increase the height a bit.
  4. Ah okay. I think I forgot looking into that anyway.
  5. I'll try to use that. Can't we also use that in other places, like the tab view and queue list?

@Chaphasilor
Copy link
Collaborator Author

Made a few changes. Better now?

@Komodo5197
Copy link

Most of the fixes seem good.

  • You removed the -1 default from QueueListTile but forgot TrackListTile, so that fix isn't working.
  • I noticed that the favorite buttons in the queue and playlist screens don't highlight the current playlist the way that the buttons on the player screen and current track queue widget do, and I believe they should.
  • All copies of the currently playing track are being highlighted in the queue and playback history. It's probably correct to just turn off track highlighting entirely in both these locations.
  • Your padding changes to the alphabet scroller cause it to overlap with the scrollbar. Is this intentional?
  • Why are we making the tracks tiles in the next up list slightly narrower than all the other queue items?

@Chaphasilor
Copy link
Collaborator Author

Chaphasilor commented Nov 11, 2024

  1. Fixed
  2. That might make sense, but I'm not sure. I honestly wouldn't have considered it because these tracks don't have a "queue source" yet, but adding new tracks to a playlist while listening to that playlist could make sense. Right now the playlist is only highlighted for tracks that are already part of the currently playing playlist, so we might need to change the logic for that. I'll think about it.
  3. Ah right, that's a bit weird. I think we could limit the highlighting on the history screen to just the latest track, and get rid of it in the queue list
  4. That's on Windows? Doesn't overlap on Android. I'll take a look
  5. Just to differentiate things. Original design had a thick border around it, I only kept the padding. The "Next Up" section is like squeezing in some additional tracks into the queue, it's not really part of the regular queue

@Chaphasilor
Copy link
Collaborator Author

Let's talk about showing the current playlist for non-queue tracks some other time. The current logic requires a FinampQueueItem, which I don't want to "fake", so we need some other logic anyway. But right now I'm not sure what makes the most sense, showing the current playlist at the top only if the track is part of the playlist (seems a bit random/unpredictable if you're not accessing the track through the playlist or queue), or showing the playlist even if the track is not part of the playlist (to make it easier to build playlists).
We should probably ask some users about this...

Either way, I'll merge this now so that I can test out the full release version today and push the release tonight...
If you find anything else, feel free to let me know so that I can fix it in the redesign branch.

@Chaphasilor Chaphasilor merged commit c9e256f into redesign Nov 11, 2024
8 checks passed
@Chaphasilor Chaphasilor deleted the new-track-list-tile branch November 11, 2024 10:03
@Komodo5197
Copy link

I was just thinking about the highlighted playlist as a replacement/alternative to the removal path. So all queue items would use their queue info to allow easy removal from the playlist used to add them, if possible. And then on playlist screen, using the favorite button would be an alternative way to remove songs from that playlist instead of long pressing, to match the player screen flow. But yeah, that would involve re-arranging all the arguments to everything a bit.

@Chaphasilor
Copy link
Collaborator Author

I don't really understand your first point about the "removal from the playlist used to add them", could you explain?
About using the heart icon to remove from playlists, I don't think it's a good idea. The button should be consistent, and the same icon should always do the same thing.

But making it more efficient to remove tracks from playlists is a separate issue I think. We need to improve the playlist management functionality anyway, to include renaming, reordering, and deleting playlists, and when we do this we can add buttons for more easily adding and removing tracks from playlists.

@Komodo5197
Copy link

The existing functionality of the add to playlist popup is that when opened on player screen, it looks at the queue item source and if it was a playlist, that playlist gets pushed up above the fold with the favorites button. This is intended to make it easier to remove the song from the currently playing playlist, as opposed to having to locate that playlist in the list of all playlists yourself or go through the song menu. I'm just thinking about getting that working in more scenarios now that the favorites/add to playlist button is present in more scenarios. The extension to any item in the queue seems very natural, as you can just feed in the relevant queue items. The "playlist used to add them" phrasing is just because the playlist in the queue item source isn't necessarily what's currently playing if you added multiple playlists to the queue. This makes sense, as the source has the correct playlist which is most relevant to the track in this context. And then this same behaviour of pushing a playlist up would make sense when opening the add to playlist menu on a track on the playlist screen, but this time showing the playlist whose screen you're on, because that's the relevant context playlist the user might want to remove the song from. This makes more consistent, as the same playlist removal flow that works on the player screen would also work on the playlist screen now that it has the same favorite button. But it would also take a bit more code fanangling than the queue would.

@Chaphasilor
Copy link
Collaborator Author

Okay now I get it. I was actually surprised that this isn't yet the case on the playlist screen, and I agree that it would make a lot of sense here!

We should probably change the API a bit. While just throwing in a FinampQueueItem is an easy solution for anything queue-related, maybe it makes more sense to use FinampQueueSources instead, or maybe a more generic variant of that? If necessary we could also shift some of the metadata from the queue item to the source. Then we could add some custom getter to BaseItemDto, like .asSource, that lets us easily get just the relevant information for these purposes.
That might also help in all the places where we manually pass a parentItem or even parentId down a widget chain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Design changes redesign-beta Issues related to the beta/redsigned version of Finamp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants