-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[Android] Check if mCurrentActivity is set according to LifecycleState #23336
Conversation
Also i am confused that If |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpojer this alternate approach should avoid the TTI impact mentioned in the internal review.
I'm not change iOS.But ci is warming.---> |
@matthargett public class MyReactActivity extends Activity implements DefaultHardwareBackBtnHandler {
private ReactRootView mReactRootView;
private ReactInstanceManager mReactInstanceManager;
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
mReactRootView = new ReactRootView(this);
mReactInstanceManager = ReactInstanceManager.builder()
.setApplication(getApplication())
+ .setCurrentActivity(this)
.setBundleAssetName("index.android.bundle")
.setJSMainModulePath("index")
.addPackage(new MainReactPackage())
.setUseDeveloperSupport(BuildConfig.DEBUG)
.setInitialLifecycleState(LifecycleState.RESUMED)
.build();
// The string here (e.g. "MyReactNativeApp") has to match
// the string in AppRegistry.registerComponent() in index.js
mReactRootView.startReactApplication(mReactInstanceManager, "MyReactNativeApp", null);
setContentView(mReactRootView);
}
@Override
public void invokeDefaultOnBackPressed() {
super.onBackPressed();
}
} |
I didn't review what's the actual bug, but I just want to point out that there are use cases of creating a ReactInstanceManager outside of an activity context. Requiring activity be set initially would break that use case. For example in a brownfield app where not all activities are in RN, before the user navigates to a ReactActivity to speed things up, we can create ReactInstanceManager on the background, save it in an instance variable, then once the activity is launched we use that ReactInstanceManager already prepared. |
Similarly I don't understand how HeadlessJsTaskService where you pass null for activity would work after this change. Does it rely on that ReactNativeHost will always be creating a ReactInstanceManager first with an activity and then HeadlessJsTaskService will reuse the same ReactInstanceManager? If so that may not always be true either. |
Please observe its running order protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
mReactRootView = new ReactRootView(this);
mReactInstanceManager = ReactInstanceManager.builder()
.setApplication(getApplication())
.setBundleAssetName("index.android.bundle")
.setJSMainModulePath("index")
.addPackage(new MainReactPackage())
.setUseDeveloperSupport(BuildConfig.DEBUG)
.setInitialLifecycleState(LifecycleState.RESUMED)
.build();
// 注意这里的MyReactNativeApp必须对应“index.js”中的
// “AppRegistry.registerComponent()”的第一个参数
mReactRootView.startReactApplication(mReactInstanceManager, "MyReactNativeApp", null);
setContentView(mReactRootView);
} startReactApplication -> In We can know that In the past, Set the value of @Override
protected void onResume() {
super.onResume();
if (mReactInstanceManager != null) {
mReactInstanceManager.onHostResume(this, this);
}
} SummarySometimes the private synchronized void moveToResumedLifecycleState(boolean force) {
...
// at this time,the value of mCurrentActivity is null
currentContext.onHostResume(mCurrentActivity);
...
} So the value obtained by other modules executing public @Nullable Activity getCurrentActivity() {
if (mCurrentActivity == null) {
return null;
}
return mCurrentActivity.get();
} |
We are asynchronous to set the value of
|
Two point to set
|
I understand For your flow:
react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java Lines 693 to 697 in be282b5
But you can call So maybe you can improve code by: + if (mInitialLifecycleState == LifecycleState.RESUMED) {
Assertions.assertNotNull(
mCurrentActivity,
- "activity property has not been set with this builder");
+ "activity needs to be set if initial lifecycle state is resumed");
+ } |
You can see that by default So your problem only happens if you called react-native/ReactAndroid/src/main/java/com/facebook/react/ReactNativeHost.java Lines 66 to 76 in b002df9
|
You're right. public class MyReactActivity extends Activity implements DefaultHardwareBackBtnHandler {
private ReactRootView mReactRootView;
private ReactInstanceManager mReactInstanceManager;
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
mReactRootView = new ReactRootView(this);
mReactInstanceManager = ReactInstanceManager.builder()
.setApplication(getApplication())
.setBundleAssetName("index.android.bundle")
.setJSMainModulePath("index")
.addPackage(new MainReactPackage())
.setUseDeveloperSupport(BuildConfig.DEBUG)
.setInitialLifecycleState(LifecycleState.RESUMED)
.build();
// The string here (e.g. "MyReactNativeApp") has to match
// the string in AppRegistry.registerComponent() in index.js
mReactRootView.startReactApplication(mReactInstanceManager, "MyReactNativeApp", null);
setContentView(mReactRootView);
}
@Override
public void invokeDefaultOnBackPressed() {
super.onBackPressed();
}
} It's call |
I have a place before I said something wrong.
At this time,the state is So it caused this bug.#13439 |
881f8e8
to
0cc7932
Compare
@hey99xx Also we should change doc of android. public class MyReactActivity extends Activity implements DefaultHardwareBackBtnHandler {
private ReactRootView mReactRootView;
private ReactInstanceManager mReactInstanceManager;
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
mReactRootView = new ReactRootView(this);
mReactInstanceManager = ReactInstanceManager.builder()
.setApplication(getApplication())
+ .setCurrentActivity(this)
.setBundleAssetName("index.android.bundle")
.setJSMainModulePath("index")
.addPackage(new MainReactPackage())
.setUseDeveloperSupport(BuildConfig.DEBUG)
.setInitialLifecycleState(LifecycleState.RESUMED)
.build();
// The string here (e.g. "MyReactNativeApp") has to match
// the string in AppRegistry.registerComponent() in index.js
mReactRootView.startReactApplication(mReactInstanceManager, "MyReactNativeApp", null);
setContentView(mReactRootView);
}
@Override
public void invokeDefaultOnBackPressed() {
super.onBackPressed();
}
} |
I'll approve the PR but I'm neither working for FB nor a maintainer for react-native so you'll wait for someone else's approval.
Agreed. Do you want to send a pull request to https://github.com/facebook/react-native-website/blob/master/docs/integration-with-existing-apps.md too? |
Haha, I just submitted this PR#792. |
@mdvacca |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good.
It seems like the only thing in our way is one more internal test that is now failing. I'm unfortunately not an Android expert so I'm unsure how to get the current activity so I can set it on the builder. All I seem to have available is an It does seem that part of the test is to wait for a ReactContext to be initialized asynchronously, so I'm unsure whether this can be fixed :O |
Maybe is that? @cpojer Activity activity = (Activity)InstrumentationRegistry.getTargetContext();
build.setCurrentActivity(activity); Related to Get context of test project in Android junit test case |
I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification. |
I can't find other info for this pr because The current problem is that this test is internal . Could you please tell what to do next? |
@RoJoHub sorry I was away last week so I couldn't get to this. I just tried your proposed change but now it causes an issue when destroying the instance manager: |
Now the error prompts that the The current information can't make me locate the error.
By the way ,It's hard to locate where the error is. |
I'm sorry for making this hard but I'm not an expert on this part of the codebase so it's a bit hard. I did the cast as you suggested but maybe it didn't work properly and it is now setting the wrong thing as the activity, that then later crashes? Do you have any suggestions on how I could figure this out properly? |
@cpojer Case 1: ActivityInstrumentationTestCase2If We extend Lines 26 to 49 in b41ce43
If we want get activity,just call
Case 2: AndroidJUnit4.classIf an annotation is we should set a activity that we want to test.
If we want get activity,just call
Lines 25 to 37 in be282b5
Case 3: InstrumentationTestCase & InstrumentationRegistryIt's from [developer.android.com->startActivitySync]
Finally,we get |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Thanks to @makovkastar who fixed the internal issue we were having I was finally able to land this Pull Request. I want to apologize again for how long it took. |
This pull request was successfully merged by @RoJoHub in fb550e9. When will my fix make it into a release? | Upcoming Releases |
Summary: Issues: Related to #13439 react-native-website:Related to PR [#792](facebook/react-native-website#792) solution: #13439 (comment) When we integration with Existing Android Apps.and set LifecycleState is `LifecycleState.RESUMED`. It's lead to `mCurrentActivity` is null . At this time , the behave of set `mCurrentActivity ` which is unexpectedly. ## Changelog [Android] [Fixed] - Check if mCurrentActivity is set according to LifecycleState Pull Request resolved: #23336 Differential Revision: D14298654 Pulled By: cpojer fbshipit-source-id: 5cc17539a51154faeb838349b068d92511946f79
…k#23336) Summary: Issues: Related to facebook#13439 react-native-website:Related to PR [facebook#792](facebook/react-native-website#792) solution: facebook#13439 (comment) When we integration with Existing Android Apps.and set LifecycleState is `LifecycleState.RESUMED`. It's lead to `mCurrentActivity` is null . At this time , the behave of set `mCurrentActivity ` which is unexpectedly. ## Changelog [Android] [Fixed] - Check if mCurrentActivity is set according to LifecycleState Pull Request resolved: facebook#23336 Differential Revision: D14298654 Pulled By: cpojer fbshipit-source-id: 5cc17539a51154faeb838349b068d92511946f79
Summary
Issues: Related to #13439
react-native-website:Related to PR #792
solution: #13439 (comment)
When we integration with Existing Android Apps.and set LifecycleState is
LifecycleState.RESUMED
.It's lead to
mCurrentActivity
is null .At this time , the behave of set
mCurrentActivity
which is unexpectedly.Changelog
[Android] [Fixed] - Check if mCurrentActivity is set according to LifecycleState
Test Plan
Everything should run as usual.And Fix this Bug #13439 .