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] Recover Now-Playing Queue on Startup #547

Merged
merged 13 commits into from
Dec 15, 2023

Conversation

Komodo5197
Copy link

Occasionally saves the current queue and position in song while playing. The queue auto-loads on app startup if enabled, or earlier queues can be restored from a menu. Added a loading variant of the Now-Playing bar while the queue loads, and tweaked a bunch of theming/transition code for the bar while cleaning it up. Queues are stored as a collection of raw song ids.

@Chaphasilor
Copy link
Collaborator

Oh that sounds really awesome! Taking a quick glance at the code, it seems like there are a lot of goodies and fixes in there as well.
I should be able to give a proper review either tonight or on Saturday.

If you're using just the song ID for persisting queues, that means the queue source for each track will be lost, correct?

Also, how does the logic for handling multiple different queues work? At what point do you separate the queues, each time QueueService.startPlayback is called?

What happens if a queue can't be restored (i.e. because Finamp is in offline mode)?

Really looking forward to testing this out! 😁

@Chaphasilor Chaphasilor self-assigned this Dec 7, 2023
@Chaphasilor Chaphasilor added the enhancement Improves an existing feature label Dec 7, 2023
@Komodo5197
Copy link
Author

The track source is indeed lost. I considered trying to save it but it didn't seem worthwhile.

Right now I archive a queue whenever the app starts up or a queue is manually loaded. The oldest start getting deleted once you hit 60 stored. I considered adding a queue archive on startPlayback, but it isn't in the current code. I think I just forgot about it.

Dealing with offline mode, or server hiccups in general, was why I ended up adding the manual queue restoring. When the queue is loaded, any items that fail to load are just dropped with a single error message at the end. If you want them back, you can load an earlier queue once back online. The song ids are consistent between downloads and the server so downloaded songs from an online queue still load back up in offline mode.

@Chaphasilor
Copy link
Collaborator

Chaphasilor commented Dec 8, 2023

The track source is indeed lost. I considered trying to save it but it didn't seem worthwhile.

Maybe it would be possible to at least store the "original" source, so not the source of individual tracks, but just the global source that's shown in the queue list for example. This could possibly be modified to indicate that the queue was restored, but I'm not sure that's needed at all. The restoring should be as transparent to the user as possible.

Regarding the saved queues, maybe 60 is a bit too much. In any case, I noticed that the sorting on the "Restore Queue" screen is from oldest to newest, but it should probably be the other way around, similar to the "Playback History" screen :)

I also encountered an issue where my phone didn't have internet and none of the tabs loaded on startup, and the queue didn't get restored either. While that's fine, it would be nice to show the Now Playing Bar with a button to retry loading the latest queue.
After I restarting Finamp a few times until the connection was restored, I went to manually restore the queue and noticed an empty queue being persistet. Trying to restore it didn't do much, apart from creating another empty saved queue:

Screenshot_20231208-170620.png

Otherwise this has worked really well so far! I also love that you fixed the accent color calculation so it isn't tied to the player screen anymore :D

Reversed queue restore screen.
Added saving queue on startPlayback().
Added saving global queue source.
Added ability to easily retry queue load if all items fail.
Delayed overwriting latest until user initiates playback or modifies queue after load.
Improved song count display on queue restore screen.
@jmshrv
Copy link
Owner

jmshrv commented Dec 10, 2023

I really appreciate this! iOS really likes to kill Finamp, I really need to know how apps like Spotify keep their players forever 😅

@Chaphasilor
Copy link
Collaborator

I feel like the getTrackFromId() method doesn't really belong into the queue service. It definitely shouldn't be a public member of the main class. There are a few other files, like generateMediaItem(). I think it would be best to make getTrackFromId() private and move it below the other helper methods. What do you think?

Oh and I'm not a huge fan of the syntax with the inline for loop (queue_service.dart:336), right below it you're using a fold, so I think a filter and map would make this more consistent...

I'll do some more testing with the two commits you've added in the coming days!

@Chaphasilor
Copy link
Collaborator

