[e-m-c][Android] Create `ActivityResultsRegistry` #17572

bbarthec · 2022-05-19T14:06:53Z

Why

Extracted part of #16251
Depends on #17571

Android is proposing a new way for registering for activity results - using ComponentActivity.registerForActivityResult.

The old way of using startActivityForResult and listening for results onActivityResult is deprecated.

Even though #17571 would enforce the underlying Activity to support this new mechanism we cannot use it directly on Activity.
react-native's application lifecycle is not mirroring the Android lifecycle (and thus Activity's lifecycle) completely. The moment the ComponentActivity expects the registration to happen is not available for us neither in expo-modules-core neither in react-native's application lifecycle. What is more these two lifecycle lives independently to each other.

In order to be able to use modern Android libraries (e.g. https://github.com/CanHub/Android-Image-Cropper) we need to support this new mechanism somehow.

I've copied and adjusted the original AndroidX files in order to couple the lifecycle of registered contracts not with Activity, but with AppContext (and thus bridge) lifecycle.

How

I've copied ActivityResultsManager, ActivityResultRegistry and ActivityResultCallback from androidx libraries and adjusted them to match AppContext lifecycle:

as AppContext is alive as long as the application is alive, we don't have to save data in onSaveInstanceState and retrieve it from onCreate (rn-screens and react-navigation are even strictly forcing us to mark the saved state as null when onCreate is called in order to work properly)
registerForActivityResults mechanism is based on two different callbacks:
- main callback that is passed when launch is called on ActivityResultLauncher - this one could be called stateful callback, because it has access to ReactNative objects, like Promise, etc
- fallback callback that is registered at the very beginning of the module's lifecycle. It handles all the data that is returned to the application, but only when the Activity has been killed and recreated by the system. When this happens react-native recreates the whole application from scratch, so each module has to store possible orphaned data and provide a way to retrieve them.
  - right now we have: https://docs.expo.dev/versions/latest/sdk/imagepicker/#imagepickergetpendingresultasync
  - Flutter has similar mechanism: https://pub.dev/documentation/image_picker/latest/#handling-mainactivity-destruction-on-android

Additionally I had to modify a bit how ReactNative propagates onActivityResult - see 150c5e7. This change has to be discussed and properly applied to all projects using expo-modules-core. I'm not sure how ReactNativeDelegateWrapper is applied to every project, so I'm asking for a bit of help with this aspect 🙏 👀

Test Plan

To be testable with #17671

I've checked flows for both Images and Videos
I've also checked the main bullet point of this mechanism - how it behaves when Activity is being recreated by Android - there's a flag in Developer Options available on the device that simulates such scenario (Don't keep Activities)

lukmccall

Looks very solid 🔥🔥🔥Great job 🏅

lukmccall · 2022-06-23T08:15:36Z

packages/expo-modules-core/android/src/main/java/expo/modules/kotlin/AppContext.kt

+      requireNotNull(currentActivity) {
+        "Current Activity is not available at this moment"
+      }


That would lead to a crash when the current Activity isn't available here. I'm not sure, but maybe we should handle that differently to prevent the app from crashing 🤔

This would actually never crash, because whenever any of lifecycle method is called (except for OnCreate) the current Activity must be available, because it is actually propagating these events 😉
I went with requireNotNull to get rid of nullability (I could have gone with !!, but we don't want that, do we? 👀).
I can go with some other message, like: "This would never happen, but if it does, something is broken very much"

I think you can leave it as it is.

packages/expo-modules-core/android/src/main/java/expo/modules/kotlin/AppContext.kt

lukmccall · 2022-06-23T08:17:12Z

packages/expo-modules-core/android/src/main/java/expo/modules/kotlin/AppContext.kt

+  @MainThread
+  override suspend fun <I, O, P : Serializable> registerForActivityResult(
+    contract: ActivityResultContract<I, O>,
+    fallbackCallback: AppContextActivityResultFallbackCallback<O, P>


Can we make a fallbackCallback optional?

I wonder about this 🤔
I would rather say that we should not. Why? Because not providing this callback means you do not care about the scenario when Android kills the main Activity and I wanted to have a mechanism that works in every case. If library author decides to omit such case, then it should be done in the library scope by providing noop callback 🤔
What I can do though is to make this additional parameter optional, because it is not at this moment. No matter the conclusion around this topic I'd like to do it in separate PR, do you mind me doing that? 🤔

Ok, you convinced me ;)

...-core/android/src/main/java/expo/modules/kotlin/activityaware/OnActivityAvailableListener.kt

...core/android/src/main/java/expo/modules/kotlin/activityaware/AppCompatActivityAwareHelper.kt

.../expo-modules-core/android/src/main/java/expo/modules/kotlin/activityresult/DataPersistor.kt

packages/expo/android/src/main/java/expo/modules/ReactActivityDelegateWrapper.kt

Kudo

just leaving a nit, other than that the pr looks good to me.

Kudo · 2022-06-23T09:37:11Z

...core/android/src/main/java/expo/modules/kotlin/activityaware/AppCompatActivityAwareHelper.kt

+    val a = activity.get()
+    if (a != null) {
+      listener.onActivityAvailable(a)
+    }


is it intentional to emit the event in the adding method? if yes with this side effect, please add some comments.

Yes, it is intentional. I've adapted this class from https://android.googlesource.com/platform/frameworks/support/+/HEAD/activity/activity/src/main/java/androidx/activity/contextaware/ContextAwareHelper.java and in AndroidX they do it that way, but yeah, I'll add some comment 😉

I've added a comment to the AppCompatActivityAware interface 😉

...e/android/src/main/java/expo/modules/kotlin/activityresult/AppContextActivityResultCaller.kt

bbarthec · 2022-06-24T07:48:36Z

packages/expo/android/src/main/java/expo/modules/ReactActivityDelegateWrapper.kt

+    if (delegate.reactInstanceManager.currentReactContext == null) {
+      val reactContextListener = object : ReactInstanceEventListener {
+        override fun onReactContextInitialized(context: ReactContext?) {
+          delegate.reactInstanceManager.removeReactInstanceEventListener(this)
+          delegate.onActivityResult(requestCode, resultCode, data)
+        }
+      }
+      return delegate.reactInstanceManager.addReactInstanceEventListener(reactContextListener)
+    }


Should I add this as a CHANGELOG entry in packages/expo? 🤔 I'm not sure what is our approach towards expo package, cc @tsapeta

it's worth to add a CHANGELOG because it fixes a real bug IMO.

I agree with Kudo 👍

…rovider`

`ReactNative` apps that uses `emc` cannot hook propoerly into `Activity` lifecycle and thus we cannot cannot directly use all fancy stuff from `AndroidX`. I'm proposing similar solution for registering for results coming from 3rd party Activities as AndroidX's https://developer.android.com/training/basics/intents/result. - Create `AppContextActivityResultCallback` that mirrors https://developer.android.com/reference/androidx/activity/result/ActivityResultCallback and has additional parameter `launchingActivityHasBeenKilled` - Create `AppContextActivityResultCaller` that mirrors https://developer.android.com/reference/androidx/activity/result/ActivityResultCaller to accepts `AppContextActivityResultCallback` - Create `AppContextActivityResultRegistry` that mirros https://developer.android.com/reference/androidx/activity/result/ActivityResultRegistry, but does not throw upon too late initialization. The original class forces registration in `onCreate` lifecycle method and when using `ReactNative` we are not able to hook into that method properly. Moreover the original class is coupled with Activity's lifecycle and we need something that would outlive Activity (and is hooked into `AppContext` lifecycle) - Create `ActivityResultsManager` that is encapsulating all `Activity`-related-for-result-registration outside `AppContext`

…ityResult` via `AppContext`

…xtActivityResultCaller`

…Activity reacreation - create `AppCompatActivityAware` mechanism for detecting propoerly moment when `Activity` is firstly created (similar to `onCreate` method) - convert `registerForActivityResult` API from single-callback based to two-callback based - main callback is straghtforward callback that is used when the Activity outlives the process of launching other 3rd party Activity - secodnary (fallback) callback is stripped from RN context (no promise, etc) and only handles the scanerio when the Activity is recreated, it preserves the optional options though - create initial API for serialization mechanism that would help preserve additional options during Activity recreation - adapt `AppContext` to these changes - postpone registering modules to the very end of `AppContext` creation as the previous approach would crash now

This is an interface that would allow persisting data in between Activity destruction and recreation

A helper class that is responsince for storing and retrieving data using `SharedPrefernces`

…nd Activity recreation - cleanup, unify and comment data structures - cleanup both code flows - lifecycle-based flow - `onActivityResult` (aka `dispatch`) flow - adapt `DataPersistor` - remove unused code

Implement pairs of methods (setter and getter) for each supported data kind

…nitialisation

It was happening because listener was added to the listeners pool adter it was called with the value. If it tires to unregister itself in its body it was happening before it was even added to the registered listeners pool.

.../expo-modules-core/android/src/main/java/expo/modules/kotlin/activityresult/DataPersistor.kt

bbarthec requested a review from tsapeta as a code owner May 19, 2022 14:06

bbarthec requested review from lukmccall, barthap and Kudo May 19, 2022 14:07

expo-bot added the bot: suggestions ExpoBot has some suggestions label May 19, 2022

bbarthec force-pushed the @bbarthec/e-m-c/android/create-activity-results-registry branch from 074dbea to a091ccd Compare May 19, 2022 14:30

bbarthec changed the base branch from main to @bbarthec/e-m-c/android/create-current-activity-provider May 19, 2022 14:31

Base automatically changed from @bbarthec/e-m-c/android/create-current-activity-provider to main May 24, 2022 13:02

bbarthec mentioned this pull request May 27, 2022

[image-picker][Android] Rewrite module to Sweet API #17668

Merged

4 tasks

bbarthec added a commit that referenced this pull request May 27, 2022

[to be reverted] Add ActivityResultsRegistry [#17572](#17572)

ee54cec

bbarthec mentioned this pull request May 27, 2022

[image-picker][Android] Migrate to registerForActivityResult #17671

Merged

2 tasks

bbarthec force-pushed the @bbarthec/e-m-c/android/create-activity-results-registry branch from 4525a53 to 0605c91 Compare June 1, 2022 13:08

expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Jun 1, 2022

bbarthec force-pushed the @bbarthec/e-m-c/android/create-activity-results-registry branch from 0605c91 to 52e337a Compare June 22, 2022 16:22

bbarthec requested a review from ide as a code owner June 23, 2022 08:16

expo-bot added bot: suggestions ExpoBot has some suggestions and removed bot: passed checks ExpoBot has nothing to complain about labels Jun 23, 2022

lukmccall reviewed Jun 23, 2022

View reviewed changes

Kudo approved these changes Jun 23, 2022

View reviewed changes

barthap approved these changes Jun 23, 2022

View reviewed changes

...e/android/src/main/java/expo/modules/kotlin/activityresult/AppContextActivityResultCaller.kt Outdated Show resolved Hide resolved

bbarthec commented Jun 24, 2022

View reviewed changes

bbarthec requested a review from lukmccall June 24, 2022 08:20

bbarthec added 7 commits June 24, 2022 10:30

[emc][Android] Create ReactActivityProvider

f69a3b3

[emc][Android] Implement ReactActivityProvider in AppContext

ab02560

[emc][Android] Replace ReactActivityProvider with `CurrentActivityP…

556ae98

…rovider`

[amc][Android] Expose registerForActivityResult and `launchForActiv…

489f6d4

…ityResult` via `AppContext`

[skip ci] Update CHANGELOG

23583f6

[emc][Android] Add missing launchForActivityResult to the `AppConte…

555d5fb

…xtActivityResultCaller`

bbarthec added 13 commits June 24, 2022 10:39

[emc][Android] Cleanup and format code

dcbad8e

[emc][Android] Create Bundleable interface

873122d

This is an interface that would allow persisting data in between Activity destruction and recreation

[emc][Android] Early version of DataPersistor

71e90fe

A helper class that is responsince for storing and retrieving data using `SharedPrefernces`

[emc][Android] Adapt Bundleable

186751d

[emc][Android] Cleanup ActivityResultRegistry, adapt Bundleable a…

d8e3252

…nd Activity recreation - cleanup, unify and comment data structures - cleanup both code flows - lifecycle-based flow - `onActivityResult` (aka `dispatch`) flow - adapt `DataPersistor` - remove unused code

[emc][Android] Remove Bundleable in favour of Serializable

f571917

[emc][Android] Implement DataPersistor logic

58351f7

Implement pairs of methods (setter and getter) for each supported data kind

[emc] Improve comments

0339868

[emc] :expo-modules-core:spotlessApply

d7884a7

[e-m-c] Wait with dispatching onActivityResult for ReactContext i…

ba814bf

…nitialisation

[emc] Apply review suggestions

67c48f2

Apply review suggestions

55dc9c2

bbarthec force-pushed the @bbarthec/e-m-c/android/create-activity-results-registry branch from c501756 to 55dc9c2 Compare June 24, 2022 08:41

expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Jun 24, 2022

[emc] Fix multiple call to removed listener

d4c4f8a

It was happening because listener was added to the listeners pool adter it was called with the value. If it tires to unregister itself in its body it was happening before it was even added to the registered listeners pool.

lukmccall approved these changes Jun 27, 2022

View reviewed changes

.../expo-modules-core/android/src/main/java/expo/modules/kotlin/activityresult/DataPersistor.kt Outdated Show resolved Hide resolved

[skip ci] Apply code suggestion

158796b

bbarthec merged commit fb8c1e0 into main Jun 27, 2022

bbarthec deleted the @bbarthec/e-m-c/android/create-activity-results-registry branch June 27, 2022 07:23

mauricedoepke mentioned this pull request Jan 1, 2023

Expo ImagePicker fails to launch. Call to function 'ExponentImagePicker.launchCameraAsync' has been rejected. #19512

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[e-m-c][Android] Create `ActivityResultsRegistry` #17572

[e-m-c][Android] Create `ActivityResultsRegistry` #17572

bbarthec commented May 19, 2022 •

edited

lukmccall left a comment

lukmccall Jun 23, 2022

bbarthec Jun 23, 2022

lukmccall Jun 27, 2022

lukmccall Jun 23, 2022

bbarthec Jun 23, 2022 •

edited

lukmccall Jun 27, 2022

Kudo left a comment

Kudo Jun 23, 2022

bbarthec Jun 23, 2022

bbarthec Jun 23, 2022

bbarthec Jun 24, 2022

Kudo Jun 24, 2022

tsapeta Jun 24, 2022

[e-m-c][Android] Create ActivityResultsRegistry #17572

[e-m-c][Android] Create ActivityResultsRegistry #17572

Conversation

bbarthec commented May 19, 2022 • edited

Why

How

Test Plan

lukmccall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbarthec Jun 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kudo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[e-m-c][Android] Create `ActivityResultsRegistry` #17572

[e-m-c][Android] Create `ActivityResultsRegistry` #17572

bbarthec commented May 19, 2022 •

edited

bbarthec Jun 23, 2022 •

edited