Support for visibility tracking in model groups #1091
Conversation
pmecho
force-pushed
the
pmecho--model-group-visibility-tracking
branch
from
4d1fc7d to
ad95284
Compare
|
@pmecho, just a heads-up. I am fixing Once fixed, I will be able to validate this change by adding a test case where the carousel is in a group. |
|
@eboudrant oh great! I noticed that was broken early today and put it on my list to fix for this. I'm working on adding tests for this model group change in the |
pmecho
changed the title
|
@elihart and @eboudrant this should be good to go now. |
| * 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. |
There was a problem hiding this comment.
Hum, I am wondering if we can make it works. What are the conditions when we won't receive INVISIBLE event?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
A test case would be nice.
There was a problem hiding this comment.
@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.
epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyVisibilityTracker.kt
Outdated
Show resolved
Hide resolved
| class EpoxyVisibilityTrackerModelGroupTest { | ||
|
|
||
| @get:Rule | ||
| var activityRule = activityScenarioRule<TestActivity>() |
There was a problem hiding this comment.
So I am not super familiar with activityRule, do you think we should migrate the existing tests in epoxy-adapter to epoxy-integrationtest too? They only rely on Robolectric. I am worried this change will introduce code duplication for the assert helper part.
There was a problem hiding this comment.
Yeah this module still uses Robolectric. I do think we should move the existing tests over but was planning on doing that in a separate PR to keep this one focused. ActivityScenario will be the replacement for the now deprecated Robolectric.setupActivity()
There was a problem hiding this comment.
regarding detecting added or removed models within the group, there is ModelGroupHolder.bindGroupIfNeeded which checks for changes to the models and using the data there we should be able to tell if a model is removed or added (based on model id). not sure how hard it would be to link up that code though
| eventOriginForDebug: String, | ||
| viewHolder: EpoxyViewHolder | ||
| ) { | ||
| val recyclerView = attachedRecyclerView ?: return |
There was a problem hiding this comment.
it looks like we could pass the recyclerview through from where this is called to avoid having this be nullable.
|
I looked a bit into I thought about trying to keep a reference to the epoxy holders in a given model group in the visibility tracker. When processing the group we could iterate through the children and detach those that don't exist anymore. However, since these may not be changes on the recycler view adapter, we likely won't get callbacks to reprocess the visibility of the model group. I'm going to go ahead and merge this for now and we can invest time to improve that should we need to. |
|
@pmecho @elihart we just realized the doc is not up to date and this section could probably be removed by now : https://github.com/airbnb/epoxy/wiki/Grouping-Models#visibility-events |
|
@eboudrant great catch! I can get that updated |
Epoxy models nested inside a model group don't get visibility callbacks, only the outer group gets one. This change iterates through the children of the model group and calls the same visibility checks as top level children, only with the model group view as the parent.
One thing to note is that model removals from the group won't currently get notified of visibility changes. This also means they won't get re-notified if they are added back in. This is due to the
EpoxyModelGroupnot knowing when a model is removed or not. It'll take a bit more work to handle that but I'm not sure if we have any use cases for adding/removing views from a model group at this time. I added a comment in the docs forEpoxyVisibilityTrackerthat this isn't yet supported.I created a new test inside
epoxy-integrationtestthat usesActivityScenarioso we don't add more that use the deprecatedRobolectric.setupActivitymethod. I copied theTrackerTestModel,AssertHelperand associated helper methods from the existingEpoxyVisibilityTrackerTest, with some minor modifications. This should make it easy to move those over to the integration test module in the near future.