Add support for nested RecyclerView to EpoxyVisibilityTracker - Part 2 #633
Conversation
…vertical direction for the same model.
Introduce a boolean var `viewDrawn`
…/nested_visibility_tracking # Conflicts: # epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyVisibilityItem.java # epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyVisibilityTracker.java # kotlinsample/src/main/java/com/airbnb/epoxy/kotlinsample/views/CarouselNoSnap.kt
There was a problem hiding this comment.
@eboudrant nice - did an initial pass with some thoughts. Generally the approach is good I think, but have some ideas on how to clean it up potentially.
| public void onChildAttachedToWindow(@NonNull View child) { | ||
| if (child instanceof RecyclerView) { | ||
| RecyclerView recyclerView = (RecyclerView) child; | ||
| if (recyclerView.getAdapter() instanceof BaseEpoxyAdapter) { |
There was a problem hiding this comment.
is if (recyclerView.getAdapter() instanceof BaseEpoxyAdapter) { necessary? if the child has had attach called on it then it will work, otherwise it is a no-op, right?
There was a problem hiding this comment.
The idea here was if the child is a RecyclerView which is not managed by epoxy there is no point to have visibility tracking. But that's true that the same check is in EpoxyVisibilityTracker.registerNestedRecyclerView ... we could remove it from here. This way no need to hookup onChildAttachedToWindow method.
When you say attach do you mean EpoxyVisibilityTracker.attach ? Because the idea of this implementation was to automatically attach nested RecyclerView. May be it is a bad idea and it would be better to have user explicitly call EpoxyVisibilityTracker.attach for nested RecyclerViews.
There was a problem hiding this comment.
Actually we could use a OnChildAttachStateChangeListener to automatically attach nested visibility trackers from the parent. Not sure what is the best, auto-attach nested or let the the developer attach them ?
There was a problem hiding this comment.
So as discussed I changed to use OnChildAttachStateChangeListener which is simpler.
Thanks @elihart
epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyVisibilityTracker.java
Outdated
Show resolved
Hide resolved
epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyVisibilityTracker.java
Outdated
Show resolved
Hide resolved
epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyVisibilityTracker.java
Outdated
Show resolved
Hide resolved
epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyVisibilityTracker.java
Outdated
Show resolved
Hide resolved
…ster it manually. Via a `OnAttachStateChangeListener` we will lookup for the parent to eventually register the nested tracker to the parent tracker.
There was a problem hiding this comment.
Sorry for the delay, have been on vacation for three weeks!
The description still mentions a manual method registerNestedRecyclerView - I think that was removed in favor of complete automation?
The changes look much cleaner with the tag approach :) I am a bit confused by how it works though - for the OnAttachStateChangeListener to be added to a nested recycler view we need to call attach on it, which is still manual.
Can we get rid of the new OnAttachStateChangeListener completely, and instead use the existing OnChildAttachStateChangeListener#onChildViewAttachedToWindow in the recyclerview on line 340?
So when we call attach on a recyclerview, we add a listener for child views being added to the recyclerview - in the listener's onChildViewAttachedToWindow we can check if the view is a RecyclerView, and if so it is a nested RV and we can use your existing logic.
epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyVisibilityTracker.java
Outdated
Show resolved
Hide resolved
|
@eboudrant this looks great to me - do you mind adding some tests before I merge? thanks! |
| * @param recyclerView the recycler view | ||
| * @return true if managed by an {@link BaseEpoxyAdapter} | ||
| */ | ||
| private boolean notEpoxyManaged(RecyclerView recyclerView) { |
| import com.airbnb.epoxy.ModelView | ||
| import com.airbnb.epoxy.ModelView.Size | ||
|
|
||
| @ModelView(saveViewState = true, autoLayout = Size.MATCH_WIDTH_WRAP_HEIGHT) | ||
| class CarouselNoSnap(context: Context) : Carousel(context) { | ||
|
|
||
| init { | ||
| EpoxyVisibilityTracker().attach(this) |
This is part 2 :
EpoxyVisibilityTrackeron any nested recycler view.EpoxyVisibilityTracker.Let's discuss the implementation and I will add unit tests later...