From 747bcf25fffb3d3e3b6ec0ef52e884255f144bb4 Mon Sep 17 00:00:00 2001 From: conradchen Date: Thu, 23 Dec 2021 16:07:56 -0500 Subject: [PATCH] [ButtonGroup] Simplify MaterialButtonTogglerGroup's checking logic 1. Consolidates single selected ID and multiple selected IDs to a single selected ID set. 2. Separates View states and internal checked states so we can focus and enforce policies much easier on only internal states. Resolves https://github.com/material-components/material-components-android/issues/2263 PiperOrigin-RevId: 418054400 --- .../button/MaterialButtonToggleGroup.java | 139 ++++++------------ .../button/MaterialButtonToggleGroupTest.java | 54 +++++++ 2 files changed, 103 insertions(+), 90 deletions(-) diff --git a/lib/java/com/google/android/material/button/MaterialButtonToggleGroup.java b/lib/java/com/google/android/material/button/MaterialButtonToggleGroup.java index 2f216bc3e5a..2763df0c93b 100644 --- a/lib/java/com/google/android/material/button/MaterialButtonToggleGroup.java +++ b/lib/java/com/google/android/material/button/MaterialButtonToggleGroup.java @@ -49,8 +49,10 @@ import com.google.android.material.shape.ShapeAppearanceModel; import java.util.ArrayList; import java.util.Comparator; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; +import java.util.Set; import java.util.SortedMap; import java.util.TreeMap; @@ -170,7 +172,7 @@ public int compare(MaterialButton v1, MaterialButton v2) { private boolean singleSelection; private boolean selectionRequired; - @IdRes private int checkedId; + private Set checkedIds = new HashSet<>(); public MaterialButtonToggleGroup(@NonNull Context context) { this(context, null); @@ -191,8 +193,11 @@ public MaterialButtonToggleGroup( setSingleSelection( attributes.getBoolean(R.styleable.MaterialButtonToggleGroup_singleSelection, false)); - checkedId = - attributes.getResourceId(R.styleable.MaterialButtonToggleGroup_checkedButton, View.NO_ID); + if (attributes.hasValue(R.styleable.MaterialButtonToggleGroup_checkedButton)) { + checkedIds.add( + attributes.getResourceId( + R.styleable.MaterialButtonToggleGroup_checkedButton, View.NO_ID)); + } selectionRequired = attributes.getBoolean(R.styleable.MaterialButtonToggleGroup_selectionRequired, false); setChildrenDrawingOrderEnabled(true); @@ -204,11 +209,7 @@ public MaterialButtonToggleGroup( @Override protected void onFinishInflate() { super.onFinishInflate(); - - // Checks the appropriate button as requested via XML - if (checkedId != View.NO_ID) { - checkForced(checkedId, true); - } + updateCheckedIds(checkedIds); } @Override @@ -234,11 +235,8 @@ public void addView(View child, int index, ViewGroup.LayoutParams params) { // Sets sensible default values and an internal checked change listener for this child setupButtonChild(buttonChild); - // Reorders children if a checked child was added to this layout - if (buttonChild.isChecked()) { - updateCheckedStates(buttonChild.getId(), true); - setCheckedId(buttonChild.getId()); - } + // Update button group's checked states + checkInternal(buttonChild.getId(), buttonChild.isChecked()); // Saves original corner data ShapeAppearanceModel shapeAppearanceModel = buttonChild.getShapeAppearanceModel(); @@ -327,11 +325,7 @@ public void onInitializeAccessibilityNodeInfo(@NonNull AccessibilityNodeInfo inf * @see #getCheckedButtonId() */ public void check(@IdRes int id) { - if (id == checkedId) { - return; - } - - checkForced(id, true); + checkInternal(id, true); } /** @@ -344,7 +338,7 @@ public void check(@IdRes int id) { * @see #getCheckedButtonId() */ public void uncheck(@IdRes int id) { - checkForced(id, false); + checkInternal(id, false); } /** @@ -357,16 +351,7 @@ public void uncheck(@IdRes int id) { * @see #getCheckedButtonId() */ public void clearChecked() { - skipCheckedStateTracker = true; - for (int i = 0; i < getChildCount(); i++) { - MaterialButton child = getChildButton(i); - child.setChecked(false); - - dispatchOnButtonChecked(child.getId(), false); - } - skipCheckedStateTracker = false; - - setCheckedId(View.NO_ID); + updateCheckedIds(new HashSet()); } /** @@ -385,7 +370,7 @@ public void clearChecked() { */ @IdRes public int getCheckedButtonId() { - return singleSelection ? checkedId : View.NO_ID; + return singleSelection && !checkedIds.isEmpty() ? checkedIds.iterator().next() : View.NO_ID; } /** @@ -402,15 +387,15 @@ public int getCheckedButtonId() { */ @NonNull public List getCheckedButtonIds() { - List checkedIds = new ArrayList<>(); + List orderedCheckedIds = new ArrayList<>(); for (int i = 0; i < getChildCount(); i++) { - MaterialButton child = getChildButton(i); - if (child.isChecked()) { - checkedIds.add(child.getId()); + int childId = getChildButton(i).getId(); + if (checkedIds.contains(childId)) { + orderedCheckedIds.add(childId); } } - return checkedIds; + return orderedCheckedIds; } /** @@ -505,12 +490,6 @@ private void setCheckedStateForView(@IdRes int viewId, boolean checked) { } } - private void setCheckedId(int checkedId) { - this.checkedId = checkedId; - - dispatchOnButtonChecked(checkedId, true); - } - /** * Sets a negative marginStart on all but the first child, if two adjacent children both have a * stroke width greater than 0. This prevents a double-width stroke from being drawn for two @@ -686,49 +665,40 @@ private static void updateBuilderWithCornerData( .setBottomRightCornerSize(cornerData.bottomRight); } - /** - * When a checked child is added, or a child is clicked, updates checked state and draw order of - * children to draw all checked children on top of all unchecked children. - * - *

