rasmuslos / AmpFin

Native Jellyfin music player for iOS & iPadOS
Other
107 stars 10 forks source link

feat: Save audio type on download v2 #31

Closed gnattu closed 1 month ago

gnattu commented 3 months ago

This will save file mime-type and encode it into a negative number, and then encode it into the downloadId key of the OfflineTrack. When this key is nil, it defaults to -1, which corresponds to flac to match the old behavior, and the item downloaded in an older version can still be played.

I really don't want to resort to something like this, but the problem is out of my control, and I believe it's a SwiftData issue. I'm concerned that any further changes to the Data-schema for the Persistence Manager will be impossible until Apple fixes this migration issue. I can't even complete the migration by removing everything from the old context. However, for our current use case, this solution will work, as the downloadId provided by URLSession is never negative, and we have a finite number of file types to support.

rasmuslos commented 3 months ago

You are right, adding a new property with a default value should be covered by an automatic lightweight migration. I get two code level support requests for free for each of my apple developer membership years, I think I will just send them a request and let some apple engineer come up with a solution (hopefully).

But SwiftData has so many flaws it is baffling that they released it, especially with a basically not existing documentation...

gnattu commented 3 months ago

I think I will just send them a request and let some apple engineer come up with a solution (hopefully).

I believe this is a bug rather than an incomplete migration. I've already attempted the complex migration, and it turns out it never reaches the didMigrated stage because SwiftData fails to create a correct database table with a valid schema.

If they know a workaround for this, then good, but I'm afraid we have to use my workaround for iOS 17 and Apple needs to fix this in a following iOS update.

rasmuslos commented 3 months ago

I also tried to create a complex migration, and mine failed as well. I am currently trying to create a minimal example and it is really strange, because most of what I try works, only the specific model OfflineTrack cannot be migrated...

gnattu commented 3 months ago

only the specific model OfflineTrack cannot be migrated

My guess is that the OfflineTrack model directly uses the ReducedAlbum struct, which also contains a property named name. During the migration, SwiftData attempts to flatten the structure, causing issues with duplicate column names. For example, it correctly creates separate columns for id, naming them ZID and ZID1 to avoid conflicts. However, for the name property, it fails to differentiate, resulting in attempts to create two columns named ZNAME, leading to an invalid schema.

rasmuslos commented 3 months ago

I have been able to reliably reproduce the bug:

If a model contains a codable which in turn contains another codable and all three share at least one property name migrations will fail, no matter what. This is a problem for us because the OfflineTrack model contains the properties id & name. Item.ReducedArtist does as well. Because this struct is stored in the Track.ReducedAlbum, which is stored inside the model the app crashes. I will open a bug report with apple, I think we should wait with this until then.

SwiftDataBugReport.zip

Edit: Feedback-ID: FB13716836

gnattu commented 1 month ago

Any updates on this? It is now having conflicts

rasmuslos commented 1 month ago

I tried to migrate the SwiftData database without any luck, so the bug is still there. But this PR still uses the downloadId property to store the container, so there is some work to be done anyways.

With iOS 18 on the horizon I highly doubt that this will be fixed in iOS 17 but maybe we are lucky...

gnattu commented 1 month ago

if we have to work this in a different way then I'd prefer an implementation that can avoid this problem so that we don't have to wait for apple. This is a big problem because we cannot download alac files now.

Now the problem is:

Is it acceptable to create a new model just to store the file info?

gnattu commented 1 month ago

nvm, even adding new model to current persistence manager is not acceptable it seems

rasmuslos commented 1 month ago

nvm, even adding new model to current persistence manager is not acceptable it seems

Does it crash? I don't think adding new models is a problem, but I encountered errors when adding attributes to unrelated models, maybe that's the problem...

Adding a new models as a temporary solution seems like a good idea.

gnattu commented 1 month ago

nvm, even adding new model to current persistence manager is not acceptable it seems

Does it crash? I don't think adding new models is a problem, but I encountered errors when adding attributes to unrelated models, maybe that's the problem...

Adding a new models as a temporary solution seems like a good idea.

It does. I added a new Model in Model/Util called OfflineFile and structured like the lyrics, just change the lyric to filetype in string type. And adding this Model to the Schema of PersistenceManager will also throw a migration error. So we need to create another manager called like "offline file manager" which does not share the old schema to make it work, if we want to go this route.

gnattu commented 1 month ago

It seems like adding another modelContainer directly to the downloadManager would work. If you are fine with this approach I will re-implement with this.

rasmuslos commented 1 month ago

While thinking about the best way to implement this I had another idea: The problem appears to be that the Sqlite database is structured in a way that SwiftData cannot cope with.

We could just create a new one (avoiding the current problems, I would do some testing beforehand to ensure that migrations work this time), migrate all existing data over, and then delete the old one. This would happen automatically at launch. This would create a permanent solution.

I would leave the old store intact for a few versions though, just to be sure.

gnattu commented 1 month ago

We could just create a new one (avoiding the current problems, I would do some testing beforehand to ensure that migrations work this time), migrate all existing data over, and then delete the old one. This would happen automatically at launch.

This sounds great, but this looks like a long-term solution. I'm going to make a temporary one with the dedicated model container in the mean time to solve the short-term problems. If your new data structure comes in time then great, if it does not, we can use the short term solution.

gnattu commented 1 month ago

Closed as preferring a new version