Restore instance data in ImportConfirmationDialog before checking the data#13003
Restore instance data in ImportConfirmationDialog before checking the data#13003dustdfg wants to merge 1 commit intoTeamNewPipe:devfrom
Conversation
… data Prevents crash on check of uninitialized data after screen rotation Co-authored-by: Siddhesh Dhainje <siddheshdhainje2001@gmail.com>
github-actions
bot
added
size/small
dustdfg
force-pushed
the
soundclod_import
branch
from
5144159 to
a40b42b
Compare
github-actions
bot
added
size/small
| Bridge.restoreInstanceState(this, savedInstanceState); | ||
| if (resultServiceIntent == null) { | ||
| throw new IllegalStateException("Result intent is null"); | ||
| } |
There was a problem hiding this comment.
Do we even need this check after it? We either set value via show method (100%) either it will be restored (from automatically stored state).
Is there any possibility that after we saved correct state (100%) in onCreate we will still receive null?
| .setCancelable(true) | ||
| .setNegativeButton(R.string.cancel, null) | ||
| .setPositiveButton(R.string.ok, (dialogInterface, i) -> { | ||
| if (resultServiceIntent != null && getContext() != null) { |
There was a problem hiding this comment.
Here is the same about checking resultServiceIntent.
getContextcheck could also be omitted? Especially because higher we use requireContext? But it is a part of callback. It is called later and probably not guaranteed to succeed later?
There was a problem hiding this comment.
Well, in theory someone could use setResultServiceIntent() and set it to null. I therefore prefer the other PR because it mitigates this issue by removing the method completely.
There was a problem hiding this comment.
Well, in theory someone could use setResultServiceIntent() and set it to null. I therefore prefer the other PR because it mitigates this issue by removing the method completely.
I was planning to merge that method with constructor but IIRC somewhere in history there was some explanation why it shouldn't be done. But it is not 100% because maybe I confused it with another file I was reading in recent days...
|
@TobiGr found out that NewPipe/gradle/libs.versions.toml Line 49 in 2704c20 |
2 participants
What is it?
Description of the changes in your PR
Restore instance data in ImportConfirmationDialog before checking the data. Prevents crash on check of uninitialized data after screen rotation)
Before/After Screenshots/Screen Record
Fixes the following issue(s)
Relies on the following changes
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
Footnotes
I don't remember correct word but I hope you understood ↩