From 40d825aada21ef712e948a2d737c82f59566aa61 Mon Sep 17 00:00:00 2001 From: Guy Carmeli Date: Mon, 23 Mar 2020 12:19:49 +0200 Subject: [PATCH] Fix title component not being replaced via mergeOptions (#6066) This commit fixes a bug related to setting a new react component as title via mergeOptions. When a title component was set, and the user tried to replace the existing title with a new title via mergeOptions, the new title was not created and instead the current title was reapplied. Fixes #5377 --- .../presentation/StackPresenter.java | 33 +++++++---- .../TitleBarReactViewController.java | 13 +++-- .../topbar/TopBarController.java | 5 ++ .../views/titlebar/TitleBar.java | 4 +- .../utils/TitleBarHelper.java | 7 +++ .../viewcontrollers/StackPresenterTest.java | 58 ++++++++++++++++++- .../TitleBarReactViewControllerTest.java | 10 ++-- .../viewcontrollers/TitleBarTest.java | 11 +++- playground/src/commons/Options.js | 1 - 9 files changed, 114 insertions(+), 28 deletions(-) diff --git a/lib/android/app/src/main/java/com/reactnativenavigation/presentation/StackPresenter.java b/lib/android/app/src/main/java/com/reactnativenavigation/presentation/StackPresenter.java index 46ad2d97ca9..ea0f2abd2d4 100644 --- a/lib/android/app/src/main/java/com/reactnativenavigation/presentation/StackPresenter.java +++ b/lib/android/app/src/main/java/com/reactnativenavigation/presentation/StackPresenter.java @@ -177,14 +177,13 @@ private void applyTopBarOptions(Options options, StackController stack, ViewCont if (topBarOptions.title.component.hasValue()) { if (titleControllers.containsKey(component)) { - topBar.setTitleComponent(titleControllers.get(component).getView()); + topBarController.setTitleComponent(titleControllers.get(component)); } else { - TitleBarReactViewController controller = new TitleBarReactViewController(activity, titleViewCreator); + TitleBarReactViewController controller = new TitleBarReactViewController(activity, titleViewCreator, topBarOptions.title.component); controller.setWaitForRender(topBarOptions.title.component.waitForRender); titleControllers.put(component, controller); - controller.setComponent(topBarOptions.title.component); controller.getView().setLayoutParams(getComponentLayoutParams(topBarOptions.title.component)); - topBar.setTitleComponent(controller.getView()); + topBarController.setTitleComponent(controller); } } @@ -415,19 +414,19 @@ private void mergeTopBarOptions(Options options, StackController stack, ViewCont } if (topBarOptions.title.height.hasValue()) topBar.setTitleHeight(topBarOptions.title.height.get()); - if (topBarOptions.title.text.hasValue()) topBar.setTitle(topBarOptions.title.text.get()); if (topBarOptions.title.topMargin.hasValue()) topBar.setTitleTopMargin(topBarOptions.title.topMargin.get()); if (topBarOptions.title.component.hasValue()) { - if (titleControllers.containsKey(component)) { - topBar.setTitleComponent(titleControllers.get(component).getView()); - } else { - TitleBarReactViewController controller = new TitleBarReactViewController(activity, titleViewCreator); - titleControllers.put(component, controller); - controller.setComponent(topBarOptions.title.component); + TitleBarReactViewController controller = findTitleComponent(topBarOptions.title.component); + if (controller == null) { + controller = new TitleBarReactViewController(activity, titleViewCreator, topBarOptions.title.component); + perform(titleControllers.put(component, controller), ViewController::destroy); controller.getView().setLayoutParams(getComponentLayoutParams(topBarOptions.title.component)); - topBar.setTitleComponent(controller.getView()); } + topBarController.setTitleComponent(controller); + } else if (topBarOptions.title.text.hasValue()) { + perform(titleControllers.remove(component), ViewController::destroy); + topBar.setTitle(topBarOptions.title.text.get()); } if (topBarOptions.title.color.hasValue()) topBar.setTitleTextColor(topBarOptions.title.color.get()); @@ -478,6 +477,16 @@ private void mergeTopBarOptions(Options options, StackController stack, ViewCont } } + private TitleBarReactViewController findTitleComponent(com.reactnativenavigation.parse.Component component) { + for (TitleBarReactViewController controller : titleControllers.values()) { + if (ObjectUtils.equalsNotNull(controller.getComponent().name.get(null), component.name.get(null)) && + ObjectUtils.equalsNotNull(controller.getComponent().componentId.get(null), component.componentId.get(null))) { + return controller; + } + } + return null; + } + private void mergeTopTabsOptions(TopTabsOptions options) { if (options.selectedTabColor.hasValue() && options.unselectedTabColor.hasValue()) topBar.applyTopTabsColors(options.selectedTabColor, options.unselectedTabColor); if (options.fontSize.hasValue()) topBar.applyTopTabsFontSize(options.fontSize); diff --git a/lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/TitleBarReactViewController.java b/lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/TitleBarReactViewController.java index 6016d624f12..f24dbb4e900 100644 --- a/lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/TitleBarReactViewController.java +++ b/lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/TitleBarReactViewController.java @@ -13,11 +13,16 @@ public class TitleBarReactViewController extends ViewController { private final TitleBarReactViewCreator reactViewCreator; - private Component component; + private final Component component; - public TitleBarReactViewController(Activity activity, TitleBarReactViewCreator reactViewCreator) { + public Component getComponent() { + return component; + } + + public TitleBarReactViewController(Activity activity, TitleBarReactViewCreator reactViewCreator, Component component) { super(activity, CompatUtils.generateViewId() + "", new YellowBoxDelegate(), new Options(), new ViewControllerOverlay(activity)); this.reactViewCreator = reactViewCreator; + this.component = component; } @Override @@ -49,8 +54,4 @@ public void sendOnNavigationButtonPressed(String buttonId) { public String getCurrentComponentName() { return null; } - - public void setComponent(Component component) { - this.component = component; - } } diff --git a/lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/topbar/TopBarController.java b/lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/topbar/TopBarController.java index 416e0145533..9c108794d1a 100644 --- a/lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/topbar/TopBarController.java +++ b/lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/topbar/TopBarController.java @@ -6,6 +6,7 @@ import com.reactnativenavigation.anim.TopBarAnimator; import com.reactnativenavigation.parse.AnimationOptions; import com.reactnativenavigation.utils.CompatUtils; +import com.reactnativenavigation.viewcontrollers.TitleBarReactViewController; import com.reactnativenavigation.views.StackLayout; import com.reactnativenavigation.views.topbar.TopBar; @@ -93,4 +94,8 @@ public void resetViewProperties() { topBar.setRotationY(0); topBar.setRotation(0); } + + public void setTitleComponent(TitleBarReactViewController component) { + topBar.setTitleComponent(component.getView()); + } } diff --git a/lib/android/app/src/main/java/com/reactnativenavigation/views/titlebar/TitleBar.java b/lib/android/app/src/main/java/com/reactnativenavigation/views/titlebar/TitleBar.java index 1db3a4338c0..a3e48f0d9b4 100644 --- a/lib/android/app/src/main/java/com/reactnativenavigation/views/titlebar/TitleBar.java +++ b/lib/android/app/src/main/java/com/reactnativenavigation/views/titlebar/TitleBar.java @@ -29,6 +29,7 @@ import static com.reactnativenavigation.utils.UiUtils.runOnPreDrawOnce; import static com.reactnativenavigation.utils.ViewUtils.findChildByClass; import static com.reactnativenavigation.utils.ViewUtils.findChildrenByClass; +import static com.reactnativenavigation.utils.ViewUtils.removeFromParent; @SuppressLint("ViewConstructor") public class TitleBar extends Toolbar { @@ -70,6 +71,7 @@ public String getTitle() { } public void setComponent(View component) { + if (this.component == component) return; clearTitle(); clearSubtitle(); this.component = component; @@ -175,7 +177,7 @@ private void clearSubtitle() { private void clearComponent() { if (component != null) { - removeView(component); + removeFromParent(component); component = null; } } diff --git a/lib/android/app/src/test/java/com/reactnativenavigation/utils/TitleBarHelper.java b/lib/android/app/src/test/java/com/reactnativenavigation/utils/TitleBarHelper.java index 41577b291fe..e2ecaa614fb 100644 --- a/lib/android/app/src/test/java/com/reactnativenavigation/utils/TitleBarHelper.java +++ b/lib/android/app/src/test/java/com/reactnativenavigation/utils/TitleBarHelper.java @@ -35,6 +35,13 @@ public static Button reactViewButton(String name) { return button; } + public static Component titleComponent(String componentId) { + Component component = new Component(); + component.componentId = new Text(componentId); + component.name = new Text(componentId); + return component; + } + public static Button iconButton(String id, String icon) { Button button = new Button(); button.id = "someButton"; diff --git a/lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/StackPresenterTest.java b/lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/StackPresenterTest.java index 2e51591f857..c5e7911f3e0 100644 --- a/lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/StackPresenterTest.java +++ b/lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/StackPresenterTest.java @@ -84,9 +84,10 @@ public class StackPresenterTest extends BaseTest { private Button textBtn2 = TitleBarHelper.textualButton("btn2"); private Button componentBtn1 = TitleBarHelper.reactViewButton("btn1_"); private Button componentBtn2 = TitleBarHelper.reactViewButton("btn2_"); + private Component titleComponent1 = TitleBarHelper.titleComponent("component1"); + private Component titleComponent2 = TitleBarHelper.titleComponent("component2"); private TopBarController topBarController; private ChildControllersRegistry childRegistry; - private IconResolver iconResolver; @Override public void beforeEach() { @@ -98,7 +99,7 @@ public TitleBarReactView create(Activity activity, String componentId, String co } }; renderChecker = spy(new RenderChecker()); - iconResolver = new IconResolver(activity, ImageLoaderMock.mock()); + IconResolver iconResolver = new IconResolver(activity, ImageLoaderMock.mock()); uut = spy(new StackPresenter(activity, titleViewCreator, new TopBarBackgroundViewCreatorMock(), new TopBarButtonCreatorMock(), iconResolver, renderChecker, new Options())); createTopBarController(); @@ -309,6 +310,59 @@ public void mergeTopBarOptions() { uut.mergeChildOptions(options, EMPTY_OPTIONS, parent, child); } + @Test + public void applyTopBarOptions_setTitleComponent() { + Options applyComponent = new Options(); + applyComponent.topBar.title.component.name = new Text("Component1"); + applyComponent.topBar.title.component.componentId = new Text("Component1id"); + uut.applyChildOptions(applyComponent, parent, child); + verify(topBarController).setTitleComponent(any()); + } + + @Test + public void mergeTopBarOptions_settingTitleDestroysComponent() { + Options componentOptions = new Options(); + componentOptions.topBar.title.component = titleComponent1; + uut.applyChildOptions(componentOptions, parent, child); + ArgumentCaptor applyCaptor = ArgumentCaptor.forClass(TitleBarReactViewController.class); + verify(topBarController).setTitleComponent(applyCaptor.capture()); + + Options titleOptions = new Options(); + titleOptions.topBar.title.text = new Text("Some title"); + uut.mergeChildOptions(titleOptions, Options.EMPTY, parent, child); + assertThat(applyCaptor.getValue().isDestroyed()).isTrue(); + } + + @Test + public void mergeTopBarOptions_doesNotRecreateTitleComponentIfEquals() { + Options options = new Options(); + options.topBar.title.component = titleComponent1; + uut.applyChildOptions(options, parent, child); + ArgumentCaptor applyCaptor = ArgumentCaptor.forClass(TitleBarReactViewController.class); + verify(topBarController).setTitleComponent(applyCaptor.capture()); + + uut.mergeChildOptions(options, Options.EMPTY, parent, child); + verify(topBarController, times(2)).setTitleComponent(applyCaptor.getValue()); + } + + @Test + public void mergeTopBarOptions_previousTitleComponentIsDestroyed() { + Options options = new Options(); + options.topBar.title.component = titleComponent1; + uut.applyChildOptions(options, parent, child); + ArgumentCaptor applyCaptor = ArgumentCaptor.forClass(TitleBarReactViewController.class); + verify(topBarController).setTitleComponent(applyCaptor.capture()); + + Options toMerge = new Options(); + toMerge.topBar.title.component = titleComponent2; + uut.mergeChildOptions(toMerge, Options.EMPTY, parent, child); + ArgumentCaptor mergeCaptor = ArgumentCaptor.forClass(TitleBarReactViewController.class); + verify(topBarController, times(2)).setTitleComponent(mergeCaptor.capture()); + + assertThat(applyCaptor.getValue()).isNotEqualTo(mergeCaptor.getValue()); + assertThat(applyCaptor.getValue().isDestroyed()).isTrue(); + } + @Test public void mergeTopTabsOptions() { Options options = new Options(); diff --git a/lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/TitleBarReactViewControllerTest.java b/lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/TitleBarReactViewControllerTest.java index 63ba38bbe58..747736496ff 100644 --- a/lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/TitleBarReactViewControllerTest.java +++ b/lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/TitleBarReactViewControllerTest.java @@ -23,9 +23,8 @@ public class TitleBarReactViewControllerTest extends BaseTest { public void beforeEach() { viewCreator = spy(new TitleBarReactViewCreatorMock()); activity = newActivity(); - uut = new TitleBarReactViewController(activity, viewCreator); - createComponent(); - uut.setComponent(component); + component = createComponent(); + uut = new TitleBarReactViewController(activity, viewCreator, component); } @Test @@ -34,9 +33,10 @@ public void createView() { verify(viewCreator).create(activity, component.componentId.get(), component.name.get()); } - private void createComponent() { - component = new Component(); + private Component createComponent() { + Component component = new Component(); component.componentId = new Text("compId"); component.name = new Text("compName"); + return component; } } diff --git a/lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/TitleBarTest.java b/lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/TitleBarTest.java index 5fede2d54e5..55c1b0b3491 100644 --- a/lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/TitleBarTest.java +++ b/lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/TitleBarTest.java @@ -124,6 +124,15 @@ public void setComponent_addsComponentToTitleBar() { verify(uut).addView(component); } + @Test + public void setComponent_doesNothingIfComponentIsAlreadyAdded() { + View component = new View(activity); + uut.setComponent(component); + + uut.setComponent(component); + verify(uut).addView(component); + } + @Test public void addView_overflowIsEnabledForButtonsContainer() { ActionMenuView buttonsContainer = mock(ActionMenuView.class); @@ -141,7 +150,7 @@ public void clear() { assertThat(uut.getTitle()).isNullOrEmpty(); assertThat(uut.getMenu().size()).isZero(); assertThat(uut.getNavigationIcon()).isNull(); - verify(uut).removeView(title); + assertThat(title.getParent()).isNull(); } @Test diff --git a/playground/src/commons/Options.js b/playground/src/commons/Options.js index 9e463bb175b..6d837d906e9 100644 --- a/playground/src/commons/Options.js +++ b/playground/src/commons/Options.js @@ -3,7 +3,6 @@ const Colors = require('./Colors'); const { Dimensions } = require('react-native'); const height = Math.round(Dimensions.get('window').height); const width = Math.round(Dimensions.get('window').width); -console.log('guyca', `height: ${height} width: ${width}`); const { useCustomAnimations, useSlowOpenScreenAnimations,