From 9b2bb84b8586cf07f3c5f7529177ecd82312420b Mon Sep 17 00:00:00 2001 From: Alexey Makhrov Date: Thu, 19 Feb 2015 08:27:54 -0800 Subject: [PATCH 1/3] CollectionBinder: Add unit tests for sorting algorithm, both for a dedicated container and for a container with some other static content. --- .../CollectionBinder/sorting.spec.js | 103 +++++++++++++++++- 1 file changed, 101 insertions(+), 2 deletions(-) diff --git a/spec/javascripts/CollectionBinder/sorting.spec.js b/spec/javascripts/CollectionBinder/sorting.spec.js index 9ae6ee2..de75f5d 100644 --- a/spec/javascripts/CollectionBinder/sorting.spec.js +++ b/spec/javascripts/CollectionBinder/sorting.spec.js @@ -50,6 +50,18 @@ describe("CollectionBinder: sorting", function(){ } }); + var ParentViewWithStaticContent = ParentViewAutoSort.extend({ + render: function () { + this.$el.append('
Some static content that precedes the collection. Can be a table header, for instance
'); + + return ParentViewAutoSort.prototype.render.apply(this, arguments); + }, + + getStaticContentIndex: function () { + return this.$('.static-content').index(); + } + }); + var createViewForTest = function (View, collection) { var view = new View ({collection: collection}); @@ -213,8 +225,6 @@ describe("CollectionBinder: sorting", function(){ it('Should rearrange elements on collection.sort()', function () { var view = createView(this.collection); - var collectionBinder = view.collectionBinder; - this.collection.reset([{id: 1}, {id: 2}]); expect(view.getNestedElementIndex(1)).toBe(0); @@ -233,4 +243,93 @@ describe("CollectionBinder: sorting", function(){ expect(view.getNestedElementIndex(1)).toBe(1); }); }); + + + var testSequenceSorting = function (sequence, withStaticContent) { + var View = ParentViewAutoSort, minIndex = 0; + + if (withStaticContent) { + View = ParentViewWithStaticContent; + minIndex = 1; + } + + var collection = new Backbone.Collection(); + + var view = createViewForTest(View, collection); + + var data = _.map(sequence, function (d) { + return {id: d}; + }); + + collection.reset(data); + + _.each(sequence, function (id, index) { + expect(view.getNestedElementIndex(id)).toBe(index + minIndex); + }); + + collection.comparator = 'id'; + collection.sort(); + + if (withStaticContent) { + // index of the static content should remain 0 + expect(view.getStaticContentIndex()).toBe(0); + } + + // index of element should be index of the corresponding model in the collection plus + collection.each(function (model, index) { + expect(view.getNestedElementIndex(model.id)).toBe(index + minIndex); + }); + }; + + + describe('With the sort algorithm', function () { + var sequences = [ + [1, 2, 3, 4, 5, 6], + [5, 4, 3, 2, 1], + [1, 2, 4, 5, 3], + [3, 5, 1, 8, 6] + ]; + + _.each(sequences, function (sequence, index) { + it('Should properly sort sequence: '+ sequence, function () { + testSequenceSorting(sequence); + }); + }); + }); + + + describe('With a container having static content before collection elements', function () { + var createView = function (collection) { + return createViewForTest(ParentViewWithStaticContent, collection); + }; + + it('Should insert the first element after the static content on collection.add with {at: 0}', function () { + var view = createView(this.unorderedCollection); + + this.unorderedCollection.add({id: 1}); + this.unorderedCollection.add({id: 0}, {at: 0}); + + // index of element should be index of the corresponding model in the collection plus 1 + // because index=0 is the index of the static content prepended to the container + expect(view.getNestedElementIndex(0)).toBe(0+1); + expect(view.getNestedElementIndex(1)).toBe(1+1); + }); + + it('Should not touch the leading static content on collection.sort()', function () { + testSequenceSorting([1,2,0,3], true); + }); + + var sequences = [ + [1, 2, 3, 4, 5, 6], + [5, 4, 3, 2, 1], + [1, 2, 4, 5, 3], + [3, 5, 1, 8, 6] + ]; + + _.each(sequences, function (sequence, index) { + it('Should properly sort sequence: '+ sequence, function () { + testSequenceSorting(sequence); + }); + }); + }); }); \ No newline at end of file From ce7cc328fcad8dcf420e543dbf86f556f18ddea8 Mon Sep 17 00:00:00 2001 From: Alexey Makhrov Date: Thu, 19 Feb 2015 08:35:48 -0800 Subject: [PATCH 2/3] CollectionBinder: Update algorithm in sortRootEls to take static content into account and don't move it to bottom each time --- Backbone.CollectionBinder.js | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/Backbone.CollectionBinder.js b/Backbone.CollectionBinder.js index d53d5ef..41dcb9a 100644 --- a/Backbone.CollectionBinder.js +++ b/Backbone.CollectionBinder.js @@ -160,15 +160,28 @@ }, sortRootEls: function(){ + // sorting an empty collection or a collection with a single element won't change anything + if (this._collection.length < 2) return; + + var children = $(this._elManagerFactory._getParentEl()).children(); + var indexes = this._collection.models.map(function (model) { + var modelElManager = this.getManagerForModel(model); + var modelEl = modelElManager.getEl(); + return modelEl.index(); + }, this); + + var minIndex = Math.min.apply(Math, indexes); + this._collection.each(function(model, modelIndex){ var modelElManager = this.getManagerForModel(model); if(modelElManager){ + var realIndex = modelIndex + minIndex; var modelEl = modelElManager.getEl(); var currentRootEls = $(this._elManagerFactory._getParentEl()).children(); - if(currentRootEls[modelIndex] !== modelEl[0]){ + if(currentRootEls[realIndex] !== modelEl[0]){ modelEl.detach(); - modelEl.insertBefore(currentRootEls[modelIndex]); + modelEl.insertBefore(currentRootEls[realIndex]); } } }, this); From 327dfc2cd2544f1e9d5a4d0eaa694e0347b74bbe Mon Sep 17 00:00:00 2001 From: Alexey Makhrov Date: Thu, 19 Feb 2015 08:38:07 -0800 Subject: [PATCH 3/3] CollectionBinder: Rewrite the sorting algorithm in sortRootEls to make less DOM calls --- Backbone.CollectionBinder.js | 95 ++++++++++++++++++++++++++++++------ 1 file changed, 79 insertions(+), 16 deletions(-) diff --git a/Backbone.CollectionBinder.js b/Backbone.CollectionBinder.js index 41dcb9a..395f25d 100644 --- a/Backbone.CollectionBinder.js +++ b/Backbone.CollectionBinder.js @@ -163,28 +163,91 @@ // sorting an empty collection or a collection with a single element won't change anything if (this._collection.length < 2) return; + // Algorithm: one by one take elements and put to the correct position in DOM + // Computational complexity: O(n^2) + // DOM operations: + // - $.children() calls: 1 + // - $.before/after calls: n + + + // ----- + // prepare data for sorting + var children = $(this._elManagerFactory._getParentEl()).children(); - var indexes = this._collection.models.map(function (model) { + + var order = 0; + + var getElData = function (model) { var modelElManager = this.getManagerForModel(model); + if (!modelElManager) return; + var modelEl = modelElManager.getEl(); - return modelEl.index(); - }, this); + if (!modelEl) return; - var minIndex = Math.min.apply(Math, indexes); + return { + el: modelEl, + elIndex: children.index(modelEl), + order: order++ + } + }; - this._collection.each(function(model, modelIndex){ - var modelElManager = this.getManagerForModel(model); - if(modelElManager){ - var realIndex = modelIndex + minIndex; - var modelEl = modelElManager.getEl(); - var currentRootEls = $(this._elManagerFactory._getParentEl()).children(); - - if(currentRootEls[realIndex] !== modelEl[0]){ - modelEl.detach(); - modelEl.insertBefore(currentRootEls[realIndex]); - } + var elements = _.chain(this._collection.models) + .map(getElData, this) + .compact() + .value(); + + var minElIndex = _.min(elements, function (e) { return e.elIndex; }).elIndex; + var maxElIndex = _.max(elements, function (e) { return e.elIndex; }).elIndex; + + // ----- + // define some helpers + + var findElByOrder = function (order) { + return _.findWhere(elements, {order: order}); + }; + + var findElByElIndex = function (elIndex) { + return _.findWhere(elements, {elIndex: elIndex}); + }; + + var moveEl = function (el, oldIndex, newIndex) { + var moveForward = newIndex > oldIndex; + + if (newIndex < maxElIndex) { + var nextEl = findElByElIndex(newIndex + (moveForward ? 1 : 0)); + nextEl.el.before(el); + } else { + var lastEl = findElByElIndex(newIndex); + lastEl.el.after(el); } - }, this); + }; + + var updateIndexesAfterMove = function (oldIndex, newIndex) { + var moveForward = newIndex > oldIndex; + + // update indexes of all elements that were shifted after the current element movement + _.each(elements, function (e) { + var elIndex = e.elIndex; + if (moveForward && elIndex > oldIndex && elIndex <= newIndex) e.elIndex--; + if (!moveForward && elIndex >= newIndex && elIndex < oldIndex) e.elIndex++; + }); + }; + + // ----- + // Actual sorting happens here + _.times(elements.length, function (order) { + var elementData = findElByOrder(order); + var oldIndex = elementData.elIndex; + + var newIndex = order + minElIndex; + + if (oldIndex === newIndex) return; + + moveEl(elementData.el, oldIndex, newIndex); + updateIndexesAfterMove(oldIndex, newIndex); + + elementData.elIndex = newIndex; + }); } });