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

Support for visibility tracking in model groups #1091

Merged
merged 8 commits into from
Nov 25, 2020
Merged
Show file tree
Hide file tree
Changes from all 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: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
#
- Add support for `EpoxyModelGroup` in the `EpoxyVisibilityTracker` (#1091)

# 4.2.0 (Nov 11, 2020)
- Add notify model changed method (#1063)
- Update to Kotlin 1.4.20-RC and remove dependency on kotlin-android-extensions
pmecho marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ import java.util.HashMap
* backed by an Epoxy controller. Once attached the events will be forwarded to the Epoxy model (or
* to the Epoxy view when using annotations).
*
* Note that support for visibility events on an [EpoxyModelGroup] is somewhat limited. Only model
* additions will receive visibility events. Models that are removed from the group will not receive
* events (e.g. [VisibilityState.INVISIBLE]) because the model group does not keep a reference,
* nor does it get notified of model removals.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hum, I am wondering if we can make it works. What are the conditions when we won't receive INVISIBLE event?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@eboudrant If the model group no longer has the model in it, it will not find that model's view on a view change, thus not re-compute visibility on it. If the model group does still retain the model but the visibility changes then I think it should work just fine. I can try to add a test for that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

A test case would be nice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@eboudrant just added a couple of test cases for this. Hiding/showing models works as expected. The only outstanding problem is when the model is completely removed from the group. Looking into Eli's suggestion to see if we can do something there.

*
* @see OnVisibilityChanged
*
* @see OnVisibilityStateChanged
Expand Down Expand Up @@ -208,20 +213,77 @@ class EpoxyVisibilityTracker {
// Preemptive check for child's parent validity to prevent `IllegalArgumentException` in
// `getChildViewHolder`.
val isParentValid = child.parent == null || child.parent === recyclerView
val holder = if (isParentValid) recyclerView.getChildViewHolder(child) else null
if (holder is EpoxyViewHolder) {
val changed = processVisibilityEvents(
val viewHolder = if (isParentValid) recyclerView.getChildViewHolder(child) else null
if (viewHolder is EpoxyViewHolder) {
val epoxyHolder = viewHolder.holder
processChild(recyclerView, child, detachEvent, eventOriginForDebug, viewHolder)
if (epoxyHolder is ModelGroupHolder) {
processModelGroupChildren(recyclerView, epoxyHolder, detachEvent, eventOriginForDebug)
}
}
}

/**
* Loop through the children of the model group and process visibility events on each one in
* relation to the model group's layout. This will attach or detach trackers to any nested
* [RecyclerView]s.
*
* @param epoxyHolder the [ModelGroupHolder] with children to process
* @param detachEvent true if the child was just detached
* @param eventOriginForDebug a debug strings used for logs
*/
private fun processModelGroupChildren(
recyclerView: RecyclerView,
epoxyHolder: ModelGroupHolder,
detachEvent: Boolean,
eventOriginForDebug: String
) {
// Iterate through models in the group and process each of them instead of the group
for (groupChildHolder in epoxyHolder.viewHolders) {
// Since the group is likely using a ViewGroup other than a RecyclerView, handle the
// potential of a nested RecyclerView. This cannot be done through the normal flow
// without recursively searching through the view children.
if (groupChildHolder.itemView is RecyclerView) {
if (detachEvent) {
processChildRecyclerViewDetached(groupChildHolder.itemView)
} else {
processChildRecyclerViewAttached(groupChildHolder.itemView)
}
}
processChild(
recyclerView,
holder,
groupChildHolder.itemView,
detachEvent,
eventOriginForDebug
eventOriginForDebug,
groupChildHolder
)
if (changed) {
if (child is RecyclerView) {
val tracker = nestedTrackers[child]
tracker?.processChangeEvent("parent")
}
}
}
}

/**
* Process visibility events for a view and propagate to a nested tracker if the view is a
* [RecyclerView].
*
* @param child the view to process for visibility event
* @param detachEvent true if the child was just detached
* @param eventOriginForDebug a debug strings used for logs
* @param viewHolder the view holder for the child view
*/
private fun processChild(
recyclerView: RecyclerView,
child: View,
detachEvent: Boolean,
eventOriginForDebug: String,
viewHolder: EpoxyViewHolder
) {
val changed = processVisibilityEvents(
recyclerView,
viewHolder,
detachEvent,
eventOriginForDebug
)
if (changed && child is RecyclerView) {
nestedTrackers[child]?.processChangeEvent("parent")
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:orientation="vertical" />
Loading