From 42a6917eeee149f7348a4eaf524ba76bac1240cf Mon Sep 17 00:00:00 2001 From: Guy Carmeli Date: Wed, 1 Apr 2020 16:53:37 +0300 Subject: [PATCH] Refactor right buttons (#6090) Refactor right buttons This pr started with an attempt to eliminate button flickering entirely when updating buttons which contain react components via mergeOptions. While the attempt wasn't 100% successful, I dit get some insights in the process. * It's not possible to add buttons at specific indices. Buttons contain an order property which determines the order of the button in the TopBar. To somewhat overcome this limitation, we can let users control button order via options. * When a right most component button is replaced with another component button, the rest of the buttons shift to the right since the newly added component isn't measured when it's added to the menu and its width is zero. We usually handle this situation with the `waitForRender` option but since buttons are measured with `MeasureSpec.UNSPECIFIED`, their dimensions are zero. Because of this, I've added options to set button dimensions. * When updating buttons via mergeOptions, if a component button is already added to the menu with the same order we will not remove and added it again. This mitigates flickering in some situations. * Textual button style properties were applied by traversing the view hierarchy and searching for the TextView corresponding to the button and updating its styles directly. There was an inherent bug in this logic where if two buttons contained the same text, styles could have been applied to the wrong TextView. We now apply styles directly on the button using spans. --- .../parse/Component.java | 15 +- .../parse/params/Button.java | 17 ++- .../parse/params/Text.java | 4 + .../presentation/StackPresenter.java | 101 ++++++------ .../utils/ButtonPresenter.java | 72 +-------- .../reactnativenavigation/utils/ButtonSpan.kt | 27 ++++ .../utils/CollectionUtils.java | 44 +++++- .../utils/Functions.java | 4 + .../reactnativenavigation/utils/IdFactory.kt | 17 +++ .../utils/ObjectUtils.java | 4 + .../TitleBarButtonController.java | 47 ++++-- .../topbar/TopBarController.java | 41 ++++- .../views/titlebar/TitleBar.java | 51 +++++-- .../views/titlebar/TitleBarButtonCreator.java | 9 +- .../titlebar/TitleBarReactButtonView.java | 23 ++- .../views/topbar/TopBar.java | 39 +++-- .../com/reactnativenavigation/TestUtils.java | 4 +- ...ck.java => TitleBarButtonCreatorMock.java} | 13 +- .../utils/TitleBarHelper.java | 16 +- .../viewcontrollers/StackPresenterTest.java | 100 +++++++----- .../TitleBarButtonControllerTest.java | 61 ++++++++ .../viewcontrollers/TitleBarTest.java | 77 ---------- .../TopBarButtonControllerTest.java | 73 +-------- .../viewcontrollers/TopBarControllerTest.java | 144 ++++++++++++++++++ .../stack/StackControllerTest.java | 4 +- lib/src/interfaces/Options.ts | 8 + playground/src/screens/OptionsScreen.js | 10 +- playground/src/screens/RoundedButton.js | 3 +- playground/src/testIDs.js | 2 + 29 files changed, 652 insertions(+), 378 deletions(-) create mode 100644 lib/android/app/src/main/java/com/reactnativenavigation/utils/ButtonSpan.kt create mode 100644 lib/android/app/src/main/java/com/reactnativenavigation/utils/IdFactory.kt rename lib/android/app/src/test/java/com/reactnativenavigation/mocks/{TopBarButtonCreatorMock.java => TitleBarButtonCreatorMock.java} (65%) create mode 100644 lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/TitleBarButtonControllerTest.java create mode 100644 lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/TopBarControllerTest.java diff --git a/lib/android/app/src/main/java/com/reactnativenavigation/parse/Component.java b/lib/android/app/src/main/java/com/reactnativenavigation/parse/Component.java index 854585ecf03..0b39cf7b2f3 100644 --- a/lib/android/app/src/main/java/com/reactnativenavigation/parse/Component.java +++ b/lib/android/app/src/main/java/com/reactnativenavigation/parse/Component.java @@ -2,9 +2,12 @@ import com.reactnativenavigation.parse.params.Bool; import com.reactnativenavigation.parse.params.NullBool; +import com.reactnativenavigation.parse.params.NullNumber; import com.reactnativenavigation.parse.params.NullText; +import com.reactnativenavigation.parse.params.Number; import com.reactnativenavigation.parse.params.Text; import com.reactnativenavigation.parse.parsers.BoolParser; +import com.reactnativenavigation.parse.parsers.NumberParser; import com.reactnativenavigation.parse.parsers.TextParser; import org.json.JSONObject; @@ -18,6 +21,8 @@ public static Component parse(JSONObject json) { result.componentId = TextParser.parse(json, "componentId"); result.alignment = Alignment.fromString(TextParser.parse(json, "alignment").get("")); result.waitForRender = BoolParser.parse(json, "waitForRender"); + result.width = NumberParser.parse(json, "width"); + result.height = NumberParser.parse(json, "height"); return result; } @@ -26,12 +31,16 @@ public static Component parse(JSONObject json) { public Text componentId = new NullText(); public Alignment alignment = Alignment.Default; public Bool waitForRender = new NullBool(); + public Number width = new NullNumber(); + public Number height = new NullNumber(); void mergeWith(Component other) { if (other.componentId.hasValue()) componentId = other.componentId; if (other.name.hasValue()) name = other.name; if (other.waitForRender.hasValue()) waitForRender = other.waitForRender; if (other.alignment != Alignment.Default) alignment = other.alignment; + if (other.width.hasValue()) width = other.width; + if (other.height.hasValue()) height = other.height; } public void mergeWithDefault(Component defaultOptions) { @@ -39,6 +48,8 @@ public void mergeWithDefault(Component defaultOptions) { if (!name.hasValue()) name = defaultOptions.name; if (!waitForRender.hasValue()) waitForRender = defaultOptions.waitForRender; if (alignment == Alignment.Default) alignment = defaultOptions.alignment; + if (!width.hasValue()) width = defaultOptions.width; + if (!height.hasValue()) height = defaultOptions.height; } public boolean hasValue() { @@ -49,6 +60,8 @@ public boolean equals(Component other) { return name.equals(other.name) && componentId.equals(other.componentId) && alignment.equals(other.alignment) && - waitForRender.equals(other.waitForRender); + waitForRender.equals(other.waitForRender) && + width.equals(other.width) && + height.equals(other.height); } } diff --git a/lib/android/app/src/main/java/com/reactnativenavigation/parse/params/Button.java b/lib/android/app/src/main/java/com/reactnativenavigation/parse/params/Button.java index 7f40d9fb29e..a119b03a980 100644 --- a/lib/android/app/src/main/java/com/reactnativenavigation/parse/params/Button.java +++ b/lib/android/app/src/main/java/com/reactnativenavigation/parse/params/Button.java @@ -6,9 +6,10 @@ import com.reactnativenavigation.parse.Component; import com.reactnativenavigation.parse.parsers.BoolParser; import com.reactnativenavigation.parse.parsers.ColorParser; -import com.reactnativenavigation.parse.parsers.NumberParser; +import com.reactnativenavigation.parse.parsers.FractionParser; import com.reactnativenavigation.parse.parsers.TextParser; import com.reactnativenavigation.utils.CompatUtils; +import com.reactnativenavigation.utils.IdFactory; import com.reactnativenavigation.utils.TypefaceLoader; import org.json.JSONArray; @@ -19,10 +20,12 @@ import androidx.annotation.Nullable; +import static com.reactnativenavigation.utils.ObjectUtils.take; + public class Button { public String instanceId = "btn" + CompatUtils.generateViewId(); - @Nullable public String id; + public String id = "btn" + CompatUtils.generateViewId(); public Text accessibilityLabel = new NullText(); public Text text = new NullText(); public Bool enabled = new NullBool(); @@ -30,7 +33,7 @@ public class Button { public Number showAsAction = new NullNumber(); public Colour color = new NullColor(); public Colour disabledColor = new NullColor(); - public Number fontSize = new NullNumber(); + public Fraction fontSize = new NullFraction(); private Text fontWeight = new NullText(); @Nullable public Typeface fontFamily; public Text icon = new NullText(); @@ -56,7 +59,7 @@ public boolean equals(Button other) { private static Button parseJson(JSONObject json, TypefaceLoader typefaceManager) { Button button = new Button(); - button.id = json.optString("id"); + button.id = take(json.optString("id"), "btn" + CompatUtils.generateViewId()); button.accessibilityLabel = TextParser.parse(json, "accessibilityLabel"); button.text = TextParser.parse(json, "text"); button.enabled = BoolParser.parse(json, "enabled"); @@ -64,7 +67,7 @@ private static Button parseJson(JSONObject json, TypefaceLoader typefaceManager) button.showAsAction = parseShowAsAction(json); button.color = ColorParser.parse(json, "color"); button.disabledColor = ColorParser.parse(json, "disabledColor"); - button.fontSize = NumberParser.parse(json, "fontSize"); + button.fontSize = FractionParser.parse(json, "fontSize"); button.fontFamily = typefaceManager.getTypeFace(json.optString("fontFamily", "")); button.fontWeight = TextParser.parse(json, "fontWeight"); button.testId = TextParser.parse(json, "testID"); @@ -116,6 +119,10 @@ public boolean hasIcon() { return icon.hasValue(); } + public int getIntId() { + return IdFactory.Companion.get(component.componentId.get(id)); + } + private static Number parseShowAsAction(JSONObject json) { final Text showAsAction = TextParser.parse(json, "showAsAction"); if (!showAsAction.hasValue()) { diff --git a/lib/android/app/src/main/java/com/reactnativenavigation/parse/params/Text.java b/lib/android/app/src/main/java/com/reactnativenavigation/parse/params/Text.java index e8b996c6775..a179d96fadb 100644 --- a/lib/android/app/src/main/java/com/reactnativenavigation/parse/params/Text.java +++ b/lib/android/app/src/main/java/com/reactnativenavigation/parse/params/Text.java @@ -7,6 +7,10 @@ public Text(String value) { super(value); } + public int length() { + return hasValue() ? value.length() : 0; + } + @NonNull @Override public String toString() { 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 ea0f2abd2d4..6bd784164fd 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 @@ -25,7 +25,6 @@ import com.reactnativenavigation.utils.StatusBarUtils; import com.reactnativenavigation.utils.UiUtils; import com.reactnativenavigation.viewcontrollers.IReactView; -import com.reactnativenavigation.viewcontrollers.ReactViewCreator; import com.reactnativenavigation.viewcontrollers.TitleBarButtonController; import com.reactnativenavigation.viewcontrollers.TitleBarReactViewController; import com.reactnativenavigation.viewcontrollers.ViewController; @@ -33,6 +32,7 @@ import com.reactnativenavigation.viewcontrollers.stack.StackController; import com.reactnativenavigation.viewcontrollers.topbar.TopBarBackgroundViewController; import com.reactnativenavigation.viewcontrollers.topbar.TopBarController; +import com.reactnativenavigation.views.titlebar.TitleBarButtonCreator; import com.reactnativenavigation.views.titlebar.TitleBarReactViewCreator; import com.reactnativenavigation.views.topbar.TopBar; import com.reactnativenavigation.views.topbar.TopBarBackgroundViewCreator; @@ -44,12 +44,14 @@ import java.util.List; import java.util.Map; +import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.annotation.RestrictTo; import androidx.appcompat.widget.Toolbar; import static com.reactnativenavigation.utils.CollectionUtils.*; import static com.reactnativenavigation.utils.ObjectUtils.perform; +import static com.reactnativenavigation.utils.ObjectUtils.take; public class StackPresenter { private static final int DEFAULT_TITLE_COLOR = Color.BLACK; @@ -66,7 +68,7 @@ public class StackPresenter { private TitleBarButtonController.OnClickListener onClickListener; private final RenderChecker renderChecker; private final TopBarBackgroundViewCreator topBarBackgroundViewCreator; - private final ReactViewCreator buttonCreator; + private final TitleBarButtonCreator buttonCreator; private Options defaultOptions; private List currentRightButtons = new ArrayList<>(); @@ -79,7 +81,7 @@ public class StackPresenter { public StackPresenter(Activity activity, TitleBarReactViewCreator titleViewCreator, TopBarBackgroundViewCreator topBarBackgroundViewCreator, - ReactViewCreator buttonCreator, + TitleBarButtonCreator buttonCreator, IconResolver iconResolver, RenderChecker renderChecker, Options defaultOptions) { @@ -268,25 +270,24 @@ private void applyTopBarVisibility(TopBarOptions options, AnimationsOptions anim } private void applyButtons(TopBarOptions options, ViewController child) { - List