Skip to content

Commit

Permalink
Fix the issue that values of the item decoration is misplaced (#294)
Browse files Browse the repository at this point in the history 
when the user scrolls the RecyclerView.

This was because of the reasons that:
- FlexboxItemDecoration#isFirstItemInLine wasn't working correctly (it
expected the flex lines only being calculated in the
FlexboxHelper#calculateFlexLines, not expecting the already calculated
flex lines).

- In the layoutFlexLine method in FlexboxLayoutManager,
calculateItemDecorationsForChild needed to be called before added to the
RecyclerView because the view man be created in that method, in that
case calcurated decoration's inset values are not set at the time of
creation.
  • Loading branch information
@thagikura
thagikura committed Jun 16, 2017
1 parent fcf4425 commit b3c4e4a
Show file tree
Hide file tree
Showing 4 changed files with 195 additions and 4 deletions.
158 changes: 158 additions & 0 deletions flexbox/src/androidTest/java/com/google/android/flexbox/test/FlexboxLayoutManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
import android.content.pm.ActivityInfo;
import android.content.res.Configuration;
import android.graphics.drawable.Drawable;
import android.os.Build;
import android.support.annotation.DrawableRes;
import android.support.test.InstrumentationRegistry;
import android.support.test.espresso.ViewAction;
import android.support.test.espresso.action.CoordinatesProvider;
Expand All @@ -56,6 +58,7 @@
import com.google.android.flexbox.AlignItems;
import com.google.android.flexbox.AlignSelf;
import com.google.android.flexbox.FlexDirection;
import com.google.android.flexbox.FlexLine;
import com.google.android.flexbox.FlexWrap;
import com.google.android.flexbox.FlexboxItemDecoration;
import com.google.android.flexbox.FlexboxLayoutManager;
Expand Down Expand Up @@ -3209,6 +3212,154 @@ public void run() {

}

@Test
@FlakyTest
public void testItemDecoration_withScrolling_direction_row() throws Throwable {
// This test verifies the case that the item decoration set through FlexboxItemDecoration
// is misplaced after the user scrolls the RecyclerView
// https://github.com/google/flexbox-layout/issues/285

final FlexboxTestActivity activity = mActivityRule.getActivity();
final FlexboxLayoutManager layoutManager = new FlexboxLayoutManager();
final TestAdapter adapter = new TestAdapter();
final Drawable decorationDrawable = getDrawable(activity, R.drawable.divider);
mActivityRule.runOnUiThread(new Runnable() {
@Override
public void run() {
activity.setContentView(R.layout.recyclerview);
RecyclerView recyclerView = (RecyclerView) activity.findViewById(R.id.recyclerview);
layoutManager.setFlexDirection(FlexDirection.ROW);
recyclerView.setLayoutManager(layoutManager);
recyclerView.setAdapter(adapter);
FlexboxItemDecoration decoration = new FlexboxItemDecoration(activity);
decoration.setDrawable(decorationDrawable);
recyclerView.addItemDecoration(decoration);
for (int i = 0; i < 50; i++) {
FlexboxLayoutManager.LayoutParams lp = createLayoutParams(activity, 100, 70);
adapter.addItem(lp);
}
// The first line has 1 item, the following lines have more than 1 items
// RecyclerView width: 320, height: 240.
// Flex line 1: 1 items
// Flex line 2: 3 items
// Flex line 3: 3 items
// ...
}
});
InstrumentationRegistry.getInstrumentation().waitForIdleSync();
assertThat(layoutManager.getFlexDirection(), is(FlexDirection.ROW));
assertThat(layoutManager.getTopDecorationHeight(layoutManager.getChildAt(0)), is(0));
assertThat(layoutManager.getTopDecorationHeight(layoutManager.getChildAt(1)), is(0));
assertThat(layoutManager.getTopDecorationHeight(layoutManager.getChildAt(2)), is(0));
for (FlexLine flexLine : layoutManager.getFlexLines()) {
View firstViewInLine = layoutManager.getChildAt(flexLine.getFirstIndex());
if (firstViewInLine != null) {
assertThat(layoutManager.getLeftDecorationWidth(firstViewInLine), is(0));
}
}

onView(withId(R.id.recyclerview)).perform(swipe(GeneralLocation.BOTTOM_CENTER,
GeneralLocation.TOP_CENTER));
onView(withId(R.id.recyclerview)).perform(swipe(GeneralLocation.BOTTOM_CENTER,
GeneralLocation.TOP_CENTER));
onView(withId(R.id.recyclerview)).perform(swipe(GeneralLocation.BOTTOM_CENTER,
GeneralLocation.TOP_CENTER));

onView(withId(R.id.recyclerview)).perform(swipe(GeneralLocation.TOP_CENTER,
GeneralLocation.BOTTOM_CENTER));
onView(withId(R.id.recyclerview)).perform(swipe(GeneralLocation.TOP_CENTER,
GeneralLocation.BOTTOM_CENTER));
onView(withId(R.id.recyclerview)).perform(swipe(GeneralLocation.TOP_CENTER,
GeneralLocation.BOTTOM_CENTER));
onView(withId(R.id.recyclerview)).perform(swipe(GeneralLocation.TOP_CENTER,
GeneralLocation.BOTTOM_CENTER));

// Verify even after the scrolling, decoration values are set correctly
assertThat(layoutManager.getTopDecorationHeight(layoutManager.getChildAt(0)), is(0));
assertThat(layoutManager.getTopDecorationHeight(layoutManager.getChildAt(1)), is(0));
assertThat(layoutManager.getTopDecorationHeight(layoutManager.getChildAt(2)), is(0));
for (FlexLine flexLine : layoutManager.getFlexLines()) {
View firstViewInLine = layoutManager.getChildAt(flexLine.getFirstIndex());
if (firstViewInLine != null) {
assertThat(layoutManager.getLeftDecorationWidth(firstViewInLine), is(0));
}
}
}

@Test
@FlakyTest
public void testItemDecoration_withScrolling_direction_column() throws Throwable {
// This test verifies the case that the item decoration set through FlexboxItemDecoration
// is misplaced after the user scrolls the RecyclerView
// https://github.com/google/flexbox-layout/issues/285

final FlexboxTestActivity activity = mActivityRule.getActivity();
final FlexboxLayoutManager layoutManager = new FlexboxLayoutManager();
final TestAdapter adapter = new TestAdapter();
final Drawable decorationDrawable = getDrawable(activity, R.drawable.divider);
mActivityRule.runOnUiThread(new Runnable() {
@Override
public void run() {
activity.setContentView(R.layout.recyclerview);
RecyclerView recyclerView = (RecyclerView) activity.findViewById(R.id.recyclerview);
layoutManager.setFlexDirection(FlexDirection.COLUMN);
recyclerView.setLayoutManager(layoutManager);
recyclerView.setAdapter(adapter);
FlexboxItemDecoration decoration = new FlexboxItemDecoration(activity);
decoration.setDrawable(decorationDrawable);
recyclerView.addItemDecoration(decoration);
for (int i = 0; i < 50; i++) {
FlexboxLayoutManager.LayoutParams lp = createLayoutParams(activity, 100, 70);
adapter.addItem(lp);
}
// The first line has 1 item, the following lines have more than 1 items
// RecyclerView width: 320, height: 240.
// Flex line 1: 1 items
// Flex line 2: 3 items
// Flex line 3: 3 items
// ...
}
});
InstrumentationRegistry.getInstrumentation().waitForIdleSync();
assertThat(layoutManager.getFlexDirection(), is(FlexDirection.COLUMN));
assertThat(layoutManager.getLeftDecorationWidth(layoutManager.getChildAt(0)), is(0));
assertThat(layoutManager.getLeftDecorationWidth(layoutManager.getChildAt(1)), is(0));
assertThat(layoutManager.getLeftDecorationWidth(layoutManager.getChildAt(2)), is(0));
for (FlexLine flexLine : layoutManager.getFlexLines()) {
View firstViewInLine = layoutManager.getChildAt(flexLine.getFirstIndex());
if (firstViewInLine != null) {
assertThat(layoutManager.getTopDecorationHeight(firstViewInLine), is(0));
}
}

onView(withId(R.id.recyclerview)).perform(swipe(GeneralLocation.CENTER_RIGHT,
GeneralLocation.CENTER_LEFT));
onView(withId(R.id.recyclerview)).perform(swipe(GeneralLocation.CENTER_RIGHT,
GeneralLocation.CENTER_LEFT));
onView(withId(R.id.recyclerview)).perform(swipe(GeneralLocation.CENTER_RIGHT,
GeneralLocation.CENTER_LEFT));

onView(withId(R.id.recyclerview)).perform(swipe(GeneralLocation.CENTER_LEFT,
GeneralLocation.CENTER_RIGHT));
onView(withId(R.id.recyclerview)).perform(swipe(GeneralLocation.CENTER_LEFT,
GeneralLocation.CENTER_RIGHT));
onView(withId(R.id.recyclerview)).perform(swipe(GeneralLocation.CENTER_LEFT,
GeneralLocation.CENTER_RIGHT));
onView(withId(R.id.recyclerview)).perform(swipe(GeneralLocation.CENTER_LEFT,
GeneralLocation.CENTER_RIGHT));

// Verify even after the scrolling, decoration values are set correctly
assertThat(layoutManager.getLeftDecorationWidth(layoutManager.getChildAt(0)), is(0));
assertThat(layoutManager.getLeftDecorationWidth(layoutManager.getChildAt(1)), is(0));
assertThat(layoutManager.getLeftDecorationWidth(layoutManager.getChildAt(2)), is(0));
for (FlexLine flexLine : layoutManager.getFlexLines()) {
View firstViewInLine = layoutManager.getChildAt(flexLine.getFirstIndex());
if (firstViewInLine != null) {
assertThat(layoutManager.getTopDecorationHeight(firstViewInLine), is(0));
}
}
}