If {@code singleSelection} is true, this will unselect any other children as well. - * - *

If {@code selectionRequired} is true, and the last child is unchecked it will undo the - * deselection. - * - * @param childId ID of child whose checked state may have changed - * @param childIsChecked Whether the child is checked - * @return Whether the checked state for childId has changed. - */ - private boolean updateCheckedStates(int childId, boolean childIsChecked) { - List checkedButtonIds = getCheckedButtonIds(); - if (selectionRequired && checkedButtonIds.isEmpty()) { - // undo deselection - setCheckedStateForView(childId, true); - checkedId = childId; - return false; - } - - // un select previous selection - if (childIsChecked && singleSelection) { - checkedButtonIds.remove((Integer) childId); - for (int buttonId : checkedButtonIds) { - setCheckedStateForView(buttonId, false); - dispatchOnButtonChecked(buttonId, false); + private void checkInternal(@IdRes int buttonId, boolean checked) { + Set checkedIds = new HashSet<>(this.checkedIds); + if (checked && !checkedIds.contains(buttonId)) { + if (singleSelection && !checkedIds.isEmpty()) { + checkedIds.clear(); + } + checkedIds.add(buttonId); + } else if (!checked && checkedIds.contains(buttonId)) { + // Do not uncheck button if no selection remains when selection is required. + if (!selectionRequired || checkedIds.size() > 1) { + checkedIds.remove(buttonId); } + } else { + // No state change, do nothing + return; } - return true; + updateCheckedIds(checkedIds); } - private void dispatchOnButtonChecked(@IdRes int buttonId, boolean checked) { - for (OnButtonCheckedListener listener : onButtonCheckedListeners) { - listener.onButtonChecked(this, buttonId, checked); + private void updateCheckedIds(Set checkedIds) { + for (int i = 0; i < getChildCount(); i++) { + int buttonId = getChildButton(i).getId(); + setCheckedStateForView(buttonId, checkedIds.contains(buttonId)); + if (this.checkedIds.contains(buttonId) != checkedIds.contains(buttonId)) { + dispatchOnButtonChecked(buttonId, checkedIds.contains(buttonId)); + } } + this.checkedIds = new HashSet<>(checkedIds); + invalidate(); } - private void checkForced(int checkedId, boolean checked) { - MaterialButton button = findViewById(checkedId); - if (button != null) { - button.setChecked(checked); + private void dispatchOnButtonChecked(@IdRes int buttonId, boolean checked) { + for (OnButtonCheckedListener listener : onButtonCheckedListeners) { + listener.onButtonChecked(this, buttonId, checked); } } @@ -795,22 +765,11 @@ private void updateChildOrder() { private class CheckedStateTracker implements MaterialButton.OnCheckedChangeListener { @Override public void onCheckedChanged(@NonNull MaterialButton button, boolean isChecked) { - // Prevents infinite recursion + // Checked state change is triggered by the button group, do not update checked ids again. if (skipCheckedStateTracker) { return; } - - if (singleSelection) { - checkedId = isChecked ? button.getId() : View.NO_ID; - } - - boolean buttonCheckedStateChanged = updateCheckedStates(button.getId(), isChecked); - if (buttonCheckedStateChanged) { - // Dispatch button.isChecked instead of isChecked in case its checked state was updated - // internally. - dispatchOnButtonChecked(button.getId(), button.isChecked()); - } - invalidate(); + checkInternal(button.getId(), isChecked); } } diff --git a/lib/javatests/com/google/android/material/button/MaterialButtonToggleGroupTest.java b/lib/javatests/com/google/android/material/button/MaterialButtonToggleGroupTest.java index 005e3e2bf6e..e7598362858 100644 --- a/lib/javatests/com/google/android/material/button/MaterialButtonToggleGroupTest.java +++ b/lib/javatests/com/google/android/material/button/MaterialButtonToggleGroupTest.java @@ -36,6 +36,7 @@ import androidx.test.core.app.ApplicationProvider; import com.google.android.material.button.MaterialButtonToggleGroup.OnButtonCheckedListener; import com.google.android.material.shape.ShapeAppearanceModel; +import java.util.List; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -183,6 +184,59 @@ public void singleSelection_withoutSelectionRequired_unSelects() { assertThat(((Checkable) button).isChecked()).isFalse(); } + @Test + public void singleSelection_doesNotMultiSelect() { + toggleGroup.setSingleSelection(true); + + View button1 = toggleGroup.getChildAt(0); + button1.performClick(); + View button2 = toggleGroup.getChildAt(1); + button2.performClick(); + + assertThat(((Checkable) button1).isChecked()).isFalse(); + assertThat(((Checkable) button2).isChecked()).isTrue(); + } + + @Test + public void singleSelection_doesNotMultiSelect_programmatically() { + toggleGroup.setSingleSelection(true); + + View button1 = toggleGroup.getChildAt(0); + int id1 = ViewCompat.generateViewId(); + button1.setId(id1); + + View button2 = toggleGroup.getChildAt(1); + int id2 = ViewCompat.generateViewId(); + button2.setId(id2); + + toggleGroup.check(id1); + toggleGroup.check(id2); + + assertThat(((Checkable) button1).isChecked()).isFalse(); + assertThat(((Checkable) button2).isChecked()).isTrue(); + } + + @Test + public void multiSelection_correctSelectedIds() { + toggleGroup.setSingleSelection(false); + + View button1 = toggleGroup.getChildAt(0); + int id1 = ViewCompat.generateViewId(); + button1.setId(id1); + + View button2 = toggleGroup.getChildAt(1); + int id2 = ViewCompat.generateViewId(); + button2.setId(id2); + + toggleGroup.check(id1); + toggleGroup.check(id2); + + List checkedIds = toggleGroup.getCheckedButtonIds(); + assertThat(checkedIds.contains(id1)).isTrue(); + assertThat(checkedIds.contains(id2)).isTrue(); + assertThat(checkedIds.size()).isEqualTo(2); + } + @Test public void multiSelection_withSelectionRequired_unSelectsIfTwo() { toggleGroup.setSingleSelection(false);