[Android] AppState #5152

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@skv-headless
Contributor

skv-headless commented Jan 6, 2016

No description provided.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 6, 2016

By analyzing the blame information on this pull request, we identified @vjeux, @zjj010104 and @mkonicek to be potential reviewers.

By analyzing the blame information on this pull request, we identified @vjeux, @zjj010104 and @mkonicek to be potential reviewers.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Jan 6, 2016

Contributor

We have this module internally and want to open source it, let me compare the implementations.

Contributor

mkonicek commented Jan 6, 2016

We have this module internally and want to open source it, let me compare the implementations.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Jan 6, 2016

Contributor

Thanks for working on this!

Contributor

mkonicek commented Jan 6, 2016

Thanks for working on this!

@skv-headless

This comment has been minimized.

Show comment
Hide comment
@skv-headless

skv-headless Jan 6, 2016

Contributor

No problem, thanks for quick response. If I can help you somehow to deliver it faster just let me know.

Contributor

skv-headless commented Jan 6, 2016

No problem, thanks for quick response. If I can help you somehow to deliver it faster just let me know.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Jan 12, 2016

Contributor

I think we should still keep AppStateIOS.ios.js, AppStateIOS.android.js but introduce a new API AppState.js, similar to what we did with Switch and Alert for example.

Here's the internal module:
Java: https://gist.github.com/mkonicek/039f35748c0a073dd1b5
JS: https://gist.github.com/mkonicek/00c3638a4cf5d9ed8db7

If you can make AppState module that has a single API on Android and iOS that would be amazing. Looks like the AppStateAndroid.js might be redundant? Unfortunately the iOS and Android version were implemented by two different people but we've learned from that and now always think about both platforms at once when adding new APIs.

Contributor

mkonicek commented Jan 12, 2016

I think we should still keep AppStateIOS.ios.js, AppStateIOS.android.js but introduce a new API AppState.js, similar to what we did with Switch and Alert for example.

Here's the internal module:
Java: https://gist.github.com/mkonicek/039f35748c0a073dd1b5
JS: https://gist.github.com/mkonicek/00c3638a4cf5d9ed8db7

If you can make AppState module that has a single API on Android and iOS that would be amazing. Looks like the AppStateAndroid.js might be redundant? Unfortunately the iOS and Android version were implemented by two different people but we've learned from that and now always think about both platforms at once when adding new APIs.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Jan 12, 2016

Contributor

Sorry for the delay on this!

Contributor

mkonicek commented Jan 12, 2016

Sorry for the delay on this!

@skv-headless

This comment has been minimized.

Show comment
Hide comment
@skv-headless

skv-headless Jan 13, 2016

Contributor

I'll update pr in few days. Could you please don't release AppStateAndroid? I want to make it same for both platforms as you said.

Contributor

skv-headless commented Jan 13, 2016

I'll update pr in few days. Could you please don't release AppStateAndroid? I want to make it same for both platforms as you said.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 15, 2016

@skv-headless updated the pull request.

@skv-headless updated the pull request.

+
+var React = require('react-native');
+var {
+ AppState,

This comment has been minimized.

@eslint-bot

eslint-bot Jan 15, 2016

property AppState Property not found in Object.create

@eslint-bot

eslint-bot Jan 15, 2016

property AppState Property not found in Object.create

This comment has been minimized.

@mkonicek

mkonicek Jan 19, 2016

Contributor

You can ignore this lint warning.

@mkonicek

mkonicek Jan 19, 2016

Contributor

You can ignore this lint warning.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 15, 2016

@skv-headless updated the pull request.

@skv-headless updated the pull request.

@skv-headless

This comment has been minimized.

Show comment
Hide comment
@skv-headless

skv-headless Jan 15, 2016

Contributor

@mkonicek could you please review?

Contributor

skv-headless commented Jan 15, 2016

@mkonicek could you please review?

+
+var React = require('react-native');
+var {
+ AppState,

This comment has been minimized.

@eslint-bot

eslint-bot Jan 15, 2016

property AppState Property not found in Object.create

@eslint-bot

eslint-bot Jan 15, 2016

property AppState Property not found in Object.create

+public class AppStateModule extends ReactContextBaseJavaModule
+ implements LifecycleEventListener {
+
+ private String mAppState;

This comment has been minimized.

@skv-headless

skv-headless Jan 15, 2016

Contributor

What default value should I set? In your implementation is 'uninitialized' https://gist.github.com/mkonicek/039f35748c0a073dd1b5#file-appstatemodule-java-L27 but in AppStateIOS said that it can be null

@skv-headless

skv-headless Jan 15, 2016

Contributor

What default value should I set? In your implementation is 'uninitialized' https://gist.github.com/mkonicek/039f35748c0a073dd1b5#file-appstatemodule-java-L27 but in AppStateIOS said that it can be null

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Jan 15, 2016

Collaborator

Since we're introducing a new API, we should use promises instead of callbacks. Refer #4971

Collaborator

satya164 commented Jan 15, 2016

Since we're introducing a new API, we should use promises instead of callbacks. Refer #4971

+ @Override
+ public void onHostPause() {
+ mAppState = "background";
+ getReactApplicationContext().getJSModule(RCTDeviceEventEmitter.class)

This comment has been minimized.

@satya164

satya164 Jan 17, 2016

Collaborator

Maybe also have a method sendAppStateEvent?

private void sendAppStateEvent(WritableMap appState) {
  getReactApplicationContext().getJSModule(RCTDeviceEventEmitter.class)
    .emit("appStateDidChange", appState);
}
@satya164

satya164 Jan 17, 2016

Collaborator

Maybe also have a method sendAppStateEvent?

private void sendAppStateEvent(WritableMap appState) {
  getReactApplicationContext().getJSModule(RCTDeviceEventEmitter.class)
    .emit("appStateDidChange", appState);
}
+ }
+
+ @ReactMethod
+ public void getCurrentAppState(Callback success, Callback error) {

This comment has been minimized.

@satya164

satya164 Jan 17, 2016

Collaborator

Should return a Promise instead of callbacks.

@ReactMethod
public void getCurrentAppState(Promise promise) {
  promise.resolve(createAppStateEventMap();
}
@satya164

satya164 Jan 17, 2016

Collaborator

Should return a Promise instead of callbacks.

@ReactMethod
public void getCurrentAppState(Promise promise) {
  promise.resolve(createAppStateEventMap();
}

This comment has been minimized.

@skv-headless

skv-headless Jan 20, 2016

Contributor

I use callback not to touch ios native code. It is internal api, in future we will be able to change it to promises.

@skv-headless

skv-headless Jan 20, 2016

Contributor

I use callback not to touch ios native code. It is internal api, in future we will be able to change it to promises.

+RCTDeviceEventEmitter.addListener(
+ 'appStateDidChange',
+ (appStateData) => {
+ AppState.currentState = appStateData.app_state;

This comment has been minimized.

@satya164

satya164 Jan 17, 2016

Collaborator

What's the naming convention? camelCase or underscores? @mkonicek @nicklockwood ?

@satya164

satya164 Jan 17, 2016

Collaborator

What's the naming convention? camelCase or underscores? @mkonicek @nicklockwood ?

This comment has been minimized.

@mkonicek

mkonicek Jan 18, 2016

Contributor

It's camelCase in JS.

@mkonicek

mkonicek Jan 18, 2016

Contributor

It's camelCase in JS.

This comment has been minimized.

@skv-headless

skv-headless Jan 19, 2016

Contributor

I took it from ios https://github.com/facebook/react-native/blob/master/React/Modules/RCTAppState.m#L88. For now AppState and AppStateIOS are working with same obj-c implementation.

@skv-headless

skv-headless Jan 19, 2016

Contributor

I took it from ios https://github.com/facebook/react-native/blob/master/React/Modules/RCTAppState.m#L88. For now AppState and AppStateIOS are working with same obj-c implementation.

This comment has been minimized.

@mkonicek

mkonicek Jan 19, 2016

Contributor

Yup let's keep it so we don't have to change the iOS implementation in this PR.

@mkonicek

mkonicek Jan 19, 2016

Contributor

Yup let's keep it so we don't have to change the iOS implementation in this PR.

+ * - `active` - The app is running in the foreground
+ * - `background` - The app is running in the background. The user is either
+ * in another app or on the home screen
+ * - `inactive` - This is a transition state that currently never happens for

This comment has been minimized.

@satya164

satya164 Jan 17, 2016

Collaborator

Why document this if this never happens?

@satya164

satya164 Jan 17, 2016

Collaborator

Why document this if this never happens?

This comment has been minimized.

@skv-headless

skv-headless Jan 19, 2016

Contributor

according to blame @vjeux should know

@skv-headless

skv-headless Jan 19, 2016

Contributor

according to blame @vjeux should know

+ // after app launch. Trying to get currentState when mounting App component
+ // will likely to have the initial value here.
+ // Initialize to 'active' instead of null.
+ currentState: ('active' : ?string),

This comment has been minimized.

@satya164

satya164 Jan 17, 2016

Collaborator

Should this be a synchronous property? We can already see it not working properly.

If we really want it to be synchronous, another approach will be to set a constant initialState on native side which populates this value. Then subsequently change it on change events.

@satya164

satya164 Jan 17, 2016

Collaborator

Should this be a synchronous property? We can already see it not working properly.

If we really want it to be synchronous, another approach will be to set a constant initialState on native side which populates this value. Then subsequently change it on change events.

This comment has been minimized.

@skv-headless

skv-headless Jan 19, 2016

Contributor
  1. Could you please explain We can already see it not working properly what do you mean by it is not working?
  2. It seems that it works same way that you explained.
@skv-headless

skv-headless Jan 19, 2016

Contributor
  1. Could you please explain We can already see it not working properly what do you mean by it is not working?
  2. It seems that it works same way that you explained.

This comment has been minimized.

@satya164

satya164 Jan 19, 2016

Collaborator
  1. As per your comment,
// TODO: getCurrentAppState callback seems to be called at a really late stage
// after app launch. Trying to get currentState when mounting App component
// will likely to have the initial value here.

The value is not really synchronous, and causes these kind of issues

  1. You're setting a property on JS and changing it, not defining a constant on Native, but whichever works
@satya164

satya164 Jan 19, 2016

Collaborator
  1. As per your comment,
// TODO: getCurrentAppState callback seems to be called at a really late stage
// after app launch. Trying to get currentState when mounting App component
// will likely to have the initial value here.

The value is not really synchronous, and causes these kind of issues

  1. You're setting a property on JS and changing it, not defining a constant on Native, but whichever works
@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Jan 18, 2016

Contributor

Thanks again for working on it! I'm a bit swamped but will try to review tomorrow.

Contributor

mkonicek commented Jan 18, 2016

Thanks again for working on it! I'm a bit swamped but will try to review tomorrow.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 19, 2016

@skv-headless updated the pull request.

@skv-headless updated the pull request.

+
+var React = require('react-native');
+var {
+ AppState,

This comment has been minimized.

@eslint-bot

eslint-bot Jan 19, 2016

property AppState Property not found in Object.create

@eslint-bot

eslint-bot Jan 19, 2016

property AppState Property not found in Object.create

Libraries/AppState/AppState.js
+
+RCTAppState.getCurrentAppState().then((appStateData) => {
+ AppState.currentState = appStateData.app_state;
+}).catch(logError)

This comment has been minimized.

@eslint-bot

eslint-bot Jan 19, 2016

semi: Missing semicolon.

@eslint-bot

eslint-bot Jan 19, 2016

semi: Missing semicolon.

This comment has been minimized.

@skv-headless

skv-headless Jan 20, 2016

Contributor
  • remove catch
  • Check for ios. Probably doesn't work.
@skv-headless

skv-headless Jan 20, 2016

Contributor
  • remove catch
  • Check for ios. Probably doesn't work.

This comment has been minimized.

@mkonicek

mkonicek Jan 20, 2016

Contributor

Not sure I understand? Would adding the semicolon make eslint happy? :)

@mkonicek

mkonicek Jan 20, 2016

Contributor

Not sure I understand? Would adding the semicolon make eslint happy? :)

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Jan 19, 2016

Contributor

Is AppState.js a copy of AppStateIOS.ios.js or have you made any changes? Knowing that would make the review easier.

Contributor

mkonicek commented Jan 19, 2016

Is AppState.js a copy of AppStateIOS.ios.js or have you made any changes? Knowing that would make the review easier.

+public class AppStateModule extends ReactContextBaseJavaModule
+ implements LifecycleEventListener {
+
+ private String mAppState = "active";

This comment has been minimized.

@mkonicek

mkonicek Jan 19, 2016

Contributor

Default state should be "uninitialized" as per the internal module: https://gist.github.com/mkonicek/039f35748c0a073dd1b5

What's the rationale for setting it to "active"?

@mkonicek

mkonicek Jan 19, 2016

Contributor

Default state should be "uninitialized" as per the internal module: https://gist.github.com/mkonicek/039f35748c0a073dd1b5

What's the rationale for setting it to "active"?

+
+ @Override
+ public void onHostResume() {
+ mAppState = "active";

This comment has been minimized.

@mkonicek

mkonicek Jan 19, 2016

Contributor

nit: Please use constants like in https://gist.github.com/mkonicek/039f35748c0a073dd1b5

+
+ @Override
+ public void onHostDestroy() {
+

This comment has been minimized.

+ return appState;
+ }
+
+ private void sendAppStateEvent() {

This comment has been minimized.

@mkonicek

mkonicek Jan 19, 2016

Contributor

nit: sendAppStateChangeEvent would be clearer.

@mkonicek

mkonicek Jan 19, 2016

Contributor

nit: sendAppStateChangeEvent would be clearer.

+import com.facebook.react.bridge.ReactMethod;
+import com.facebook.react.bridge.WritableMap;
+
+import static com.facebook.react.modules.core.DeviceEventManagerModule.RCTDeviceEventEmitter;

This comment has been minimized.

@mkonicek

mkonicek Jan 19, 2016

Contributor

(and of course no newline before the line)

@mkonicek

mkonicek Jan 19, 2016

Contributor

(and of course no newline before the line)

@@ -64,6 +65,7 @@
new LocationModule(reactContext),
new NetworkingModule(reactContext),
new NetInfoModule(reactContext),
+ new AppStateModule(reactContext),

This comment has been minimized.

@mkonicek

mkonicek Jan 19, 2016

Contributor

Please sort these alphabetically (also the ToastModule).

@mkonicek

mkonicek Jan 19, 2016

Contributor

Please sort these alphabetically (also the ToastModule).

+ * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * @providesModule AppStateExample

This comment has been minimized.

@mkonicek

mkonicek Jan 19, 2016

Contributor

Thanks a lot for adding a cross-platform example!

@mkonicek

mkonicek Jan 19, 2016

Contributor

Thanks a lot for adding a cross-platform example!

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Jan 19, 2016

Contributor

Getting close! Very excited!

Contributor

mkonicek commented Jan 19, 2016

Getting close! Very excited!

@skv-headless

This comment has been minimized.

Show comment
Hide comment
@skv-headless

skv-headless Jan 19, 2016

Contributor

AppState.js is a copy of AppStateIOS.ios.js

Contributor

skv-headless commented Jan 19, 2016

AppState.js is a copy of AppStateIOS.ios.js

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Jan 19, 2016

Contributor

AppState.js is a copy of AppStateIOS.ios.js

Thanks! I guessed that was the case.

Contributor

mkonicek commented Jan 19, 2016

AppState.js is a copy of AppStateIOS.ios.js

Thanks! I guessed that was the case.

@skv-headless

This comment has been minimized.

Show comment
Hide comment
@skv-headless

skv-headless Jan 19, 2016

Contributor

@mkonicek agree with all notes, thank you for review. I'll try to fix tomorrow.

Contributor

skv-headless commented Jan 19, 2016

@mkonicek agree with all notes, thank you for review. I'll try to fix tomorrow.

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Jan 19, 2016

Collaborator

AppState.js is a copy of AppStateIOS.ios.js

If that's the case, can you also get rid of AppState.ios.js, and for platform specific things, add a Platform.OS check?

Collaborator

satya164 commented Jan 19, 2016

AppState.js is a copy of AppStateIOS.ios.js

If that's the case, can you also get rid of AppState.ios.js, and for platform specific things, add a Platform.OS check?

@skv-headless

This comment has been minimized.

Show comment
Hide comment
@skv-headless

skv-headless Jan 20, 2016

Contributor

@satya164 there is no platform specific code. At first version of PR I deleted AppStateIOS.js module. But @mkonicek asked me to left this component as it is. I think for compatibility with previous version. I copied it to keep them AppState and AppStateIOS independent.

Contributor

skv-headless commented Jan 20, 2016

@satya164 there is no platform specific code. At first version of PR I deleted AppStateIOS.js module. But @mkonicek asked me to left this component as it is. I think for compatibility with previous version. I copied it to keep them AppState and AppStateIOS independent.

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Jan 20, 2016

Collaborator

If the APIs are exactly the same, maybe we can just require the AppState file within AppStateIOS and re-export?

Collaborator

satya164 commented Jan 20, 2016

If the APIs are exactly the same, maybe we can just require the AppState file within AppStateIOS and re-export?

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Jan 20, 2016

Contributor

I like the idea of making AppStateIOS a wrapper around AppState. We can add deprecation warnings and finally delete AppStateIOS a few releases down the line. Let's do that in a separate PR though, this one is already large enough.

Contributor

mkonicek commented Jan 20, 2016

I like the idea of making AppStateIOS a wrapper around AppState. We can add deprecation warnings and finally delete AppStateIOS a few releases down the line. Let's do that in a separate PR though, this one is already large enough.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Jan 20, 2016

Contributor

@skv-headless Cool! Once you address the comments I think we can merge this.

Contributor

mkonicek commented Jan 20, 2016

@skv-headless Cool! Once you address the comments I think we can merge this.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 20, 2016

@skv-headless updated the pull request.

@skv-headless updated the pull request.

@skv-headless

This comment has been minimized.

Show comment
Hide comment
@skv-headless

skv-headless Jan 20, 2016

Contributor

@mkonicek updated.
Plan for the future:

  1. deprecation warning to AppStateIOS
  2. android memory event
Contributor

skv-headless commented Jan 20, 2016

@mkonicek updated.
Plan for the future:

  1. deprecation warning to AppStateIOS
  2. android memory event
+
+var React = require('react-native');
+var {
+ AppState,

This comment has been minimized.

@eslint-bot

eslint-bot Jan 20, 2016

property AppState Property not found in Object.create

@eslint-bot

eslint-bot Jan 20, 2016

property AppState Property not found in Object.create

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Jan 21, 2016

Contributor

Awesome!

@facebook-github-bot shipit

Contributor

mkonicek commented Jan 21, 2016

Awesome!

@facebook-github-bot shipit

@bestander bestander referenced this pull request Jan 21, 2016

Closed

[release-0.19] Commits to cherry pick #5462

9 of 10 tasks complete
@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment

@ghost ghost closed this in c2d75d7 Jan 21, 2016

mkonicek added a commit that referenced this pull request Jan 29, 2016

Android AppState
Summary: Closes #5152

Reviewed By: svcscm

Differential Revision: D2850250

Pulled By: mkonicek

fb-gh-sync-id: 0b5063fa7121d4e304a70da8573c9ba1d05a757c

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment