Fix selections after sorting or filtering

[ibdafna]

Signed-off-by: Itay Dafna i.b.dafna@gmail.com

This PR fixes the issue reported in #213 .

• Selections applied before sorting are cleared after sorting (no changes made here).
• Selections applied after sorting will refer to the correct data model.

[ibdafna]

No tests?

@gaborbernat It needs a DOM element which we won't have access to without it running in a browser, so running it without a browser would fail - we could potentially mock the response but that would be redundant since we already test selections separately. All feasible Python tests have already been added

[gaborbernat]

No tests?

@gaborbernat It needs a DOM element which we won't have access to without it running in a browser, so running it without a browser would fail - we could potentially mock the response but that would be redundant since we already test selections separately. All feasible Python tests have already been added

Maybe if you're talking about the end result. But for the python part you should be able to write an ui agnostic test, that just validates the right data gets passed to the ui layer.

[ibdafna]

No tests?

@gaborbernat It needs a DOM element which we won't have access to without it running in a browser, so running it without a browser would fail - we could potentially mock the response but that would be redundant since we already test selections separately. All feasible Python tests have already been added

Maybe if you're talking about the end result. But for the python part you should be able to write an ui agnostic test, that just validates the right data gets passed to the ui layer.

As I mentioned - we already have a UI agnostic selections test 👍

[gaborbernat]

As I mentioned - we already have a UI agnostic selections test

Where? And why they did not need to be changed when you modified this code. If tomorrow I open a PR and revert your changes inside ipydatagrid/datagrid.py I'd expect the CI to fail to stop me from introducing a regression. Without any test, I fail to see how will this happen 🤔 If the tests run outside of this CI/repo IMHO then they're not hosted in the right place because it allows me to break the main branch without getting a notice of it.

[ibdafna]

@gaborbernat I am happy to answer further questions offline

[kaiayoung]

Can we revert this? By instantiating a new DataGrid widget every time .selected_cell_values() is called, a new copy of a potentially large dataset is created. These extra copies won’t be garbage collected as long as the widget manager/comm still references them. A more robust solution will certainly take some more time and effort, but we may want to cut a release before that work is completed.

Also, regarding testing, the ._visible_rows trait is how the front end currently reports the results of any sort/filter transforms. That can be set kernel-side to test any functionality that depends on the result of any transforms without requiring a front end to be running.