checkmark not shown when selecting a row progammatically

[gastonceron]

When selecting a row programmatically (for instance from a click to a button) the associated check box does not get checked. I modified the program "example-checkbox-row-select.html" from the examples folder for my testing. See below:

select row 5

<button id="unselectRow5" style="position:relative;height:30px">
  unselect row 5</button>

<button id="unselectAllRows" style="position:relative;height:30px">
  unselect all rows</button>

. . . etc.

<script>
var grid;
 
$("#selectRow5").click(function() {
    let rowIndex = 5; // use row 5 for this example
    let selectedRowsIndexes = grid.getSelectedRows();
    selectedRowsIndexes.push(rowIndex);
    grid.setSelectedRows(selectedRowsIndexes);
});

$("#unselectRow5").click(function() {
    let rowIndex = 5; // use row 5 for this example
    let newSelectedRowsIndexes = [];
    let oldSelectedRowsIndexes = grid.getSelectedRows();
    while (oldSelectedRowsIndexes.length != 0) {
        let indexFound = oldSelectedRowsIndexes.pop();
        if (indexFound != rowIndex) {
            newSelectedRowsIndexes.push(indexFound)
        }
    }
    grid.setSelectedRows(newSelectedRowsIndexes);
});

$("#unselectAllRows").click(function() {
    grid.setSelectedRows([]);
});
</script>

1 - Start the program, Click the first button (select row 5). Row 5 gets selected but the checkbox is not checked. The box shows checked when manually selecting any other row by clicking on their check box.
2 - Start the program, Click the first button twice. The checkbox is cheked on the second click. Click the second button (unselect row 5). The row is unselected but the check mark is not removed. Click any other row. The check mark now is removed.
3 – Start the program. Click the first button twice. The check mark is checked. Click on the second button (unselect row 5). The check mark is not un-checked. Click on the third button (unselect all rows). The check mark is not unchecked. Click on the checkbox of any other row. The check mark on row 5 is unchecked.
SOLUTION:
Modify function handleSelectedRangesChanged(e, ranges) (around line 2683 of slick.grid.js)
comment lines 2703 and 2705. See below:
// if (simpleArrayEquals(previousSelectedRows, selectedRows)) {
trigger(self.onSelectedRowsChanged, {rows: getSelectedRows()}, e);
// }

[6pac]

Thanks for raising this, will check it out on the weekend

[ghiscoding]

That seems very very risky to remove a trigger, you might fix something by breaking something else (or someone else code)

[6pac]

I seem to recall there were some parts of the selection/current row logic that were very murky, the more I looked into it the more complex it got, and finally did nothing pending a complete rewite. Hopefully this is not that part of the code ...

[ghiscoding]

not sure if it's also related to this issue #345, seems different but who knows.

Also worth noting that this issue came from this Stack Overflow question

[ghiscoding]

That is when adding more and more Cypress E2E tests does help and becomes more important. Every new PR that I do, I'm trying to add new tests as much as possible (now at 78 tests yay), though I wish someday that someone else will contribute in adding these tests too lol

[gastonceron]

In response to the kind observation from ghiscoding, I am not suggesting to remove a trigger. On the contrary, the line "if (simpleArrayEquals(pr...etc" that I am commenting out prevents the trigger to be fired on some circumstances. On the case of my examples, they fail because the trigger is not fired due to the line I commented out.

Removing the line (and its closing bracket a couple lines below) allows the trigger to fire on all occasions.

I think that the purpose of that line is to prevent the trigger on some cases for speed optimization reasons, but I am not sure of that for I did not take the time to fully understand the code.

[gastonceron]

My fault, I did not mean to close this. My apologies.

[ghiscoding]

@6pac
I have good and bad news for you, I saw you made a commit to try to fix this issue however it seems that the few Cypress E2E tests that I had added some time ago now fails. I tried re-running the GitHub Actions to retest but it still fails and the Good News is that these E2E tests now have a greater reason to exist 😉

Here's the test that fails

1) Example - Checkbox Header Row should click on Select All and display previous and new selected rows:
     CypressError: Timed out retrying: Expected to find content: '1,3,5,7,9,11,13,15,17,19,21,23,25,27,29,31,33,35,37,39,41,43,45,47,49,51,53,55,57,59,61,63,65,67,69,71,73,75,77,79,81,83,85,87,89,91,93,95,97,99' within the element: <div#selectedRows> but never did.

That is when the "Select All" button is clicked, it seems to be broken after your last commit.

Perhaps you can run the Cypress E2E tests on your local, you can follow the instruction I wrote in README

[6pac]

and am I correct in that the only problem with the previous release was the wrong version number in the package-lock.json file?

[6pac]

I can't find any reason for that test not to work. It works locally for me, and I didn't modify that example or any code that depends on it.

It's now 12:30pm HERE and I'm turning in!

[6pac]

@gastonceron thanks a lot for that bug report! That was a really bad bug - the getSelectedRows() method was returning the actual internal selectedRows variable and the button code was modifying it, hence when calling the 'selections changed' code, the change was already made and no change was detected. I've modified slick.grid.js so that getSelectedRows() returns a copy of the array instead, and now it's all back to normal.

[ghiscoding]

@6pac I will look at that test over the weekend then. I don't think the package.json has any effect on the content of the npm bundle though.

[6pac]

from what I could see, the package.json contained the correct version, it was the package-lock.json that didn't because it's new and I hadn't updated my release script to sync the version number to it.

[ghiscoding]

@6pac
I looked at the Cypress test failing and got latest code, it really is a new bug that you introduced. The test is for making sure that when using the icon override (only show the checkbox on every 2nd row), it should return only the possible rows (1, 3, 5, ...) but after your change it now returns even rows that don't have the checkbox (0, 1, 2, 3, 4, 5, ...).

I'll take a look this weekend and try to fix this, but that is a new bug. I'm reopening the issue to remind myself to look into this.

[6pac]

It's OK, I've fixed this now too. An additional bug in the plugin. It was using if (rowItem.selectableRow !== false) { rather than checkSelectableOverride(row, dataContext, _grid) in one last spot

[ghiscoding]

oh ok cool, so are all the Cypress tests passing now? That'd be great, 1 less thing to look into :)

...yup I see the GitHub Action is all green now and also tried locally, also all green. Sweet, I guess we could close this issue since you've fixed it all. Thanks

Still glad I started adding all these E2E tests, it's starting to help.

[gastonceron]

Thanks for your help. My program is running fine now with the new version 2.4.21 Gaston From: Ben McIntyre [mailto:[email protected]] Sent: Friday, March 27, 2020 11:25 PM To: 6pac/SlickGrid <[email protected]> Cc: Gaston Ceron <[email protected]>; Mention <[email protected]> Subject: Re: [6pac/SlickGrid] checkmark not shown when selecting a row progammatically (#478) It's OK, I've fixed this now too. An additional bug in the plugin. It was using if (rowItem.selectableRow !== false) { rather than checkSelectableOverride(row, dataContext, _grid) in one last spot — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#478 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC22Z2GOLSRXUQUXJATXIATRJVU2LANCNFSM4LUKAVFQ> .