Skip to content

Commit

Permalink
Add Activity to onActivityResult listener interface
Browse files Browse the repository at this point in the history 
Summary:
The Android lifecycle is weird: turns out `onActivityResult` is called before `onResume`. This means `getCurrentActivity()` could return the wrong instance, or `null` if the activity was destroyed. To give developers access to the Activity receiving the result (which is also about to become the current activity), pass it as an argumento the listener.

Fixes github issue #8694.

Reviewed By: donyu

Differential Revision: D3704141

fbshipit-source-id: e7e00ccc28114f97415e5beab8c9b10cb1e530be
  • Loading branch information
@foghina
foghina authored and Facebook Github Bot 6 committed Aug 13, 2016
1 parent d22003a commit fbd2e13
Show file tree
Hide file tree
Showing 6 changed files with 22 additions and 9 deletions.
2 changes: 1 addition & 1 deletion ReactAndroid/src/main/java/com/facebook/react/ReactActivityDelegate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ protected void onDestroy() {
public void onActivityResult(int requestCode, int resultCode, Intent data) {
if (getReactNativeHost().hasInstance()) {
getReactNativeHost().getReactInstanceManager()
.onActivityResult(requestCode, resultCode, data);
.onActivityResult(getPlainActivity(), requestCode, resultCode, data);
}
}

Expand Down
6 changes: 5 additions & 1 deletion ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,11 @@ public abstract void onHostResume(
*/
public abstract void onHostDestroy(Activity activity);

public abstract void onActivityResult(int requestCode, int resultCode, Intent data);
public abstract void onActivityResult(
Activity activity,
int requestCode,
int resultCode,
Intent data);
public abstract void showDevOptionsDialog();

/**
Expand Down
4 changes: 2 additions & 2 deletions ReactAndroid/src/main/java/com/facebook/react/XReactInstanceManagerImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -607,9 +607,9 @@ private void moveToBeforeCreateLifecycleState() {
}

@Override
public void onActivityResult(int requestCode, int resultCode, Intent data) {
public void onActivityResult(Activity activity, int requestCode, int resultCode, Intent data) {
if (mCurrentReactContext != null) {
mCurrentReactContext.onActivityResult(requestCode, resultCode, data);
mCurrentReactContext.onActivityResult(activity, requestCode, resultCode, data);
}
}

Expand Down
6 changes: 4 additions & 2 deletions ReactAndroid/src/main/java/com/facebook/react/bridge/ActivityEventListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,19 @@

package com.facebook.react.bridge;

import android.app.Activity;
import android.content.Intent;

/**
* Listener for receiving activity events.
* Listener for receiving activity events. Consider using {@link BaseActivityEventListener} if
* you're not interested in all the events sent to this interface.
*/
public interface ActivityEventListener {

/**
* Called when host (activity/service) receives an {@link Activity#onActivityResult} call.
*/
void onActivityResult(int requestCode, int resultCode, Intent data);
void onActivityResult(Activity activity, int requestCode, int resultCode, Intent data);

/**
* Called when a new intent is passed to the activity
Expand Down
9 changes: 8 additions & 1 deletion ReactAndroid/src/main/java/com/facebook/react/bridge/BaseActivityEventListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,23 @@

package com.facebook.react.bridge;

import android.app.Activity;
import android.content.Intent;

/**
* An empty implementation of {@link ActivityEventListener}
*/
public class BaseActivityEventListener implements ActivityEventListener {

@Override
/**
* @deprecated use {@link #onActivityResult(Activity, int, int, Intent)} instead.
*/
@Deprecated
public void onActivityResult(int requestCode, int resultCode, Intent data) { }

@Override
public void onActivityResult(Activity activity, int requestCode, int resultCode, Intent data) { }

@Override
public void onNewIntent(Intent intent) { }
}
4 changes: 2 additions & 2 deletions ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,9 @@ public void destroy() {
/**
* Should be called by the hosting Fragment in {@link Fragment#onActivityResult}
*/
public void onActivityResult(int requestCode, int resultCode, Intent data) {
public void onActivityResult(Activity activity, int requestCode, int resultCode, Intent data) {
for (ActivityEventListener listener : mActivityEventListeners) {
listener.onActivityResult(requestCode, resultCode, data);
listener.onActivityResult(activity, requestCode, resultCode, data);
}
}

Expand Down

18 comments on commit fbd2e13

@marcshilling
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@foghina is this not a breaking change? How can I make my module support 0.33 while also being backwards compatible?

@satya164
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcshilling There are 2 methods, one with the old signature marked as deprecated. So you should be able to use the old signature until its removed

@foghina
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a breaking change if you were using ActivityEventListener directly, instead of BaseActivityEventListener. Sorry, but there was no other way. Consider using the base class in the future to avoid compilation errors as we might change the interface in the future.

@subratpanda1
Copy link

@subratpanda1 subratpanda1 commented on fbd2e13 Sep 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am getting crash on invoking JS callback from onActivityResult after upgrading to 0.33.0 .

Stack Trace:
AndroidRuntime: FATAL EXCEPTION: main
AndroidRuntime: Process: com.<****>, PID: 13496
AndroidRuntime: java.lang.AbstractMethodError: abstract method "void com.facebook.react.bridge.ActivityEventListener.onActivityResult(android.app.Activity, int, int, android.content.Intent)"
AndroidRuntime: at com.facebook.react.bridge.ReactContext.onActivityResult(ReactContext.java:210)
AndroidRuntime: at com.facebook.react.XReactInstanceManagerImpl.onActivityResult(XReactInstanceManagerImpl.java:612)
AndroidRuntime: at com.facebook.react.ReactActivityDelegate.onActivityResult(ReactActivityDelegate.java:134)

@marcshilling
Copy link

@marcshilling marcshilling commented on fbd2e13 Sep 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@satya164 @foghina but even using BaseActivityEventListener, when running on < RN 0.33, the code won't compile because the new onActivityResult method signature does not exist. Am I missing something?

EDIT: Ah, I guess I can just not mark the new signature as @Override and it works on both 33 and below. Sweet!

@cbrevik
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcshilling Would you have to remove the @Override on the old signature as well then? If not then it will probably throw a compiler exception at 33 and above?

@marcshilling
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbrevik not required (because the old signature is still defined as deprecated), but I went ahead and did it to prevent issues after it eventually goes away. Tested on both 33 and 32 and it works great.

@foghina
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't have to remove any @Override. You should be able to compile against the newest RN but run on older RN. This is how Android works as well: you always compile against the newest SDK but put runtime checks in to ensure compatibility with older versions. In this case that would be implementing the old callback to simply call the new one.

@foghina
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't worry about it though, I'd just make it work with the latest RN only and if people aren't willing to upgrade they can use the older version of your library as well.

@lekenny
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@subratpanda1 I catch the same problem as you. how did you solve.

@subratpanda1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could not solve. I reverted back to 0.32.0.

@satya164
Copy link
Contributor

@satya164 satya164 commented on fbd2e13 Sep 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just updated to 0.33.0. Getting the following,

Process: chat.belong.hello, PID: 23792
java.lang.AbstractMethodError: abstract method "void com.facebook.react.bridge.ActivityEventListener.onActivityResult(android.app.Activity, int, int, android.content.Intent)"
    at com.facebook.react.bridge.ReactContext.onActivityResult(ReactContext.java:210)
    at com.facebook.react.XReactInstanceManagerImpl.onActivityResult(XReactInstanceManagerImpl.java:612)
    at com.facebook.react.ReactActivityDelegate.onActivityResult(ReactActivityDelegate.java:134)
    at com.facebook.react.ReactActivity.onActivityResult(ReactActivity.java:77)
    at chat.belong.hello.MainActivity.onActivityResult(MainActivity.java:64)
    at android.app.Activity.dispatchActivityResult(Activity.java:6470)
    at android.app.ActivityThread.deliverResults(ActivityThread.java:3716)
    at android.app.ActivityThread.handleSendResult(ActivityThread.java:3763)
    at android.app.ActivityThread.-wrap16(ActivityThread.java)
    at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1403)
    at android.os.Handler.dispatchMessage(Handler.java:102)
    at android.os.Looper.loop(Looper.java:148)
    at android.app.ActivityThread.main(ActivityThread.java:5443)
    at java.lang.reflect.Method.invoke(Native Method)
    at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:728)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:618)

The code in MainActivity.java is the following,

    @Override
    public void onActivityResult(int requestCode, int resultCode, Intent data) {
        super.onActivityResult(requestCode, resultCode, data);
        MainApplication.getCallbackManager().onActivityResult(requestCode, resultCode, data);
    }

@satya164
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@foghina Any idea what's happening here?

@eronisko
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a breaking change if you were using ActivityEventListener directly, instead of BaseActivityEventListener.

I'm guessing lots of people would have been using ActivityEventListener because even the 0.34 docs describe it as the way to add native modules. There is no mention of the BaseActivityEventListener class anywhere in the docs.

@foghina
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eronisko thanks for the heads-up, I'll update the docs.

@tyao1
Copy link
Contributor

@tyao1 tyao1 commented on fbd2e13 Sep 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I met the same problem as @subratpanda1. Still not working in 0.34.0

@vyky
Copy link

@vyky vyky commented on fbd2e13 Sep 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting the same error as @subratpanda1

I'm sorry but this hack affect more bad then good. Please consider revert back.

@satya164
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@subratpanda1 @XiaoBuu @vyky I found that a module was still using the old API. The error went away after updating the module to use the new API.

Please sign in to comment.