Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add view visibility checks to EpoxyVisibilityItem and decouple RecyclerView #1052

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion blessedDeps.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ rootProject.ext.deps = [
androidArchCoreTesting : "android.arch.core:core-testing:$ANDROID_ARCH_TESTING",
androidTestRunner : "com.android.support.test:runner:$ANDROID_TEST_RUNNER",
androidAnnotations : "androidx.annotation:annotation:$ANDROIDX_ANNOTATION",
androidTestCore : "androidx.test:core:1.2.0",
androidTestCore : "androidx.test:core:1.3.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed to update this to run the activity scenario tests. Otherwise the tests would crash.

androidTestExtJunitKtx : "androidx.test.ext:junit-ktx:1.1.2",
androidLegacy : "androidx.legacy:legacy-support-v4:$ANDROIDX_LEGACY",
versionedParcelable : "androidx.versionedparcelable:versionedparcelable:$ANDROIDX_VERSIONED_PARCELABLE",
dataBindingAdapters : "androidx.databinding:databinding-adapters:$ANDROIDX_DATABINDING_ADAPTERS",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,17 @@

import android.graphics.Rect;
import android.view.View;
import android.view.ViewGroup;

import androidx.annotation.IntRange;
import androidx.annotation.NonNull;
import androidx.annotation.Px;
import androidx.recyclerview.widget.RecyclerView;

/**
* This class represent an item in the {@link androidx.recyclerview.widget.RecyclerView} and it is
* This class represent an item in a {@link android.view.ViewGroup} and it is
* being reused with multiple model via the update method. There is 1:1 relationship between an
* EpoxyVisibilityItem and a child within the {@link androidx.recyclerview.widget.RecyclerView}.
* EpoxyVisibilityItem and a child within the {@link android.view.ViewGroup}.
*
* It contains the logic to compute the visibility state of an item. It will also invoke the
* visibility callbacks on {@link com.airbnb.epoxy.EpoxyViewHolder}
Expand Down Expand Up @@ -46,11 +47,15 @@ class EpoxyVisibilityItem {
private boolean fullyVisible = false;
private boolean visible = false;
private boolean focusedVisible = false;
private int viewVisibility = View.GONE;

/** Store last value for de-duping */
private int lastVisibleHeightNotified = NOT_NOTIFIED;
private int lastVisibleWidthNotified = NOT_NOTIFIED;

EpoxyVisibilityItem() {
}

EpoxyVisibilityItem(int adapterPosition) {
reset(adapterPosition);
}
Expand All @@ -59,10 +64,10 @@ class EpoxyVisibilityItem {
* Update the visibility item according the current layout.
*
* @param view the current {@link com.airbnb.epoxy.EpoxyViewHolder}'s itemView
* @param parent the {@link androidx.recyclerview.widget.RecyclerView}
* @param parent the {@link android.view.ViewGroup}
* @return true if the view has been measured
*/
boolean update(@NonNull View view, @NonNull RecyclerView parent, boolean detachEvent) {
boolean update(@NonNull View view, @NonNull ViewGroup parent, boolean detachEvent) {
// Clear the rect before calling getLocalVisibleRect
localVisibleRect.setEmpty();
boolean viewDrawn = view.getLocalVisibleRect(localVisibleRect) && !detachEvent;
Expand All @@ -72,6 +77,7 @@ boolean update(@NonNull View view, @NonNull RecyclerView parent, boolean detachE
viewportWidth = parent.getWidth();
visibleHeight = viewDrawn ? localVisibleRect.height() : 0;
visibleWidth = viewDrawn ? localVisibleRect.width() : 0;
viewVisibility = view.getVisibility();
return height > 0 && width > 0;
}

Expand Down Expand Up @@ -139,11 +145,15 @@ boolean handleChanged(EpoxyViewHolder epoxyHolder, boolean visibilityChangedEnab
boolean changed = false;
if (visibleHeight != lastVisibleHeightNotified || visibleWidth != lastVisibleWidthNotified) {
if (visibilityChangedEnabled) {
epoxyHolder.visibilityChanged(
100.f / height * visibleHeight,
100.f / width * visibleWidth,
visibleHeight, visibleWidth
);
if (viewVisibility == View.GONE) {
epoxyHolder.visibilityChanged(0f, 0f, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to check that viewVisibility changed? could it have already been gone but had the size change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! I'd agree it makes sense to also check that. I'll make that change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elihart While testing this I realized the size won't change while it's gone since the layout pass won't re-measure. I did find a case that makes us need this last visibility notified check though. If the view transitions from gone to visible the size won't change so we would need that check. I've added it and this should be good to go now.

} else {
epoxyHolder.visibilityChanged(
100.f / height * visibleHeight,
100.f / width * visibleWidth,
visibleHeight, visibleWidth
);
}
}
lastVisibleHeightNotified = visibleHeight;
lastVisibleWidthNotified = visibleWidth;
Expand All @@ -153,7 +163,7 @@ boolean handleChanged(EpoxyViewHolder epoxyHolder, boolean visibilityChangedEnab
}

private boolean isVisible() {
return visibleHeight > 0 && visibleWidth > 0;
return viewVisibility == View.VISIBLE && visibleHeight > 0 && visibleWidth > 0;
}

private boolean isInFocusVisible() {
Expand All @@ -163,9 +173,10 @@ private boolean isInFocusVisible() {
// The model has entered the focused range either if it is larger than half of the viewport
// and it occupies at least half of the viewport or if it is smaller than half of the viewport
// and it is fully visible.
return (totalArea >= halfViewportArea)
? (visibleArea >= halfViewportArea)
: totalArea == visibleArea;
return viewVisibility == View.VISIBLE &&
((totalArea >= halfViewportArea)
? (visibleArea >= halfViewportArea)
: totalArea == visibleArea);
}

private boolean isPartiallyVisible(@IntRange(from = 0, to = 100) int thresholdPercentage) {
Expand All @@ -176,11 +187,11 @@ private boolean isPartiallyVisible(@IntRange(from = 0, to = 100) int thresholdPe
final int visibleArea = visibleHeight * visibleWidth;
final float visibleAreaPercentage = (visibleArea / (float) totalArea) * 100;

return visibleAreaPercentage >= thresholdPercentage;
return viewVisibility == View.VISIBLE && visibleAreaPercentage >= thresholdPercentage;
}

private boolean isFullyVisible() {
return visibleHeight == height && visibleWidth == width;
return viewVisibility == View.VISIBLE && visibleHeight == height && visibleWidth == width;
}

void shiftBy(int offsetPosition) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@
* with this annotation will be called when the visibility state is changed.
* <p>
* Annotated methods should follow this signature :
* `@OnVisibilityStateChange
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These references were incorrect so I updated them

* public void method(@VisibilityState int state)`
* `@OnVisibilityStateChanged
* public void method(@Visibility int state)`
* <p>
* Possible States are declared in {@link com.airbnb.epoxy.OnModelVisibilityStateChangedListener}.
* Possible States are declared in {@link com.airbnb.epoxy.VisibilityState}.
* <p>
* The equivalent methods on the model is
* {@link com.airbnb.epoxy.EpoxyModel#onVisibilityStateChanged}
Expand Down
4 changes: 4 additions & 0 deletions epoxy-integrationtest/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ dependencies {
testImplementation rootProject.deps.mockito
testImplementation rootProject.deps.mockito_inline
testImplementation rootProject.deps.androidTestCore
testImplementation rootProject.deps.androidTestExtJunitKtx
testImplementation('androidx.test.espresso:espresso-core:3.1.0-alpha2', {
exclude group: 'com.android.support', module: 'support-annotations'
})
}

repositories {
Expand Down
2 changes: 2 additions & 0 deletions epoxy-integrationtest/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
android:label="@string/app_name"
android:theme="@style/Theme.AppCompat.Light">

<activity android:name=".TestActivity"/>

</application>

</manifest>
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package com.airbnb.epoxy.integrationtest

import android.app.Activity

/**
* Empty activity used for view testing.
*/
class TestActivity : Activity()
Loading