Use JSON serialization for settings #9887
Conversation
litetex
force-pushed
the
json-serialization
branch
from
c8f3d59
to
ab71672
Compare
|
Kudos, SonarCloud Quality Gate passed! |
SameenAhnaf
added
bug
|
Hi, just found this issue and I want to add my two cents here. I've just recovered newpipe after an android upgrade and faced that the android backup is not working on Android 13, so I have the backup on my PC, with the data, but I cannot use that to recover the state. I found that the import/export functionality exported a zip file (great), and there are two files there, the database and the settings. I have the database because it was on the backup, but the settings were serialized differently, they are stored as an XML on the data folder and as some Java Serialization on the export. Now you change that from Java-Serialization to JSON, ok, that might be more readable, but why to backup with a different format at all? Why not zip the XML settings file as is? So the backup should be stored on the export zip as Thanks. |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
If I may give some thoughts: It would be much more user friendly if there is still (maybe in a transition period) the possibility to import the old backups. If you want to load an old newpipe backup in a newer version and have to uninstall the app first and install an old version, this is in my opinion not the best solution, if not no solution at all. |
| private static ObjectMapper newMapper() { | ||
| return new ObjectMapper(); | ||
| } |
There was a problem hiding this comment.
According to Phind an ObjectMappershould be reused, because it's creation is relativly expensive.
SO source: https://stackoverflow.com/a/57671444
It kind of makes sense to me because in the spring boot world you have one instance of it and use it for all http requests unless specific config is required
| } catch (final IOException ioe) { | ||
| throw new UncheckedIOException(ioe); | ||
| } |
There was a problem hiding this comment.
Any reason to wrap it in UncheckedIOException?
It seems that exception is used in the stream API and ideally one should also throw it for stream related stuff.
https://stackoverflow.com/a/23402640
As a user, I’d expect settings import to continue working even for old formats. So even if the format is changed, I do not believe the deserializer of the old format should go away. Only the serializer. |
|
Will be taken care of separately |
What is it?
Description of the changes in your PR
Replaces Java serialization with JSON serialization
This was changed inside
Unfinished Downloads (Downloadmission)1StateSaver11 Ignored for now because they only store the data internally, which makes them irrelevant and the effort is too great; might be fixed later in another PR (StateSaver may e.g. be replaced by Icepick)
Additionally a bug was fixed where the export of the database was not possible because it was not initialized.
It's highly recommended to do a new backup afterwards (when this is released in a new version).
If a old backup - that was created before these changes took effect - should be imported, it has to be done in an old version of the app and then the app needs to be updated.
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.
Due diligence