What about reordering items without rerendering? #259

Merged
merged 7 commits into from Aug 23, 2012

Conversation

Projects
None yet
6 participants
@mbest
Owner

mbest commented May 23, 2012

now algorithm findEditScriptFromEditDistanceMatrix() has 3 statuses: added, deleted, retained...
what about reordered? (this feature will provide reordering node without removing and adding)

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Jan 5, 2012

Owner

The issue is that checking for moved items is more work computationally. Check out what I posted on the forum about this: http://groups.google.com/group/knockoutjs/browse_thread/thread/7b159e1d230cb138/3d7fc2e8fb1273be

Owner

mbest commented Jan 5, 2012

The issue is that checking for moved items is more work computationally. Check out what I posted on the forum about this: http://groups.google.com/group/knockoutjs/browse_thread/thread/7b159e1d230cb138/3d7fc2e8fb1273be

@vamp

This comment has been minimized.

Show comment Hide comment
@vamp

vamp Jan 6, 2012

mbest, thanks for your response

vamp commented Jan 6, 2012

mbest, thanks for your response

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Mar 4, 2012

Owner

I spent some time this week to see if I could modify the compareArrays function to check for moved items without reducing performance. The main performance improvement would come from optimizing the existing compareArrays code. I used jsPerf extensively to test different scenarios and code combinations. Often, something would be faster in one browser but be slower in another. Finally, though, i came up with a new compareArrays function that finds moved items and is generally faster than the old version.

Here's a jsPerf test page that includes debug and minified versions of the current code and my updates. It tests each version with five comparisons--four from the compareArrays spec and a fifth that I added. Testing in the latest versions of Chrome, Firefox, and Internet Explorer shows that the new version is about 30% faster in Chrome and IE and about 10% faster in Firefox.

Once Knockout 2.1 is released, I'll work on doing a pull request with this update.

Owner

mbest commented Mar 4, 2012

I spent some time this week to see if I could modify the compareArrays function to check for moved items without reducing performance. The main performance improvement would come from optimizing the existing compareArrays code. I used jsPerf extensively to test different scenarios and code combinations. Often, something would be faster in one browser but be slower in another. Finally, though, i came up with a new compareArrays function that finds moved items and is generally faster than the old version.

Here's a jsPerf test page that includes debug and minified versions of the current code and my updates. It tests each version with five comparisons--four from the compareArrays spec and a fifth that I added. Testing in the latest versions of Chrome, Firefox, and Internet Explorer shows that the new version is about 30% faster in Chrome and IE and about 10% faster in Firefox.

Once Knockout 2.1 is released, I'll work on doing a pull request with this update.

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Mar 5, 2012

Owner

I added a second jsPerf page to show the performance of each comparison individually.

Owner

mbest commented Mar 5, 2012

I added a second jsPerf page to show the performance of each comparison individually.

@vamp

This comment has been minimized.

Show comment Hide comment
@vamp

vamp Mar 5, 2012

looks good, this optimization will be produced in 2.1?

vamp commented Mar 5, 2012

looks good, this optimization will be produced in 2.1?

@SteveSanderson

This comment has been minimized.

Show comment Hide comment
@SteveSanderson

SteveSanderson May 22, 2012

Contributor

Michael, I see you marked this as "major impact". Do you have a specific implementation in mind? Looking at the jsPerf posting, the code looks minified or obfuscated so it's kind of difficult to follow :)

I'd certainly not think a 10-30% gain would justify a "major impact" change, but perhaps you could clarify what change you have in mind.

Contributor

SteveSanderson commented May 22, 2012

Michael, I see you marked this as "major impact". Do you have a specific implementation in mind? Looking at the jsPerf posting, the code looks minified or obfuscated so it's kind of difficult to follow :)

I'd certainly not think a 10-30% gain would justify a "major impact" change, but perhaps you could clarify what change you have in mind.

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest May 22, 2012

Owner

"Major" in this case refers to the amount of code change, primarily replacing the compareArrays code. Interface-wise, the impact is moderate:

  • The compareArrays output will be backwards compatible. If an item has moved, there will still be both deleted and added entries, but they will have an additional property to indicate that they moved from/to another location.
  • The changes to arrayToDomNodeChildren are minor. Moved entries will act like retained ones and won't trigger beforeDelete or afterAdd/afterRender callbacks.
Owner

mbest commented May 22, 2012

"Major" in this case refers to the amount of code change, primarily replacing the compareArrays code. Interface-wise, the impact is moderate:

  • The compareArrays output will be backwards compatible. If an item has moved, there will still be both deleted and added entries, but they will have an additional property to indicate that they moved from/to another location.
  • The changes to arrayToDomNodeChildren are minor. Moved entries will act like retained ones and won't trigger beforeDelete or afterAdd/afterRender callbacks.

mbest added a commit that referenced this pull request May 23, 2012

#259 - Optimize compareArrays in preparation for adding support for …
…move detection.

This new code is faster and smaller, and passes all specs.

mbest added a commit that referenced this pull request May 23, 2012

#259 - compareArrays now finds and reports moved entries.
The returned "edit script" is backwards compatible because moved entries still include "deleted" and "added" entries. To indicate where they moved to/from, they will now also have a "moved" property.

Added and deleted entries will also have an "idx" property that's needed for the move detection code. We could remove the property afterwards, but that would hurt performance a bit (and add code). The only problem with the extra property is the need to modify the specs since they don't tolerate extra properties.

One of the existing specs already has an entry that triggers the "moved" detection, so I modified it to match the new behavior. Also I added a spec to demonstrate how compareArrays handles an array changed to completely new items.

mbest added a commit that referenced this pull request May 23, 2012

#259 - Finally, modify setDomNodeChildrenFromArrayMapping to handle …
…moved items.

Some other minor changes to setDomNodeChildrenFromArrayMapping benefit size and performance slightly.
Modified an existing spec to show that moved items don't trigger any new mapping or callback calls.
@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest May 23, 2012

Owner

Do you have a specific implementation in mind?

I've created a branch with the changes divided into three commits, and attached it to this issue. Please look it over when you have time.

Owner

mbest commented May 23, 2012

Do you have a specific implementation in mind?

I've created a branch with the changes divided into three commits, and attached it to this issue. Please look it over when you have time.

@ghost ghost assigned mbest May 31, 2012

@mbest mbest referenced this pull request Jun 6, 2012

Merged

Templating fixes #144

@mbest mbest referenced this pull request Jun 29, 2012

Closed

July 2012 iteration planning #549

mbest added some commits May 23, 2012

#259 - Optimize compareArrays in preparation for adding support for …
…move detection.

This new code is faster and smaller, and passes all specs.
#259 - compareArrays now finds and reports moved entries.
The returned "edit script" is backwards compatible because moved entries still include "deleted" and "added" entries. To indicate where they moved to/from, they will now also have a "moved" property.

Added and deleted entries will also have an "idx" property that's needed for the move detection code. We could remove the property afterwards, but that would hurt performance a bit (and add code). The only problem with the extra property is the need to modify the specs since they don't tolerate extra properties.

One of the existing specs already has an entry that triggers the "moved" detection, so I modified it to match the new behavior. Also I added a spec to demonstrate how compareArrays handles an array changed to completely new items.
#259 - Finally, modify setDomNodeChildrenFromArrayMapping to handle …
…moved items.

Some other minor changes to setDomNodeChildrenFromArrayMapping benefit size and performance slightly.
Modified an existing spec to show that moved items don't trigger any new mapping or callback calls.
@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Jul 2, 2012

Owner

I've updated the code for this to be based on the current master branch. So it's ready to review/merge again.

Owner

mbest commented Jul 2, 2012

I've updated the code for this to be based on the current master branch. So it's ready to review/merge again.

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Jul 16, 2012

Owner

Here are my notes on the changes to compareArrays: mbest#19

Owner

mbest commented Jul 16, 2012

Here are my notes on the changes to compareArrays: mbest#19

@SteveSanderson

This comment has been minimized.

Show comment Hide comment
@SteveSanderson

SteveSanderson Jul 26, 2012

Contributor

This is really great, and I'd definitely like to have it included in 2.3. I'm delighted with how you've been able to add broadly-applicable functionality while retaining backwards compatibility, improving performance in multiple ways, and not substantially increasing code size. We set standards high for core changes and you're totally nailing it :)

I'd like to step through it line-by-line before merging and possibly tweak the API very slightly (for example, saying index instead of idx, since generally the rest of the KO API uses unabbreviated words), so let's schedule this for an actual merge in the August iteration.

One scenario I'm slightly unsure about is when totally changing the contents of an array to an entirely different set. If I'm reading this correctly, the current implementation will be O(n^2) while trying to identify matches between the old and new sets (which could dominate the overall computation time). I know it's hard to do a merge on two arbitrary sets any faster than this in JavaScript (because there's no general object hashing function, so you can't do a hash-merge), but perhaps we don't need to...

Since the existing behavior doesn't identify moves at all, we could define the new behavior as "sometimes detects moves, but isn't guaranteed", and then for the implementation place a fixed limit on how many moves we look for (e.g., 10). The 95%+ use case is moving a single item, so we'd easily cover this - in cases where large numbers of items move, we effectively fall back on the old add+remove behavior.

What do you think? Does it make sense to account for this scenario like this, or is there some reason why the new match-detection would never be the bottleneck anyway?

Contributor

SteveSanderson commented Jul 26, 2012

This is really great, and I'd definitely like to have it included in 2.3. I'm delighted with how you've been able to add broadly-applicable functionality while retaining backwards compatibility, improving performance in multiple ways, and not substantially increasing code size. We set standards high for core changes and you're totally nailing it :)

I'd like to step through it line-by-line before merging and possibly tweak the API very slightly (for example, saying index instead of idx, since generally the rest of the KO API uses unabbreviated words), so let's schedule this for an actual merge in the August iteration.

One scenario I'm slightly unsure about is when totally changing the contents of an array to an entirely different set. If I'm reading this correctly, the current implementation will be O(n^2) while trying to identify matches between the old and new sets (which could dominate the overall computation time). I know it's hard to do a merge on two arbitrary sets any faster than this in JavaScript (because there's no general object hashing function, so you can't do a hash-merge), but perhaps we don't need to...

Since the existing behavior doesn't identify moves at all, we could define the new behavior as "sometimes detects moves, but isn't guaranteed", and then for the implementation place a fixed limit on how many moves we look for (e.g., 10). The 95%+ use case is moving a single item, so we'd easily cover this - in cases where large numbers of items move, we effectively fall back on the old add+remove behavior.

What do you think? Does it make sense to account for this scenario like this, or is there some reason why the new match-detection would never be the bottleneck anyway?

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Jul 27, 2012

Owner

Thanks, Steve. This was very much a labor of love for me since I really enjoyed working on it.

I used idx because it's shorter, and I didn't think of it as an exported property. But since it's there and could be useful, changing it to index is fine.

There are two algorithms here. The first is the modified Levenshtein distance algorthm. For two arrays, with m being the length of the smaller one and n the length of the larger one, the algorithm will be be O(n) if m is constant or m = n (best case), and O(m^2) if n ~= 2m (worst case). The actual number of comparisons is m(n - m + 2) - 1 when m and n are different, and 3m - 2 when they are equal (the algorithm is slightly different when the arrays have equal length).

The second algorithm looks for moved items among those marked as added (a) and deleted (d). This algorithm is O(a*d) and thus depends on the contents of the arrays rather than directly on their sizes. In the worst case, where all items in an array are replaced, this will be O(m*n) with the total comparison for both algorithms being m(2n - m + 2) - 1.

As you mentioned, placing a limit on the move detection algorithm would help with the worst case performance of the algorithms. I think we can do this by limiting the number of consecutive failed comparisons to something like 10*m. This would make the added overhead of the move algorithm linear, and the worst case performance would remain at O(m^2) with the total comparisons being m(n - m + 12) - 1.

Owner

mbest commented Jul 27, 2012

Thanks, Steve. This was very much a labor of love for me since I really enjoyed working on it.

I used idx because it's shorter, and I didn't think of it as an exported property. But since it's there and could be useful, changing it to index is fine.

There are two algorithms here. The first is the modified Levenshtein distance algorthm. For two arrays, with m being the length of the smaller one and n the length of the larger one, the algorithm will be be O(n) if m is constant or m = n (best case), and O(m^2) if n ~= 2m (worst case). The actual number of comparisons is m(n - m + 2) - 1 when m and n are different, and 3m - 2 when they are equal (the algorithm is slightly different when the arrays have equal length).

The second algorithm looks for moved items among those marked as added (a) and deleted (d). This algorithm is O(a*d) and thus depends on the contents of the arrays rather than directly on their sizes. In the worst case, where all items in an array are replaced, this will be O(m*n) with the total comparison for both algorithms being m(2n - m + 2) - 1.

As you mentioned, placing a limit on the move detection algorithm would help with the worst case performance of the algorithms. I think we can do this by limiting the number of consecutive failed comparisons to something like 10*m. This would make the added overhead of the move algorithm linear, and the worst case performance would remain at O(m^2) with the total comparisons being m(n - m + 12) - 1.

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Jul 27, 2012

Owner

I decided to go ahead and make those two changes (property name to index, and limiting the move algorithm) since I was thinking about this stuff anyway.

Owner

mbest commented Jul 27, 2012

I decided to go ahead and make those two changes (property name to index, and limiting the move algorithm) since I was thinking about this stuff anyway.

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Jul 27, 2012

Owner

Here's something else we should discuss:

With this update, there's a possible compatibility issue regarding the afterAdd and beforeRemove callbacks. Currently, if an item in the array is moved, those callbacks will be called for that item because it will be added in one place and removed from another. With this change, since the DOM nodes for that item are simply moved from one location to another, we can't call beforeRemove on them (and it doesn't make sense to call afterAdd either). Since I've never used those callbacks, I'm not sure how serious of a problem this might be, or what we might do to alleviate it.

Owner

mbest commented Jul 27, 2012

Here's something else we should discuss:

With this update, there's a possible compatibility issue regarding the afterAdd and beforeRemove callbacks. Currently, if an item in the array is moved, those callbacks will be called for that item because it will be added in one place and removed from another. With this change, since the DOM nodes for that item are simply moved from one location to another, we can't call beforeRemove on them (and it doesn't make sense to call afterAdd either). Since I've never used those callbacks, I'm not sure how serious of a problem this might be, or what we might do to alleviate it.

@SteveSanderson

This comment has been minimized.

Show comment Hide comment
@SteveSanderson

SteveSanderson Jul 27, 2012

Contributor

Thanks for thinking through the perf implications. I'll consider that in detail too when I inspect the code properly, but I'm sure you've got a pretty clear view of what makes sense here.

Regarding afterAdd and beforeRemove callbacks: yeah, you're right. And the same applies to init binding callbacks too, since we'd no longer be creating and binding new nodes on move. There are probably two main scenarios:

  • If someone uses these to initialize or dispose nodes in some way, then this change won't affect them (other than making their code more efficient).
  • But if someone uses afterAdd/beforeRemove/init to animate entrances and exits, then this change would mean animations no longer occur when moving items. I expect it will be fairly unusual for someone to combine animations with a reorderable list and actually want the "new item" animation to apply when you've actually just moved an entry. It would be a stretch to call this a "breaking change" (and it's not like you'd get an error or crash) but someone out there might disagree.

I'm usually loathe to add extra options and flags just based on an abstract discussion and without the use case genuinely coming from a real experience, but maybe here the quick solution is to allow an extra foreach option - moveItems: false - which actually disables the second (move-detection) part of the algorithm entirely. I feel a bit bad for suggesting an extra flag (that ends up in documentation and diminishes the clarity of what foreach means), but anyway, what do you think?

Contributor

SteveSanderson commented Jul 27, 2012

Thanks for thinking through the perf implications. I'll consider that in detail too when I inspect the code properly, but I'm sure you've got a pretty clear view of what makes sense here.

Regarding afterAdd and beforeRemove callbacks: yeah, you're right. And the same applies to init binding callbacks too, since we'd no longer be creating and binding new nodes on move. There are probably two main scenarios:

  • If someone uses these to initialize or dispose nodes in some way, then this change won't affect them (other than making their code more efficient).
  • But if someone uses afterAdd/beforeRemove/init to animate entrances and exits, then this change would mean animations no longer occur when moving items. I expect it will be fairly unusual for someone to combine animations with a reorderable list and actually want the "new item" animation to apply when you've actually just moved an entry. It would be a stretch to call this a "breaking change" (and it's not like you'd get an error or crash) but someone out there might disagree.

I'm usually loathe to add extra options and flags just based on an abstract discussion and without the use case genuinely coming from a real experience, but maybe here the quick solution is to allow an extra foreach option - moveItems: false - which actually disables the second (move-detection) part of the algorithm entirely. I feel a bit bad for suggesting an extra flag (that ends up in documentation and diminishes the clarity of what foreach means), but anyway, what do you think?

@rniemeyer

This comment has been minimized.

Show comment Hide comment
@rniemeyer

rniemeyer Jul 27, 2012

Owner

I agree with Steve's analysis of the afterAdd/beforeRemove/init issue. In almost all cases that I have seen, this change will only be beneficial. I just helped someone last week work around an animation issue related to moves being a remove + add. I can't recall specific cases that would be problematic.

However, I do think that adding a flag to revert back to the old behavior would be a good idea and/or providing an afterMove callback. An afterMove callback would seem to fit the spirit of our current implementation best.

Owner

rniemeyer commented Jul 27, 2012

I agree with Steve's analysis of the afterAdd/beforeRemove/init issue. In almost all cases that I have seen, this change will only be beneficial. I just helped someone last week work around an animation issue related to moves being a remove + add. I can't recall specific cases that would be problematic.

However, I do think that adding a flag to revert back to the old behavior would be a good idea and/or providing an afterMove callback. An afterMove callback would seem to fit the spirit of our current implementation best.

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Jul 30, 2012

Owner

Steve and Ryan, thanks for the feedback about the notification issues.

I'd prefer the afterMove callback over a moveItems flag. I think if we got complaints about the change that couldn't be solved with afterMove, we could then consider adding moveItems.

Owner

mbest commented Jul 30, 2012

Steve and Ryan, thanks for the feedback about the notification issues.

I'd prefer the afterMove callback over a moveItems flag. I think if we got complaints about the change that couldn't be solved with afterMove, we could then consider adding moveItems.

@SteveSanderson

This comment has been minimized.

Show comment Hide comment
@SteveSanderson

SteveSanderson Jul 31, 2012

Contributor

I can understand the benefit of afterMove - it certainly fits well into the existing beforeRemove and afterAdd pattern.

Ryan/Michael, how much risk do you think there is of anyone relying on the current afterAdd/init callbacks to do something important after a "move" operation? As in, should we be regarding this as a breaking change, or is that so unlikely that we shouldn't expect anyone to need to disable this new behavior? I'm happy either way.

Contributor

SteveSanderson commented Jul 31, 2012

I can understand the benefit of afterMove - it certainly fits well into the existing beforeRemove and afterAdd pattern.

Ryan/Michael, how much risk do you think there is of anyone relying on the current afterAdd/init callbacks to do something important after a "move" operation? As in, should we be regarding this as a breaking change, or is that so unlikely that we shouldn't expect anyone to need to disable this new behavior? I'm happy either way.

@rniemeyer

This comment has been minimized.

Show comment Hide comment
@rniemeyer

rniemeyer Jul 31, 2012

Owner

I am generally pretty cautious about breaking changes, but in this case I feel that the risk is extremely low and that we do not need to consider it a breaking change.

I am kind of wondering if we need both beforeMove and afterMove callbacks to properly animate a move. I would hate add bloat, it has just crossed my mind.

Additionally, my sortable plugin (ultimately it is a wrapper to foreach), already uses beforeMove and afterMove for its own callbacks, but I would say that that is the risk that a plugin takes when it extends the base and it would have to find a way to handle both.

Owner

rniemeyer commented Jul 31, 2012

I am generally pretty cautious about breaking changes, but in this case I feel that the risk is extremely low and that we do not need to consider it a breaking change.

I am kind of wondering if we need both beforeMove and afterMove callbacks to properly animate a move. I would hate add bloat, it has just crossed my mind.

Additionally, my sortable plugin (ultimately it is a wrapper to foreach), already uses beforeMove and afterMove for its own callbacks, but I would say that that is the risk that a plugin takes when it extends the base and it would have to find a way to handle both.

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Jul 31, 2012

Owner

I am kind of wondering if we need both beforeMove and afterMove

I don't think that would work. If the steps would be like this:

  1. Call beforeMove with each node to be moved.
  2. Move nodes to new location.
  3. Call afterMove with each node that was moved.

If the beforeMove callback started an animation with the nodes, it would immediately get highjacked by the actual move of the nodes and then get tangled with the afterMove animation. So the only way to get it to work would be if the beforeMove callback called another callback when it was done with the animation that would then do the move and call afterMove. It would tricky, though, to know where to insert the new nodes after the animation, especially if a lot of the items in the array were moved at the same time, such as when sorting.

In short, I don't think we should add beforeMove.

Edit: See below for how this can actually work.

Owner

mbest commented Jul 31, 2012

I am kind of wondering if we need both beforeMove and afterMove

I don't think that would work. If the steps would be like this:

  1. Call beforeMove with each node to be moved.
  2. Move nodes to new location.
  3. Call afterMove with each node that was moved.

If the beforeMove callback started an animation with the nodes, it would immediately get highjacked by the actual move of the nodes and then get tangled with the afterMove animation. So the only way to get it to work would be if the beforeMove callback called another callback when it was done with the animation that would then do the move and call afterMove. It would tricky, though, to know where to insert the new nodes after the animation, especially if a lot of the items in the array were moved at the same time, such as when sorting.

In short, I don't think we should add beforeMove.

Edit: See below for how this can actually work.

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Jul 31, 2012

Owner

Here is an example that shows, with the current code, how sorting will be animated: http://jsfiddle.net/mbest/NXeJc/

Here is the same code using my fork that includes this move update, which shows that sorting will not be animated: http://jsfiddle.net/mbest/qU4qf/

Probably the easiest way for a user to make the animated sorting work would be to disable the move handling using something like moveItems: false. I think it would be hard to make a beforeMove/afterMove callback to handle this animation in a clean way.

I think a better solution than adding a moveItems option to foreach would be to add a compareOptions (or similar) option that would be passed directly to compareArrays as a new third parameter.

Owner

mbest commented Jul 31, 2012

Here is an example that shows, with the current code, how sorting will be animated: http://jsfiddle.net/mbest/NXeJc/

Here is the same code using my fork that includes this move update, which shows that sorting will not be animated: http://jsfiddle.net/mbest/qU4qf/

Probably the easiest way for a user to make the animated sorting work would be to disable the move handling using something like moveItems: false. I think it would be hard to make a beforeMove/afterMove callback to handle this animation in a clean way.

I think a better solution than adding a moveItems option to foreach would be to add a compareOptions (or similar) option that would be passed directly to compareArrays as a new third parameter.

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Aug 1, 2012

Owner

Regarding animating a sort, I found this cool example. Compared to this, the sort animation that's done using beforeRemove/afterAdd looks pretty bad.

Owner

mbest commented Aug 1, 2012

Regarding animating a sort, I found this cool example. Compared to this, the sort animation that's done using beforeRemove/afterAdd looks pretty bad.

@rniemeyer

This comment has been minimized.

Show comment Hide comment
@rniemeyer

rniemeyer Aug 1, 2012

Owner

I had not thought about the async aspect of before to after callbacks. It would be pretty messy to fully support it. The only thought that I have is providing a single onMove callback where the developer would be completely responsible for moving it and would be given the node, the parent, and new index or something like that. Still sounds messy. I am not convinced that it is really worth the effort.

As far as sorting goes, I kind of like the samples here: http://isotope.metafizzy.co/

Owner

rniemeyer commented Aug 1, 2012

I had not thought about the async aspect of before to after callbacks. It would be pretty messy to fully support it. The only thought that I have is providing a single onMove callback where the developer would be completely responsible for moving it and would be given the node, the parent, and new index or something like that. Still sounds messy. I am not convinced that it is really worth the effort.

As far as sorting goes, I kind of like the samples here: http://isotope.metafizzy.co/

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Aug 1, 2012

Owner

Here's a version of the above examples with beforeMove and afterMove callbacks in place: http://jsfiddle.net/mbest/9gvDL/ The former records the vertical position of the node, and the latter animates the move. I haven't done any thorough testing with the changes this required in Knockout, but if you think this might be a good way to go, I'll spend some more time testing and cleaning up the code before including it in this pull request.

Owner

mbest commented Aug 1, 2012

Here's a version of the above examples with beforeMove and afterMove callbacks in place: http://jsfiddle.net/mbest/9gvDL/ The former records the vertical position of the node, and the latter animates the move. I haven't done any thorough testing with the changes this required in Knockout, but if you think this might be a good way to go, I'll spend some more time testing and cleaning up the code before including it in this pull request.

@rniemeyer

This comment has been minimized.

Show comment Hide comment
@rniemeyer

rniemeyer Aug 1, 2012

Owner

Well, I definitely think that the sorting effect is quite nice in your sample. Saving the original position in the before callback and handling all of the animation in the after callback is certainly a clever way to avoid the async callback issues discussed earlier.

Owner

rniemeyer commented Aug 1, 2012

Well, I definitely think that the sorting effect is quite nice in your sample. Saving the original position in the before callback and handling all of the animation in the after callback is certainly a clever way to avoid the async callback issues discussed earlier.

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Aug 15, 2012

Owner

To support beforeMove/afterMove, I had to make some changes to the logic in setDomNodeChildrenFromArrayMapping:

  • While going through the edit script, it merely tracks which nodes are to be added, moved and removed without modifying the DOM. This is so that all of the beforeMove notifications will happen before any nodes are changed and the previous position of the nodes is accurate.
  • "Retained" items are treated the same way as "moved" items: if the new position in the array is different from the old position, it will run the move callbacks.
  • Node processing is done in this order: 1) call beforeRemove or beforeMove on removed or moved nodes, 2) remove nodes (if no beforeRemove callback), 3) re-order/add nodes, 4) call callbackAfterAddingNodes on added nodes, 5) call afterMove on moved nodes, and 6) call afterAdd on added nodes.

I will spend some time this week to get the code for this change ready to review/merge.

Owner

mbest commented Aug 15, 2012

To support beforeMove/afterMove, I had to make some changes to the logic in setDomNodeChildrenFromArrayMapping:

  • While going through the edit script, it merely tracks which nodes are to be added, moved and removed without modifying the DOM. This is so that all of the beforeMove notifications will happen before any nodes are changed and the previous position of the nodes is accurate.
  • "Retained" items are treated the same way as "moved" items: if the new position in the array is different from the old position, it will run the move callbacks.
  • Node processing is done in this order: 1) call beforeRemove or beforeMove on removed or moved nodes, 2) remove nodes (if no beforeRemove callback), 3) re-order/add nodes, 4) call callbackAfterAddingNodes on added nodes, 5) call afterMove on moved nodes, and 6) call afterAdd on added nodes.

I will spend some time this week to get the code for this change ready to review/merge.

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Aug 16, 2012

Owner

Okay, this is ready for review. Example using new callbacks: http://jsfiddle.net/mbest/9gvDL/

Owner

mbest commented Aug 16, 2012

Okay, this is ready for review. Example using new callbacks: http://jsfiddle.net/mbest/9gvDL/

@SteveSanderson SteveSanderson merged commit 33cecba into master Aug 23, 2012

@SteveSanderson

This comment has been minimized.

Show comment Hide comment
@SteveSanderson

SteveSanderson Aug 23, 2012

Contributor

First, it's really brilliant to have this extra functionality in here. Thanks so much Michael for implementing it! Clearly this took a lot of careful thought to make performant and backward-compatible. Brilliant job!

The edit detection and DOM node reordering logic is now seriously pushing at the limit of algorithmic sophistication that we can realistically maintain in this project (without factoring it out into a more abstract set of algorithmic helpers). It's taken me a long time to make sense of it - though even then with shortcuts - and I don't expect anyone outside the project to have the motivation to do so. Hopefully we can avoid letting this logic continue to grow, however slowly, over time.

I love this example, BTW: http://jsfiddle.net/mbest/9gvDL/. There might be demand for that kind of animated sorting to be made available as a custom binding plugin (animatedforeach ?).

One question: the optimization whereby move detection is truncated if the number of failed comparisons exceeds a multiple of the smallest input array size - can anyone think of a clear way to explain that in docs? As in, what kind of intuitive description can we give for this rule? People will want to have an intuitive sense of which kinds of array mutations lead to complete move detection and which won't. I'm not sure how to explain it, other than just saying "if you make a large number of additions or deletions at once (not counting reorderings), for example if you replace the entire array contents, then reordering detection will not apply". But people may want a more precise description, without having to understand the actual algorithm.

Contributor

SteveSanderson commented Aug 23, 2012

First, it's really brilliant to have this extra functionality in here. Thanks so much Michael for implementing it! Clearly this took a lot of careful thought to make performant and backward-compatible. Brilliant job!

The edit detection and DOM node reordering logic is now seriously pushing at the limit of algorithmic sophistication that we can realistically maintain in this project (without factoring it out into a more abstract set of algorithmic helpers). It's taken me a long time to make sense of it - though even then with shortcuts - and I don't expect anyone outside the project to have the motivation to do so. Hopefully we can avoid letting this logic continue to grow, however slowly, over time.

I love this example, BTW: http://jsfiddle.net/mbest/9gvDL/. There might be demand for that kind of animated sorting to be made available as a custom binding plugin (animatedforeach ?).

One question: the optimization whereby move detection is truncated if the number of failed comparisons exceeds a multiple of the smallest input array size - can anyone think of a clear way to explain that in docs? As in, what kind of intuitive description can we give for this rule? People will want to have an intuitive sense of which kinds of array mutations lead to complete move detection and which won't. I'm not sure how to explain it, other than just saying "if you make a large number of additions or deletions at once (not counting reorderings), for example if you replace the entire array contents, then reordering detection will not apply". But people may want a more precise description, without having to understand the actual algorithm.

@RoyJacobs

This comment has been minimized.

