feat: android orientation management #679

WoLewicki · 2020-10-22T11:31:26Z

Description

SC will stand for ScreenContainer. Added orientation management on Android. On every update of native-stack or navigators with using SC we check if there is orientation change needed. We do it by checking the top Screen and, if there is no child HeaderConfig (which has the priority) and there is a HeaderConfig above the Screen, changing the orientation according to it. If there are no child SCs in the Screen in onContainerUpdate even though the Screen has a child SC, it means that the child has not fired onAttachedToWindow, where the registration of child SC happens. It is not a problem, because then the correct update of orientation will be fired in onAttachedToWindow of the child Screen HeaderConfig, if it exists.

Changes

Added getHeaderConfig for unified checking for if Screen has a header
Abstracted updateChildFragments method for updating SC/ScreenStack children on update
Added hasChildScreenWithConfig that parses child SCs and checks if any of their Screens have HeaderConfig

Test code and steps to reproduce

Test42.js in TestsExample project.

Checklist

Included code example that can be used to test this change
Updated TS types
Updated documentation:
Ensured that CI passes

kmagiera

Accepting to unblock but I'm a bit hesitant of the code that does view hierarchy traversal. I don't understand why is it necessary – change events are fired at all levels when new screen appear, no?

Lets maybe discuss that last issue over call. Looks good otherwise.

kmagiera · 2020-12-15T11:41:52Z

android/src/main/java/com/swmansion/rnscreens/ScreenFragment.java

+    return null;
+  }
+
+  protected static boolean checkIfChildWithConfig(View view) {


rename this to make it more descriptive. Do we really want to traverse all view hierarchy? Maybe it should be enought to only traverse fragments hierarchy?

kmagiera · 2020-12-15T11:43:33Z

android/src/main/java/com/swmansion/rnscreens/ScreenStackHeaderConfig.java

    }

+    // orientation
+    if (!ScreenFragment.checkIfChildWithConfig(getScreen() != null ? getScreen().getChildAt(1) : null)) {


This look weird. What's the point of calling checkIfChildWithConfig(null)? Does it have side effect? I also commented that the name of this method isn't really very informative. Also its role isn't very clear by looking at that name neither by looking at the way we use it in different places.

kmagiera · 2020-12-15T11:44:31Z

android/src/main/java/com/swmansion/rnscreens/ScreenStackHeaderConfig.java

    return null;
  }

