From 22f80e8de6007301090a279455b5edc7b3f7dd3f Mon Sep 17 00:00:00 2001 From: deenairn Date: Thu, 3 Dec 2015 14:56:53 +0000 Subject: [PATCH 1/5] Issue#128: Added fix to have reference equality check for datarows. --- src/directives/hotTable.js | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/directives/hotTable.js b/src/directives/hotTable.js index 6932ede6..3f7fb7e2 100644 --- a/src/directives/hotTable.js +++ b/src/directives/hotTable.js @@ -104,6 +104,15 @@ }; scope.hotInstance = settingFactory.initializeHandsontable(element, scope.htSettings); + var dataRowsRefresh = function(scope, newValue) { + // If reference to data rows is not changed then only re-render table + if (scope.hotInstance.getSettings().data === newValue) { + settingFactory.renderHandsontable(scope.hotInstance); + } else { + scope.hotInstance.loadData(newValue); + } + } + // TODO: Add watch properties descriptor + needs perf test. Watch full equality vs toJson angular.forEach(bindingsKeys, function(key) { scope.$watch(key, function(newValue, oldValue) { @@ -111,18 +120,24 @@ return; } if (key === 'datarows') { - // If reference to data rows is not changed then only re-render table - if (scope.hotInstance.getSettings().data === newValue) { - settingFactory.renderHandsontable(scope.hotInstance); - } else { - scope.hotInstance.loadData(newValue); - } + dataRowsRefresh(scope, newValue); } else if (newValue !== oldValue) { scope.htSettings[key] = newValue; settingFactory.updateHandsontableSettings(scope.hotInstance, scope.htSettings); } }, ['datarows', 'columns', 'rowHeights', 'colWidths', 'rowHeaders', 'colHeaders'].indexOf(key) >= 0); }); + + /** + * Check for reference equality changes for datarows + * TODO: must the remaining bindingsKeys need to be added also if their reference changes + */ + scope.$watch('datarows', function(newValue) { + if (newValue === void 0) { + return; + } + dataRowsRefresh(scope, newValue); + }); /** * Check if data length has been changed From a956a9c6dfa4686e5a2ea5550aa9d4ed40259b1b Mon Sep 17 00:00:00 2001 From: Donald Nairn Date: Sat, 5 Dec 2015 19:06:25 +0000 Subject: [PATCH 2/5] Update to add test of change to scope ensure table is updated correctly. --- src/directives/hotTable.js | 9 ++++---- .../hotTable/watchingOptions.spec.js | 23 +++++++++++++++++-- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/directives/hotTable.js b/src/directives/hotTable.js index 3f7fb7e2..491462c4 100644 --- a/src/directives/hotTable.js +++ b/src/directives/hotTable.js @@ -107,10 +107,10 @@ var dataRowsRefresh = function(scope, newValue) { // If reference to data rows is not changed then only re-render table if (scope.hotInstance.getSettings().data === newValue) { - settingFactory.renderHandsontable(scope.hotInstance); - } else { - scope.hotInstance.loadData(newValue); - } + settingFactory.renderHandsontable(scope.hotInstance); + } else { + scope.hotInstance.loadData(newValue); + } } // TODO: Add watch properties descriptor + needs perf test. Watch full equality vs toJson @@ -120,6 +120,7 @@ return; } if (key === 'datarows') { + // If reference to data rows is not changed then only re-render table dataRowsRefresh(scope, newValue); } else if (newValue !== oldValue) { scope.htSettings[key] = newValue; diff --git a/test/directives/hotTable/watchingOptions.spec.js b/test/directives/hotTable/watchingOptions.spec.js index 2b8f8f4e..84af9d2a 100644 --- a/test/directives/hotTable/watchingOptions.spec.js +++ b/test/directives/hotTable/watchingOptions.spec.js @@ -15,14 +15,33 @@ describe('hotTable - Watching options', function() { }); it('should create table with `datarows` attribute', function() { - rootScope.value = [[]]; + rootScope.value = [[null]]; var scope = angular.element(compile('')(rootScope)).isolateScope(); scope.$digest(); - expect(scope.hotInstance.getData()).toBe(rootScope.value); + expect(scope.hotInstance.getData()).toEqual(rootScope.value); }); + + it('should ensure a copy of `datarows` is made when the reference is changed but it has the same value', function() { + rootScope.value = [[null]]; + var scope = angular.element(compile('')(rootScope)).isolateScope(); + + scope.$digest(); + expect(scope.hotInstance.getData()).toEqual(rootScope.value); + + scope.value = [[null]]; + + scope.$digest(); + + expect(scope.hotInstance.getData()).toEqual(rootScope.value); + + rootScope.value.push([null]); + + expect(scope.hotInstance.getData()).toEqual(rootScope.value); + }); + it('should create table with `dataSchema` attribute', function() { rootScope.value = {id: null}; var scope = angular.element(compile('')(rootScope)).isolateScope(); From 4ca32202ac5b50c176eba15aaddaf11bd9eac2cf Mon Sep 17 00:00:00 2001 From: Donald Nairn Date: Mon, 7 Dec 2015 00:05:37 +0000 Subject: [PATCH 3/5] Revised unit tests. --- .../hotTable/watchingOptions.spec.js | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/test/directives/hotTable/watchingOptions.spec.js b/test/directives/hotTable/watchingOptions.spec.js index 84af9d2a..1dddf078 100644 --- a/test/directives/hotTable/watchingOptions.spec.js +++ b/test/directives/hotTable/watchingOptions.spec.js @@ -15,31 +15,32 @@ describe('hotTable - Watching options', function() { }); it('should create table with `datarows` attribute', function() { - rootScope.value = [[null]]; + rootScope.value = [[]]; var scope = angular.element(compile('')(rootScope)).isolateScope(); scope.$digest(); - expect(scope.hotInstance.getData()).toEqual(rootScope.value); + expect(scope.hotInstance.getSettings().data).toBe(rootScope.value); }); - it('should ensure a copy of `datarows` is made when the reference is changed but it has the same value', function() { - rootScope.value = [[null]]; + it('should ensure a new copy of `datarows` is made when the reference is changed but it has the same value', function() { + rootScope.value = [[1,2,3,4]]; var scope = angular.element(compile('')(rootScope)).isolateScope(); scope.$digest(); - expect(scope.hotInstance.getData()).toEqual(rootScope.value); - - scope.value = [[null]]; - + expect(scope.hotInstance.getSettings().data).toBe(rootScope.value); + + rootScope.value = [[1, 2, 3, 4]]; + + rootScope.$digest(); + + expect(scope.hotInstance.getSettings().data).toBe(rootScope.value); + + rootScope.value.push([[5, 6, 7, 8]]); scope.$digest(); - - expect(scope.hotInstance.getData()).toEqual(rootScope.value); - - rootScope.value.push([null]); - - expect(scope.hotInstance.getData()).toEqual(rootScope.value); + + expect(scope.hotInstance.getSettings().data).toBe(rootScope.value); }); it('should create table with `dataSchema` attribute', function() { From 2ac5c3a9d6c13524b9ac6c241e31deae809bbd01 Mon Sep 17 00:00:00 2001 From: deenairn Date: Mon, 7 Dec 2015 11:20:16 +0000 Subject: [PATCH 4/5] Alter watch so that it only updates based on reference changes. --- src/directives/hotTable.js | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/directives/hotTable.js b/src/directives/hotTable.js index 3f7fb7e2..4cbe83e8 100644 --- a/src/directives/hotTable.js +++ b/src/directives/hotTable.js @@ -104,15 +104,6 @@ }; scope.hotInstance = settingFactory.initializeHandsontable(element, scope.htSettings); - var dataRowsRefresh = function(scope, newValue) { - // If reference to data rows is not changed then only re-render table - if (scope.hotInstance.getSettings().data === newValue) { - settingFactory.renderHandsontable(scope.hotInstance); - } else { - scope.hotInstance.loadData(newValue); - } - } - // TODO: Add watch properties descriptor + needs perf test. Watch full equality vs toJson angular.forEach(bindingsKeys, function(key) { scope.$watch(key, function(newValue, oldValue) { @@ -120,7 +111,12 @@ return; } if (key === 'datarows') { - dataRowsRefresh(scope, newValue); + // If reference to data rows is not changed then only re-render table + if (scope.hotInstance.getSettings().data === newValue) { + settingFactory.renderHandsontable(scope.hotInstance); + } else { + scope.hotInstance.loadData(newValue); + } } else if (newValue !== oldValue) { scope.htSettings[key] = newValue; settingFactory.updateHandsontableSettings(scope.hotInstance, scope.htSettings); @@ -133,10 +129,9 @@ * TODO: must the remaining bindingsKeys need to be added also if their reference changes */ scope.$watch('datarows', function(newValue) { - if (newValue === void 0) { - return; + if (scope.hotInstance.getSettings().data !== newValue) { + scope.hotInstance.loadData(newValue); } - dataRowsRefresh(scope, newValue); }); /** From d3b2049dc6419ec60774019b8d88df7ba633378a Mon Sep 17 00:00:00 2001 From: Donald Nairn Date: Mon, 7 Dec 2015 18:15:30 +0000 Subject: [PATCH 5/5] Messed up last commit via the merge. This should update the tests correctly for Travis CI. --- .../hotTable/watchingOptions.spec.js | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/test/directives/hotTable/watchingOptions.spec.js b/test/directives/hotTable/watchingOptions.spec.js index 2b8f8f4e..86d9ffad 100644 --- a/test/directives/hotTable/watchingOptions.spec.js +++ b/test/directives/hotTable/watchingOptions.spec.js @@ -20,9 +20,29 @@ describe('hotTable - Watching options', function() { scope.$digest(); - expect(scope.hotInstance.getData()).toBe(rootScope.value); + expect(scope.hotInstance.getSettings().data).toBe(rootScope.value); }); + + it('should ensure a new copy of `datarows` is made when the reference is changed but it has the same value', function() { + rootScope.value = [[1,2,3,4]]; + var scope = angular.element(compile('')(rootScope)).isolateScope(); + + scope.$digest(); + + expect(scope.hotInstance.getSettings().data).toBe(rootScope.value); + + rootScope.value = [[1, 2, 3, 4]]; + rootScope.$digest(); + + expect(scope.hotInstance.getSettings().data).toBe(rootScope.value); + + rootScope.value.push([[5, 6, 7, 8]]); + scope.$digest(); + + expect(scope.hotInstance.getSettings().data).toBe(rootScope.value); + }); + it('should create table with `dataSchema` attribute', function() { rootScope.value = {id: null}; var scope = angular.element(compile('')(rootScope)).isolateScope(); @@ -831,4 +851,4 @@ describe('hotTable - Watching options', function() { expect(scope.hotInstance.getSettings().className).toBe('foo'); expect(scope.hotInstance.getSettings().data).toEqual([[1]]); }); -}); +}); \ No newline at end of file