Automatically download playlist metadata on first launch #905
Conversation
|
@Komodo5197 would you mind taking a look? I'll still have to test this out to see if it actually works, but maybe there's something that could be improved with the download code already ^^ |
|
That seems about as I expected. I don't 100% remember how this migration script works when conducting the initial login, so definitely test that. Also, it might make more sense to chain the future so that the playlist download gets added before we start the repair, although this probably doesn't really matter. |
|
From what I saw there's a hidden setting that is false by default and gets set to true after the migration, so it should be fine. I thought about that, but wanted to prioritize the migration of existing downloads over the metadata. Is the download system even a proper FIFO queue? |
|
@Komodo5197 I noticed that in |
- also reworded the download item name for it
|
I noticed that my previous approach couldn't work, because the migration is ran right after startup when the downloads service is initialized, but for new installations we don't even have a server address or viewId at that point. So I moved to to a callback in the music screen and added the same kind of one-off conditional logic. While testing that I also noticed that the download migration currently doesn't work, even on the
Haven't had time to take a closer look yet... |
|
Yes, the divergence in setting values is to give different values for migrating vs fresh installs. Regarding your code, I'm not sure it actually needs server info? It should be able to just register the download at any point and then the sync should just wait until there's actually server info to use. I don't believe supplying a viewId is necessary for this download. Also, there's another music screen hook for starting the queue restore which is currently in build, it should probably be cleaned up to init or maybe even in main, I think it was where it was just for an error handling context, but GlobalSnackbar fixes that. The migration not working is concerning. If you've got the logs handy from the failure, I'd love to look at it and see if something stands out, otherwise I'll try to reproduce it myself at some point. |
|
Sure, logs are here: Ah, didn't scroll down far enough to see the queue restore stuff :P |
|
I had to change the type for Only somewhat related, but how much effort would it be to include the image sizes in the download item size? Right now playlist metadata shows up as 0 byte, but downloaded almost 190 images in my case. Even though images aren't counted as "real" downloads, I think they should be included in the total size... |
|
The core migration of metadata nodes blocks the progress of main and will always be complete before your code runs. The only thing you might overlap with is the repair/sync, which is supposedly fine. My first guess would be that you just caught things that were still running which would have been correct eventually. Another possibility is that the early download adding did prevent syncing immediately, but it would have fixed itself on the next sync at reboot. Do you have the logs for this? Images are counted. I tried with the latest beta release and playlist metadata has a non-zero size? |
|
It should've been completed. I was doing other things while it compiled and it was probably sitting idle for a few minutes post launch before I checked. Screen was on all the time. The sync seemed complete. I can try to repro later. The metadata being 0 bytes happened for me with this PR and a fresh install. Basically when only the metadata was downloaded, and nothing else. |
|
You didn't assign a download location when adding the metadata download, so no actual files could be created. This explains both the missing images and the 0 size. |
|
Oh, so there's no default location? I'll fix that |
|
Okay, I'm supplying the location now. Testing now. |
|
The default location is a user setting. We want it unset so that you start getting prompts once you add another download option unless you specifically choose otherwise. |
|
I see. How about a default metadata location that can't be changed and points to internal storage? |
|
I've now tested this with migrations from the stable version, fresh installs, and upgrades from 0.9.11 (both with and without playlist metadata downloaded before the upgrade). Everything seems to work as expected. Any comments before I merge this? |
lib/screens/music_screen.dart
Outdated
|
|
||
| // make sure playlist info is downloaded for users upgrading from older versions and new installations AFTER logging in and selecting their libraries/views | ||
| if (!FinampSettingsHelper.finampSettings.hasDownloadedPlaylistInfo) { | ||
| // check if metadata already downloaded to avoid possible duplicates |
There was a problem hiding this comment.
Is this status check actually necessary? We shouldn't ever run this block more than once due to the setting, and we shouldn't be able to produce duplicates of this anyway, The existing one should just be updated with the new profile and resynced.
There was a problem hiding this comment.
I wasn't sure how duplicated are handled, the check was just in case a user had already manually downloaded the metadata. If it's fine to re-add it, we can get rid of the check :)
There was a problem hiding this comment.
Yeah, everything should operate fine. I don't think it's been possible to re-add something required before, but adding something that already has info uses basically the same update/overwrite path and that happens all the time without issue.
There was a problem hiding this comment.
I'll give it another try without the check tomorrow 👍🏻
There was a problem hiding this comment.
yep, seems to behave the same!
|
Okay, this should be ready then. I'll merge it once the checks pass and if we find any bugs we can fix them later on. |
Chaphasilor
changed the title
This enhances the user experience when adding tracks from playlists (as it shows which playlists the track is already part of), enables removing tracks from playlists using the playlist actions menu, and shows partially downloaded playlists in offline mode.