Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add extra caller property to onSelectedRowsChanged #659

Merged
merged 1 commit into from
Dec 6, 2021
Merged

feat: add extra caller property to onSelectedRowsChanged #659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions plugins/slick.cellselectionmodel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,20 @@
return !areDifferent;
}

function setSelectedRanges(ranges) {
function setSelectedRanges(ranges, caller) {
// simple check for: empty selection didn't change, prevent firing onSelectedRangesChanged
if ((!_ranges || _ranges.length === 0) && (!ranges || ranges.length === 0)) { return; }

// if range has not changed, don't fire onSelectedRangesChanged
var rangeHasChanged = !rangesAreEqual(_ranges, ranges);

_ranges = removeInvalidRanges(ranges);
if (rangeHasChanged) { _self.onSelectedRangesChanged.notify(_ranges); }
if (rangeHasChanged) {
// provide extra "caller" argument through SlickEventData to avoid breaking pubsub event that only accepts an array of selected range
var eventData = new Slick.EventData();
Object.defineProperty(eventData, 'detail', { writable: true, configurable: true, value: { caller: caller || "SlickCellSelectionModel.setSelectedRanges" } });
_self.onSelectedRangesChanged.notify(_ranges, eventData);
Copy link
Collaborator Author

@ghiscoding ghiscoding Dec 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@6pac could you review and provide feedback to this PR please.
I had to resolve to cheating code to get this working, I'm passing extra data via a SlickEventData (which was not made for that), but I don't see any other ways to pass extra data since onSelectedRowsChanged and onSelectedRangesChanged only resolve arrays of SlickRange and/or row number. We typically return an object that we can extend in the future but these events only have arrays which we can't really extend and so no ways of providing extra stuff which is why I resolved to using cheat code. Are you ok with the change? Do you have any other suggestions that might work?

Copy link
Owner

@6pac 6pac Dec 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can see, the calling pattern for onSelectedRowsChanged in the core grid is:

  if (simpleArrayEquals(previousSelectedRows, selectedRows)) {
    trigger(
        self.onSelectedRowsChanged, 
        {   rows: getSelectedRows(), 
            previousSelectedRows: previousSelectedRows
        }, 
        e);
  }

So this uses the 'args-is-an-object' convention, and you should be able to just add the property.
For onSelectedRangesChanged, it appears only to be used in cellselectionmodel and rowselectionmodel (though it may also be called by user code), with the call:

  _self.onSelectedRangesChanged.notify(_ranges);

This doesn't follow the convention of 'args-is-an-object', which really should be corrected, but that would be a breaking change. The only non-breaking way I can think of is to take advantage of the fact that in js an array is also an object. So we could do, for example:

  var args = _ranges;
  args.ranges = _ranges;
  args.caller = caller;
  _self.onSelectedRangesChanged.notify(args);

a bit of a hack, but technically correct. It may be confusing when looking in the debugger, but I think a comment at the call would be enough to demystify that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I can give a try next week but I'm not crazy about this approach and I don't think that would work because the other side is really expecting an array not an object, but since nearly everything is an object in JS we never know, I'll give a try next week. If that doesn't work, my approach, even if not ideal, is still working and for the most part this code is used only internally (to communicate between SlickGrid & the plugins) so we might be ok since it's not that expose to the outside user.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. What I'm suggesting is that in internal code we treat it as an object. Setting it as an array is only to maintain backwards compatibilty for legacy user calls.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that my proposed solution is a hack, I think yours works equally well, so either-or. The issue with onSelectedRangesChanged does need to go on the list for when we finally release a breaking version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@6pac yeah I would rather merge it as it is, I know it works with use case in our Salesforce environment and so I would rather use that for now. It's mostly an internal thing but you never know when someone hooks into any of the onSelectedRangesChanged events (I'm sure there's someone in the world doing that) so I would rather not break them and my solution even though hacky as well isn't too bad and it works without breaking anything

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so we are ready to merge this and release?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait give me another 5min, I'm giving a try with your hack which is basically this SO

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems to work too, but I'm not sure how that would work with TypeScript. I would say let's merge with the code I have now (it works with TypeScript in my lib) and we can keep it as a 2nd option. Let me merge my PR then

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think Typescript would like it. As they say in the SO post, it's not that it won't work, it's just quite an unusual pattern and might be quite confusing for some developers. OK, willl pull and release.

}
}

function getSelectedRanges() {
Expand Down
14 changes: 7 additions & 7 deletions plugins/slick.checkboxselectcolumn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@
var remIdx = selectedRows.indexOf(removeList[i]);
selectedRows.splice(remIdx, 1);
}
_grid.setSelectedRows(selectedRows);
_grid.setSelectedRows(selectedRows, "click.cleanup");
}
}

Expand Down Expand Up @@ -179,9 +179,9 @@
if (_selectedRowsLookup[row]) {
_grid.setSelectedRows($.grep(_grid.getSelectedRows(), function (n) {
return n != row;
}));
}), "click.toggle");
} else {
_grid.setSelectedRows(_grid.getSelectedRows().concat(row));
_grid.setSelectedRows(_grid.getSelectedRows().concat(row), "click.toggle");
}
_grid.setActiveCell(row, getCheckboxColumnCellIndex());
}
Expand All @@ -193,7 +193,7 @@
addRows[addRows.length] = rowArray[i];
}
}
_grid.setSelectedRows(_grid.getSelectedRows().concat(addRows));
_grid.setSelectedRows(_grid.getSelectedRows().concat(addRows), "SlickCheckboxSelectColumn.selectRows");
}

function deSelectRows(rowArray) {
Expand All @@ -205,7 +205,7 @@
}
_grid.setSelectedRows($.grep(_grid.getSelectedRows(), function (n) {
return removeRows.indexOf(n) < 0;
}));
}), "SlickCheckboxSelectColumn.deSelectRows");
}

function handleHeaderClick(e, args) {
Expand All @@ -226,9 +226,9 @@
rows.push(i);
}
}
_grid.setSelectedRows(rows);
_grid.setSelectedRows(rows, "click.selectAll");
} else {
_grid.setSelectedRows([]);
_grid.setSelectedRows([], "click.selectAll");
}
e.stopPropagation();
e.stopImmediatePropagation();
Expand Down
14 changes: 10 additions & 4 deletions plugins/slick.rowselectionmodel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,20 @@
}

function setSelectedRows(rows) {
setSelectedRanges(rowsToRanges(rows));
setSelectedRanges(rowsToRanges(rows), "SlickRowSelectionModel.setSelectedRows");
}

function setSelectedRanges(ranges) {
function setSelectedRanges(ranges, caller) {
// simple check for: empty selection didn't change, prevent firing onSelectedRangesChanged
if ((!_ranges || _ranges.length === 0) && (!ranges || ranges.length === 0)) { return; }
if ((!_ranges || _ranges.length === 0) && (!ranges || ranges.length === 0)) {
return;
}
_ranges = ranges;
_self.onSelectedRangesChanged.notify(_ranges);

// provide extra "caller" argument through SlickEventData to avoid breaking pubsub event that only accepts an array of selected range
var eventData = new Slick.EventData();
Object.defineProperty(eventData, 'detail', { writable: true, configurable: true, value: { caller: caller || "SlickRowSelectionModel.setSelectedRanges" } });
_self.onSelectedRangesChanged.notify(_ranges, eventData);
}

function getSelectedRanges() {
Expand Down
16 changes: 13 additions & 3 deletions slick.grid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2840,7 +2840,17 @@ if (typeof Slick === "undefined") {
setCellCssStyles(options.selectedCellCssClass, hash);

if (simpleArrayEquals(previousSelectedRows, selectedRows)) {
trigger(self.onSelectedRowsChanged, {rows: getSelectedRows(), previousSelectedRows: previousSelectedRows}, e);
var caller = e && e.detail && e.detail.caller || 'click';
var newSelectedAdditions = getSelectedRows().filter(function(i) { return previousSelectedRows.indexOf(i) < 0 });
var newSelectedDeletions = previousSelectedRows.filter(function(i) { return getSelectedRows().indexOf(i) < 0 });

trigger(self.onSelectedRowsChanged, {
rows: getSelectedRows(),
previousSelectedRows: previousSelectedRows,
caller : caller,
changedSelectedRows: newSelectedAdditions,
changedUnselectedRows: newSelectedDeletions
}, e);
}
}

Expand Down Expand Up @@ -5915,12 +5925,12 @@ if (typeof Slick === "undefined") {
return selectedRows.slice(0);
}

function setSelectedRows(rows) {
function setSelectedRows(rows, caller) {
if (!selectionModel) {
throw new Error("SlickGrid Selection model is not set");
}
if (self && self.getEditorLock && !self.getEditorLock().isActive()) {
selectionModel.setSelectedRanges(rowsToRanges(rows));
selectionModel.setSelectedRanges(rowsToRanges(rows), caller || "SlickGrid.setSelectedRows");
}
}

Expand Down