-
-
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
Crash when pressing search button #6522
Comments
Are you just using 0.21.5 or did you specifically use the GitHub Action APK build of my PR? |
I built from remote/dev on the commit specified |
@TacoTheDank the issue also occurs on the github action build of the PR |
I see 🤔 However, I don't really see why this could be triggering this bug to occur. @opusforlife2 Any ideas? |
Wow, dude. I'm flattered, but I'm not a dev. 🤭 |
I did find a "fix" #6394 (comment) |
I believe I have made a development. This bug most likely has to do with two things:
I believe these are the source of this new bug (and may be the cause of possibly undiscovered new bugs as well). There is likely some new behavior that NewPipe has not sufficiently accounted for, and that is the source of this bug. But, given all the weird shit we have to do around the app just to dispose of stuff properly when views and stuff are destroyed, I suppose this isn't surprising. But this is the price of progress. If we're too afraid to take risks, we'd never get anywhere 😁 (Btw this bug came about because of the library bumps in #6394) |
@TacoTheDank So should this change be reverted for now, until a solution is found? |
…ragment It seems due to TeamNewPipe#6394 updating the FragmentX library there was a change to the order of lifecycle calls, as such onResume() was no longer before onCreateOptionsMenu() creating a null pointer exception when using service in onCreateOptionsMenu() as it is only set in onResume(). By moving the initialization of service to onStart() which still happens before onCreateOptionsMenu() this crash can be avoided. This commit also adds a check for a null service to prevent future crashes for similar issues.
Observing the logging in this situation the following behavior occurs:
My approach to this would be moving setting the service from A draft of this fix is here: Douile@384ca66 |
Can you open a pull request? |
@opusforlife2 I think it'll be okay. This bump would've needed to be done sooner or later anyway, so it's better that we catch it now. Also, @Douile has made a PR with the fix. For some reason it didn't occur to me to test the searching function, even though I usually do when making PRs, so this happened to slip by me. Big oops on my part. |
Checklist
Steps to reproduce the bug
Similar to #6231 but different null pointer exception
0.
Build the app fromdev
branchActual behaviour
App crashes (ui error)
Expected behavior
App doesn't crash (no null pointer exception)
Screenshots/Screen recordings
Logs
Exception
Crash log
The text was updated successfully, but these errors were encountered: