-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rewrite import and export subscriptions functionality using coroutines #11759
base: refactor
Are you sure you want to change the base?
Rewrite import and export subscriptions functionality using coroutines #11759
Conversation
Quality Gate passedIssues Measures |
try { | ||
@OptIn(ExperimentalSerializationApi::class) | ||
return json.decodeFromStream<SubscriptionData>(`in`).subscriptions | ||
} catch (e: Throwable) { | ||
throw InvalidSourceException("Couldn't parse json", e) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a cursory look, this seems to be a lot stricter than the previous json parser.
e.g. the serviceId
would default to 0
, and items in the list would simply be skipped if they don’t conform to the schema.
Do we want to change that behavior?
if (mode == 0 && value == null && serviceId == 0) { | ||
throw new IllegalStateException("Input data not provided"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to crash when rotating the screen while the confirmation dialog is visible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At that point the module is not yet restored, I don’t know what this is supposed to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed in the commit I added, I left out the check since it will always use the @Parcelize
decoders from the SubscriptionImportInput
subclasses.
e99c0bc
to
dbd11a6
Compare
I rebased on current Unfortunately, there is no |
app/src/main/java/org/schabi/newpipe/local/subscription/workers/SubscriptionImportWorker.kt
Outdated
Show resolved
Hide resolved
Once the missing error handling is added on import & the question about how leniently to parse the json is resolved, I’d say LGTM! |
Quality Gate passedIssues Measures |
What is it?
Description of the changes in your PR
CoroutineWorker
for better performance and readability of the code.Before/After Screenshots/Screen Record
Export
Screen_recording_20241129_071419.webm
Android 6.0
Screen_recording_20241129_064527.webm
Android 6.0
Screen_recording_20241129_064829.mp4
Android 15
Fixes the following issue(s)
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. You can find more info and a video demonstration on this wiki page.
Due diligence