Add artist top songs #552
Add artist top songs #552
Conversation
|
Now queues work, and there's no duplicate code, there's a little bit of unprettyness in the SongListTile class as I used the isInPlaylist for artist views now too (to display the correct name somewhere I don't remember, I think in the queue), that should probably be changed |
android/app/build.gradle
Outdated
| @@ -44,7 +44,7 @@ android { | |||
|
|
|||
| defaultConfig { | |||
| // TODO: Specify your own unique Application ID (https://developer.android.com/studio/build/application-id.html). | |||
| applicationId "com.unicornsonlsd.finamp" | |||
There was a problem hiding this comment.
Don't forget to change that
|
Example image |
|
Regarding the app ID, I think the way @jmshrv and myself always do it is to just overwrite the currently installed version with the debug version 😅 I also use my old phone for initial testing. |
|
I'm pretty much always on a dev build lol, I really need a button to redownload everything I last had because I keep corrupting the databases |
Tiefseetauchner
force-pushed
the
addArtistTopSongs
branch
from
09626ef to
f169c01
Compare
Tiefseetauchner
force-pushed
the
addArtistTopSongs
branch
from
c92f472 to
b34526f
Compare
|
I'm currently testing another PR on my phone, so I'll take a look at the actual running app later. For now I just looked through the code. But this seems nice so far! :) |
Tiefseetauchner
force-pushed
the
addArtistTopSongs
branch
from
67812bd to
012b5e3
Compare
- fix null issues - have jellyfin sort songs
|
Thanks for the auto builds btw, now I can work without being able to build it xD |
| style: const TextStyle(fontSize: 18, fontWeight: FontWeight.bold), | ||
| )), | ||
| SongsSliverList( | ||
| childrenForList: songs.take(5).toList(), |
There was a problem hiding this comment.
Sorry for missing this at first, but we should probably use the limit parameter for the getItems() call instead. Right now there's no need to get all songs by an artist, and the performance difference could be significant if an artist has hundreds of songs.
Especially since we're waiting for both future to complete before building the widget
There was a problem hiding this comment.
Actually there is kind of, it's used for the queue. We might be able to do it asynchronously though after the widget is loaded, or just accept that only five songs are in the queue
There was a problem hiding this comment.
Ah I see. Yeah the initial page with 5 tracks + fetching the other songs after playback starts would be a good approach I think. I'm also planning to implement this for the regular songs tab to enable in-order playback starting at a specific song
There was a problem hiding this comment.
Okay, I don't think I can do that with my current knowledge of dart, I suspect we'd pass a delegate through but I cannot figure out how to await the result in a future in a synchronous method to pass it in as a delegate. I'll set it to limit to 5 for now and see whether I can figure it out, any help would be greatly appreciated as that's a primary usecase for me currently
There was a problem hiding this comment.
I'm planning to support this in the QueueService somehow, where you can specify some initial tracks as well as the initial index, and then to callbacks/futures(?) that fetch songs to be prepended (first track to first of initial tracks, if any) and songs to be appended (last of initial tracks to last tracks, if any) to the queue. I imagining that should work, but I haven't tested it yet
|
Okay, this looks good to me! We can merge from my side, if that's fine with you? :) |
|
Yep |
|
Merged, thanks a ton! 😁 |
|
@Tiefseetauchner re: application ID, I actually "looked into it" and there is a way in the gradle config to set a suffix for debug builds, so I'll change that up in the |
From issue #551
Please don't be shy to roast the code, first time ever working in flutter, I can't imagine it being any good. I don't know what the usual way to do the application id is, I left it in because I'll probably debug but I suspect we'll remove it just before merging (if this is merged)
There's a TODO I can't resolve, as I don't really understand the technical use of all the variables of
SongsSliverListI realised while pondering over the changes that this affects the genre selection too, I think that's actually a pretty neat feature
One thing that's still left is to actually add the queue, I do not know how to do that. We have the sorted - wait is that what the queue variable is for - oh nevermind, i'll figure that out later, I'll leave this as a draft for now