Handle partial visibility state

[mediavrog]

Requirements for our project have visibility tracking of elements at 50% visibility. Since both VisibilityState.FOCUSED_VISIBLE and VisibilityState.FULL_IMPRESSION_VISIBLE do not cover this state, we were thinking of extending the visibility tracker.

The idea would be to offer some api to define a threshold (range 0..1) for when a visibility state would be triggered. This could either be applied to the VisibilityState.FULL_IMPRESSION_VISIBLE, which would redefine what a full impression is in the app context, or preferably introduce a new state VisibilityState.PARTIAL_IMPRESSION_VISIBLE. Implementation would be very similar to VisibilityItem#handleFullImpressionVisible.

Now we could probably handle this using the visibilityChanged, but it would be more involved to not double-count the impression and it would trigger quite often (so we would not be able to disable the noisy onChanged (visibilityChangedEnabled).

I think there might be more use-cases which would warrant to have a single-event impression for a certain threshold of visibility. If you think it might be useful, I will whip up a PR within the next weeks.

[eboudrant]

I think that is a good idea. At this point I am wondering if we should have a configurable threshold value to trigger the VisibilityState.FOCUSED_VISIBLE or introduce a new VisibilityState.PARTIAL_IMPRESSION_VISIBLE state (could be named VisibilityState.THRESHOLD__VISIBLE ?). I am leaning toward a new state. Also I think we may need to have the _INVISIBLE state too.

[elihart]

I haven't personally used the visibility system, so I don't have much insight, but I support whatever you think is best @eboudrant 👍

[mediavrog]

Hi @eboudrant, sorry for leaving this discussion for a long time.
I have created a draft of a PR over at https://github.com/smartnews/epoxy/pull/1/files, could you please give feedback on the general direction of this approach? Does this seem reasonable?

TODO

• tests
• threshold _INVISIBLE state?

Do you think one threshold would be ok, or should it support defining a list of thresholds?

[eboudrant]

Thanks, I added few comments but overall looks good.

Could you clarify "defining a list of thresholds"? I think the only case were we wanted to use it would be with one threshold only. Do you need multiple?

[mediavrog]

We do not need multiple right now. I just wondered if anybody would, but I guess for that case they can still revert to using onVisibilityChanged. So I also think a single threshold is enough actually.

[mediavrog]

I addressed some feedback from @eboudrant and think in general the code is in good shape so I opened a PR here now. Let's continue the discussion directly on the PR.