Show comment Hide comment
@RoyJacobs

RoyJacobs Aug 23, 2012

Contributor

I would just like to briefly post a huge "+1" here for @mbest. This problem is decidedly non-trivial and I'm really excited that this is now available. It provides the opportunity for much richer interaction design and that's something that really benefits KO. KO was already the best way to build a site, but it can now also be used more and more to replace these one-off jquery plugins that are typically used for the, well, "bling" (*) on a site.

(*) Term may not be trendy enough

Contributor

RoyJacobs commented Aug 23, 2012

I would just like to briefly post a huge "+1" here for @mbest. This problem is decidedly non-trivial and I'm really excited that this is now available. It provides the opportunity for much richer interaction design and that's something that really benefits KO. KO was already the best way to build a site, but it can now also be used more and more to replace these one-off jquery plugins that are typically used for the, well, "bling" (*) on a site.

(*) Term may not be trendy enough

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Aug 23, 2012

Owner

Thanks, Steve and Roy.

I'll try to come up with some examples which trigger the move the detection truncation. That might help with explaining it.

Owner

mbest commented Aug 23, 2012

Thanks, Steve and Roy.

I'll try to come up with some examples which trigger the move the detection truncation. That might help with explaining it.

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Aug 25, 2012

Owner

Steve, the description you gave is pretty accurate, although not entirely. Even when the move detection is stopped, any moves already detected still count; and "retained" items can also be counted as moves.

Because the move detection algorithm proceeds through the added and deleted items in a specific order, moves will always be detected near the beginning of the process, but if the number of moved items is small compared to the number of added/deleted items, and the moves would be detected near the end of the process, they're likely to skipped.

An example would be arrays that are different except for a single item moved from/to near the top of the larger array to/from near the bottom of the smaller array. Moves in other directions would likely be found (near top -> near top, near bottom -> near bottom, and near bottom of larger array -> near top of smaller array).

Owner

mbest commented Aug 25, 2012

Steve, the description you gave is pretty accurate, although not entirely. Even when the move detection is stopped, any moves already detected still count; and "retained" items can also be counted as moves.

Because the move detection algorithm proceeds through the added and deleted items in a specific order, moves will always be detected near the beginning of the process, but if the number of moved items is small compared to the number of added/deleted items, and the moves would be detected near the end of the process, they're likely to skipped.

An example would be arrays that are different except for a single item moved from/to near the top of the larger array to/from near the bottom of the smaller array. Moves in other directions would likely be found (near top -> near top, near bottom -> near bottom, and near bottom of larger array -> near top of smaller array).

mbest added a commit that referenced this pull request Sep 25, 2012

Add specs for each of the foreach callbacks (afterRender, afterAdd, b…
…eforeRemove, beforeMove, and afterMove) and fix some problems related to those callbacks that were introduced in #259.
@jpa00

This comment has been minimized.

Show comment Hide comment
@jpa00

jpa00 Oct 9, 2012

About setDomNodeChildrenFromArrayMapping as it appears in 2.2.0rc. It seems that if there are items that are queued for deletion, the method takes a potentially large performance hit, because it ends up removing these items only after the add/reorder-loop, effectively first moving all of the items after the removable ones. Is there a reason for doing things in this order, especially in the case of items with no beforeRemove-callbacks? With an array of 1000+ items, I noticed a massive boost in performance when these items are removed before the loop.

jpa00 commented Oct 9, 2012

About setDomNodeChildrenFromArrayMapping as it appears in 2.2.0rc. It seems that if there are items that are queued for deletion, the method takes a potentially large performance hit, because it ends up removing these items only after the add/reorder-loop, effectively first moving all of the items after the removable ones. Is there a reason for doing things in this order, especially in the case of items with no beforeRemove-callbacks? With an array of 1000+ items, I noticed a massive boost in performance when these items are removed before the loop.

@mbest

This comment has been minimized.

Show comment Hide comment
@mbest

mbest Oct 9, 2012

Owner

Is there a reason for doing things in this order?

The beforeRemove calls will need to happen after since otherwise the add/reorder loop will add the nodes back in. But without beforeRemove, the removes can happen before. I'll make sure this gets changed for 2.2.

Owner

mbest commented Oct 9, 2012

Is there a reason for doing things in this order?

The beforeRemove calls will need to happen after since otherwise the add/reorder loop will add the nodes back in. But without beforeRemove, the removes can happen before. I'll make sure this gets changed for 2.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment