Replace getForegroundActivity with applyToForegroundActivity[Task] #3350
Conversation
Coverage Report 1Affected Products
Test Logs
Notes |
Size Report 1Affected Products
Test Logs
Notes |
lfkellogg
force-pushed
the
lk/get-foreground-with-consumer
branch
from
dcddb94 to
fc64da9
Compare
lfkellogg
force-pushed
the
lk/get-foreground-with-consumer
branch
from
fc64da9 to
48347d2
Compare
lfkellogg
force-pushed
the
lk/get-foreground-with-consumer
branch
from
48347d2 to
e585921
Compare
|
/test copyright-check |
|
|
||
| @Before | ||
| public void setup() { | ||
| activity = Robolectric.buildActivity(TestActivity.class).create().get(); |
| import java.util.ArrayDeque; | ||
| import java.util.Queue; | ||
|
|
||
| class FirebaseAppDistributionLifecycleNotifier implements Application.ActivityLifecycleCallbacks { | ||
|
|
||
| /** A functional interface for a function that takes an activity and does something with it. */ | ||
| interface ActivityConsumer { | ||
| void consume(Activity activity) throws FirebaseAppDistributionException; |
There was a problem hiding this comment.
Looks good! Would like to chat about this consume stuff since I am not familiar with it.
|
/retest |
|
/test smoke-tests |
lfkellogg
force-pushed
the
lk/get-foreground-with-consumer
branch
from
a368452 to
27db9e6
Compare
lfkellogg
changed the title
| interface ActivityConsumer<T> { | ||
| void consume(Activity activity); | ||
| } |
There was a problem hiding this comment.
I kept this as a "Consumer" instead of a "Function" like we talked about, because otherwise we would have had to change our void handlers like openRedirectUrlInPlay to return a meaningless value, which felt unnecessary.
There was a problem hiding this comment.
Changing them from void to Void solves that problem, but if there's no use case for any return values right now, keeping it as void consume(...) works for me.
| } | ||
|
|
||
| /** A {@link SuccessContinuation} that takes an activity and returns a new {@link Task}. */ | ||
| interface ActivityContinuation<T> extends SuccessContinuation<Activity, T> {} |
There was a problem hiding this comment.
Leveraging the Task API's SuccessContinuation interface here.
There was a problem hiding this comment.
I can't decide if extending SuccessContinuation to bind to a specific type makes it cleaner (because there's one less type to worry about) or makes my head hurt (because I have to remember that there's an is a relationship here). I'm ok keeping it like this if y'all think it is better. 🙃
There was a problem hiding this comment.
Ah, you're right I actually like just keeping it as a SuccessContinuation. We're still enforcing that it accepts an Activity. No need to add another type 👍
Updated.
| */ | ||
| Task<Activity> getForegroundActivity() { | ||
| <T> Task<T> applyToForegroundActivityTask(ActivityContinuation<T> continuation) { |
There was a problem hiding this comment.
I named this applyToForegroundActivityTask as a parallel to onSuccessTask, since it seemed analogous: it accepts a function that returns a Task (a continuation).
|
|
||
| class FirebaseAppDistributionLifecycleNotifier implements Application.ActivityLifecycleCallbacks { | ||
|
|
||
| /** An {@link Executor} that runs tasks on the current thread. */ | ||
| private static final Executor DIRECT_EXECUTOR = Runnable::run; |
There was a problem hiding this comment.
Equivalent to the executor used internally by the Tasks API in certain cases: TaskExecutors.
|
/test smoke-tests |
2 similar comments
|
/test smoke-tests |
|
/test smoke-tests |
| interface ActivityConsumer<T> { | ||
| void consume(Activity activity); | ||
| } |
There was a problem hiding this comment.
Changing them from void to Void solves that problem, but if there's no use case for any return values right now, keeping it as void consume(...) works for me.
| } | ||
|
|
||
| /** A {@link SuccessContinuation} that takes an activity and returns a new {@link Task}. */ | ||
| interface ActivityContinuation<T> extends SuccessContinuation<Activity, T> {} |
There was a problem hiding this comment.
I can't decide if extending SuccessContinuation to bind to a specific type makes it cleaner (because there's one less type to worry about) or makes my head hurt (because I have to remember that there's an is a relationship here). I'm ok keeping it like this if y'all think it is better. 🙃
|
/test smoke-tests |
|
@lfkellogg: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
This allows the handler to be run immediately after the foreground activity is resumed. When relying on a Task handler, you're actually enqueueing the handler to either the main thread queue or our own executor's queue, and there's no guarantee that the activity is still in the foreground when it's called. While a race condition should be rare, it's better to avoid it if possible.
An alternative would be to pass in a "direct executor" that runs the listener in the same thread that sets the result, but this approach enforces that approach so the caller doesn't have to remember to do that.