Add view visibility checks to EpoxyVisibilityItem and decouple RecyclerView #1052
Conversation
Make EpoxyVisibilityItem more generic so it can work with a ViewGroup instead of just a RecyclerView Fix docs in OnVisibilityStateChanged
| @@ -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", | |||
There was a problem hiding this comment.
I needed to update this to run the activity scenario tests. Otherwise the tests would crash.
| @@ -10,10 +10,10 @@ | |||
| * with this annotation will be called when the visibility state is changed. | |||
| * <p> | |||
| * Annotated methods should follow this signature : | |||
| * `@OnVisibilityStateChange | |||
There was a problem hiding this comment.
These references were incorrect so I updated them
| visibleHeight, visibleWidth | ||
| ); | ||
| if (viewVisibility == View.GONE) { | ||
| epoxyHolder.visibilityChanged(0f, 0f, 0, 0); |
There was a problem hiding this comment.
do we need to check that viewVisibility changed? could it have already been gone but had the size change?
There was a problem hiding this comment.
Nice catch! I'd agree it makes sense to also check that. I'll make that change.
There was a problem hiding this comment.
@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.
|
Nice addition! We are also starting to have models outside of What I am missing where how do you use it? Don't you need to update/refactor the |
|
@eboudrant thanks for taking a look, glad you approve. You're right that usage is missing. we have a fair amount of tooling internally for using models outside of a RecyclerView, part of which handles visibility tracking for models outside of a recyclerview. This PR upstreams just part of that. I think this may be a good time to upstream more of it (maybe in a new module). |
|
@eboudrant Eli was spot on. We're using an internal class for the visibility tracking when used outside of a |
|
Well now I can't wait these other PRs 👍 |
|
Oof looks like CI is failing due to the flaky integration tests noted here: #1050 |
|
@pmecho if you rebase the tests should be fixed! then we can merge and get a release out |
Hi @pmecho ... when do you think it is a good time work on a new change to upstream the |
|
@eboudrant heh it keeps getting bumped down my priority list. This may be the nudge I needed though. I will work on it this next week and maybe have something by end of week or the beginning of the next. When are you thinking you'll need it? |
|
@pmecho the timing you propose works for me. I don't want to change your priorities list. I would say we would starts to need visibility in about a month. Thanks! |
Problems
EpoxyVisibilityItemto continue to return true even though the view is not visible on the screen.EpoxyVisibilityItemis strongly coupled with aRecyclerView. We have code in our app that uses epoxy outside of aRecyclerViewand wish to incorporate the visibility tracking with that.Solutions
updatecall and check it when thehandle...methods are called.RecyclerViewargument to use aViewGroup. This will support the existing use case as well as allowing it to be used outside ofRecyclerViews. A default constructor was added out of convenience but is by no means needed. The existing constructor really just sets the adapter position which isn't used if not using aRecyclerView