Introduce partial impression visibility states #973

mediavrog · 2020-05-15T23:17:19Z

fixes #838

introduces PARTIAL_IMPRESSION_VISIBLE and PARTIAL_IMPRESSION_INVISIBLE visibility states
threshold for partial impression can be set via EpoxyVisibilityTracker#setPartialImpressionThresholdPercentage
the threshold will be automatically applied to nested trackers as well if they do not have their own tracker set
no event is fired if the threshold cannot be met due to the element being larger than the view port and the set threshold too high to trigger. This is consistent with the behavior of FULL_IMPRESSION_VISIBLE
updated :kotlinsample to reflect the addition

Tests

I added some tolerance to the AssertHelper:
TOLERANCE_PIXELS (value=1) was currently not being tolerant, as it was compared using < instead of <=
for visibleHeight and visibleWidth I added a delta to cover the 49% case to test for an element that got VISIBLE, but not PARTIAL_IMPRESSION_VISIBLE

As for tests, since setting the threshold will add a new dimension to the test matrix, I was not sure if I should adjust the whole testsuite to cope for this. Basically all of

testDataAttachedToRecyclerView, testScrollToPosition, testMoveDataDown, testInsertData, testScrollBy, testDeleteData, testMoveDataUp

could be tested once with a threshold set, and once without.
Please let me know if I should cope for this.

Wiki Content Suggestion

Visibility Events

Partial Impression Visible: Triggered when the percentage of the visible area of a view is
at least the specified threshold. This threshold can be set via EpoxyVisibilityTracker#setPartialImpressionThresholdPercentage. This event does not fire if the threshold is set to null or if the threshold cannot be met due to the element being larger than the viewport and the set threshold too high.
Partial Impression Invisible: Triggered when the percentage of the visible area of a view is
no longer at least the specified threshold. Also see Partial Impression Visible.

…SSION_VISIBLE

elihart · 2020-05-15T23:55:16Z

epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyVisibilityTracker.java

@@ -82,6 +83,8 @@ private static void setTracker(

  private boolean onChangedEnabled = true;

+  private int partialImpressionThresholdPercentage = 0;


should it default to something, or default to null in which case handlePartialImpressionVisible is not called?

Good point. Let's default to null and not call the method as you described.
Only question is if there should be a separate method from setPartialImpressionThresholdPercentage then to disable partial threshold (and internally set to null) or if setPartialImpressionThresholdPercentage should accept a nullable Integer instead of simple int

I guess I see that it shortcircuits to doing nothing when set to 0, in which case the listeners are never called. i think this is a little confusing since a threshold of 0 seems like it would always trigger it to be called. maybe use -1 or null instead?

~~The other idea is to just not call handlePartialImpressionVisible in case of 0~~

Saw your update now. Good point about 0 as a threshold that should always trigger.

If using -1 to disable the events, would you prefer a separate method to disable the events then in regards to public API? Or just annotate setPartialImpressionThresholdPercentage's parameter with @IntRange(from = -1, to = 100) (seems a bit odd). I guess null is better in that case.

A value of 0 remains a special case, as it would have to fire if at least one pixel of the element is visible on the screen (would act the same as VISIBLE).

How about 0b6c6c2

How about @IntRange(from = 1, to = 99), possible values would be null or any int from 1 to 99
Btw, if we have 100 we could use the full impression event so 100 is not really needed.

I would leave this decision to you as the maintainers of the library as it is more about the public API and what is expected here.

for sure the extremes will basically duplicate existing events:

0 acts like VISIBLE

100 acts like FULL_IMPRESSION_VISIBLE (with an added _INVISIBLE state)

I think having the full range from 0 to 100 grew on me. I think of it this way: If other visibility events were to drop out and be removed, this one should still be fully functional and in that case 0 and 100 are valid use cases.

I don't have a strong opinion on it and since you handle 0 and 100 having @IntRange(from = 0 to = 100) is fine.

elihart · 2020-05-15T23:58:10Z

epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyVisibilityTracker.java

@@ -100,6 +103,19 @@ public void setOnChangedEnabled(boolean enabled) {
    onChangedEnabled = enabled;
  }

+  /**
+   * Set the threshold of percentage visible area to identify the partial impression view state.


maybe note that the default makes partial impression events never called?

adressed also in 0b6c6c2

eboudrant · 2020-05-18T18:24:00Z

epoxy-adapter/src/test/java/com/airbnb/epoxy/EpoxyVisibilityTrackerTest.kt

@@ -687,6 +882,7 @@ class EpoxyVisibilityTrackerTest {
    fun setup() {
        Robolectric.setupActivity(Activity::class.java).apply {
            setContentView(EpoxyRecyclerView(this).apply {
+                epoxyVisibilityTracker.setPartialImpressionThresholdPercentage(50)


That's nice to update all the tests with a partialImpressionThresholdPercentage value !

I would have let the existing tests as "it is" and added more test to validate the different partialImpressionThresholdPercentage scenarios.

Yeah that's why I asked about the test matrix. If necessary, I can write some code to dupe the cases once with and once without partial imps.

As for tests, since setting the threshold will add a new dimension to the test matrix, I was not sure if I should adjust the whole testsuite to cope for this. Basically all of
testDataAttachedToRecyclerView, testScrollToPosition, testMoveDataDown, testInsertData, testScrollBy, testDeleteData, testMoveDataUp
could be tested once with a threshold set, and once without.

kotlinsample/src/main/java/com/airbnb/epoxy/kotlinsample/models/OnVisibilityEventDrawable.kt

eboudrant · 2020-05-18T18:30:02Z

epoxy-adapter/src/main/java/com/airbnb/epoxy/EpoxyVisibilityTracker.java

+   *                            {@link VisibilityState#PARTIAL_IMPRESSION_VISIBLE} events.
+   */
+  public void setPartialImpressionThresholdPercentage(
+      @IntRange(from = 0, to = 100) int thresholdPercentage


I am wondering if 0 should be considered as null. Is it even a valid threshold for this use case?

See this discussion
#973 (comment)

eboudrant

Looks good to me, thanks !

elihart

great, thanks for this addition :) I'll make a release with it soon

mediavrog added 12 commits October 31, 2019 22:58

initial commit

fbd42c6

rename threshold to thresholdPercentage

832f83d

pass partial visibility threshold to child recycler views

8a9f4a7

update kotlin sample to handle partial state; sample uses 75 percent

09907bd

introduce PARTIAL_IMPRESSION_INVISIBLE

263ce39

be more specific in doc about when threshold is met for PARTIAL_IMPRE…

2dbbec6

…SSION_VISIBLE

remove debug statement

e36c934

kotlinsample: move partial state circle right next to visible

ad020e3

basic tests for partial visibility

b7a7ba0

test without partial viz

a30c82f

remove unused reset of partial viz threshold

288ff35

format code

d09a4a1

elihart reviewed May 15, 2020

View reviewed changes

disable partial impression via null and handle 0 like VISIBLE

0b6c6c2

mediavrog force-pushed the 838-partial-visibility branch from b6d43a5 to 0b6c6c2 Compare May 17, 2020 23:57

eboudrant reviewed May 18, 2020

View reviewed changes

eboudrant approved these changes May 18, 2020

View reviewed changes

elihart approved these changes May 18, 2020

View reviewed changes

mediavrog mentioned this pull request May 19, 2020

[kotlinsample] FULL_IMPRESSION_VISIBLE does not trigger for nested horizontal recyclerview on emulator API 19 #977

Open

elihart merged commit bfa2607 into airbnb:master May 19, 2020

elihart mentioned this pull request May 21, 2020

Release 3.11.0 #978

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce partial impression visibility states #973

Introduce partial impression visibility states #973

mediavrog commented May 15, 2020 •

edited

Loading

elihart May 15, 2020

mediavrog May 15, 2020 •

edited

Loading

elihart May 15, 2020

mediavrog May 15, 2020 •

edited

Loading

mediavrog May 16, 2020 •

edited

Loading

mediavrog May 16, 2020

mediavrog May 17, 2020 •

edited

Loading

eboudrant May 18, 2020

mediavrog May 18, 2020

eboudrant May 19, 2020

elihart May 15, 2020 •

edited

Loading

mediavrog May 17, 2020 •

edited

Loading

eboudrant May 18, 2020

mediavrog May 18, 2020

eboudrant May 18, 2020

mediavrog May 18, 2020

eboudrant left a comment

elihart left a comment

		@@ -82,6 +83,8 @@ private static void setTracker(

		private boolean onChangedEnabled = true;

		private int partialImpressionThresholdPercentage = 0;

Introduce partial impression visibility states #973

Introduce partial impression visibility states #973

Conversation

mediavrog commented May 15, 2020 • edited Loading

Tests

Wiki Content Suggestion

Choose a reason for hiding this comment

mediavrog May 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mediavrog May 15, 2020 • edited Loading

Choose a reason for hiding this comment

mediavrog May 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mediavrog May 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elihart May 15, 2020 • edited Loading

Choose a reason for hiding this comment

mediavrog May 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eboudrant left a comment

Choose a reason for hiding this comment

elihart left a comment

Choose a reason for hiding this comment

mediavrog commented May 15, 2020 •

edited

Loading

mediavrog May 15, 2020 •

edited

Loading

mediavrog May 15, 2020 •

edited

Loading

mediavrog May 16, 2020 •

edited

Loading

mediavrog May 17, 2020 •

edited

Loading

elihart May 15, 2020 •

edited

Loading

mediavrog May 17, 2020 •

edited

Loading