iOS really likes to kill Finamp, I really need to know how apps like Spotify keep their players forever 😅

I thought that's what the "Enter Low-Priority State on Pause" setting is for, if I don't disable it Android also always kills the app in the background...
Or is that setting Android-only?

Added direct dependancy on collections package.
Made loadSavedQueue cleaner.
@Chaphasilor
Copy link
Collaborator

Just encountered an issue where the accent color for a cover couldn't be calculated, probably because the cover failed to fetch in the provider. At some point the cover was loaded correctly in both the player screen and now playing bar, but the color was still the one from the previous cover:

Screenshot_20231211-133314.png

Screenshot_20231211-133327~2.png

Increased PaletteGenerator timeout from 2 to 5 seconds.
@Komodo5197
Copy link
Author

I believe the problem was that PaletteGenerator had a 2 second timeout, and then once it failed would never re-run because the album image never changed. I upped it to 5 seconds and moved a bunch of provider code around so that I could force a recalculation on track change. This also detached theme updating from AlbumImage, so redraw loops shouldn't be a concern now.

@Chaphasilor
Copy link
Collaborator

Thank you for that! That's a very welcome change, and finally a proper solution. Is there anything you're still planning to implement for this PR, or would you consider this ready to merge (after some more testing)?

@Komodo5197
Copy link
Author

The last plan I had for this PR was the switch to batched item lookups, which I got in a day or two ago. I'm ready to merge as soon as all your concerns/testing are cleared up.

@Chaphasilor
Copy link
Collaborator

Okay, from my testing I didn't see any issue.
It's not ideal that we have three different locations loading album covers, sometimes the album cover on the player screen is loaded first and sometimes the accent color is calculated first. But that's only a cosmetic issue, and nothing you need to worry about with this PR. I'll probably merge it some time tomorrow :)

@Komodo5197
Copy link
Author

I thought about the multiple image loads and messed with the idea of feeding the album covers from the currentAlbumImageProvider, but the problem is you need image sizes from the UI to make the requests. I spent a bunch of time trying to figure out sizing things, but now that I'm writing this I've noticed that the audio service seems to be loading full size images for the OS. So I'll just grab a full size image in currentAlbumImageProvide and then distribute it to everyone to sync everything up better. I can get that finished up tomorrow.

@Chaphasilor
Copy link
Collaborator

Okay that sounds nice. Yeah we're getting at least one full-size cover for the current track anyway, and it won't hurt to use the full-size image in places like the now playing bar or queue list. As long as the other items in the list items use a low resolution for bandwidth and performance reasons, everything should be fine.

While you're looking at replacing the image source, maybe you can figure out why the cover on the player screen always reloads when you open the screen 😅

Perform now-playing queue album precaching in currentAlbumImageProvider instead of AlbumImage.
Directly handle image loading error in playerScreenThemeProvider instead of waiting for timeout.
@Komodo5197
Copy link
Author

Okay, I centralized the current album cover loading and precaching. I also tweaked the theme provider to hopefully respond to loading errors better instead of just having a 5 second timeout. Regarding the reloading, hopefully this fixes it due to the now-playing bar using the same image as the player so all the caches and providers are already ready, but I'm not planning to investigate if it doesn't.

@Chaphasilor
Copy link
Collaborator

You are awesome! 😁

I'll take a look today and get back to you tonight (hopefully) :)

@Chaphasilor
Copy link
Collaborator

Okay so I didn't perform any additional testing, but the queue restoring works with the gray accent and the album cover is much better like this! 😁
The whole player screen feels so much more polished now, so thanks again for taking the time to fix this <3

I'll just fix up the merge conflict and merge this...

@Chaphasilor Chaphasilor merged commit 82628e7 into jmshrv:redesign Dec 15, 2023
2 checks passed
@Chaphasilor
Copy link
Collaborator

Merged, thank you so much for the effort! Hoping to see another PR from you soon 😁

@Komodo5197 Komodo5197 deleted the redesign branch December 16, 2023 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves an existing feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants