Closed Komodo5197 closed 1 month ago
Chaphasilor
commented
1 month ago I've had this error pop up a couple times with my main installation, but I also can't test if that fixes it because it's signed by James.
Do you want me to go back to an older version and test the upgrade, or are you confident enough that we can just merge this?
Komodo5197
commented
1 month ago Some testing would be nice, but I’m pretty confident in this fix, so you can just move ahead if you want. It’s basically just pulling the fix I actually tested in the desktop branch out from getfinampcollectionchildren to the Finampcollection property so that the new code from the offline favorites is also using it.
Chaphasilor
commented
1 month ago Okay, I'll see what I can do. Is going back to 0.9.6 and downloading a few favorites before upgrading enough, or does it require more specific steps?
Komodo5197
commented
1 month ago Downloading any of the finampcollections, such as all favorites, and then switching to 9.7 should break syncing for that item.
Chaphasilor
commented
1 month ago So the core change of the PR seems to work. Downloaded all three finampCollections in 0.9.6, upgraded to 0.9.7, got a bunch of errors, switched to this PR, errors gone, sync completed, repair also working.
Didn't attempt downloading the playlist info collection on 0.9.7, but it probably shouldn't affect the migration to this PR / 0.9.8...
Komodo5197
commented
1 month ago Nice. Yeah, I don't think the playlist info collection should have any bearing on this. Also, I noticed that issue where lyrics don't work for downloaded items when online - I'm thinking this is because the merging in metadata_provider was stripping the lyrics mediaStreams. If so, this PR should fix that because I messed with how the info gets passed around/merged a bit.
Chaphasilor
commented
1 month ago Yeah I had the same theory about the media stream merging. I'll take a closer look tomorrow!
Chaphasilor
commented
1 month ago Re: lyrics bug, I was simply missing a negation here: https://github.com/jmshrv/finamp/blob/cec5b3d9256062ff51503fd3c9971a120077f0b9/lib/services/metadata_provider.dart#L152 The idea was to add all non-audio/default streams from the online data, and then add the audio streams from the offline data.
But it seems with your change to returning an empty list for transcoded downloads, you break lyrics for those tracks completely, since the lyrics are only stored as a mediaStream, and not separately like the audio or image :P
The comments you added are very helpful though!
Komodo5197
commented
1 month ago Okay, that makes sense. Although I've made all the streams but lyrics default to the offline ones default to the offline ones because they are probably part of the file? Not like it matters when we don't use them. Anyway, I can't actually check because my server is still on 10.8, but hopefully lyrics for transcoded downloads work now.
Chaphasilor
commented
1 month ago whoops, didn't see you already fixed that bug. sorry about the merge conflict!
Chaphasilor
commented
1 month ago Lyrics for transcoded downloads in offline mode are still broken, because the stream type is called Lyric and not Lyrics. We should probably use an enum for that anyway 😅
Komodo5197
commented
1 month ago Whoops. Yeah, an actual Enum would be nice. I guess that's another benefit if/when we migrate the api models to be autogenerated.
I actually forgot I needed to check the sync favorites offline settings as well as the filter status, so it's a good thing that you did put a separate fix in.
Chaphasilor
commented
1 month ago I just copied the filter from the one in music_screen_tab_view :P
Lyrics are working now, and the favorite index is also correct, thanks!
Fixes an issue where a null check error is thrown while syncing older finampCollections that do not have a FinampCollection object attached. I haven't actually tested it with a legacy item yet.