/**
* Creates a new flex item.
*
Expand All @@ -3228,5 +3379,12 @@ private static ViewAction swipe(CoordinatesProvider from, CoordinatesProvider to
return new GeneralSwipeAction(Swipe.FAST, from, to, Press.FINGER);
}

private static Drawable getDrawable(Context context, @DrawableRes int resId) {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
return context.getResources().getDrawable(resId, null);
} else {
return context.getResources().getDrawable(resId);
}
}
}

7 changes: 7 additions & 0 deletions flexbox/src/main/java/com/google/android/flexbox/FlexLine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,13 @@ public float getTotalFlexShrink() {
return mTotalFlexShrink;
}

/**
* @return the first view's index included in this flex line.
*/
public int getFirstIndex() {
return mFirstIndex;
}

/**
* Updates the position of the flex line from the contained view.
*
Expand Down
20 changes: 16 additions & 4 deletions flexbox/src/main/java/com/google/android/flexbox/FlexboxItemDecoration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.google.android.flexbox;

import static android.support.v7.widget.RecyclerView.NO_POSITION;

import android.content.Context;
import android.content.res.TypedArray;
import android.graphics.Canvas;
Expand Down Expand Up @@ -112,8 +114,8 @@ private void setOffsetAlongCrossAxis(Rect outRect, int position,
if (flexLines.size() == 0) {
return;
}
FlexLine lastLine = flexLines.get(flexLines.size() - 1);
if (lastLine.mLastIndex > position) {
int flexLineIndex = layoutManager.getPositionToFlexLineIndex(position);
if (flexLineIndex == 0) {
return;
}

Expand Down Expand Up @@ -142,7 +144,7 @@ private void setOffsetAlongCrossAxis(Rect outRect, int position,

private void setOffsetAlongMainAxis(Rect outRect, int position,
FlexboxLayoutManager layoutManager, List<FlexLine> flexLines, int flexDirection) {
if (isFirstItemInLine(position, flexLines)) {
if (isFirstItemInLine(position, flexLines, layoutManager)) {
return;
}

Expand Down Expand Up @@ -273,13 +275,23 @@ private boolean needsVerticalDecoration() {
/**
* @return {@code true} if the given position is the first item in a flex line.
*/
private boolean isFirstItemInLine(int position, List<FlexLine> flexLines) {
private boolean isFirstItemInLine(int position, List<FlexLine> flexLines,
FlexboxLayoutManager layoutManager) {
int flexLineIndex = layoutManager.getPositionToFlexLineIndex(position);
if (flexLineIndex != NO_POSITION &&
flexLineIndex < layoutManager.getFlexLinesInternal().size() &&
layoutManager.getFlexLinesInternal().get(flexLineIndex).mFirstIndex == position) {
return true;
}
if (position == 0) {
return true;
}
if (flexLines.size() == 0) {
return false;
}
// Check if the position is the "lastIndex + 1" of the last line in case the FlexLine which
// has the View, whose index is position is not included in the flexLines. (E.g. flexLines
// is being calculated
FlexLine lastLine = flexLines.get(flexLines.size() - 1);
return lastLine.mLastIndex == position - 1;
}
Expand Down
14 changes: 14 additions & 0 deletions flexbox/src/main/java/com/google/android/flexbox/FlexboxLayoutManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1349,8 +1349,10 @@ private int layoutFlexLineMainAxisHorizontal(FlexLine flexLine, LayoutState layo
}

if (layoutState.mLayoutDirection == LayoutState.LAYOUT_END) {
calculateItemDecorationsForChild(view, TEMP_RECT);
addView(view);
} else {
calculateItemDecorationsForChild(view, TEMP_RECT);
addView(view, indexInFlexLine);
indexInFlexLine++;
}
Expand Down Expand Up @@ -1471,8 +1473,10 @@ private int layoutFlexLineMainAxisVertical(FlexLine flexLine, LayoutState layout
childBottom -= (lp.rightMargin + getBottomDecorationHeight(view));

if (layoutState.mLayoutDirection == LayoutState.LAYOUT_END) {
calculateItemDecorationsForChild(view, TEMP_RECT);
addView(view);
} else {
calculateItemDecorationsForChild(view, TEMP_RECT);
addView(view, indexInFlexLine);
indexInFlexLine++;
}
Expand Down Expand Up @@ -2262,6 +2266,16 @@ private View findOneVisibleChild(int fromIndex, int toIndex, boolean completelyV
return null;
}

/**
* @param position the index of the view
* @return the index of the {@link FlexLine}, which includes the view whose index is passed as
* the position argument.
*/
int getPositionToFlexLineIndex(int position) {
assert mFlexboxHelper.mIndexToFlexLine != null;
return mFlexboxHelper.mIndexToFlexLine[position];
}

/**
* LayoutParams used by the {@link FlexboxLayoutManager}, which stores per-child information
* required for the Flexbox.
Expand Down

0 comments on commit b3c4e4a

Please sign in to comment.