-
Notifications
You must be signed in to change notification settings - Fork 729
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
Implements visibility events for EpoxyRecyclerView/EpoxyViewHolder. #560
Implements visibility events for EpoxyRecyclerView/EpoxyViewHolder. #560
Conversation
This is cool, thanks for doing all this work - it definitely fills a good use case. I'll try to make time to review more closely later this week |
Ok, @elihart take your time. I think I need to push another change today about having a |
…nd `@OnVisibilityStateChanged` Add `OnModelVisibilityStateChangedListener` and `OnModelVisibilityChangedListener` interfaces
…ome non-related lines)
@elihart I did another update. It use same mechanism that bound/unbound listener (
I had to update the test resources that is why the huge diff ! Somehow the It's ready for review ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eboudrant just went through it - looks amazing overall! Thank you so much for such a great contribution.
Most comments are small things, the biggest high level comment I have is to decouple this from LinearLayout and EpoxyRecyclerView, and of course it will be important to have tests.
How much do you like kotlin? I would be open to having this be in kotlin, as the first kotlin code shipped in the epoxy library (I've been meaning to start using kotlin in the main library, and this seems like a good time) - if you are interested in converting the java code to kotlin.
public void visibilityStateChanged(@VisibilityState int visibilityState, @NonNull T view) { | ||
} | ||
|
||
public void visibilityChanged(float percentVisibleHeight, float percentVisibleWidth, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use @FloatRange ?
@@ -155,6 +156,13 @@ public void bind(@NonNull T view, @NonNull EpoxyModel<?> previouslyBoundModel) { | |||
public void unbind(@NonNull T view) { | |||
} | |||
|
|||
public void visibilityStateChanged(@VisibilityState int visibilityState, @NonNull T view) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these will need javadoc
/** | ||
* Enable visibility event tracking : | ||
* <p> | ||
* - Visible Event: this event is triggered when at least one pixel of the Component is visible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of enumerating the event types and their descriptions, what do you think of just linking to the annotation declaration? It seems better to not duplicate this information so we don't have to worry about keeping it in sync.
I think it would be more useful here to describe how to use the visibility tracking, ie what functions on the models to use, or we could link to a wiki page
epoxy-annotations/src/main/java/com/airbnb/epoxy/OnVisibilityStateChanged.java
Show resolved
Hide resolved
epoxy-annotations/src/main/java/com/airbnb/epoxy/OnVisibilityStateChanged.java
Show resolved
Hide resolved
+ "The listener will contribute to this model's hashCode state per the {@link\n" | ||
+ "com.airbnb.epoxy.EpoxyAttribute.Option#DoNotHash} rules.\n" | ||
+ "<p>\n" | ||
+ "You may clear the listener by setting a null value, or by calling " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove You may clear the listener by setting a null value, or by calling {@link #reset()} */
mutable models are not a pattern we encourage, and is the wrong advice for most people since so few use EpoxyAdapter
+ "The listener will contribute to this model's hashCode state per the {@link\n" | ||
+ "com.airbnb.epoxy.EpoxyAttribute.Option#DoNotHash} rules.\n" | ||
+ "<p>\n" | ||
+ "You may clear the listener by setting a null value, or by calling " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
epoxy-processor/src/main/java/com/airbnb/epoxy/ModelViewProcessor.kt
Outdated
Show resolved
Hide resolved
Add float ranges, rename to sizeInScrollingDirection, remove dependency to LinearLayoutManager, remove dependency to EpoxyRecyclerView, make visibility tracker public, add toto regarding nested list (carousel).
@Px | ||
private int sizeInScrollingDirection; | ||
|
||
private int otherSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be nice for this to be a more descriptive name
|
||
if (vi.update(itemView, recyclerView, vertical)) { | ||
// View is measured, process events | ||
vi.handleVisible(epoxyHolder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems inefficient here to have to call every single one of these handle methods. It looks like it is possible to be both visible and invisible at the same time here (if detach event = true), which feels like a bug. Perhaps you should let detach events "win" and not fire their counterpart (e.g. only fire handleInvisible if the detach event is true).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code executed inside the handle...()
methods is not complex but the user need to be careful when implementing the callbacks, especially the most granular one (onVisibilityChanged
). However I will see if some optimisation can be done...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, to be clearer about my comment, it seems like a possible source of bugs that both visible and invisible could be true if detach = true. It seems like it should be
if(!vi.handleVisible(eh)) { vi.handleInvisible(eh) }
or inverted, since invisible is the only state of those two that takes detach into account. If you don't do this, then the callbacks could emit a sequential visible + invisible event.
|
||
@Override | ||
public void onScrolled(@NonNull RecyclerView recyclerView, int dx, int dy) { | ||
processChildren(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this efficient in large lists? Unsure if you maybe want to debounce calling this with every scroll
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
processChildren()
just process RecyclerView
's children, not the whole list so it should be fine. In order to notify the VisibilityChangedEvent
this is what we need.
May be we could add a flag on the EpoxyVisibilityTracker
to specify if we are interested in such granular events.
… for visibility annotations checking.
…ation_fixes2 PR comments #2
@elihart about Kotlin, I could re-write. I did not used Kotlin because client code was all in Java (actually I started in Kotlin, then realized that so converted to Java ;) ). However I think before moving to Kotlin we should add then same level of checking than Java hava with checkstyle. I don't think the project use |
*/ | ||
@Config(sdk = [21], manifest = TestRunner.MANIFEST_PATH) | ||
@RunWith(TestRunner::class) | ||
class EpoxyVisibilityTrackerTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added unit tests (in Kotlin). @elihart please review and let me know what you think. I think theses are the first tests in the lib to assert on actual UI (RecyclerView
). Btw, I was able to fix few bugs related to use case where scrollToPosition(...)
is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eboudrant this looks just about ready! most of my comments are minor, the only large thing I'm wondering about is position changes due to other item inserts/removals
your unit tests look great, thank you for adding those. in all of the generated code tests I don't think I see one that uses the new annotations for @ModelView (or likely it's just hidden among all the changes) - can you verify one exists, or can I help you set one up?
} | ||
|
||
@Override | ||
public void onVisibilityChanged(float percentVisibleHeight, float percentVisibleWidth, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:@FloatRange annotations here too
visible = false; | ||
focusedVisible = false; | ||
adapterPosition = newAdapterPosition; | ||
lastVisibleSizeNotified = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use a constant for -1
?
} | ||
|
||
private boolean isVisible() { | ||
// true when at least one pixel of the Component is visible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be javadoc on the method
} | ||
|
||
private boolean isInvisible(boolean detachEvent) { | ||
// true when when the Component no longer has any pixels on the screen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
// true when when the Component no longer has any pixels on the screen | ||
boolean invisible = visibleSize <= 0 || detachEvent; | ||
if (invisible) { | ||
visible = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not ideal that what would appear to be a simple getter has the side effect of updating the visible
field. maybe this method can be named to reflect that?
alternatively I feel that tracking both visible
and visibleSize
could be redundant - what if we could derive isVisible()
purely from visibleSize
? -1 or null could represent detached.
I see that update
already sets visibleSize
to 0 if detachEvent is true, so the logic in this method feels unnecessary
Same thoughts apply to isUnfocusedVisible
} | ||
|
||
if (vi.getAdapterPosition() != epoxyHolder.getAdapterPosition()) { | ||
// EpoxyVisibilityItem being re-used for a different position |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment doesn't seem quite right - it implies the view was recycled to be used with a different epoxy model, but the position could also have changed because other items were inserted or removed.
I think this can cause a bug since it clears the flags that say we have notified a visibility event, so if an item stays completely on screen (and pixel position may not even change if insertion event happened off screen) we will renotify incorrectly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will revisit the tracker. Regarding the bug you are absolutely right, I missed it. I was able to reproduce the bug via unit test (disabled for now, I will look for a fix later).
} | ||
percentVisibleSize = detachEvent ? 0 : 100.f / sizeInScrollingDirection * visibleSize; | ||
if (visibleSize != sizeInScrollingDirection) { | ||
fullyVisible = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edit: i realized that fullyVisible
is tracked so we don't notify the event twice (at least that is my understanding) - maybe that could be renamed to hasNotifiedFullyVisible
?
additionally it seems more clear and safer to set this field explicitly where we do the notify call
original:
fullyVisible
is also set in isFullImpressionVisible
. seems it should be only set in one place
even better, isFullImpressionVisible
seems to be derived only from visibleSize != sizeInScrollingDirection
which are also stored in fields, instead of storing data derived from that that could get out of sync can we just do this?
boolean isFullImpressionVisible() {
return visibleSize == sizeInScrollingDirection
}
* @param heightVisible The visible height in pixel | ||
* @param widthVisible The visible width in pixel | ||
*/ | ||
void onVisibilityChanged(T model, V view, float percentHeightVisible, float percentWidthVisible, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: @FloatRange here too would be nice (although it is already in the javadoc)
* backed by an Epoxy controller. Once attached the events will be forwarded to the Epoxy model (or | ||
* to the Epoxy view when using annotations). | ||
* <p> | ||
* Note regarding nested lists: The visibility event tracking is not properly handled yet. This is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious if you have ideas on how to do this - could we automatically attach to nested recyclerviews?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automatically attach yes and if, I remember well, any nested recycler view will have to listen to its parent recycler view event (bind, attach, scroll). I planned to address it a separate PR as it should not impact code gen.
"OnVisibilityStateChangedView_throwsIfNoParams.java", | ||
"must have exactly 1 parameter (method: onVisibilityStateChanged)") | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit test for visibility annotations on @ModelView added.
Visibility events in Epoxy Inspired from Litho : https://fblitho.com/docs/visibility-handling
As this require to add few listener on the
RecyclerView
the trqacking is opt-in :Works with
EpoxyModelWithHolder
And with annotations on the custom view :
Or from the model via :
OnModelVisibilityStateChangedListener
/OnModelVisibilityChangedListener
:Unit testing for the visibility tracker can be found in
EpoxyVisibilityTrackerTest.kt