Fix #642: OnDemandList: Refactor how store observers are pruned

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);
 	}
 });
 

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(){});

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));
+		});
+	});
+});