mleibman / SlickGrid

A lightning fast JavaScript grid/spreadsheet
http://wiki.github.com/mleibman/SlickGrid
MIT License
6.81k stars 1.98k forks source link

Grouping doesn't work properly #896

Open dtang01 opened 10 years ago

dtang01 commented 10 years ago

I was learning the "example-grouping" example, and came across a bug which can be repeated as follows: (1) Change the loadData(50) to loadData(20) - this is changed in order to to display all groups in one page for a clear view (2) Click Load 20 rows to load only 20 randomly generated rows (3) Click "Collapse all groups" button to collapse and display all groups in one page (4) Now, please pay attention to the last group (in my case it is called "Duration 28 (2 items)"). Then expand the FIRST group (in my case it is "Duration 2 (1 Items)"), you will see that the last group is duplicated (in my case I saw two "Duration 28 (2 items)" groups), one is still at its original position before expansion, the other moves down to the right place after expansion.

icoxfog417 commented 10 years ago

it is mentioned in #841 too. Maybe same issue.

dtang01 commented 10 years ago

yes, it is the same issue. I used the method you suggested, it fixed the duplication issue. But the change brought another issue: the expansion/collapse seems stopped functioning after I hit the "collapse all" button.

I guess we need investigate on...

dtang01 commented 10 years ago

I finally get it work properly by modifying the "function getRowDiffs(rows, newRows)" function... Here is what I did: (a) change "to = newRows.length" to "to = newRows.length - 1" (b) change "to = Math.min(newRows.length," to "to = Math.min(newRows.length - 1," (c) change "for (var i = from, rl = rows.length; i < to; i++) {" to "for (var i = from, rl = rows.length; i <= to; i++) {"

Many thanks to "icoxfog417" for his/her good suggestion!

The code is following:

function getRowDiffs(rows, newRows) {
  var item, r, eitherIsNonData, diff = [];
  var from = 0, to = newRows.length - 1;

  if (refreshHints && refreshHints.ignoreDiffsBefore) {
    from = Math.max(0,
        Math.min(newRows.length, refreshHints.ignoreDiffsBefore));
  }

  if (refreshHints && refreshHints.ignoreDiffsAfter) {
    to = Math.min(newRows.length - 1,
        Math.max(0, refreshHints.ignoreDiffsAfter));
  }

  for (var i = from, rl = rows.length; i <= to; i++) {
    if (i >= rl) {
      diff[diff.length] = i;
    } else {
      item = newRows[i];
      r = rows[i];

      if ((groupingInfos.length && (eitherIsNonData = (item.__nonDataRow) || (r.__nonDataRow)) &&
          item.__group !== r.__group ||
          item.__group && !item.equals(r))
          || (eitherIsNonData &&
          // no good way to compare totals since they are arbitrary DTOs
          // deep object comparison is pretty expensive
          // always considering them 'dirty' seems easier for the time being
          (item.__groupTotals || r.__groupTotals))
          || item[idProperty] != r[idProperty]
          || (updated && updated[item[idProperty]])
          ) {
        diff[diff.length] = i;
      }
    }
  }
  return diff;
}
icoxfog417 commented 10 years ago

I think it is the matter of slick.groupitemmetadataprovider.js.

refreshHints.ignoreDiffsAfter means "after this index , the difference is ignored" . (in the code , we can write like this).

for(i = refreshHints.ignoreDiffsBefore ; i < refreshHints.ignoreDiffsAfter,i++){
  //not ignored rows
}

The code in slick.dataview.js is implemented so. But in slick.groupitemmetadataprovider.js , refreshHints.ignoreDiffsAfter is set from range that calculated by _grid.getRenderedRange().

var range = _grid.getRenderedRange();
this.getData().setRefreshHints({
    ignoreDiffsBefore: range.top,
    ignoreDiffsAfter: range.bottom
});

range.bottom is the last index of the row in grid, and it have to be checked its difference. So, we have to + 1 to range.bottom for ignoreDiffsAfter .

I made pull request for this issue , please check it.