Skip to content

[Android] Fix crash on device rotate#97

Closed
FeralAI wants to merge 8 commits into
wix:masterfrom
the-mx-group:android_rotation_fix
Closed

[Android] Fix crash on device rotate#97
FeralAI wants to merge 8 commits into
wix:masterfrom
the-mx-group:android_rotation_fix

Conversation

@FeralAI
Copy link
Copy Markdown

@FeralAI FeralAI commented Jun 25, 2016

This PR fixes crashes when rotating, at least on the project I'm working on. This is related to and fixes issue #94.

@FeralAI
Copy link
Copy Markdown
Author

FeralAI commented Jun 25, 2016

For more info, I found the fix here: http://stackoverflow.com/a/27937906

@FeralAI
Copy link
Copy Markdown
Author

FeralAI commented Jun 25, 2016

This isn't fully fixed since my rotate events aren't firing, but it doesn't crash either. I'll work with it a bit more to see if I can get it working.

EDIT: Turns out it's working fine, I was just having an issue with one of my modules. The PR still fixes the crashing.

@FeralAI FeralAI changed the title Fix android crash on device rotate [Android] Fix crash on device rotate Jun 25, 2016
@FeralAI
Copy link
Copy Markdown
Author

FeralAI commented Jun 26, 2016

Last two commits fix the menu icon state when onConfigurationChanged fires.


@SuppressWarnings({"ConstantConditions"})
public void setNavUpButton(Screen screen) {
public void setNavUpButton(Screen screen, boolean isBack) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@jskuby Why is this change necessary? Sending boolean params to functions that change what the behaviour of the function is quit an anti pattern.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This was a quick update so it can be called from onConfigurationChanged since my menu icon was losing its state on rotation.

How should we go about handling this? Separate functions for styling and setting the state?

Copy link
Copy Markdown
Collaborator

@guyca guyca Jun 28, 2016

Choose a reason for hiding this comment

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

@jskuby Can we move the setNavUpButton call to StyleHelper.updateStyles? If so, seems like we determine if the back button needs to be displayed or not buy calling getScreenStackSize() which is already public. Am I right?

In RnnToolbar we can grab a the BaseReactActivity instance by calling getContext().

I should really kill StylesHelper and move the logic to RnnToolbar. I'll do it after we finish with your PR's to avoid conflicts.
wdyt?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I took a look and that all seems good @guyca. I probably won't have time to work on this until tomorrow, but I'll see what I can do to move this along.

@FeralAI
Copy link
Copy Markdown
Author

FeralAI commented Jun 30, 2016

@guyca After moving the setNavUpButton call into StyleHelper.updateStyles there is no need to call the super navigation methods. Right now I've removed them and the annotation for the super call, but left the implementations. How should those be handled? We can make them abstract, but RootActivity will then need noop implementations (i think, i'm unsure on that one).

@guyca
Copy link
Copy Markdown
Collaborator

guyca commented Jun 30, 2016

Thanks @jskuby! I'll check it out, hopefully later today

… into android_rotation_fix

# Conflicts:
#	android/app/src/main/java/com/reactnativenavigation/activities/BottomTabActivity.java
@ronaldheft
Copy link
Copy Markdown

Any ETA on the merge?

@ronaldheft
Copy link
Copy Markdown

@jskuby What issues were you having with rotation events not firing? I've pulled this change into my fork, and I'm trying to use react-native-orientation to lock my app in portrait. I'm not seeing react-native-orientation pick up the rotation events.

@ronaldheft
Copy link
Copy Markdown

^ For anyone experiencing the issues I described above, I figured it out. ^

The issue is the hierarchy of class extensions. React Native's MainActivity extends RootActivity which extends BaseReactActivity. When React Native Navigation starts up, it change's the app's Activity from MainActivity to either BottomTabActivity or SingleScreenActivity. The issue is both of those classes extend BaseReactActivity directly, so any listeners you place in RootActivity will not be firing since that activity is no longer running.

I ended up adding the listener needed for the orientation module to my fork directly in BaseReactActivity. I also added android:screenOrientation="portrait" to React Native Navigation's AndroidManifest.xml for the various activities.

Thanks @jskuby for the original code that got me started on figuring out what in the hell is going on.

@drorbiran
Copy link
Copy Markdown
Contributor

Hi,
Unfortunately, we can't merge your PR.
We will address this issue in the near future. You can follow #258

@drorbiran drorbiran closed this Aug 28, 2016
@dg92
Copy link
Copy Markdown

dg92 commented Oct 18, 2016

what is the solution ?

@guyca
Copy link
Copy Markdown
Collaborator

guyca commented Oct 18, 2016

@dg92 For now, I suggest you lock you app to portrait mode by adding portraitOnlyMode=true property when calling startApp

  Navigation.startSingleScreenApp({
    screen: {
      screen: 'core.LoginScreen',
      title: 'Login'
    },
    portraitOnlyMode: true
  })

@dg92
Copy link
Copy Markdown

dg92 commented Oct 18, 2016

@guyca no its not working for me.
case actionTypes.app.screens.login: Navigation.startSingleScreenApp({ screen: { screen: 'auth.Login', navigatorStyle: navBarStyle, passProps: action.payload.props }, portraitOnlyMode: true }); return {...state, currentScreen: 'login'};

@guyca
Copy link
Copy Markdown
Collaborator

guyca commented Oct 18, 2016

@dg92 portraitOnlyMode works only on Android for now and is available in the experimental branch. If you're using the stable 1.30 version you'll need to upgrade

@dg92
Copy link
Copy Markdown

dg92 commented Oct 18, 2016

@guyca using "react-native-navigation": "^2.0.0-experimental.105", and checking on android 4.4.4

@guyca
Copy link
Copy Markdown
Collaborator

guyca commented Oct 18, 2016

@dg92 portraitOnlyMode was added in experimental 125 if i'm not mistaken

@vivekparekh8
Copy link
Copy Markdown

vivekparekh8 commented Nov 1, 2016

@ronaldheft My scenario : whole app is portrait using rnn and one of the screens is landscape. so I am using react-native-orientation. My code for landscape screen

componentWillMount(){
Orientation.lockToLandscape()
}

For me, whole app restarts on doing this
Can you help.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants