Sorting a List doesn't update in a template correctly #1114

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
6 participants
@asavoy
Contributor

asavoy commented Jun 25, 2014

I'm trying to use .comparator and .sort() as described in can.Map.prototype.sort to change the sorting of a can.List that is bound in a Mustache template.

The can.List gets correctly sorted, but there appear to be a few bugs that prevent the template from reflecting the sorted data correctly.

Please refer to this JSFiddle:

  • Calling list.sort() won't update the rendered template at all.
  • But calling can.trigger(list, 'length') or list.push(newItem) WILL force the update, but only for {{#key}}...{{/key}} syntax. It's not the worst workaround, but I prefer to use {{#each}} because that provides {{@index}}.
  • The {{#each}} syntax will never show list sorted, even after adding items to it.
    • (Another weird thing is, in this case, adding an item will render the new item twice.)

I've added tests that cover all of these issues.

@DVSoftware

This comment has been minimized.

Show comment
Hide comment
@DVSoftware

DVSoftware Jun 25, 2014

Contributor

I also experienced this issue

Contributor

DVSoftware commented Jun 25, 2014

I also experienced this issue

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jun 25, 2014

Contributor

We should add @index to #key.

Sort should fire a remove and add event for the whole list.

Sent from my iPhone

On Jun 25, 2014, at 3:23 AM, asavoy notifications@github.com wrote:

I'm trying to use .comparator and .sort() as described in can.Map.prototype.sort to change the sorting of a can.List that is bound in a Mustache template.

The can.List gets correctly sorted, but there appear to be a few bugs that prevent the template from reflecting the sorted data correctly.

Please refer to this JSFiddle:

Calling list.sort() won't update the rendered template at all.
But calling can.trigger(list, 'length') or list.push(newItem) WILL force the update, but only for {{#key}}...{{/key}} syntax. It's not the worst workaround, but I prefer to use {{#each}} because that provides {{@index}}.
The {{#each}} syntax will never show list sorted, even after adding items to it.

(Another weird thing is, in this case, adding an item will render the new item twice.)
I've added tests that cover all of these issues.

You can merge this Pull Request by running

git pull https://github.com/asavoy/canjs bug-binding-sorted-list
Or view, comment on, or merge it at:

#1114

Commit Summary

Added failing tests to show that sorting a list won't cause live-binded elements to reflect the sorted list correctly.
File Changes

M map/sort/sort_test.js (64)
Patch Links:

https://github.com/bitovi/canjs/pull/1114.patch
https://github.com/bitovi/canjs/pull/1114.diff

Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jun 25, 2014

We should add @index to #key.

Sort should fire a remove and add event for the whole list.

Sent from my iPhone

On Jun 25, 2014, at 3:23 AM, asavoy notifications@github.com wrote:

I'm trying to use .comparator and .sort() as described in can.Map.prototype.sort to change the sorting of a can.List that is bound in a Mustache template.

The can.List gets correctly sorted, but there appear to be a few bugs that prevent the template from reflecting the sorted data correctly.

Please refer to this JSFiddle:

Calling list.sort() won't update the rendered template at all.
But calling can.trigger(list, 'length') or list.push(newItem) WILL force the update, but only for {{#key}}...{{/key}} syntax. It's not the worst workaround, but I prefer to use {{#each}} because that provides {{@index}}.
The {{#each}} syntax will never show list sorted, even after adding items to it.

(Another weird thing is, in this case, adding an item will render the new item twice.)
I've added tests that cover all of these issues.

You can merge this Pull Request by running

git pull https://github.com/asavoy/canjs bug-binding-sorted-list
Or view, comment on, or merge it at:

#1114

Commit Summary

Added failing tests to show that sorting a list won't cause live-binded elements to reflect the sorted list correctly.
File Changes

M map/sort/sort_test.js (64)
Patch Links:

https://github.com/bitovi/canjs/pull/1114.patch
https://github.com/bitovi/canjs/pull/1114.diff

Reply to this email directly or view it on GitHub.

@asavoy

This comment has been minimized.

Show comment
Hide comment
@asavoy

asavoy Jun 26, 2014

Contributor

@justinbmeyer I've got the remove/add events working and covered by a test. But would like your help on the last failing test on pushing to a sorted list: {{#each}} renders the pushed item twice at the end of the list, instead of once in the sorted position. I can't figure out a way to resolve this without rewriting rewriting the push function completely, and changing the order of events triggered there.

Contributor

asavoy commented Jun 26, 2014

@justinbmeyer I've got the remove/add events working and covered by a test. But would like your help on the last failing test on pushing to a sorted list: {{#each}} renders the pushed item twice at the end of the list, instead of once in the sorted position. I can't figure out a way to resolve this without rewriting rewriting the push function completely, and changing the order of events triggered there.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jun 27, 2014

Contributor

I'm not sure what push and unshift should do on a list with a comparator. It might just add to the end or the start of the list and leave the user to call .sort() again. The sort plugin might add an add method that would insert (via bisection) items into the sorted list.

The sort plugin really needs an overhaul. Maybe it's a good time to discuss what a really good sort plugin would look like.

Contributor

justinbmeyer commented Jun 27, 2014

I'm not sure what push and unshift should do on a list with a comparator. It might just add to the end or the start of the list and leave the user to call .sort() again. The sort plugin might add an add method that would insert (via bisection) items into the sorted list.

The sort plugin really needs an overhaul. Maybe it's a good time to discuss what a really good sort plugin would look like.

@asavoy

This comment has been minimized.

Show comment
Hide comment
@asavoy

asavoy Jun 27, 2014

Contributor

@justinbmeyer I did get a little bit closer with the assumption that push() and unshift() should insert items into their sorted position.

At this point it appears to work, except for one part: that sortedList.push(item) isn't telling {{#each}} to move the new item into the sorted position. For that it looks like view.live.list() needs to support the 'move' event. I haven't been able to figure out how that should work, so would like to get your opinion on whether that is workable, or perhaps an overhaul is really necessary. Certainly were a few bugs in there :(

Contributor

asavoy commented Jun 27, 2014

@justinbmeyer I did get a little bit closer with the assumption that push() and unshift() should insert items into their sorted position.

At this point it appears to work, except for one part: that sortedList.push(item) isn't telling {{#each}} to move the new item into the sorted position. For that it looks like view.live.list() needs to support the 'move' event. I haven't been able to figure out how that should work, so would like to get your opinion on whether that is workable, or perhaps an overhaul is really necessary. Certainly were a few bugs in there :(

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jun 27, 2014

Contributor

Why would list need to support the 'move' event? Shouldn't an add event for the newly added item be enough?

Contributor

justinbmeyer commented Jun 27, 2014

Why would list need to support the 'move' event? Shouldn't an add event for the newly added item be enough?

@asavoy

This comment has been minimized.

Show comment
Hide comment
@asavoy

asavoy Jun 27, 2014

Contributor

I tried that first, it required rewriting more of the sort module and broke more tests. It looked like it was closer to the original intent of the plugin to push the item onto the end, and then move it into the sorted position. Or so I had interpreted the _changes() function :)

On 28 Jun 2014, at 3:19 am, Justin Meyer notifications@github.com wrote:

Why would list need to support the 'move' event? Shouldn't an add event for the newly added item be enough?


Reply to this email directly or view it on GitHub.

Contributor

asavoy commented Jun 27, 2014

I tried that first, it required rewriting more of the sort module and broke more tests. It looked like it was closer to the original intent of the plugin to push the item onto the end, and then move it into the sorted position. Or so I had interpreted the _changes() function :)

On 28 Jun 2014, at 3:19 am, Justin Meyer notifications@github.com wrote:

Why would list need to support the 'move' event? Shouldn't an add event for the newly added item be enough?


Reply to this email directly or view it on GitHub.

asavoy added some commits Jun 28, 2014

sort: I think we shouldn't sort() in push() - it is assumed that the …
…list is sorted already from an earlier direct call to sort(), or because items were pushed into it (which inserts them into their sorted position).
sort: I believe this condition was incorrect, because the regexp woul…
…d match "12" or "1." but not "1". And thus we'd only need to check the first char is a number.
sort: For sorted lists, push() and unshift() will add items at their …
…sorted position. This fixes the failing tests on sort.
@asavoy

This comment has been minimized.

Show comment
Hide comment
@asavoy

asavoy Jun 28, 2014

Contributor

I managed to get sort working with the 'add' event, instead of going down the 'move' event approach - much easier.

I had to change 2 tests, but I'm pretty sure they were incorrect in the first place, please see the commit messages. This is the reason the build fails: the compatibility test run is still executing these incorrect tests.

Contributor

asavoy commented Jun 28, 2014

I managed to get sort working with the 'add' event, instead of going down the 'move' event approach - much easier.

I had to change 2 tests, but I'm pretty sure they were incorrect in the first place, please see the commit messages. This is the reason the build fails: the compatibility test run is still executing these incorrect tests.

@asavoy

This comment has been minimized.

Show comment
Hide comment
@asavoy

asavoy Jul 3, 2014

Contributor

Would like to get an opinion from someone on the CanJS team about the breaking "compatibility" test cases - I'm pretty sure they were incorrect in the first place.

I think a sort plugin is important for anything beyond simple listing of data. For example, select lists, tables and data grids need to be able to control the ordering of list members.

Contributor

asavoy commented Jul 3, 2014

Would like to get an opinion from someone on the CanJS team about the breaking "compatibility" test cases - I'm pretty sure they were incorrect in the first place.

I think a sort plugin is important for anything beyond simple listing of data. For example, select lists, tables and data grids need to be able to control the ordering of list members.

@justinbmeyer justinbmeyer added the Bug label Jul 29, 2014

@justinbmeyer justinbmeyer added this to the 2.1.3 milestone Jul 29, 2014

@justinbmeyer justinbmeyer self-assigned this Jul 29, 2014

@daffl daffl modified the milestones: 2.1.3, 2.1.4 Aug 7, 2014

@stevenvachon

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Sep 22, 2014

Contributor

Functionally, I'm fine with can.List.prototype.sort(). Basically all I want is Array.prototype.sort() with an event triggered at the end. Unfortunately, it does not currently trigger this event.

Contributor

stevenvachon commented Sep 22, 2014

Functionally, I'm fine with can.List.prototype.sort(). Basically all I want is Array.prototype.sort() with an event triggered at the end. Unfortunately, it does not currently trigger this event.

@stevenvachon

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Oct 9, 2014

Contributor

I managed to get the view to update by triggering "remove" and "add" as Justin suggested using:

this._triggerChange("0", "remove", undefined, oldVal);
this._triggerChange("0", "add", newVal, oldVal);

The downside to this is that those events are triggered to any user listener. Whatever logic they have occurring on those listeners will also occur when they sort. If there was a way to silently trigger those "add"/"remove" events, we could avoid interference and publicly trigger a "sort" (or similar) event.

Contributor

stevenvachon commented Oct 9, 2014

I managed to get the view to update by triggering "remove" and "add" as Justin suggested using:

this._triggerChange("0", "remove", undefined, oldVal);
this._triggerChange("0", "add", newVal, oldVal);

The downside to this is that those events are triggered to any user listener. Whatever logic they have occurring on those listeners will also occur when they sort. If there was a way to silently trigger those "add"/"remove" events, we could avoid interference and publicly trigger a "sort" (or similar) event.

@isadovskiy

This comment has been minimized.

Show comment
Hide comment
@isadovskiy

isadovskiy Dec 19, 2014

Want to ask about "move" event support for bindings in view. From performance point, what will be better? Moving existed DOM elements around or re-create them (remove/add flow)?

Want to ask about "move" event support for bindings in view. From performance point, what will be better? Moving existed DOM elements around or re-create them (remove/add flow)?

@isadovskiy

This comment has been minimized.

Show comment
Hide comment
@isadovskiy

isadovskiy Dec 24, 2014

I tried the solution, posted by @asavoy and it fixed data binding issue for sorted list.

However I noticed current approach at the moment causes very significant performance issues. I have a big table, which I need to sort. Updating view via sort plugin and data binding cause significant delays. Basically all table DOM items are re-created, which takes few seconds in my case. I implemented DOM items reordering manually, to detach/append existed DOM items into correct places/order. As a result sorting without list binding works almost instantly for my big table.

It made me think sort plugin should actually use "move" event, handled gracefully by view, to make data binding works efficiently in this case.

I tried the solution, posted by @asavoy and it fixed data binding issue for sorted list.

However I noticed current approach at the moment causes very significant performance issues. I have a big table, which I need to sort. Updating view via sort plugin and data binding cause significant delays. Basically all table DOM items are re-created, which takes few seconds in my case. I implemented DOM items reordering manually, to detach/append existed DOM items into correct places/order. As a result sorting without list binding works almost instantly for my big table.

It made me think sort plugin should actually use "move" event, handled gracefully by view, to make data binding works efficiently in this case.

@daffl daffl modified the milestones: 2.3.0, 2.2.0 Jan 10, 2015

@akagomez akagomez referenced this pull request Feb 17, 2015

Merged

Complete the Sort plugin #1454

@daffl daffl removed this from the 2.3.0 milestone Feb 18, 2015

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Feb 18, 2015

Contributor

This will be fixed via #1454 in 2.2.

Contributor

daffl commented Feb 18, 2015

This will be fixed via #1454 in 2.2.

@daffl daffl closed this Feb 18, 2015

@daffl daffl added this to the 2.2.0 milestone Feb 19, 2015

@daffl daffl removed the fixed in branch label Feb 20, 2015

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