Skip to content

Commit

Permalink
Refactor right buttons (#6090)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@guyca
guyca authored Apr 1, 2020
1 parent a0172bd commit 42a6917
Show file tree
Hide file tree
Showing 29 changed files with 652 additions and 378 deletions.
15 changes: 14 additions & 1 deletion lib/android/app/src/main/java/com/reactnativenavigation/parse/Component.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
}
Expand All @@ -26,19 +31,25 @@ 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) {
if (!componentId.hasValue()) componentId = defaultOptions.componentId;
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() {
Expand All @@ -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);
}
}
17 changes: 12 additions & 5 deletions lib/android/app/src/main/java/com/reactnativenavigation/parse/params/Button.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -19,18 +20,20 @@

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();
public Bool disableIconTint = new NullBool();
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();
Expand All @@ -56,15 +59,15 @@ 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");
button.disableIconTint = BoolParser.parse(json, "disableIconTint");
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");
Expand Down Expand Up @@ -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()) {
Expand Down
4 changes: 4 additions & 0 deletions lib/android/app/src/main/java/com/reactnativenavigation/parse/params/Text.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ public Text(String value) {
super(value);
}

public int length() {
return hasValue() ? value.length() : 0;
}

@NonNull
@Override
public String toString() {
Expand Down
101 changes: 54 additions & 47 deletions lib/android/app/src/main/java/com/reactnativenavigation/presentation/StackPresenter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@
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;
import com.reactnativenavigation.viewcontrollers.button.IconResolver;
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;
Expand All @@ -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;
Expand All @@ -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<TitleBarButtonController> currentRightButtons = new ArrayList<>();
Expand All @@ -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) {
Expand Down Expand Up @@ -268,25 +270,24 @@ private void applyTopBarVisibility(TopBarOptions options, AnimationsOptions anim
}

private void applyButtons(TopBarOptions options, ViewController child) {
List<Button> rightButtons = mergeButtonsWithColor(options.buttons.right, options.rightButtonColor, options.rightButtonDisabledColor);
List<Button> leftButtons = mergeButtonsWithColor(options.buttons.left, options.leftButtonColor, options.leftButtonDisabledColor);

if (rightButtons != null) {
List<TitleBarButtonController> rightButtonControllers = getOrCreateButtonControllers(componentRightButtons.get(child.getView()), rightButtons);
if (options.buttons.right != null) {
List<Button> rightButtons = mergeButtonsWithColor(options.buttons.right, options.rightButtonColor, options.rightButtonDisabledColor);
List<TitleBarButtonController> rightButtonControllers = getOrCreateButtonControllersByInstanceId(componentRightButtons.get(child.getView()), rightButtons);
componentRightButtons.put(child.getView(), keyBy(rightButtonControllers, TitleBarButtonController::getButtonInstanceId));
if (!CollectionUtils.equals(currentRightButtons, rightButtonControllers)) {
currentRightButtons = rightButtonControllers;
topBar.setRightButtons(rightButtonControllers);
topBarController.applyRightButtons(currentRightButtons);
}
} else {
currentRightButtons = null;
topBar.clearRightButtons();
}

if (leftButtons != null) {
List<TitleBarButtonController> leftButtonControllers = getOrCreateButtonControllers(componentLeftButtons.get(child.getView()), leftButtons);
if (options.buttons.left != null) {
List<Button> leftButtons = mergeButtonsWithColor(options.buttons.left, options.leftButtonColor, options.leftButtonDisabledColor);
List<TitleBarButtonController> leftButtonControllers = getOrCreateButtonControllersByInstanceId(componentLeftButtons.get(child.getView()), leftButtons);
componentLeftButtons.put(child.getView(), keyBy(leftButtonControllers, TitleBarButtonController::getButtonInstanceId));
topBar.setLeftButtons(leftButtonControllers);
topBarController.setLeftButtons(leftButtonControllers);
} else {
topBar.clearLeftButtons();
}
Expand All @@ -298,19 +299,25 @@ private void applyButtons(TopBarOptions options, ViewController child) {
topBar.setOverflowButtonColor(options.rightButtonColor.get(Color.BLACK));
}

private List<TitleBarButtonController> getOrCreateButtonControllers(@Nullable Map<String, TitleBarButtonController> currentButtons, @Nullable List<Button> buttons) {
private List<TitleBarButtonController> getOrCreateButtonControllersByInstanceId(@Nullable Map<String, TitleBarButtonController> currentButtons, @Nullable List<Button> buttons) {
if (buttons == null) return null;
Map<String, TitleBarButtonController> result = new LinkedHashMap<>();
forEach(buttons, b -> result.put(b.instanceId, getOrDefault(currentButtons, b.instanceId, () -> createButtonController(b))));
return new ArrayList<>(result.values());
}

private List<TitleBarButtonController> getOrCreateButtonControllersById(@Nullable Map<String, TitleBarButtonController> currentButtons,@NonNull List<Button> buttons) {
ArrayList result = new ArrayList<TitleBarButtonController>();
for (Button b : buttons) {
result.put(b.instanceId, currentButtons != null && currentButtons.containsKey(b.instanceId) ? currentButtons.get(b.instanceId) : createButtonController(b));
result.add(take(first(perform(currentButtons, null, Map::values), button -> button.getId().equals(b.id)), createButtonController(b)));
}
return new ArrayList<>(result.values());
return result;
}

private TitleBarButtonController createButtonController(Button button) {
TitleBarButtonController controller = new TitleBarButtonController(activity,
iconResolver,
new ButtonPresenter(topBar.getTitleBar(), button),
new ButtonPresenter(button),
button,
buttonCreator,
onClickListener
Expand Down Expand Up @@ -354,50 +361,50 @@ private void mergeOrientation(OrientationOptions orientationOptions) {
}

private void mergeButtons(TopBarOptions options, TopBarButtons buttons, View child) {
List<Button> rightButtons = mergeButtonsWithColor(buttons.right, options.rightButtonColor, options.rightButtonDisabledColor);
List<Button> leftButtons = mergeButtonsWithColor(buttons.left, options.leftButtonColor, options.leftButtonDisabledColor);
mergeRightButtons(options, buttons, child);
mergeLeftButton(options, buttons, child);
mergeBackButton(buttons);
}

List<TitleBarButtonController> rightButtonControllers = getOrCreateButtonControllers(componentRightButtons.get(child), rightButtons);
List<TitleBarButtonController> leftButtonControllers = getOrCreateButtonControllers(componentLeftButtons.get(child), leftButtons);
private void mergeRightButtons(TopBarOptions options, TopBarButtons buttons, View child) {
if (buttons.right == null) return;
List<Button> rightButtons = mergeButtonsWithColor(buttons.right, options.rightButtonColor, options.rightButtonDisabledColor);
List<TitleBarButtonController> toMerge = getOrCreateButtonControllersById(componentRightButtons.get(child), rightButtons);
List<TitleBarButtonController> toRemove = difference(currentRightButtons, toMerge, TitleBarButtonController::equals);
forEach(toRemove, TitleBarButtonController::destroy);

if (rightButtonControllers != null) {
Map previousRightButtons = componentRightButtons.put(child, keyBy(rightButtonControllers, TitleBarButtonController::getButtonInstanceId));
if (previousRightButtons != null) forEach(previousRightButtons.values(), TitleBarButtonController::destroy);
}
if (leftButtonControllers != null) {
Map previousLeftButtons = componentLeftButtons.put(child, keyBy(leftButtonControllers, TitleBarButtonController::getButtonInstanceId));
if (previousLeftButtons != null) forEach(previousLeftButtons.values(), TitleBarButtonController::destroy);
if (!CollectionUtils.equals(currentRightButtons, toMerge)) {
topBarController.mergeRightButtons(toMerge, toRemove);
currentRightButtons = toMerge;
}
if (options.rightButtonColor.hasValue()) topBar.setOverflowButtonColor(options.rightButtonColor.get());
}

if (buttons.right != null) {
if (!CollectionUtils.equals(currentRightButtons, rightButtonControllers)) {
currentRightButtons = rightButtonControllers;
topBar.setRightButtons(rightButtonControllers);
}
}
if (buttons.left != null) topBar.setLeftButtons(leftButtonControllers);
private void mergeLeftButton(TopBarOptions options, TopBarButtons buttons, View child) {
if (buttons.left == null) return;
List<Button> leftButtons = mergeButtonsWithColor(buttons.left, options.leftButtonColor, options.leftButtonDisabledColor);
List<TitleBarButtonController> toMerge = getOrCreateButtonControllersById(componentLeftButtons.get(child), leftButtons);
componentLeftButtons.put(child, keyBy(toMerge, TitleBarButtonController::getButtonInstanceId));
topBarController.setLeftButtons(toMerge);
}

private void mergeBackButton(TopBarButtons buttons) {
if (buttons.back.hasValue()) {
if (buttons.back.visible.isFalse()) {
topBar.clearLeftButtons();
} else {
topBar.setBackButton(createButtonController(buttons.back));
}
}

if (options.rightButtonColor.hasValue()) topBar.setOverflowButtonColor(options.rightButtonColor.get());
}

@Nullable
private List<Button> mergeButtonsWithColor(List<Button> buttons, Colour buttonColor, Colour disabledColor) {
List<Button> result = null;
if (buttons != null) {
result = new ArrayList<>();
for (Button button : buttons) {
Button copy = button.copy();
if (!button.color.hasValue()) copy.color = buttonColor;
if (!button.disabledColor.hasValue()) copy.disabledColor = disabledColor;
result.add(copy);
}
private List<Button> mergeButtonsWithColor(@NonNull List<Button> buttons, Colour buttonColor, Colour disabledColor) {
List<Button> result = new ArrayList<>();
for (Button button : buttons) {
Button copy = button.copy();
if (!button.color.hasValue()) copy.color = buttonColor;
if (!button.disabledColor.hasValue()) copy.disabledColor = disabledColor;
result.add(copy);
}
return result;
}
Expand Down
Loading

0 comments on commit 42a6917

Please sign in to comment.