+  public int getScreenOrientation(){


Suggested change

public int getScreenOrientation(){

public int getScreenOrientation() {

kmagiera · 2020-12-17T08:54:02Z

android/src/main/java/com/swmansion/rnscreens/ScreenStackFragment.java

-  public void onStackUpdate() {
+  @Override
+  public void onContainerUpdate() {
    View child = mScreenView.getChildAt(0);


call super?

android/src/main/java/com/swmansion/rnscreens/ScreenContainer.java

android/src/main/java/com/swmansion/rnscreens/ScreenFragment.java

kmagiera

Left a few more comments inline. sorry for delaying this but I think we are really close.

kmagiera · 2021-01-07T13:24:08Z

android/src/main/java/com/swmansion/rnscreens/ScreenContainer.java

    tryCommitTransaction();
  }
+
+  protected void updateChildFragments() {


There is nothing that we update in this method. maybe rename to notifyOnUpdate or notifyContainerUpdate or sth else along these lines

android/src/main/java/com/swmansion/rnscreens/ScreenStackHeaderConfig.java

kmagiera · 2021-01-07T13:32:00Z

android/src/main/java/com/swmansion/rnscreens/ScreenFragment.java

+    ScreenStackHeaderConfig config = findHeaderConfig();
+    boolean configInChild = hasChildScreenWithConfig(getScreen());
+    if (!configInChild && config != null && config.getScreenFragment().getActivity() != null) {
+      // if there is no child with config, we look for a parent with config to set the orientation


I'm still a bit confused by this. Why we check if children have config while findHeaderConfig searches for the config in current container or in its parents? If it isn't trivial to explain maybe we can discuss over call and figure out best way to describe it here.

Let's have a structure of ScreenStack -> ScreenContainer -> ScreenStack. We do this check in case the update of the container was triggered after setting the orientation in the child stack, which would result in setting the orientation of the parent ScreenStack even though the child ScreenStack should have priority. At the same time, we want to have an option to change the orientation on every update of container, because the middle ScreenContainer can have children that are not ScreenStacks, so after navigating from a child that is a ScreenStack to a child that is not, we want the orientation to be taken and applied from the parent ScreenStack.

Is this explanation clear enough?

Also, it is probably good to change the code to only look for the parent Screen with config if there is no child with config to avoid traversing the hierarchy when not needed.

kmagiera · 2021-01-07T13:34:25Z

android/src/main/java/com/swmansion/rnscreens/ScreenFragment.java

+    for (ScreenContainer sc : screen.getFragment().getChildScreenContainers()) {
+      if (sc instanceof ScreenStack) {
+        // we check only the top screen of stack for header config
+        Screen topScreen = ((ScreenStack) sc).getTopScreen();


We may already be doing this in othre places but it struct me that we refer to subclasses (stack) from places that operate on a lower level of abstraction (screen) . Maybe we can add some interface method to screen container such that we can avoid testing against the type here.

I think we don't do it in other places. I can add getTopScreen method to ScreenContainer that will return the Screen with ActivityState.ON_TOP.

kmagiera

Accepting this to ublock. Code looks mostly fine, I left a few more comments.

android/src/main/java/com/swmansion/rnscreens/ScreenContainer.java

kmagiera · 2021-01-19T14:43:48Z

android/src/main/java/com/swmansion/rnscreens/ScreenContainer.java

    mFragmentManager.executePendingTransactions();

    performUpdate();
+    notifyContainerUpdate();


shouldn't this be called directly from perfomUpdate? Are there any other ways performUpdate method is called when we don't want notify to be triggered? If not why?

I think not. I added this here since we call it both in ScreenContainer and ScreenStack this way and if it is in perfomUpdate, then we need to use this method in both classes. I don't have a strong opinion on this.

android/src/main/java/com/swmansion/rnscreens/ScreenStack.java

kmagiera · 2021-01-19T14:52:30Z

android/src/main/java/com/swmansion/rnscreens/ScreenFragment.java

+    return null;
+  }
+
+  protected boolean hasChildScreenWithConfig(Screen screen) {


Maybe it'd be worth keeping track of whether there are children with config upon container updates. It seem like this parameter can change when we add new children or when the "top screen changes". Maybe we could add some bottom-up notification mechanism that'd be triggered when such an even occur that'd allow to keep the up-to-date information whether there are children with config instead of doing this traversal which potentially runs on every level in screen container hierarchy and runs the same code for the same container instances multiple times.

kmagiera · 2021-01-25T14:13:58Z

android/src/main/java/com/swmansion/rnscreens/ScreenFragment.java

+    ViewParent parent = getScreen().getContainer();
+    while (parent != null) {
+      if (parent instanceof Screen) {
+        ((Screen) parent).getFragment().mHasChildWithConfig = hasChildWithConfig;


If this does not result in mHasChildWithConfig change there is no point in updating other parents up the view hierarchy (there will be no updates as well) – we can break the loop if this occurs

kmagiera · 2021-01-25T14:16:53Z

android/src/main/java/com/swmansion/rnscreens/ScreenFragment.java

+    return false;
+  }
+
+  protected void markParentFragments(boolean hasChildWithConfig) {


I don't think this is correct. If there is one childrent with config and one without, then when the second one updates, it will send information to the parent that there is no config (but the first childrent still has config).

I think that instead this method should actually check flags with all its children (not recursively) and then if the new value for the flag is different than the previous one also notify its parent

SC will stand for ScreenContainer. Added orientation management on Android. On every update of native-stack or navigators with using SC we check if there is orientation change needed. We do it by checking the top Screen and, if there is no child HeaderConfig (which has the priority) and there is a HeaderConfig above the Screen, changing the orientation according to it. If there are no child SCs in the Screen in onContainerUpdate even though the Screen has a child SC, it means that the child has not fired onAttachedToWindow, where the registration of child SC happens. It is not a problem, because then the correct update of orientation will be fired in onAttachedToWindow of the child Screen HeaderConfig, if it exists.

WoLewicki mentioned this pull request Oct 22, 2020

feat: Android orientation management #655

Closed

kmagiera approved these changes Dec 15, 2020

View reviewed changes

WoLewicki force-pushed the @wolewicki/android-orientation branch from 70c1380 to 4043353 Compare December 16, 2020 14:07

kmagiera reviewed Dec 17, 2020

View reviewed changes

WoLewicki added 8 commits December 22, 2020 12:02

feat: android orientation management

85186e9

fix: restore missing prop

12843d6

fix: indent

cd74246

feat: try to update orientation on every container update

66643ad

feat: change logic for nested stacks

7743d57

feat: rebase and add info

62a33fc

feat: traverse through ScreenContainers

04d365f

feat: add version with all checks

beb5f06

WoLewicki force-pushed the @wolewicki/android-orientation branch from e8578cc to beb5f06 Compare December 22, 2020 11:02

WoLewicki added 4 commits December 22, 2020 12:07

fix: add prop to main README

c8492ac

feat: add working solution with SCs traversing

690f2da

fix: remove not needed change in SC

40379fd

fix: make sure we don't cause NPE

46e73da

jgonet reviewed Dec 23, 2020

View reviewed changes

android/src/main/java/com/swmansion/rnscreens/ScreenContainer.java Outdated Show resolved Hide resolved

android/src/main/java/com/swmansion/rnscreens/ScreenFragment.java Outdated Show resolved Hide resolved

kmagiera reviewed Jan 7, 2021

View reviewed changes

WoLewicki added 2 commits January 11, 2021 13:57

feat: add requested changes and better traversing

b26a29a

fix: restore comments

e240462

kmagiera approved these changes Jan 19, 2021

View reviewed changes

WoLewicki added 2 commits January 20, 2021 18:17

fix: add nullable next to return type

f93592c

feat: add version with keeping track of updates

ddaab72

kmagiera reviewed Jan 25, 2021

View reviewed changes

feat: revert previous commit

0b0ad91

WoLewicki merged commit bcd5e4d into master Jan 25, 2021

WoLewicki deleted the @wolewicki/android-orientation branch January 25, 2021 14:52

	public int getScreenOrientation(){
	public int getScreenOrientation() {

Uh oh!

feat: android orientation management #679

feat: android orientation management #679

Uh oh!

Conversation

WoLewicki commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Test code and steps to reproduce

Checklist

Uh oh!

kmagiera left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kmagiera left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmagiera left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

WoLewicki commented Oct 22, 2020 •

edited

Loading