search

Karumi / Dexter

Android library that simplifies the process of requesting permissions at runtime.
http://karumi.com
Apache License 2.0
5.23k stars 671 forks source link

Fix issue where Lopper.loop() is called even if a looper already exists #155

Closed bastienpaulfr closed 7 years ago

bastienpaulfr commented 7 years ago

I have run into a bug where execution flow blocked in Lopper.loop() called from Dexter, whereas I wanted that execution continue. Indeed, execution thread of Dexter was run from a handler. So a Looper already exists !

This is the reason of this commit.

flipper83 commented 7 years ago

I'm going to try to reproduce this bug with a test before merging it, the solution looks ok, but I don't know if a flag check fixes the bug for all cases.

@bastienpaulfr if you have any sample code to reproduce the issue, could be really useful. Thank you.

flipper83 commented 7 years ago

I was trying to reproduce the problem with a Test, but I can't. Can you give me more information about the scenario to try to reproduce it?

bastienpaulfr commented 7 years ago

I have added an Android instrumentation test in the sample to show the use case where this fix is needed. If you remove the fix, then the test is failing.

flipper83 commented 7 years ago

@bastienpaulfr I like merge you pull request but the PR have conflicts with the current master version,the changes are really small. Sorry for the delay but we are really full with other tasks. If you fixed I will merge ASAP. And sorry again.

pedrovgs commented 7 years ago

I've just removed my comment about the visibility modifiers changes because the class modifier is already "package".

bastienpaulfr commented 7 years ago

Hi,

I've added an instrumentation test because @flipper83 asked me to add code to help reproducing the issue. I might try to reproduce the issue using Robolectric but I may be difficult to handle Android Handlers and Loopers correctly.

@pedrovgs Do you know an easy way to add tests on this topic ?

Thanks,

pedrovgs commented 7 years ago

@BasThomas your tests are correct. We are asking just for a change in our file named .travis.yml to be able to execute these tests in our CI environment.

Serchinastico commented 7 years ago

I'm adding the travis configuration and have fixed the test so that it taps on the "Allow" button and finishes successfully. You can see the progress in here: https://github.com/Karumi/Dexter/tree/dev-looper-fixes

If I manage to make it run on travis then I'll merge it. Thank you all for your time!

Serchinastico commented 7 years ago

Closing this PR in favor of #174