jmshrv / finamp

A Jellyfin music client for mobile
Mozilla Public License 2.0
1.67k stars 118 forks source link

[Redesign] Offline favorites improvments #735

Closed Komodo5197 closed 1 month ago

Komodo5197 commented 1 month ago

Uses an info download of the favorites finampCollection to provide more accurate and up-to-date favorite information when online. Switches favorites provider to use this when appropriate, and re-enables favorite filter on music screen.

Also, adds an option to download info for all playlists, allowing partially downloaded playlists to be shown when offline.

Chaphasilor commented 1 month ago

Thanks for taking the time to add this!
I'll take a look at it today and try to do some testing involving other clients as well to see how it behaves.

Since we now have the ability to cache playlist metadata, I think we could use that (if available) to pre-select the ToggleableListTiles in the playlist actions menu? The metadata should usually be pretty up-to-date, and if someone modified a playlist on another device, sees the old status in Finamp, and then updates the state again, it shouldn't have any effect anyway (since duplicates in playlists are disabled by default, and removing a track "twice" also doesn't hurt).

Also, I noticed your solution for getting a BuildContext within a regular class using the GlobalSnackbar, that's really clever. I assume I could also use this for the queue service and the Android Auto stuff to provide localized strings to the OS itself?

Chaphasilor commented 1 month ago

There is no collection item for the playlist metadata:

Screenshot_20240519-152541.png

Chaphasilor commented 1 month ago

Just tried liking a track in Symfonium and then seeing how Finamp handles it.

If Finamp is in online mode and started (or resumed after more than 4h, I assume), a sync is automatically started and favorites are synced properly.

If Finamp is in offline mode and started, sync is briefly attempted, but then marked as completed. Switching to online mode will not restart a sync in this case. This should probably be changed, either by always syncing when switching from offline to online mode, or "queueing up" the sync after startup so that it is performed when offlinr mode is turned off for the first time after a restart.

Playing a downloaded track in online mode will show the correct favorite status, and that will be mostly preserved when switching to offline mode. The track will be shown as liked, but the favorite-only filter will not find it. Changing to a different track and refreshing the current view will then remove the favorite status again.

Manually triggering a sync will always update the favorite metadata, and will also immediately download the favorites if all favorites are downloaded.

Here are some logs:

(finamp-logs_favorite-sync-issues.txt), I wasn't sure if all of this was contained in the other logs.

finamp-logs_favorite-sync-issues-2.txt

And some screenshots ![Screenshot_20240519-180758.png](https://github.com/jmshrv/finamp/assets/18015147/cd8083e0-d336-470a-a332-7386c9feb8c0) ![Screenshot_20240519-180810.png](https://github.com/jmshrv/finamp/assets/18015147/807eaa21-5338-405f-9d31-40826cdea175) ![Screenshot_20240519-180825.png](https://github.com/jmshrv/finamp/assets/18015147/ba6c2413-443d-44af-b939-e77de1785b87) ![Screenshot_20240519-180833.png](https://github.com/jmshrv/finamp/assets/18015147/d774b4a9-2f50-467d-86e1-d3eedc4578c0) ![Screenshot_20240519-183635.png](https://github.com/jmshrv/finamp/assets/18015147/b4632dbf-300c-4e7b-8ea8-53d9e5e1b1dc) ![Screenshot_20240519-183651.png](https://github.com/jmshrv/finamp/assets/18015147/4a8e8049-ac18-4c59-89c0-55d09b5a90e8) ![Screenshot_20240519-183700.png](https://github.com/jmshrv/finamp/assets/18015147/41060b3d-be39-4919-a295-a2afb03b6ae6) ![Screenshot_20240519-183712.png](https://github.com/jmshrv/finamp/assets/18015147/7e404901-8784-4516-8cd3-c4c6d678d996) ![Screenshot_20240519-183719.png](https://github.com/jmshrv/finamp/assets/18015147/360b5ef8-279d-4b01-8c4f-006ccaa0bf2a) ![Screenshot_20240519-183740.png](https://github.com/jmshrv/finamp/assets/18015147/7b592229-c5c8-4852-ad20-8641d7d644e7) ![Screenshot_20240519-183748.png](https://github.com/jmshrv/finamp/assets/18015147/601baf41-72bc-4166-b605-b2a302903835)
Komodo5197 commented 1 month ago

I can add info items to the downloads screen for these two collections, but it'll be a bit weird. The favorites collection will keep getting re-generated to match the setting, even if you delete it. The all playlists collection will always show a zero file size because we only follow require links, so all the info linked images don't count.

Using the offline data to approximate playlist inclusions for the playlist menu is a good idea, I'll try to add it. You should be able to use the same method I used to get a BuildContext elsewhere, although it should probably only be a last resort, and you want to watch out for accidentally using it during early initialization and getting null.

Regarding the behavior in the face of an external like, that all seems like mostly correct behavior. The exception is not queuing a sync when started offline, which I've fixed. Falling back to the offline known status when switching to offline mode is expected, with a slight holdover while favoriteProvider created online still exists. This would behave cleaner if the offline favorites were up-to-date, which the queued sync from offline to online mode should help with.

Chaphasilor commented 1 month ago

No need to show the favorites download collection, that's a toggle in settings. But the playlist stuff should be shown, even if the size is zero. It shouldn't be zero though, since the playlist covers are downloaded/required by it?

I'll be sure to not use a null check when using the global context. I can always fall back to the current solution with untranslated strings, but in most scenarios it should work.

Yeah, I think if a sync is started when leavj ing offline mode, everything should work out just fine.
Maybe invalidating the favorite provider when toggling offline mode (same as we do with the metadata provider) is a good idea though, that way it's more obvious to the user that there is stale data. The favorite status simply disappearing at some seemingly random point could be confusing, same as the favorite filter not showing the track, even if the track is shown as a favorite in the player screen / mini player.

Thanks for the fixes and playlist improvements. Really looking forward to testing that tomorrow!

Komodo5197 commented 1 month ago

I managed to get info linked items included in file sizes in a way that hopefully works - tell me if all the sizes seem to be still correct for you. I ended up turning the playlist metadata into a fully separate FinampCollection type because trying to make most of the stuff work on info linked items was getting too messy, so now the only info linked thing is the hidden favorites metadata collection. There is an issue where none of the syncing/deleting/updating file size on change logic works because the item never changes state. This is because the playlists it points to are never considered downloaded, because we do not, in fact, download them. I'm not sure this is fixable, but I've forced it to be marked complete so at least you can see the current file size instead of 'syncing'.

Chaphasilor commented 1 month ago

The new playlist metadata on the playlist actions menu works well, and newly-created playlist (e.g. added by a Jellyfin plugin) still show up with the dashed icon like they should. I debated using the metadata to speed up loading of playlists, but in the case of new, unsynced playlists this would either cause a layout shift or mess up the order of playlists, so I think leaving it like this is best for now.

The queued sync after leaving offline mode also seems to work.

I guess we can get this merged then?

Unrelated, but another thing I noticed:
When library images are cached, a quick sync seems to also sync all nodes for which there are cached images. I guess this is done to detect changed images, and there's no way to sync less nodes?

Komodo5197 commented 1 month ago

Yeah, trying to steal downloaded metadata in more ways than this seems messy. I think this should be good to merge.

I believe detecting changed images doesn't actually involve syncing the nodes in this case. They just sync because finampCollections don't have the logic to not add unchanged children to the sync queue like normal collections do with the expectChangesInChildren logic. The nodes still generally skip themselves when processed because unless an image changed they are already images in state complete, they just have to run through the sync queue to do it. It doesn't matter too much, pretty much only albums/playlists have the volume for this optimization to matter.

Chaphasilor commented 1 month ago

That sounds good. If any of the new stuff causes issues, we won't know it before releasing it to users anyway ^^