Keep execution context on permission requested

[CarlosMChica]

@pedrovgs here is the PR to keep the execution context.

As you pointed out to add a public method to keep the execution context I did so.

But there is another possibility. We could use the old DexterInstance.checkPermission() method to just return the listener callbacks on the same thread that calls the method.

If you rather go for the 2 public methods option, then I'd rename the old DexterInstance.checkPermission() to DexterInstance.checkPermissionOnMainThread(), as the method to check permission on the same thread is called DexterInstance.checkPermissionOnSameThread(). I think this will make more evident the purpose of both methods.

Thanks.

Fix #31

[pedrovgs]

Thanks for your PR @cmc00022 I'll take a look this week :)

[Serchinastico]

This thread spec doesn't run anything on the main thread. It's relying on where it is being executed to run it in that very same thread but doesn't force to be the main thread in any way. I'd either rename it or move the logic to run things on the main thread to this class.

[pedrovgs]

As @Serchinastico says, if you want to force the execution over the main thread you should create a handler using the main looper.

[pedrovgs]

Talking to @Serchinastico we've just noticed that this listener is public :S This should be an implementation detail, the usage of this listener should be an internal detail. We should change the interface visibility to default and refactor the current implementation to hide the usage of this interface. Sorry, we didn't see this before :(

[Serchinastico]

I'd say the same about the ThreadSpec interface, it doesn't make sense to expose that part of the library. As I see it we are adding a new abstraction for no real benefit. Users of the library can actually wrap their listeners to execute its callbacks using any android/custom abstraction, with that interface we would be forcing them to understand yet another concept.

[CarlosMChica]

@pedrovgs what do you suggest then? The only reason it's public is because its package is not the same as the DexterInstance once, where the decorator is used. I could just move the decorator to the DexterInstance package.

The same issue happens with other classes situated under the threaddecorator package, such as: ThreadSpecFactory and ThreadSpec.

Let me know how you'd like to approach it.

[pedrovgs]

Given the Java visibility mechanism the only option we have is to move all the new code to the DexterInstance package :( Talking to @Serchinastico we prefer to put all this code together than to expose public APIs. The rest of the listener are thoughts to be used by the library user, but the new one you've added should be hidden to the library user to reduce the library complexity.

[CarlosMChica]

done

[pedrovgs]

@cmc00022 the change you suggest here:

I was referring to changing it on the library api too, so it's easier for library users to understand the behaviour.

Is not needed. By convention we return the response over the UI thread and thats enough. We don't need to introduce a new method.

[CarlosMChica]

ok, agree.

[PaNaVTEC]

Hi guys, about this comment:

Anyway, we should rename this class to Thread and not thread spec, because you are basically > duplicating the Thread API.

Do you really want to have a "Thread" class that the name collides with the "java.lang.Thread"?.
In addition, I don't think it is duplicating the java.lang.Thread API, it is an abstraction to change between threads, it is not the representation of a Thread by itself. I'd rather keep the original name ThreadSpec (specification) or find another better if does not represent the intention of this class.

[pedrovgs]

The ThreadSpec used in this PR is just an abstraction needed for testing purposes, because using the Android jar loaded during the test execution a call to handler.post(new Runnable {...}) is not going to execute the Runnable instance at all.

If you review the usage of this interface:

@Override public void execute(Runnable runnable) {
  if (runningMainThread()) {
      runnable.run();
    } else {
      new Handler(Looper.getMainLooper()).post(runnable);
    }
}

We are using this execute method to post a runnable and execute it, which is the same thing you can do with a Handler or a Thread(with the second one you have to create the Thread with the Runnable instance passed as a parameter during construction). I know, Thread and ThreadSpec is not the best naming option, but I prefer Thread over ThreadSpec because "specification`` is expressing a type of thread and not an object you can use to execute code in a different execution context.

Due to the main reason to create this abstraction is just for testing purposes we should try to remove the abstraction and create a HandlerFactory class without static factory methods we can use to mock the Handler instance during the test execution runtime. If I'm not wrong, @Serchinastico suggested something like this:

class HandlerFactory {

 Handler getHandler() {
    if (runningMainThread()) {
      return new Handler();
    } else {
      return new Handler(Looper.myLooper());
    }
  }

 private boolean runningMainThread() {
    return Looper.getMainLooper().getThread() == java.lang.Thread.currentThread();
  }
}

Doing this we could reduce the amount of code needed and also replace the instance obtained during the getHandler() call.

Right now I see that @cmc00022 has already changed the interface name from ThreadSpec to Thread, so I'm ok with this change. If we keep the usage of these classes as an implementation detail inside the library we can change it in the future. This is the key in this PR.

[pedrovgs]

If you want, you can remove the usage of java.lang.Thread.currentThread() changing this if to something like:

   return Looper.myLooper() == Looper.getMainLooper()

[CarlosMChica]

done

[pedrovgs]

@cmc00022 please, let me know once you've applied all the changes we've suggested to perform the last PR review :) Thanks for your time and patience.

[PaNaVTEC]

Hey @pedrovgs If you are happy with the new name it is ok.
About the goal, of the ThreadSpec from my point of view is not only about testing, is like a plugin that you can plug different Specifications, without changing the code of the methods that are using it. In this case the factory methods are providing with the spec needed.
The provided HandlerFactory does not solve the whole problem because you don't need to call .prepare() and .loop() if you are on the main thread, so this abstraction also provides you a way to hide this details inside the specifaction.
As said, if you guys are happy with the nsame, it's ok.
Thanks for you time :)

[pedrovgs]

@cmc00022 please, let me know once you've applied all the changes we've suggested to perform the last PR review :) Thanks for your time and patience.

[pedrovgs]

We should change the visibility of this constructor to default instead of public as we've done with the class signature.

[CarlosMChica]

done

[pedrovgs]

After the last PR review I think this PR is ready. Please review the last comments I've added about some methods visibility, spec naming and the usage of the Thread.sleep(). Thanks for your code, time and patience @cmc00022. Here is my 👍

Please @flipper83 I need another 👍. Review this PR carefully.

[pedrovgs]

@flipper83 do we have your ok? If you can't review this PR just tell me to ask to Sergio if he has time.

[flipper83]

Sorry I'm come back from my holidays yesterday. ok with pedro comments 👍 to the PR.

[pedrovgs]

Thank you guys! I'm going to merge this PR and release a new version.