[Redesign] Downloads Storage Rewrite

[Komodo5197]

An attempt to implement as much of the planned download rewrite as I can. It's not done yet, but I wanted to get this out there.

Highlights:

• Stores download metadata via linked items in Isar database.
• Uses background_downloader for downloads.
• Supports downloading songs, albums, artists, or genres.
• Can migrate metadata and files from existing downloads while offline.
• Can sync to download new songs if playlist/artist/genre updates.
• Requires Dart 3.0 due to Isar and requires android sdk 24/iOS 13 due to backgound_downloader.

The Design:

All download metadata is stored in a single collection of DownloadItems. Each item has a type, an optional BaseItemDto, a download state, and can require other items of arbitrary type. The primary download action is a sync, which takes a node and recalculates required children according to the node's type, fetching information from the server if needed. Then recently unconnected nodes are considered for deletion if no other nodes require them, and currently connected nodes are recursively synced. Downloads can be tracked via a DownloadStub, a simplified DownloadItem with no state used to key into it's matching DownloadItem.

The DownloadItem types:

• song - requires its image and artist/album/genre collectionInfo nodes. Can have associated file.
• image - does not require anything. Can have associated file.
• anchor - requires user-selected downloads, will never be deleted.
• collectionDownload - A playlist/album/artist/genre to be downloaded. Requires all elements of the collection, its own collectionInfo, and its image.
• collectionInfo - Metadata for a playlist/album/artist/genre. Does NOT require all elements of the collection. Songs require this to allow showing album/etc. pages without having it downloaded. Requires its image.

TODO:

• Implement external storage directories
• More testing. I don't really have a way to test in anything other than android emulator, so hopefully iOS works.
• Reimplement the downloads error screen
• Various UI improvements
• Improve threading and verify all transactions are fully safe.
• Investigate more advanced background_downloader features like notifications or pausing
• Improve user feedback during migration - maybe delaying until after app startup
• Decide whether to show partially downloaded albums while offline. Should this be a setting?
• Decide whether to show incomplete/failed song downloads while offline.
• Code cleanup and documentation.
• Make most/all download retrieval functions asynchronous.

[Chaphasilor]

Oh man, this is amazing. I'm sure @jmshrv will love this too!

Is this ready to take a look at already? Do you need any help or designs for this?
Also, why did you decided to target main with this PR? This is a major (and breaking) change, and with the redesign beta coming up, I'm not sure if this will ever be released before the beta starts. And once we have a beta, I'd like all new features to go into redesign, and only do fixes and backports for stable, to make sure the branches don't diverge too much.

Regarding your TODOs, here are some thoughts:

• Regarding more advanced downloading features, I think a highly requested one is downloading only over WiFi
• I think we should show partial collections, there could be a way to filter this to only complete ones. But I wouldn't implement this as a global setting
• I see no reason to show failed downloads in offline mode, at least not within a collection. They could still be shown on the downloads screen though

Lastly, I'm wondering if the collectionInfo type does include metadata for all items within a collection, even if they're not downloaded? For example, if I have an album with a few downloaded tracks and some that are not downloaded, the missing ones could be grayed-out but still shown. Maybe this only makes sense for certain collections (albums and playlists, maybe artists) but not others (genres, albums/artists/genres tab)?
I think it would be nice to have the full context for i.e. albums in offline mode, and if Isar can handle it, the size of the metadata shouldn't be an issue even for large libraries.
This could be a setting though, in case someone really doesn't have too much storage space available.
What do you think?

[jmshrv]

This is absolutely amazing, thanks for making what I couldn't in 3 years :)

I'll install this on my phone now and see how well it works - by the sounds of it, it should be way more reliable than Flutter downloader lol

[Komodo5197]

The actual download/delete/playback functionality seems mostly stable on my end, if you want to try it out. In regards to design help, there's a couple UI questions I haven't made a decision on if you want to weigh in, and of course I'm always open to ideas.

• With the re-sync and repair downloads buttons on the main download page, the retry failed downloads button seems pointless, especially as the resync currently retries failed downloads without additional prompting. Is there anything else I should be putting there?
• Expanding a child on the download screen shows songs for album/playlist but album for genre/artist, and you can't delete them because the parent would just re-download them on the next sync. Should I always show songs and never albums? Should I show images? Should I do anything if a song is individually downloaded?
• How should download button state be handled? My current thoughts are we want a button that is delete if the item is required by the anchor, download otherwise. And then maybe a sync button that shows up if we are required directly or indirectly, but is hidden otherwise, in line with Add sync button to playlist screen #534. Is just showing download in cases where we are actually downloaded, but would be deleted if a parent collection gets removed confusing, or fine?

I didn't really put much thought into what branch to target. If you want this targeting the redesign, I can do that. I noticed you just merged main into the redesign, so it shouldn't be to bad to merge the redesign into this.

I believe requiring Wifi is just a flag with background_downloder, so I'll throw that in. Regarding partial collection filtering, if there was an option where should I put it?

