Skip to content
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

[ScrollView] Incorrect event coalescing causes onChangeVisibleRows to fail #1782

Closed
paramaggarwal opened this issue Jun 29, 2015 · 2 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@paramaggarwal
Copy link
Contributor

Performance sensitive issue - shows on iPhone 4s.

ListView has a method called onChangeVisibleRows that is called whenever the rows visible on screen change. This method is critical to be able to implement deletion/creation of views and hence be conservative in memory usage. I have an infinite scrolling view which uses this method to only render the full rows for what is visible on screen and put placeholders for everything else.

In the RCTEventDispatcher, we coalesce events that are meant to be sent across the bridge. They are dequeued on each and every RCTFrameUpdate.

But when we coalesce the events for RCTScrollView, we are only picking the latest event. This means that the updatedChildFrames generated here are lost and the ListView in JS, does not know about the new frames of the rows - and hence cannot correctly calculate the onChangeVisibleRows changes.

To test my theory, I turned off coalescing in code and JS receives all the events. Hence, the coalescing method needs to take into account the childUpdatedFrames object from previous events here:

- (id<RCTEvent>)coalesceWithEvent:(id<RCTEvent>)newEvent
{
  return newEvent;
}

Currently, it is blindly picking up the latest event. It should instead do a merge of the events taking care of childUpdatedFrames. I'll raise a PR with the change soon, till then looking for thoughts and comments.

@paramaggarwal paramaggarwal changed the title [ListView] Event coalescing causes onChangeVisibleRows to fail [ListView] Incorrect event coalescing causes onChangeVisibleRows to fail Jun 29, 2015
@paramaggarwal paramaggarwal changed the title [ListView] Incorrect event coalescing causes onChangeVisibleRows to fail [ScrollView] Incorrect event coalescing causes onChangeVisibleRows to fail Jun 29, 2015
paramaggarwal added a commit to paramaggarwal/react-native that referenced this issue Jun 29, 2015
The `ScrollView` sends important `updatedChildFrames` data to the `ListView` to be able to implement `onChangeVisibleRows` method. Coalescing operates very strongly on older devices like the iPhone 4s where this data is then lost.

Fixes facebook#1782.
paramaggarwal added a commit to paramaggarwal/react-native that referenced this issue Jul 3, 2015
The `ScrollView` sends important `updatedChildFrames` data to the `ListView` to be able to implement `onChangeVisibleRows` method. Coalescing operates very strongly on older devices like the iPhone 4s where this data is then lost.

Fixes facebook#1782.
@VonD
Copy link
Contributor

VonD commented Jul 6, 2015

@paramaggarwal how does the event fail ?

Possibly related issue here : the visibleRows param passed to the onChangeVisibleRows event has sometimes no rows : {"s1": {}}, even though several rows are visible on screen.

I haven't been able to find a strict test case for this, but it seems to happen after rendering some rows, when scrolling rapidly.

Applying the fix from #1783 does not solve the issue for me.

@a2 a2 closed this as completed in 0f2d8e6 Jul 7, 2015
@paramaggarwal
Copy link
Contributor Author

@VonD The problem you are seeing where onChangeVisibleRows gave you {"s1": {}} sometimes is exactly what I was facing also. And spending a few days on intense debugging brought me to the solution in this PR. Where multiple events were clubbed into one - and the critical childUpdatedFrames data was sometimes lost. With this fix, onChangeVisibleRows is much more reliable.

If you are still facing the problem - maybe there are some other places also where this data is lost. Try debugging it! :)

paramaggarwal added a commit to paramaggarwal/react-native that referenced this issue Jul 17, 2015
The `ScrollView` sends important `updatedChildFrames` data to the `ListView` to be able to implement `onChangeVisibleRows` method. Coalescing operates very strongly on older devices like the iPhone 4s where this data is then lost.

Fixes facebook#1782.
Fanghao pushed a commit to discord/react-native that referenced this issue Jul 23, 2015
Summary:
The `ScrollView` sends important `updatedChildFrames` data to the `ListView` to be able to implement `onChangeVisibleRows` method. Coalescing operates very strongly on older devices like the iPhone 4s where this data is then lost.

Fixes facebook#1782.

`ListView` has a method called `onChangeVisibleRows` that is called whenever the rows visible on screen change. This method is critical to be able to implement deletion/creation of views and hence be conservative in memory usage. I have an infinite scrolling view which uses this method to only render the full rows for what is visible on screen and put placeholders for everything else.

In the `RCTEventDispatcher`, we [coalesce events](https://github.com/facebook/react-native/blob/522fd33d6f3c8fb339b0dde35b05df34c1233306/React/Base/RCTEventDispatcher.m#L135-L152) that are meant to be sent across the bridge. They are [dequeued](https://github.com/facebook/react-native/blob/522fd33d6f3c8fb339b0dde35b05df34c1233306/React/Base/RCTEventDispatcher.m#L180-L188) on each
Closes facebook#1783
Github Author: Param Aggarwal <paramaggarwal@gmail.com>
@facebook facebook locked as resolved and limited conversation to collaborators Jul 7, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

3 participants