Fix(scrolling): dreadful iPad performance, badly formed

A handful of issues, mostly comes down to poor use of $interval in the throttle method (my change), and also fixing a defect where the aggregations weren't calculated by turning the throttle into a trailing throttle. This resulted in the grid waiting 0.5s before running the aggregation, but the aggregation triggered a $digest since a bound value had changed, and in turn that called aggregation again. Moved the aggregation call to be triggered by the rowsRendered event. This event is emitted whenever visible rows change, which should suit aggregation. This meant I had to move the call to aggregation into the gridFooterCell, as I needed a destroy event in order to destroy the registered api listener.
zackurben · Apr 21, 2015 · 10dc29b · 10dc29b
1 parent 9c80ac9
commit 10dc29b
Show file tree

Hide file tree

Showing 7 changed files with 81 additions and 19 deletions.
diff --git a/Gruntfile.js b/Gruntfile.js
@@ -37,7 +37,7 @@ module.exports = function(grunt) {
       version: util.getVersion(),
       stable_version: util.getStableVersion(),
       dist: 'dist',
-      site: process.env.TRAVIS ? 'ui-grid.info' : '127.0.0.1:<%= connect.docs.options.port %>',
+      site: process.env.TRAVIS ? 'ui-grid.info' : '192.168.1.3:<%= connect.docs.options.port %>',
       banner: '/*!\n' +
         ' * <%= pkg.title || pkg.name %> - v<%= version %> - ' +
         '<%= grunt.template.today("yyyy-mm-dd") %>\n' +

diff --git a/misc/tutorial/401_AllFeatures.ngdoc b/misc/tutorial/401_AllFeatures.ngdoc
@@ -23,6 +23,7 @@ All features are enabled to get an idea of performance
       $scope.gridOptions.enableGridMenu = true;
       $scope.gridOptions.showGridFooter = true;
       $scope.gridOptions.showColumnFooter = true;
+      $scope.gridOptions.aggregationCalcThrottle = 10000;
 
       $scope.gridOptions.rowIdentity = function(row) {
         return row.id;

diff --git a/src/js/core/directives/ui-grid-footer-cell.js b/src/js/core/directives/ui-grid-footer-cell.js
@@ -60,8 +60,11 @@
               }
             });
 
+
             // Register a data change watch that would get triggered whenever someone edits a cell or modifies column defs
             var dataChangeDereg = $scope.grid.registerDataChangeCallback( updateClass, [uiGridConstants.dataChange.COLUMN]);
+            // listen for visible rows change and update aggregation values
+            $scope.grid.api.core.on.rowsRendered( $scope, $scope.col.updateAggregationValue );
 
             $scope.$on( '$destroy', dataChangeDereg );
           }

diff --git a/src/js/core/factories/Grid.js b/src/js/core/factories/Grid.js
@@ -1302,9 +1302,6 @@ angular.module('ui.grid')
   };
 
   Grid.prototype.setVisibleRows = function setVisibleRows(rows) {
-    // this is also setting processedRows, providing a cache of the rows as they
-    // came out of the rowProcessors - in particular they are sorted so we
-    // can process them for grouping
     var self = this;
 
     // Reset all the render container row caches

diff --git a/src/js/core/factories/GridColumn.js b/src/js/core/factories/GridColumn.js
@@ -119,10 +119,24 @@ angular.module('ui.grid')
 
     self.aggregationValue = undefined;
 
-    var updateAggregationValue = function() {
+    // The footer cell registers to listen for the rowsRendered event, and calls this.  Needed to be
+    // in something with a scope so that the dereg would get called
+    self.updateAggregationValue = function() {
 
      // gridUtil.logDebug('getAggregationValue for Column ' + self.colDef.name);
 
+      /** 
+       * @ngdoc property
+       * @name aggregationType
+       * @propertyOf ui.grid.class:GridOptions.columnDef
+       * @description The aggregation that you'd like to show in the columnFooter for this
+       * column.  Valid values are in uiGridConstants, and currently include `uiGridConstants.aggregationTypes.count`, 
+       * `uiGridConstants.aggregationTypes.sum`, `uiGridConstants.aggregationTypes.avg`, `uiGridConstants.aggregationTypes.min`, 
+       * `uiGridConstants.aggregationTypes.max`.
+       * 
+       * You can also provide a function as the aggregation type, in this case your function needs to accept the full
+       * set of visible rows, and return a value that should be shown 
+       */
       if (!self.aggregationType) {
         self.aggregationValue = undefined;
         return;
@@ -173,10 +187,7 @@ angular.module('ui.grid')
       }
     };
 
-    var throttledUpdateAggregationValue = gridUtil.throttle(updateAggregationValue, self.grid.options.aggregationCalcThrottle, { trailing: true });
-
-
-
+//     var throttledUpdateAggregationValue = gridUtil.throttle(updateAggregationValue, self.grid.options.aggregationCalcThrottle, { trailing: true, context: self.name });
 
     /**
      * @ngdoc function
@@ -186,15 +197,12 @@ angular.module('ui.grid')
      * Debounced using scrollDebounce option setting
      */
     this.getAggregationValue =  function() {
-      if (!self.grid.isScrollingVertically && !self.grid.isScrollingHorizontally) {
-        throttledUpdateAggregationValue();
-      }
+//      if (!self.grid.isScrollingVertically && !self.grid.isScrollingHorizontally) {
+//        throttledUpdateAggregationValue();
+//      }
 
       return self.aggregationValue;
-    } ;
-
-
-
+    };
   }
 
 
