Performance Improvements #66

bradbyte · 2017-09-22T15:03:47Z

Thanks for this package! this is probably the best sortable implementing I've worked with yet.

Here are some improvements I needed in order to use in my app.

Only increment the keys when the data keys change
I'm updating the content in the data object frequently but this results in constant rerendering. I traced it down to the componentWillReceiveProps method that checks if the data object changes. It then resets the keys causing the rerender. No use of shouldComponentUpdate would work on my end since the entire tree gets rebuilt. I corrected this by checking if the data keys change, otherwise it will simply update the state for data.
Expose toggleRowActive method to rendered component
This was a must for me as I have text input in my row and other Touchable* items... I want to control when the PanResponder should move the row. (Expose toggleActive to children #65)
Only capture relevant gestures
Currently, the responder will steal horizontal swipes even if horizontal={false} (Swipe and Sort #30) which prevents the use of a Swipe row.

jlo1 · 2017-09-25T19:52:20Z

This is fantastic! Thanks @bradbumbalough! Could we get this merged in @gitim? Thanks!

jlo1 · 2017-09-25T23:47:25Z

src/SortableList.js

 });
-
+ } else if (data && nextData && !shallowEqual(data, nextData)) {
+ this.setState({ data: nextData });


issue with code change here: this won't update the order when rendering if the order is different

Are you providing the order prop? It works fine for me, but I might not have factored in if you don't provide it. The goal is that the data gets rerendered without the keys changing. This line was the issue for me: https://github.com/gitim/react-native-sortable-list/pull/66/files/e0f5ce66ba855cf43e85a3f19456c0d5f38e67b3#diff-e7d95e98bb0472979c2ad48071012cb9R94.

yes, I was providing the order prop. What would happen is that the first if statement is false (since the keys of the data were the same), but the second if statement failed (since the actual shallowEqual comparison failed).
So this would trigger the state to update just the data, even though order should be updated to nextOrder as well.

Does that make sense?

I think so... is it if you're updating order and data at the same time? whereas right now it's assuming each happen in separate calls?

Correct. Use case for me is that when a user throws away past changes, we want to restore the last savepoint's order and data (hence, updating both at once)

What if we combined the data and order change into one if branch? would that work?

Maybe something like this:

else if ((data && nextData && !shallowEqual(data, nextData)) || (order && nextOrder && !shallowEqual(order, nextOrder)) { this.setState({ data: nextData, order: nextOrder });

jlo1 · 2017-09-25T23:51:41Z

src/SortableList.js

 const {data, order} = this.state;
 let {data: nextData, order: nextOrder} = nextProps;
-
- if (data && nextData && !shallowEqual(data, nextData)) {


@gitim, the check for shallowEqual here will always return false when the data is a nested object.
Should https://github.com/gitim/react-native-sortable-list/blob/v0.0.16/src/utils/shallowEqual.js#L17
instead be:

ignoring object comparisons,

or doing a deep comparison, via JSON.stringify(objA[keysA[i]]) !== JSON.stringify(objB[keysA[i]])?

Thanks!

Do you mean if the data is part of a nested object? (i.e. redux?).

For my case, data is a map of keys to values, where some values are maps themselves. When comparing the maps against each other, shallowEqual will always return false.

e.g.

nestedObjA = {1: "a", 2: "b", 3: "c"}; nestedObjB = {1: "a", 2: "b", 3: "c"}; // shallowEqual will always return false for data with nested objects myData = {"key" nestedObjA}; nestedObja === nestedObjB; // returns false JSON.stringify(nestedObjA) === JSON.stringify(nestedObjB); // returns true

Interesting.. I think I'm also using nested values (each row represents a model with varied maps and attributes) but I'm not having any issues getting the new orders.

It was my initial intension, because it is not expected that nested objects will be cloned

gitim · 2017-09-26T09:24:06Z

Thanks for your contribution, I will review that in this week

gitim · 2017-09-27T18:47:36Z

src/SortableList.js

 let {data: nextData, order: nextOrder} = nextProps;
-
- if (data && nextData && !shallowEqual(data, nextData)) {
+ if (data && nextData && Object.keys(data).toString() !== Object.keys(nextData).toString()) {


In case when values of data changes we need layout recalculation, for example
oldData = {1: 'Some text. Some text. Some text. Some text. Some text.'}
new Data = {1: 'Some text. Some text. Some text. Some text. Some text. Some text. Some text. Some text. Some text. Some text.'}

here a key was not changed, but value changed and a new row will be higher...

And generally about this commit, does is it really bottle neck for performance in your apps? I suppose no, because unlikely data is frequently changing

Is there a way to recalculate the layout without changing the keys? It's not the performance but the screen flash with the rows remounting... any change (typing, etc) causes it to get the new keys.

I think it maybe possible to implement it better, this was not in my mind when I wrote this component, because this feature was not need in our app, I added it after someone in issues asked me.

Currently, the component uses same rows for getting layout and showing to user, it firstly renders rows with right: 9999 and then when new layouts calculated shows them to user. For eliminating flash it should separately render rows for layout calculation and for user presentation

gitim · 2017-09-27T18:56:50Z

src/Row.js

+ const vx = Math.abs(gestureState.vx)
+
+ return this._active && (this.props.horizontal ? vx > vy : vy > vx);
+ },


This should allow for child horizontal pan responders (ie swiping).

Do you tested? Or it is just a guess?

Yes I was able to use react-native-swipe-list-view now.

Hey @bradbumbalough , would you mind posting a minimal example? I have been struggling to produce a working sortable and swipeable list for some time now. It would be very appreciated.

Tested with react-native-swipe-list-view too, works, cherry-picked to master. Thanks @bradbumbalough

Hi, I tried it in order to make react-native-sortable-list and react-native-swipe-list-view (with SwiperRow), the row swipes well but is not sortable anymore. Is there a working example somewhere ? Thanks !

- This is a performance improvement when the data object changes but not in a way that requires recalcuating the row layout. A third option in componentWillReceiveProps was also added to update the data when it changes, similar to order.

- Use `manuallyActivateRows={true}` to have the `toggleRowActive` prop exposed to rendered rows. - The out of box timer with pan responder will not activate rows.

- This should allow for child horizontal pan responders (ie swiping).

- Error message was cannot read property 'height' of undefined.

bradbyte · 2017-10-23T14:12:49Z

Rebased on master.

joelash · 2017-11-08T04:05:43Z

What the status of merging this branch?

bradbyte · 2017-11-17T14:04:38Z

@gitim Thanks for merging!! what are your thoughts on tackling the main performance issue from d1c0d05?

gitim · 2017-11-17T14:09:46Z

We definitely try to solve it as there are already several issues were opened, I'll review your solution again soon.

bradbyte · 2017-11-17T14:12:23Z

Thanks! I'll admit my solution might not be solving it at the root, but it seemed to be stemming from the keys being recreated and then having the screen flashes. What I don't know (and will actually be implementing in our app next week) is how my fix handles content change (i.e. height increasing) but the key being the same.

rikur · 2018-01-27T04:27:56Z

I could really use this, I'll test the fork and report any issues.

jpshelley · 2018-03-05T21:28:45Z

Any help needed on this PR? The idea of using the key's to reduce renders seems like a solid approach.

gianpaj · 2018-11-26T20:06:17Z

Could anybody take another stab at this? I am not well versed at animations or "algorithms" 😂

I've created a Snack on Expo since it's all JavaScript:
https://snack.expo.io/@gianpaj/react-native-sortable-list-pr-66

Copy the contents of AppHorizonal.js into App.js for the other example

jlo1 reviewed Sep 25, 2017

View reviewed changes

gitim reviewed Sep 27, 2017

View reviewed changes

Brad Bumbalough added 4 commits October 23, 2017 10:07

Expose toggleRowActive to renderItem component

e08aff4

- Use `manuallyActivateRows={true}` to have the `toggleRowActive` prop exposed to rendered rows. - The out of box timer with pan responder will not activate rows.

Only set pan responder on relevant gestures

6b9fae4

- This should allow for child horizontal pan responders (ie swiping).

Fix for when rowLayouts doesn't have layout for key yet

45214ff

- Error message was cannot read property 'height' of undefined.

bradbyte force-pushed the bb/performance branch from 9fdda7c to 45214ff Compare October 23, 2017 14:08

gitim mentioned this pull request Nov 16, 2017

Drag handler which allow dragging item when long-touch on specific part of the list item #74

Closed

bradbyte closed this Jan 27, 2020

Performance Improvements #66

Performance Improvements #66

Uh oh!

Conversation

bradbyte commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jlo1 commented Sep 25, 2017

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

Choose a reason for hiding this comment

bradbyte Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlo1 Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitim commented Sep 26, 2017

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

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

bradbyte commented Oct 23, 2017

joelash commented Nov 8, 2017

bradbyte commented Nov 17, 2017

gitim commented Nov 17, 2017

bradbyte commented Nov 17, 2017

rikur commented Jan 27, 2018

jpshelley commented Mar 5, 2018

gianpaj commented Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Labels

9 participants

bradbyte commented Sep 22, 2017 •

edited

Loading

bradbyte Sep 27, 2017 •

edited

Loading

jlo1 Sep 26, 2017 •

edited

Loading

gianpaj commented Nov 26, 2018 •

edited

Loading