Regarding showing greyed out album tracks - currently, that info is not downloaded. I could add a songInfo type and pretty easily grab everything for albums. Playlists will never have parent metadata unless they have been downloaded, so all the songs must be there although some may have failed. Persumebly they should still show, but greyed out, if this functionality is added? The metadata size probably doesn't matter, it's tiny compared to the songs even if we double it with a bunch of songInfo nodes, and it seems unlikely for the songs to have many unique images. Grabbing children for artists is messier because it introduces information for empty albums that would need to be filtered out. Also, a note - I'm adding separate copies of metadata in info types because some sort of required flag seems annoying to track and adding some sort of 'info' link types would lead to loops, which I'd rather avoid.

[Chaphasilor]

Okay. I did merge main into redesign, but it was a difference of ~500 commits, so I'll test this in the next few days. I'll take a look at this PR afterwards. But yes, please target redesign with this PR, starting next year the stable version will probably be put into maintenance mode so we can focus on the beta.

Regarding your questions:

• The repair button is nice, but if you remove the "failed downloads" button it would be good to show which items/collections failed to download somewhere, at least for debugging purposes (mainly user debugging, not dev debugging). You could use a long-press handler for now
• Maybe it would be best and easiest to maintain if you used multiple tabs for the different types, like in the main music screen. Given that all downloaded parent items will always try to download their children, I see no reason why we would need to show child items like songs in an album. This should also handle cases where I downloaded an artist and also a specific album of that artist, and then delete the artist. The album should be on the albums tab on the downloads screen, the artist on the artist tab, and when I delete the artist all the albums are deleted as well until just the one that was manually downloaded is left.
• I think there should be three or four different download states, where the last one is optional depending on how hard it is to implement:
  1. Not downloaded
  2. Directly downloaded
  3. Downloaded as (transitive) dependency
  4. Downloaded, but out-of-sync (maybe split this into two states for direct and dependency downloads?)
     For now, just implement the functionality and put the following buttons: Download (for state 1), Delete (for state 2), "Persist" (for state 3), Sync (for state 4). I'll think about the design and how to best integrate it into the new UI. Maybe the delete button could also be shown for state 3, and show a dialog explaining why it can't be deleted, and have an option that takes you to the parent.
• Grayed-out tracks for ordered lists (albums, playlists) would be nice, no need for artists or genres.
• You could add the partial collections toggle next to the favorites only toggle for now. It's only temporary. Some indicator for partial collections (especially on the downloads screen) would be nice too

Also:

• If you're feeling adventurous, I'd appreciate a "download all" button, especially with the transcoded downloads in the pipeline. For that it would be nice to have a confirmation prompt that shows an approximate total size. If you don't want to look into it, try to still keep it in mind (might require changes to the "anchor" type?)
• Be sure to handle failures due to low disk space, that might require some other handling as just retrying
• I encountered a but where trying to play an item instead deletes the download. This happend when trying to shuffle all songs and playing albums, although one album managed to play. Playlists seem to work fine. Not sure what's going on here? This is still a draft PR though, so no worries. Let me know if you need a repro or logs :)

[Komodo5197]

The adaptive icons do seem to work.

The non-floating playing bar was something I was messing with due to contrast questions with the items behind the bar. The final shadow solution does mostly fix those though. I've removed the setting.

[Chaphasilor]

So I got about 75% of the review done now. I'll finish it tomorrow :)

[Chaphasilor]

Didn't have as much time today as I had hoped, but I made some progress. I still haven't reviewed the localizations, finamp_models, and the two Isar-Downloads files (aside from a general remark). Hopefully I can get that done tomorrow, it is 4k lines though 😅

I'll post the review of the other files now so you can take a look at it!

[Chaphasilor]

Mostly minor changes, and a few things that I don't fully understand yet and would like to know the reasoning behind!

[jmshrv]

Just downloaded an album and the songs were in the wrong order in offline mode, will investigate

[Chaphasilor]

This was also reported in #593 (for the redesign) branch, so it might be related to the old system. @jmshrv did you do a clean install or a migration? 🤔

I never encountered that issue myself

[Chaphasilor]

Nothing major here. Just a few suggestions and discussion topics :)

[Chaphasilor]

Okay, that should be the rest of it. This PR definitely is too large 👀

[Komodo5197]

I looked at the album ordering code, and it seems to work fine for me. It looks like the fallback if there aren't any track numbers might be slightly different, is it possible the album relied on that somehow? I put something that might fix this into the latest commit, it basically uses the playlist code to save off the order from the server instead of trying to sort albums ourselves. It needs a downloads repair to work properly.

[Chaphasilor]

@Komodo5197 I'm guessing we could get this merged tomorrow. @jmshrv told me they'd take a look at the backup stuff, and there are only a few minor unresolved discussions left.
However, #578 is also in the pipeline, including a few offline/downloads-related things. Would you mind taking a look and giving me some general directions on how to migrate it to the new system? I'd like to get my hands dirty at least a little bit, but want to make sure I do it the right way. If it's fine for you we could also merge the other PR first and then I do the merge in this PR?

[Komodo5197]

Looking at the other PR, it doesn't seem to be doing anything too unique with the downloads. You should be able to look at the existing code in music_screen_tab_view, album_screen, queue_service, and maybe some other places where we do pretty much the same things already. I feel like it makes a bit more sense to get this merged first, but you can do the merges in whatever order seems best to you.

[Chaphasilor]

@Komodo5197 let me know when I can click the button 🥳

[Komodo5197]

The ios backup fix was the last review change on the list, so I think it's good. Click away.

[Chaphasilor]

Awesome, merged! Many thanks for all the word you put into this, we're very grateful for it! I'm sure there are many users who will be more than happy with these new additions 😁
While I'd love to see more amazing improvements from you in the future (you rock!), you definitely deserve a break after this massive PR ^^

I'm really happy with how this turned out <3

[Komodo5197]

Happy to help. Indeed, I don't anticipate doing anything else notable anytime soon, especially as this, the queue restore, and all the now playing queue improvements combine to fix pretty much every issue Finamp had that I personally cared about. I'm really looking forward to the beta.

[Chaphasilor]

@Komodo5197 while you're still familiar with these changes, would you consider adding a section about how to work with the download system to the contribution guidelines?
The new system is a bit more involved than the old one, and a (potentially not so short) explaination should help new contributors get started more quickly, and help them to do things the "right" way.
The main concern would be a summary of how the system works (the hierachic downloads, metadata fetching, queueing, etc.), and when do use what methods.

If you decided to write something up, please target the main branch for better visibility :)

I'll work on a similar section for the queue service.

[jmshrv]

Interesting migration error from @erictbar on the Discord

[Startup/INFO] 2024-02-28 08:10:33.286384: App starting, logging initialized.
[downloadsService/INFO] 2024-02-28 08:10:33.334896: Migrating images from Hive
[downloadsService/INFO] 2024-02-28 08:10:33.336354: Migrating songs from Hive
[downloadsService/INFO] 2024-02-28 08:10:33.350522: Migrating parents from Hive
[ErrorApp/SEVERE] 2024-02-28 08:10:33.356913: Null check operator used on a null value
        #0      new DownloadStub.fromItem (package:finamp/models/finamp_models.dart:758)
        #1      DownloadsService._migrateParents (package:finamp/services/downloads_service.dart:1126)
        #2      DownloadsService.migrateFromHive (package:finamp/services/downloads_service.dart:957)
        <asynchronous suspension>
        #3      _setupDownloadsHelper (package:finamp/main.dart:129)
        <asynchronous suspension>
        #4      main (package:finamp/main.dart:76)
        <asynchronous suspension>

[downloadsService/INFO] 2024-02-28 08:10:40.307891: App is being paused by OS.
[downloadsService/INFO] 2024-02-28 08:10:46.888765: App returning from background, restarting downloads.
[downloadsService/INFO] 2024-02-28 08:10:46.889568: Attempting to restart queues.
[SyncBuffer/INFO] 2024-02-28 08:10:46.890003: All syncs complete.
[DeleteBuffer/INFO] 2024-02-28 08:10:46.890140: All deletes complete.
[IsarTaskQueue/INFO] 2024-02-28 08:10:46.890556: All downloads enqueued.

The specific line is:

    String id = (type == DownloadItemType.image)
        ? item.blurHash ?? item.imageId!
        : item.id;

Migrating an image without an ID???

[781flyingdutchman]

@Komodo5197 and @jmshrv I've added the concept of a holding queue to the background downloader. It's similar in concept as the TaskQueue (it limits maximum concurrent tasks etc) but it lives on the native side and is transparent (does not change how you use the FileDownloader). Because it is native, tasks from this queue will continue to be processed even when the app is suspended by the OS.

I know you've already implemented your own version of a TaskQueue but this may be worth taking a look at. Just one warming: this is a complex addition and while it passes all tests it would not surprise me if there are some bugs. If you do choose to use this, please let me know how it's working for you.

[Komodo5197]

@781flyingdutchman Even with our TaskQueue, this seems like it would be beneficial on Android to allow for more downloading when the app is suspended. I'm wondering if it would be a good or bad idea to configure this on iOS. I think just submitting everything to the native queue has been working, so it's probably not needed.

[781flyingdutchman]

Quite possibly! It is very difficult to predict how long an app remains unsuspended when moving to the background, as it depends on memory pressure, CPU load and - in the case of Android - the OEM strategy.

Both queueing mechanisms (TaskQueue and the HoldingQueue) can be active at the same time, so one idea to test might be:

• On Android, configure the HoldingQueue (with maxConcurrent constraints) and initialize your TaskQueue without any maxConcurrent constraints. This way, you keep your interface as it is today, and keep the benefit of the TaskQueue preventing the app from choking when enqueuing a lot of tasks, and add the benefit of ongoing queue processing in the HoldingQueue (along with better persistence)
• On iOS, experiment with a) keep everything as it is, and b) follow the same approach as on Android

The FileDownloader().configure method allows you to configure the holding queue only for one platform, differently for multiple platforms, or globally, so you can try different combinations to see what performs best.

If you do experiment, I'd love to know what you learn, as it may help others using the background_downloader.