From 4ad7c229144365bb01115a9ab14e4c8a80aaf8af Mon Sep 17 00:00:00 2001 From: Alvin Savoy <alvin.savoy@gmail.com> Date: Fri, 27 Jun 2014 18:34:41 +1000 Subject: [PATCH 1/9] sort: Added failing tests to show that sorting a list won't cause live-binded elements to reflect the sorted list correctly. --- map/sort/sort_test.js | 64 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/map/sort/sort_test.js b/map/sort/sort_test.js index a6301bbda4a..9cfefff71c9 100644 --- a/map/sort/sort_test.js +++ b/map/sort/sort_test.js @@ -125,4 +125,68 @@ steal("can/map/sort", "can/test", "can/view/mustache", function () { equal(el.getElementsByTagName('li') .length, 2, 'Live binding rendered second li'); }); + + test('live binding with comparator and {{#key}}', function () { + var renderer = can.view.mustache('<ul>{{#items}}<li>{{text}}</li>{{/items}}</ul>'), + el = document.createElement('div'), + items = new can.List([{ + text: 'CCC' + }, { + text: 'BBB' + }]); + el.appendChild(renderer({ + items: items + })); + equal(el.getElementsByTagName('li')[0].innerHTML, 'CCC', + 'Unsorted list, 1st item'); + equal(el.getElementsByTagName('li')[1].innerHTML, 'BBB', + 'Unsorted list, 2nd item'); + + items.comparator = 'text'; + items.sort(); + equal(el.getElementsByTagName('li')[0].innerHTML, 'BBB', + 'Sorted list, 1st item'); + equal(el.getElementsByTagName('li')[1].innerHTML, 'CCC', + 'Sorted list, 2nd item'); + + items.push({ + text: 'AAA' + }); + equal(el.getElementsByTagName('li')[0].innerHTML, 'AAA', + 'Push to sorted list, 1st item'); + equal(el.getElementsByTagName('li').length, 3, + 'Push to sorted list, correct length'); + }); + + test('live binding with comparator and {{#each key}}', function () { + var renderer = can.view.mustache('<ul>{{#each items}}<li>{{text}}</li>{{/each}}</ul>'), + el = document.createElement('div'), + items = new can.List([{ + text: 'CCC' + }, { + text: 'BBB' + }]); + el.appendChild(renderer({ + items: items + })); + equal(el.getElementsByTagName('li')[0].innerHTML, 'CCC', + 'Unsorted list, 1st item'); + equal(el.getElementsByTagName('li')[1].innerHTML, 'BBB', + 'Unsorted list, 2nd item'); + + items.comparator = 'text'; + items.sort(); + equal(el.getElementsByTagName('li')[0].innerHTML, 'BBB', + 'Sorted list, 1st item'); + equal(el.getElementsByTagName('li')[1].innerHTML, 'CCC', + 'Sorted list, 2nd item'); + + items.push({ + text: 'AAA' + }); + equal(el.getElementsByTagName('li')[0].innerHTML, 'AAA', + 'Push to sorted list, 1st item'); + equal(el.getElementsByTagName('li').length, 3, + 'Push to sorted list, correct length'); + }); }); From bcd3d4a30d9ce06c3075d0e999f3b6b8bb854da5 Mon Sep 17 00:00:00 2001 From: Alvin Savoy <alvin.savoy@gmail.com> Date: Fri, 27 Jun 2014 01:35:18 +1000 Subject: [PATCH 2/9] sort: Add failing test for the remove-all, add-all events that should trigger after list.sort(). --- map/sort/sort_test.js | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/map/sort/sort_test.js b/map/sort/sort_test.js index 9cfefff71c9..95df1963fba 100644 --- a/map/sort/sort_test.js +++ b/map/sort/sort_test.js @@ -49,6 +49,39 @@ steal("can/map/sort", "can/test", "can/view/mustache", function () { list[0].attr('name', 'Zed'); }); + + test('sort() events', 14, function () { + var list = new can.List([{ + name: 'Justin' + }, { + name: 'Brian' + }, { + name: 'Austin' + }, { + name: 'Mihael' + }]); + list.bind('remove', function (ev, items, oldPos) { + ok(true, 'remove called'); + equal(items.length, 4, 'remove all elements'); + equal(items[0].name, 'Justin', 'remove elements in old order'); + }); + list.bind('add', function (ev, items) { + ok(true, 'add called'); + equal(items.length, 4, 'add items correct length'); + equal(items[0].name, 'Austin', 'add items in sorted order (1/4)'); + equal(items[1].name, 'Brian', 'add items in sorted order (2/4)'); + equal(items[2].name, 'Justin', 'add items in sorted order (3/4)'); + equal(items[3].name, 'Mihael', 'add items in sorted order (4/4)'); + equal(list.length, 4, 'list correct length'); + equal(list[0].name, 'Austin', 'list in sorted order (1/4)'); + equal(list[1].name, 'Brian', 'list in sorted order (2/4)'); + equal(list[2].name, 'Justin', 'list in sorted order (3/4)'); + equal(list[3].name, 'Mihael', 'list in sorted order (4/4)'); + }); + list.comparator = 'name'; + list.sort(); + }); + test('list sort with func', 1, function () { var list = new can.List([{ priority: 4, From 536d5d6d2d1c4d8e7191725b2266fa18efe8b55e Mon Sep 17 00:00:00 2001 From: Alvin Savoy <alvin.savoy@gmail.com> Date: Fri, 27 Jun 2014 01:38:31 +1000 Subject: [PATCH 3/9] sort: Fix failing tests, by redoing sort() so that it triggers remove/add events. --- map/sort/sort.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/map/sort/sort.js b/map/sort/sort.js index c62f3ac9340..cd41cc350cd 100644 --- a/map/sort/sort.js +++ b/map/sort/sort.js @@ -63,8 +63,13 @@ steal('can/util', 'can/list', function (can) { ] : [method]; if (!silent) { can.trigger(this, 'reset'); + var clone = this.splice(0, this.length); + clone.sort.apply(clone, args); + return this.splice.apply(this, [0, 0].concat(clone)); + } + else { + return Array.prototype.sort.apply(this, args); } - return Array.prototype.sort.apply(this, args); } }); // create push, pop, shift, and unshift From dea4ef7c63b5cbf7816acc5f47af231847caa253 Mon Sep 17 00:00:00 2001 From: Alvin Savoy <alvin.savoy@gmail.com> Date: Fri, 27 Jun 2014 01:39:10 +1000 Subject: [PATCH 4/9] sort: Should use item.attr(comparator) instead of item[comparator]. --- map/sort/sort.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/map/sort/sort.js b/map/sort/sort.js index cd41cc350cd..a3295727212 100644 --- a/map/sort/sort.js +++ b/map/sort/sort.js @@ -56,8 +56,8 @@ steal('can/util', 'can/list', function (can) { args = comparator ? [ function (a, b) { - a = typeof a[comparator] === 'function' ? a[comparator]() : a[comparator]; - b = typeof b[comparator] === 'function' ? b[comparator]() : b[comparator]; + a = typeof a[comparator] === 'function' ? a[comparator]() : a.attr(comparator); + b = typeof b[comparator] === 'function' ? b[comparator]() : b.attr(comparator); return a === b ? 0 : a < b ? -1 : 1; } ] : [method]; From d94e8ba5a04c8a5470c7b848b0c671b9f416391b Mon Sep 17 00:00:00 2001 From: Alvin Savoy <alvin.savoy@gmail.com> Date: Fri, 27 Jun 2014 11:53:00 +1000 Subject: [PATCH 5/9] sort: `sortedList.push(item)` shouldn't trigger two identical `add` events. Updated a test which I believe was mistakenly accepting this behavior. --- list/list.js | 2 +- map/sort/sort_test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/list/list.js b/list/list.js index acc5b0a19b4..0c724fbdecc 100644 --- a/list/list.js +++ b/list/list.js @@ -718,7 +718,7 @@ steal("can/util", "can/map", "can/map/bubble.js",function (can, Map, bubble) { // Call the original method. res = orig.apply(this, args); - if (!this.comparator || args.length) { + if (!this.comparator && args.length) { this._triggerChange("" + len, "add", args, undefined); } diff --git a/map/sort/sort_test.js b/map/sort/sort_test.js index 95df1963fba..74f163508d6 100644 --- a/map/sort/sort_test.js +++ b/map/sort/sort_test.js @@ -1,7 +1,7 @@ steal("can/map/sort", "can/test", "can/view/mustache", function () { module('can/map/sort'); - test('list events', 16, function () { + test('list events', 12, function () { var list = new can.List([{ name: 'Justin' }, { From 0f46a1d18ed85842da07f4e8970cd832de81fffd Mon Sep 17 00:00:00 2001 From: Alvin Savoy <alvin.savoy@gmail.com> Date: Sat, 28 Jun 2014 17:22:51 +1000 Subject: [PATCH 6/9] 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). --- map/sort/sort.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/map/sort/sort.js b/map/sort/sort.js index a3295727212..620a19f2a48 100644 --- a/map/sort/sort.js +++ b/map/sort/sort.js @@ -136,8 +136,6 @@ steal('can/util', 'can/list', function (can) { // args - the items added // undefined - the old value if (this.comparator && args.length) { - this.sort(null, true); - can.batch.trigger(this, 'reset', [args]); this._triggerChange('' + len, 'add', args, undefined); } return res; From 99b603345e30ebfed2dcdbca25240355ebec9be9 Mon Sep 17 00:00:00 2001 From: Alvin Savoy <alvin.savoy@gmail.com> Date: Sat, 28 Jun 2014 17:24:55 +1000 Subject: [PATCH 7/9] sort: I believe this condition was incorrect, because the regexp would match "12" or "1." but not "1". And thus we'd only need to check the first char is a number. --- map/sort/sort.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/map/sort/sort.js b/map/sort/sort.js index 620a19f2a48..14d4ed722c6 100644 --- a/map/sort/sort.js +++ b/map/sort/sort.js @@ -143,7 +143,7 @@ steal('can/util', 'can/list', function (can) { }); //- override changes for sorting proto._changes = function (ev, attr, how, newVal, oldVal) { - if (this.comparator && /^\d+./.test(attr)) { + if (this.comparator && /^\d/.test(attr)) { // get the index var index = +/^\d+/.exec(attr)[0], // and item From c9a31570e0d02115da86eb67c6e5280b44f9c919 Mon Sep 17 00:00:00 2001 From: Alvin Savoy <alvin.savoy@gmail.com> Date: Sat, 28 Jun 2014 17:28:49 +1000 Subject: [PATCH 8/9] sort: The docs for the add event say that the added item position is sent, not the new length of the list. --- map/sort/sort_test.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/map/sort/sort_test.js b/map/sort/sort_test.js index 74f163508d6..acba626898e 100644 --- a/map/sort/sort_test.js +++ b/map/sort/sort_test.js @@ -34,12 +34,11 @@ steal("can/map/sort", "can/test", "can/view/mustache", function () { equal(items[0].name, 'Alexis'); equal(oldPos, 0, 'put in right spot'); }); - list.bind('add', function (ev, items, newLength) { + list.bind('add', function (ev, items, pos) { ok(true, 'add called'); equal(items.length, 1); equal(items[0].name, 'Alexis'); - // .push returns the new length not the current position - equal(newLength, 4, 'got new length'); + equal(pos, 0, 'got sorted position'); }); list.push({ name: 'Alexis' From 50f0f9a6a33a0da936e68ac22150b05ad907c34c Mon Sep 17 00:00:00 2001 From: Alvin Savoy <alvin.savoy@gmail.com> Date: Sat, 28 Jun 2014 17:35:14 +1000 Subject: [PATCH 9/9] sort: For sorted lists, push() and unshift() will add items at their sorted position. This fixes the failing tests on sort. --- map/sort/sort.js | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/map/sort/sort.js b/map/sort/sort.js index 14d4ed722c6..6615e013e73 100644 --- a/map/sort/sort.js +++ b/map/sort/sort.js @@ -1,4 +1,4 @@ -steal('can/util', 'can/list', function (can) { +steal('can/util', 'can/list', 'can/map/bubble.js', function (can) { // Change bubble rule to bubble on change if their is a comparator var oldBubbleRule = can.List._bubbleRule; @@ -124,21 +124,22 @@ steal('can/util', 'can/list', function (can) { var proto = can.List.prototype, old = proto[name]; proto[name] = function () { - // get the items being added - var args = getArgs(arguments), - // where we are going to add items - len = where ? this.length : 0; - // call the original method - var res = old.apply(this, arguments); - // cause the change where the args are: - // len - where the additions happened - // add - items added - // args - the items added - // undefined - the old value - if (this.comparator && args.length) { - this._triggerChange('' + len, 'add', args, undefined); + if (this.comparator && arguments.length) { + // get the items being added + var args = getArgs(arguments); + var i = args.length; + while (i--) { + // Go through and convert anything to an `map` that needs to be converted. + var val = can.bubble.set(this, i, this.__type(args[i], i) ); + // Insert this item at its sorted position. + var sortedIndex = this.sortedIndex(val); + this.splice(sortedIndex, 0, val); + } + return this; + } else { + // call the original method + return old.apply(this, arguments); } - return res; }; }); //- override changes for sorting