Skip to content

Commit

Permalink
Android: Update coalesce to prefer newest event
Browse files Browse the repository at this point in the history
Summary:
The only callsite of `coalesce` looks like this:

```
newEvent.coalesce(oldEvent);
```

The default `coalesce` implementation returns the event with the most recent timestamp. When the events have the same timestamp then, using the variable names from above, `coalesce` returns `oldEvent`.

This change updates `coalesce`'s implementation to make it explicit that it returns `this` (`newEvent` in the variable names from above) in the case of a tie.

The motivation for this change is related to scroll events. In my team's app, we were seeing scroll events being emitted with the same timestamp and the coalescing logic was causing the oldest scroll event to be chosen. This was causing our JavaScript code to receive stale scroll information and the way the JavaScript code utilized this stale scroll information resulted in the ScrollView settling on the wrong scroll position.

**Test plan (required)**

Verified that scroll events work properly in a ScrollView in a test app. Also, my team's app uses this
Closes #11080

Differential Revision: D4226152

Pulled By: andreicoman11

fbshipit-source-id: d28a2569225ca95de662f2239a0fa14de0540a7d
  • Loading branch information
Adam Comella authored and Facebook Github Bot committed Nov 23, 2016
1 parent be5868d commit ea913e4
Showing 1 changed file with 2 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,13 @@ public boolean canCoalesce() {

/**
* Given two events, coalesce them into a single event that will be sent to JS instead of two
* separate events. By default, just chooses the one the is more recent.
* separate events. By default, just chooses the one the is more recent, or {@code this} if timestamps are the same.
*
* Two events will only ever try to be coalesced if they have the same event name, view id, and
* coalescing key.
*/
public T coalesce(T otherEvent) {
return (T) (getTimestampMs() > otherEvent.getTimestampMs() ? this : otherEvent);
return (T) (getTimestampMs() >= otherEvent.getTimestampMs() ? this : otherEvent);
}

/**
Expand Down

0 comments on commit ea913e4

Please sign in to comment.