diff --git a/OnDemandList.js b/OnDemandList.js index 3b54ef481..662e81727 100644 --- a/OnDemandList.js +++ b/OnDemandList.js @@ -352,17 +352,8 @@ return declare([List, _StoreMixin], { break; } var nextRow = row[traversal]; // have to do this before removing it - var lastObserverIndex, currentObserverIndex = row.observerIndex; - if(currentObserverIndex != lastObserverIndex && lastObserverIndex > -1){ - // we have gathered a whole page of observed rows, we can delete them now - var observers = grid.observers; - var observer = observers[lastObserverIndex]; - observer && observer.cancel(); - observers[lastObserverIndex] = 0; // remove it so we don't call cancel twice - } reclaimedHeight += rowHeight; count += row.count || 1; - lastObserverIndex = currentObserverIndex; // we just do cleanup here, as we will do a more efficient node destruction in the setTimeout below grid.removeRow(row, true); toDelete.push(row); @@ -565,6 +556,40 @@ return declare([List, _StoreMixin], { refreshDfd.resolve(lastResults); }); } + }, + + removeRow: function(rowElement, justCleanup){ + function chooseIndex(index1, index2){ + return index1 != null ? index1 : index2; + } + + if(rowElement){ + // Clean up observers that need to be cleaned up. + var previousNode = rowElement.previousSibling, + nextNode = rowElement.nextSibling, + prevIndex = previousNode && chooseIndex(previousNode.observerIndex, previousNode.previousObserverIndex), + nextIndex = nextNode && chooseIndex(nextNode.observerIndex, nextNode.nextObserverIndex), + thisIndex = rowElement.observerIndex; + + // Clear the observerIndex on the node being removed so it will not be considered any longer. + rowElement.observerIndex = undefined; + if(justCleanup){ + // Save the indexes from the siblings for future calls to removeRow. + rowElement.nextObserverIndex = nextIndex; + rowElement.previousObserverIndex = prevIndex; + } + + // Is this row's observer index different than those on either side? + if(thisIndex > -1 && thisIndex !== prevIndex && thisIndex !== nextIndex){ + // This is the last row that references the observer index. Cancel the observer. + var observers = this.observers; + var observer = observers[thisIndex]; + observer && observer.cancel(); + observers[thisIndex] = 0; // remove it so we don't call cancel twice + } + } + // Finish the row removal. + this.inherited(arguments); } }); diff --git a/test/intern/all.js b/test/intern/all.js index 0dfeb3bda..ecd96205e 100644 --- a/test/intern/all.js +++ b/test/intern/all.js @@ -5,5 +5,6 @@ define([ 'intern/node_modules/dojo/has!host-browser?./mixins/Keyboard', 'intern/node_modules/dojo/has!host-browser?./mixins/Selection', 'intern/node_modules/dojo/has!host-browser?./core/stores', - 'intern/node_modules/dojo/has!host-browser?./core/_StoreMixin' + 'intern/node_modules/dojo/has!host-browser?./core/_StoreMixin', + 'intern/node_modules/dojo/has!host-browser?./core/OnDemand-removeRow' ], function(){}); \ No newline at end of file diff --git a/test/intern/core/OnDemand-removeRow.js b/test/intern/core/OnDemand-removeRow.js new file mode 100644 index 000000000..f70adb7b8 --- /dev/null +++ b/test/intern/core/OnDemand-removeRow.js @@ -0,0 +1,111 @@ +define([ + "intern!tdd", + "intern/assert", + "dojo/_base/lang", + "dojo/store/Memory", + "dojo/store/Observable", + "dojo/query", + "dgrid/OnDemandList", + "put-selector/put" +], function(test, assert, lang, Memory, Observable, query, OnDemandList, put){ + + test.suite("removeRow", function(){ + + var list; + + function testInitialObservers(list, comment){ + var observers = list.observers; + [true, true, true].forEach(function(test, i){ + assert.isTrue(!!observers[i] === test, [comment, "index is " + i + ", Expected is " + test]); + }); + } + + function countObserverReferences(listId, observerIndex){ + var count = 0; + query(".dgrid-row", listId).forEach(function(row){ + if(row.observerIndex === observerIndex){ + count += 1; + } + }); + return count; + } + + test.beforeEach(function(){ + var data = []; + for(var i = 0; i < 1000; i++){ + data.push({id: i, value: i}); + } + + var store = Observable(new Memory({ data: data })); + list = new OnDemandList({ + id: "list1", + store: store, + queryRowsOverlap: 0, + renderRow: function(object){ + return put("div", object.value); + }, + minRowsPerPage: 10, + maxRowsPerPage: 10 + }); + put(document.body, list.domNode); + put(list.domNode, "[style=height:300px]"); + list.startup(); + }); + + test.afterEach(function(){ + list.destroy(); + }); + + test.test("OnDemandList w/observers - remove all, clean up only", function(){ + var countRefs = lang.partial(countObserverReferences, "list1"); + testInitialObservers(list, "Initial"); + assert.strictEqual(10, countRefs(0)); + assert.strictEqual(10, countRefs(1)); + for(var i = 0; i < 9; i++){ + var rowNode = document.getElementById("list1-row-" + i); + assert.strictEqual(0, rowNode.observerIndex, "Row's observerIndex"); + assert.strictEqual(10 - i, countRefs(0), "Iteration " + i + ": observer reference count"); + list.removeRow(rowNode, true); + testInitialObservers(list, "Iteration " + i); + assert.strictEqual(9 - i, countRefs(0), "Iteration " + i + ": observer reference count"); + assert.strictEqual(10, countRefs(1)); + } + + list.removeRow(document.getElementById("list1-row-9"), true); + assert.strictEqual(0, countRefs(0)); + assert.strictEqual(10, countRefs(1)); + [false, true, true].forEach(function(test, i){ + var observers = list.observers; + assert.isTrue(!!observers[i] === test, ["i: " + i + ", test = " + test, observers]); + }); + }); + + test.test("OnDemandList w/observers - remove last 2, clean up only", function(){ + var countRefs = lang.partial(countObserverReferences, "list1"); + // Removing the last two rows from observer #0 should not cancel the observer. + testInitialObservers(list, "Initial"); + assert.strictEqual(10, countRefs(0)); + assert.strictEqual(10, countRefs(1)); + + list.removeRow(document.getElementById("list1-row-7"), true); + testInitialObservers(list, "Removed row 8"); + assert.strictEqual(9, countRefs(0)); + assert.strictEqual(10, countRefs(1)); + + list.removeRow(document.getElementById("list1-row-8"), true); + testInitialObservers(list, "Removed row 9"); + assert.strictEqual(8, countRefs(0)); + assert.strictEqual(10, countRefs(1)); + + list.removeRow(document.getElementById("list1-row-1"), true); + testInitialObservers(list, "Removed row 1"); + assert.strictEqual(7, countRefs(0)); + assert.strictEqual(10, countRefs(1)); + + list.removeRow(document.getElementById("list1-row-0"), true); + testInitialObservers(list, "Removed row 0"); + assert.strictEqual(6, countRefs(0)); + assert.strictEqual(10, countRefs(1)); + }); + }); +}); \ No newline at end of file