Skip to content
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

Fix NullPointerException in queue handling #4555

Merged
merged 1 commit into from
Nov 8, 2020

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Oct 18, 2020

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

This fixes a NullPointerException caused by repeated getItem() calls on a queue which could possibly change in the meantime. So even though the result of getItem() was checked for nullity even before, its nullity state could change throughout time, and therefore a NPE was possible (still, really rare, and this is the best explanation I could give). Anyway, this simple fix only calls getItem() once, which should solve this problem and also possibly improving performance

Fixes the following issue(s)

Fixes part of #4546 (not linking though)

APK testing

@yashpalgoyal1304 if you are able to reproduce your issue, could you test this?
app-debug.zip

Due diligence

@TobiGr TobiGr added the bug Issue is related to a bug label Oct 18, 2020
@goyalyashpal
Copy link
Contributor

goyalyashpal commented Oct 19, 2020

thanks for considering me for test.

But since there seems no respectful place for discussions and helping people getting started, i wont be able to do that. I also dont have all the time in the world if i am greeted with such arrogance and hostility {talking about irc #fdroid}.

Thanks. I will just limit myself to ux things if there is no decent channel/platform to discuss things.

Just to make things clear - i do want(ed) to test - thats why i put efforts. And if i can have some decent and more relaxed place to talk, i have no problem in retrying.

i dont want to install latest version over my 0.19.x install.

So, I asked for alternate app (similar to standalone installs in pc)/other way to get that in #fdroid irc channel.

jochensp replied with change appid and recompile.

being untouched to compiling stuff, I did my web search thereafter, and it fetched the result that sdk can be used. I was just asking for confirmation that would that work, to which that hole person became shit-hitted.

  • gave me absolutely no way.
  • even didnt gave me feedback on my search,
  • neither pointed me where can i find it
  • reacted rudely and told me this is not android starter channel

@TobiGr
Copy link
Contributor

TobiGr commented Oct 19, 2020

@yashpalgoyal1304 There seems to be a big misunderstanding here. I took a look at your discussion in the #fdroid IRC channel. First of all, communication in IRC channels is mostly brief and happens ina quit direct way. Short answers are generally not considered or meant to be rude. It's just the IRC-typical style of writing. It is also important to check where to ask your questions. In your case, the #newpipe IRc channel would have been the right place We are also happy to answer questions directly on GitHub. #fdroid is just about everything regarding the F-Droid ecosystem.
For further info on IRC, I recommend you to read Getting help on IRC

@yashpalgoyal1304 if you are able to reproduce your issue, could you test this?
app-debug.zip

When asking users to test certain things, we always link a ZIP file, which contains an APK. This APK does not override the normal NewPipe installation, but installs as a separate app.

I hope, you did not invest too much time into searching for a solution.

@goyalyashpal
Copy link
Contributor

goyalyashpal commented Oct 20, 2020

ahhh, i used #fdroid geeeeez... i thought i was on #newpipe irc. i got confused.

🤕😬😬

@goyalyashpal
Copy link
Contributor

goyalyashpal commented Oct 20, 2020

also, i guessed that the linked zip would be containing the NewPipe separate app with the fix, but to reproduce the issue on 0.20, i need to have an old version (i.e. without fix) too....

anyways, i will try to retry. thanks for input.

@opusforlife2 opusforlife2 mentioned this pull request Oct 20, 2020
4 tasks
@TobiGr
Copy link
Contributor

TobiGr commented Oct 22, 2020

I have compiled a 0.19.8 APK that can be installed separately:
app-debug-0.19.8.zip

Thanks again

@Stypox
Copy link
Member Author

Stypox commented Oct 25, 2020

@yashpalgoyal1304 ^

@goyalyashpal
Copy link
Contributor

goyalyashpal commented Oct 25, 2020

i was unable to reproduce the issue in general app itself. Thats why i am unable to test. (@@) Any suggestions or reliable steps for reproduction?!

Re: #4546 (comment)


oh, and also for confirmation - @TobiGr the 0.19.8 zip is w/o the fix right!?

Copy link
Member Author

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TobiGr I think this can be merged even if there is no clear confirmation it fixes anything, since it reduces calls to containers accessors, thus improving performance and consistency, while keeping the logic identical.

@opusforlife2
Copy link
Collaborator

@Stypox 2 users have now confirmed that this PR doesn't fix the linked issue. Do you want to de-link it or try further changes?

@Stypox Stypox mentioned this pull request Nov 2, 2020
5 tasks
@TobiGr TobiGr merged commit f4435f9 into TeamNewPipe:dev Nov 8, 2020
This was referenced Nov 10, 2020
@Stypox Stypox deleted the playqueue-crash branch August 4, 2022 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants