Skip to content

Commit

Permalink
Update SpacingRecyclerViewItemDecoration to be configurable.
Browse files Browse the repository at this point in the history 
In order to address issues resulting from crrev/c/4990273/, we will
have to dynamically re-calculate lead-in and element spacing each time
RecyclerView measured dimension change.

There are two ways to go about it:
1. remove old and create a new spacing element - may not be needed if
   dimensions don't change; also, object allocation during measure is
   discouraged
2. update existing decorations by applying new sizes and optionally
   invalidating item decorations, if the new spacing differs.

This change prepares SpacingRecyclerViewItemDecoration to work with
the 2nd variant.

Bug: 1416985
Change-Id: I13be21ea5735069a4a7abe6f2b00cf044745e176
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4989576
Reviewed-by: Fred Mello <fredmello@chromium.org>
Commit-Queue: Tomasz Wiszkowski <ender@google.com>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1217842}
  • Loading branch information
@tomasz-wiszkowski
tomasz-wiszkowski authored and Chromium LUCI CQ committed Oct 31, 2023
1 parent f2e1916 commit 3fda73e
Show file tree
Hide file tree
Showing 6 changed files with 117 additions and 46 deletions.
3 changes: 2 additions & 1 deletion ...mnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/base/ActionChipsView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ public ActionChipsView(Context context) {
final @Px int elementSpace =
getResources().getDimensionPixelSize(R.dimen.omnibox_action_chip_spacing);

addItemDecoration(new SpacingRecyclerViewItemDecoration(leadInSpace, elementSpace / 2));
addItemDecoration(
new SpacingRecyclerViewItemDecoration(this, leadInSpace, elementSpace / 2));
}

@Override
Expand Down
55 changes: 47 additions & 8 deletions ...g/chromium/chrome/browser/omnibox/suggestions/base/SpacingRecyclerViewItemDecoration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,30 +7,69 @@
import android.graphics.Rect;
import android.view.View;

import androidx.annotation.NonNull;
import androidx.annotation.Px;
import androidx.annotation.VisibleForTesting;
import androidx.recyclerview.widget.RecyclerView;
import androidx.recyclerview.widget.RecyclerView.ItemDecoration;

public class SpacingRecyclerViewItemDecoration extends ItemDecoration {
public final @Px int leadInSpace;
public final @Px int elementSpace;
private final @NonNull RecyclerView mRecyclerView;
private @Px int mLeadInSpace;
private @Px int mElementSpace;

public SpacingRecyclerViewItemDecoration(@Px int leadInSpace, @Px int elementSpace) {
this.leadInSpace = leadInSpace;
this.elementSpace = elementSpace;
public SpacingRecyclerViewItemDecoration(
@NonNull RecyclerView parent, @Px int leadInSpace, @Px int elementSpace) {
mRecyclerView = parent;
mLeadInSpace = leadInSpace;
mElementSpace = elementSpace;
}

@Override
public void getItemOffsets(
Rect outRect, View view, RecyclerView parent, RecyclerView.State state) {
outRect.left = outRect.right = elementSpace;
outRect.left = outRect.right = mElementSpace;

if (parent.getChildAdapterPosition(view) != 0) return;

if (parent.getLayoutDirection() == View.LAYOUT_DIRECTION_RTL) {
outRect.right = leadInSpace;
outRect.right = mLeadInSpace;
} else {
outRect.left = leadInSpace;
outRect.left = mLeadInSpace;
}
}

/**
* Specify new lead in space to be used as an item decoration.
*
* <p>Triggers RecyclerView update if the new spacing is different from the old one.
*/
public void setLeadInSpace(int leadInSpace) {
if (leadInSpace != mLeadInSpace) {
mRecyclerView.invalidateItemDecorations();
}
mLeadInSpace = leadInSpace;
}

/**
* Specify new element space to be used as an item decoration.
*
* <p>Triggers RecyclerView update if the new spacing is different from the old one.
*/
public void setElementSpace(int elementSpace) {
if (elementSpace != mElementSpace) {
mRecyclerView.invalidateItemDecorations();
}
mElementSpace = elementSpace;
}

@VisibleForTesting
public int getElementSpaceForTest() {
return mElementSpace;
}

@VisibleForTesting
public int getLeadInSpaceForTest() {
return mLeadInSpace;
}
}
49 changes: 48 additions & 1 deletion ...um/chrome/browser/omnibox/suggestions/base/SpacingRecyclerViewItemDecorationUnitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;

import android.graphics.Rect;
import android.view.View;
Expand Down Expand Up @@ -38,7 +40,8 @@ public class SpacingRecyclerViewItemDecorationUnitTest {
@Before
public void setUp() {
MockitoAnnotations.initMocks(this);
mDecoration = new SpacingRecyclerViewItemDecoration(LEAD_IN_SPACE, ELEMENT_SPACE);
mDecoration =
new SpacingRecyclerViewItemDecoration(mRecyclerView, LEAD_IN_SPACE, ELEMENT_SPACE);
mOffsets = new Rect();
}

Expand Down Expand Up @@ -73,4 +76,48 @@ public void testSpacing_nonFirstElement() {
assertEquals(0, mOffsets.top);
assertEquals(0, mOffsets.bottom);
}

@Test
public void setLeadInSpace_noUpdate() {
mDecoration.setLeadInSpace(LEAD_IN_SPACE);
mDecoration.getItemOffsets(mOffsets, mChildView, mRecyclerView, /* state= */ null);
assertEquals(LEAD_IN_SPACE, mOffsets.left);
assertEquals(ELEMENT_SPACE, mOffsets.right);
assertEquals(0, mOffsets.top);
assertEquals(0, mOffsets.bottom);
verify(mRecyclerView, times(0)).invalidateItemDecorations();
}

@Test
public void setElementSpace_noUpdate() {
mDecoration.setElementSpace(ELEMENT_SPACE);
mDecoration.getItemOffsets(mOffsets, mChildView, mRecyclerView, /* state= */ null);
assertEquals(LEAD_IN_SPACE, mOffsets.left);
assertEquals(ELEMENT_SPACE, mOffsets.right);
assertEquals(0, mOffsets.top);
assertEquals(0, mOffsets.bottom);
verify(mRecyclerView, times(0)).invalidateItemDecorations();
}

@Test
public void setLeadInSpace_changeTriggersInvalidation() {
mDecoration.setLeadInSpace(2 * LEAD_IN_SPACE);
mDecoration.getItemOffsets(mOffsets, mChildView, mRecyclerView, /* state= */ null);
assertEquals(2 * LEAD_IN_SPACE, mOffsets.left);
assertEquals(ELEMENT_SPACE, mOffsets.right);
assertEquals(0, mOffsets.top);
assertEquals(0, mOffsets.bottom);
verify(mRecyclerView).invalidateItemDecorations();
}

@Test
public void setElementSpace_changeTriggersInvalidation() {
mDecoration.setElementSpace(2 * ELEMENT_SPACE);
mDecoration.getItemOffsets(mOffsets, mChildView, mRecyclerView, /* state= */ null);
assertEquals(LEAD_IN_SPACE, mOffsets.left);
assertEquals(2 * ELEMENT_SPACE, mOffsets.right);
assertEquals(0, mOffsets.top);
assertEquals(0, mOffsets.bottom);
verify(mRecyclerView).invalidateItemDecorations();
}
}
9 changes: 9 additions & 0 deletions .../org/chromium/chrome/browser/omnibox/suggestions/carousel/BaseCarouselSuggestionView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@
import org.chromium.chrome.browser.omnibox.R;
import org.chromium.chrome.browser.omnibox.styles.OmniboxResourceProvider;
import org.chromium.chrome.browser.omnibox.suggestions.RecyclerViewSelectionController;
import org.chromium.chrome.browser.omnibox.suggestions.base.SpacingRecyclerViewItemDecoration;
import org.chromium.chrome.browser.util.KeyNavigationUtil;
import org.chromium.ui.modelutil.SimpleRecyclerViewAdapter;

/** View for Carousel Suggestions. */
@MockedInTests
public class BaseCarouselSuggestionView extends RecyclerView {
private RecyclerViewSelectionController mSelectionController;
private SpacingRecyclerViewItemDecoration mDecoration;

/**
* Constructs a new carousel suggestion view.
Expand All @@ -46,6 +48,9 @@ public BaseCarouselSuggestionView(Context context, SimpleRecyclerViewAdapter ada
mSelectionController = new RecyclerViewSelectionController(getLayoutManager());
addOnChildAttachStateChangeListener(mSelectionController);

mDecoration = new SpacingRecyclerViewItemDecoration(this, 0, 0);
addItemDecoration(mDecoration);

setAdapter(adapter);
}

Expand Down Expand Up @@ -93,4 +98,8 @@ public void setSelected(boolean isSelected) {
mSelectionController = controller;
addOnChildAttachStateChangeListener(mSelectionController);
}

/* package */ SpacingRecyclerViewItemDecoration getItemDecoration() {
return mDecoration;
}
}
11 changes: 2 additions & 9 deletions ...hromium/chrome/browser/omnibox/suggestions/carousel/BaseCarouselSuggestionViewBinder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import org.chromium.chrome.browser.omnibox.styles.OmniboxResourceProvider;
import org.chromium.chrome.browser.omnibox.suggestions.SuggestionCommonProperties;
import org.chromium.chrome.browser.omnibox.suggestions.SuggestionCommonProperties.FormFactor;
import org.chromium.chrome.browser.omnibox.suggestions.base.SpacingRecyclerViewItemDecoration;
import org.chromium.ui.modelutil.PropertyKey;
import org.chromium.ui.modelutil.PropertyModel;
import org.chromium.ui.modelutil.SimpleRecyclerViewAdapter;
Expand All @@ -36,12 +35,6 @@ public static void bind(PropertyModel model, BaseCarouselSuggestionView view, Pr
}
} else if (key == SuggestionCommonProperties.DEVICE_FORM_FACTOR
|| key == BaseCarouselSuggestionViewProperties.ITEM_WIDTH) {
int itemDecoration = view.getItemDecorationCount();
while (itemDecoration > 0) {
itemDecoration--;
view.removeItemDecorationAt(itemDecoration);
}

var context = view.getContext();
// Adjust the initial offset of the MV Carousel to match the offset of the
// suggestion header.
Expand All @@ -58,8 +51,8 @@ public static void bind(PropertyModel model, BaseCarouselSuggestionView view, Pr
model.get(BaseCarouselSuggestionViewProperties.ITEM_WIDTH),
initialSpacing,
view);
view.addItemDecoration(
new SpacingRecyclerViewItemDecoration(initialSpacing, itemSpacing / 2));
view.getItemDecoration().setLeadInSpace(initialSpacing);
view.getItemDecoration().setElementSpace(itemSpacing / 2);
} else if (key == BaseCarouselSuggestionViewProperties.HORIZONTAL_FADE) {
view.setHorizontalFadingEdgeEnabled(
model.get(BaseCarouselSuggestionViewProperties.HORIZONTAL_FADE));
Expand Down
36 changes: 9 additions & 27 deletions ...chrome/browser/omnibox/suggestions/carousel/BaseCarouselSuggestionViewBinderUnitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,8 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.clearInvocations;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;

import android.content.Context;
import android.content.res.Resources;
Expand All @@ -22,7 +18,6 @@
import org.junit.Test;
import org.junit.rules.TestRule;
import org.junit.runner.RunWith;
import org.mockito.ArgumentCaptor;
import org.robolectric.annotation.Config;

import org.chromium.base.ContextUtils;
Expand All @@ -32,7 +27,6 @@
import org.chromium.chrome.browser.omnibox.styles.OmniboxResourceProvider;
import org.chromium.chrome.browser.omnibox.suggestions.SuggestionCommonProperties;
import org.chromium.chrome.browser.omnibox.suggestions.SuggestionCommonProperties.FormFactor;
import org.chromium.chrome.browser.omnibox.suggestions.base.SpacingRecyclerViewItemDecoration;
import org.chromium.chrome.browser.omnibox.test.R;
import org.chromium.chrome.test.util.browser.Features;
import org.chromium.chrome.test.util.browser.Features.EnableFeatures;
Expand Down Expand Up @@ -206,29 +200,22 @@ public void formFactor_itemSpacingEndToEnd() {
FormFactor.TABLET, Integer.MAX_VALUE, Integer.MAX_VALUE, mView));