@@ -627,7 +635,7 @@ angular.module('ui.grid')
       defaultFilters.push({});
     }
 
-/**
+    /**
      * @ngdoc property
      * @name filter
      * @propertyOf ui.grid.class:GridOptions.columnDef

diff --git a/src/js/core/services/ui-grid-util.js b/src/js/core/services/ui-grid-util.js
@@ -1082,6 +1082,11 @@ module.service('gridUtil', ['$log', '$window', '$document', '$http', '$templateC
    *    trailing (bool) - whether to trigger after throttle time ends if called multiple times
    * Updated to use $interval rather than $timeout, as protractor (e2e tests) is able to work with $interval,
    * but not with $timeout
+   * 
+   * Note that when using throttle, you need to use throttle to create a new function upfront, then use the function
+   * return from that call each time you need to call throttle.  If you call throttle itself repeatedly, the lastCall
+   * variable will get overwritten and the throttling won't work
+   * 
    * @example
    * <pre>
    * var throttledFunc =  gridUtil.throttle(function(){console.log('throttled');}, 500, {trailing: true});
@@ -1092,12 +1097,12 @@ module.service('gridUtil', ['$log', '$window', '$document', '$http', '$templateC
    */
   s.throttle = function(func, wait, options){
     options = options || {};
-    var lastCall = 0, queued = null, context, args;
+    var lastCall = 0, queued = null, context, args, rndFunctionTag = Math.floor(Math.random() * (10000));
 
     function runFunc(endDate){
       lastCall = +new Date();
       func.apply(context, args);
-      $interval(function(){ queued = null; }, 0, 1);
+      $timeout(function(){ queued = null; }, 0);
     }
 
     return function(){

diff --git a/test/unit/core/factories/GridColumn.spec.js b/test/unit/core/factories/GridColumn.spec.js
@@ -191,10 +191,14 @@ describe('GridColumn factory', function () {
 
       buildCols();
       grid.modifyRows(grid.options.data);
+
+      // this would be called by the footer cell if it were rendered 
+      var deregFn = grid.api.core.on.rowsRendered(null, grid.columns[0].updateAggregationValue);
       grid.setVisibleRows(grid.rows);
 
       expect(grid.columns[0].getAggregationValue()).toEqual(5);
       expect(grid.columns[0].getAggregationText()).toEqual('total rows: ');
+      deregFn();
     });
 
     it('count, without label', function() {
@@ -203,21 +207,29 @@ describe('GridColumn factory', function () {
 
       buildCols();
       grid.modifyRows(grid.options.data);
+
+      // this would be called by the footer cell if it were rendered 
+      var deregFn = grid.api.core.on.rowsRendered(null, grid.columns[0].updateAggregationValue);
       grid.setVisibleRows(grid.rows);
 
       expect(grid.columns[0].getAggregationValue()).toEqual(5);
       expect(grid.columns[0].getAggregationText()).toEqual('');
+      deregFn();
     });
 
     it('sum, with label', function() {
       grid.options.columnDefs[1].aggregationType = uiGridConstants.aggregationTypes.sum;
 
       buildCols();
       grid.modifyRows(grid.options.data);
+
+      // this would be called by the footer cell if it were rendered 
+      var deregFn = grid.api.core.on.rowsRendered(null, grid.columns[1].updateAggregationValue);
       grid.setVisibleRows(grid.rows);
 
       expect(grid.columns[1].getAggregationValue()).toEqual(15);
       expect(grid.columns[1].getAggregationText()).toEqual('total: ');
+      deregFn();
     });
 
     it('sum, without label', function() {
@@ -226,21 +238,29 @@ describe('GridColumn factory', function () {
 
       buildCols();
       grid.modifyRows(grid.options.data);
+
+      // this would be called by the footer cell if it were rendered 
+      var deregFn = grid.api.core.on.rowsRendered(null, grid.columns[1].updateAggregationValue);
       grid.setVisibleRows(grid.rows);
 
       expect(grid.columns[1].getAggregationValue()).toEqual(15);
       expect(grid.columns[1].getAggregationText()).toEqual('');
+      deregFn();
     });
 
     it('avg, with label', function() {
       grid.options.columnDefs[1].aggregationType = uiGridConstants.aggregationTypes.avg;
 
       buildCols();
       grid.modifyRows(grid.options.data);
+
+      // this would be called by the footer cell if it were rendered 
+      var deregFn = grid.api.core.on.rowsRendered(null, grid.columns[1].updateAggregationValue);
       grid.setVisibleRows(grid.rows);
 
       expect(grid.columns[1].getAggregationValue()).toEqual(3);
       expect(grid.columns[1].getAggregationText()).toEqual('avg: ');
+      deregFn();
     });
 
     it('avg, without label', function() {
@@ -249,21 +269,29 @@ describe('GridColumn factory', function () {
 
       buildCols();
       grid.modifyRows(grid.options.data);
+
+      // this would be called by the footer cell if it were rendered 
+      var deregFn = grid.api.core.on.rowsRendered(null, grid.columns[1].updateAggregationValue);
       grid.setVisibleRows(grid.rows);
 
       expect(grid.columns[1].getAggregationValue()).toEqual(3);
       expect(grid.columns[1].getAggregationText()).toEqual('');
+      deregFn();
     });    
 
     it('min, with label', function() {
       grid.options.columnDefs[1].aggregationType = uiGridConstants.aggregationTypes.min;
 
       buildCols();
       grid.modifyRows(grid.options.data);
+
+      // this would be called by the footer cell if it were rendered 
+      var deregFn = grid.api.core.on.rowsRendered(null, grid.columns[1].updateAggregationValue);
       grid.setVisibleRows(grid.rows);
 
       expect(grid.columns[1].getAggregationValue()).toEqual(1);
       expect(grid.columns[1].getAggregationText()).toEqual('min: ');
+      deregFn();
     });
 
     it('min, without label', function() {
@@ -272,21 +300,29 @@ describe('GridColumn factory', function () {
 
       buildCols();
       grid.modifyRows(grid.options.data);
+
+      // this would be called by the footer cell if it were rendered 
+      var deregFn = grid.api.core.on.rowsRendered(null, grid.columns[1].updateAggregationValue);
       grid.setVisibleRows(grid.rows);
 
       expect(grid.columns[1].getAggregationValue()).toEqual(1);
       expect(grid.columns[1].getAggregationText()).toEqual('');
+      deregFn();
     });
 
     it('max, with label', function() {
       grid.options.columnDefs[1].aggregationType = uiGridConstants.aggregationTypes.max;
 
       buildCols();
       grid.modifyRows(grid.options.data);
+
+      // this would be called by the footer cell if it were rendered 
+      var deregFn = grid.api.core.on.rowsRendered(null, grid.columns[1].updateAggregationValue);
       grid.setVisibleRows(grid.rows);
 
       expect(grid.columns[1].getAggregationValue()).toEqual(5);
       expect(grid.columns[1].getAggregationText()).toEqual('max: ');
+      deregFn();
     });
 
     it('max, without label', function() {
@@ -295,10 +331,14 @@ describe('GridColumn factory', function () {
 
       buildCols();
       grid.modifyRows(grid.options.data);
+
+      // this would be called by the footer cell if it were rendered 
+      var deregFn = grid.api.core.on.rowsRendered(null, grid.columns[1].updateAggregationValue);
       grid.setVisibleRows(grid.rows);
 
       expect(grid.columns[1].getAggregationValue()).toEqual(5);
       expect(grid.columns[1].getAggregationText()).toEqual('');
+      deregFn();
     });
 
     it('max, with custom label', function() {
@@ -310,10 +350,14 @@ describe('GridColumn factory', function () {
 
       buildCols();
       grid.modifyRows(grid.options.data);
+
+      // this would be called by the footer cell if it were rendered 
+      var deregFn = grid.api.core.on.rowsRendered(null, grid.columns[1].updateAggregationValue);
       grid.setVisibleRows(grid.rows);
 
       expect(grid.columns[1].getAggregationValue()).toEqual(5);
       expect(grid.columns[1].getAggregationText()).toEqual(customLabel);
+      deregFn();
     });
 
     it('max, with custom label while also being hidden', function() {
@@ -325,10 +369,14 @@ describe('GridColumn factory', function () {
 
       buildCols();
       grid.modifyRows(grid.options.data);
+
+      // this would be called by the footer cell if it were rendered 
+      var deregFn = grid.api.core.on.rowsRendered(null, grid.columns[1].updateAggregationValue);
       grid.setVisibleRows(grid.rows);
 
       expect(grid.columns[1].getAggregationValue()).toEqual(5);
       expect(grid.columns[1].getAggregationText()).toEqual('');
+      deregFn();
     });
   });