mModel.set(SuggestionCommonProperties.DEVICE_FORM_FACTOR, FormFactor.TABLET);
ArgumentCaptor<SpacingRecyclerViewItemDecoration> captor =
ArgumentCaptor.forClass(SpacingRecyclerViewItemDecoration.class);
verify(mView, times(1)).addItemDecoration(captor.capture());
var decoration = captor.getValue();
var decoration = mView.getItemDecoration();
Assert.assertEquals(
OmniboxResourceProvider.getSideSpacing(mContext), decoration.leadInSpace);
Assert.assertEquals(spacingPx / 2, decoration.elementSpace);
OmniboxResourceProvider.getSideSpacing(mContext),
decoration.getLeadInSpaceForTest());
Assert.assertEquals(spacingPx / 2, decoration.getElementSpaceForTest());
}

@Test
public void formFactor_itemDecorationsDoNotAggregate() {
mModel.set(SuggestionCommonProperties.DEVICE_FORM_FACTOR, FormFactor.TABLET);
verify(mView, times(1)).addItemDecoration(any());
Assert.assertEquals(1, mView.getItemDecorationCount());
clearInvocations(mView);

mModel.set(SuggestionCommonProperties.DEVICE_FORM_FACTOR, FormFactor.PHONE);
verify(mView, times(1)).addItemDecoration(any());
Assert.assertEquals(1, mView.getItemDecorationCount());
clearInvocations(mView);

mModel.set(SuggestionCommonProperties.DEVICE_FORM_FACTOR, FormFactor.TABLET);
verify(mView, times(1)).addItemDecoration(any());
Assert.assertEquals(1, mView.getItemDecorationCount());
}

Expand Down Expand Up @@ -263,12 +250,10 @@ public void mView_setHorizontalFadingEdgeEnabled() {
@Config(qualifiers = "sw600dp-land")
public void customVisualAlignment_classicUi() {
mModel.set(SuggestionCommonProperties.DEVICE_FORM_FACTOR, FormFactor.TABLET);
ArgumentCaptor<SpacingRecyclerViewItemDecoration> captor =
ArgumentCaptor.forClass(SpacingRecyclerViewItemDecoration.class);
verify(mView, times(1)).addItemDecoration(captor.capture());
var decoration = captor.getValue();
var decoration = mView.getItemDecoration();
Assert.assertEquals(
OmniboxResourceProvider.getSideSpacing(mContext), decoration.leadInSpace);
OmniboxResourceProvider.getSideSpacing(mContext),
decoration.getLeadInSpaceForTest());
}

@Test
Expand All @@ -293,13 +278,10 @@ public void customVisualAlignment_modernUi_smallest() {

void runCustomVisualAlignmentTest() {
mModel.set(SuggestionCommonProperties.DEVICE_FORM_FACTOR, FormFactor.TABLET);
ArgumentCaptor<SpacingRecyclerViewItemDecoration> captor =
ArgumentCaptor.forClass(SpacingRecyclerViewItemDecoration.class);
verify(mView, times(1)).addItemDecoration(captor.capture());
var decoration = captor.getValue();
var decoration = mView.getItemDecoration();
Assert.assertEquals(
OmniboxResourceProvider.getHeaderStartPadding(mContext)
- mContext.getResources().getDimensionPixelSize(R.dimen.tile_view_padding),
decoration.leadInSpace);
decoration.getLeadInSpaceForTest());
}
}

0 comments on commit 3fda73e

Please sign in to comment.