-
Notifications
You must be signed in to change notification settings - Fork 34
Description
we want to improve SET_SELECTION.
Issue:
selection
is a list of the rowIndex values of the rows to be selected.
In a table with frequent inserts and deletes, we might have timing issues. Assume the user has sorted data. Inserts/deletes that occur towards start of sorted keyset will cause rowIndex positions of following rows to be updated. This is where we might see a race condition.
If user sends a SET_SELECTION message with a rowIndex just as we have an insert that triggers an update to rowIndex values, we could select the wrong row (ie based on its previous rowIndex value).
Proposed solution:
use row KEY values rather than rowIndex values to specify rows to select.
Right now, UI sends an array of every single rowIndex value that needs to be selected. If user uses SHIFT+SELECT to select a range, every rowIndex value in that range is included in the selection
.
Using row key values, this approach won’t work. If user selects a large range that spans many pages, the UI will not necessarily have all the data for the full set of rows to be selected. This doesn’t stop it building a list of rowIndex values but it will make it impossible to provide the associated row key values.
I suggest the following, we replace the current single selection message with a number of messages
{
type: "SELECT_ROW”;
preserveExistingSelection: boolean;
rowKey: string;
vpId: string;
}
If preserveExistingSelection
is true, simply update the target row to be selected. If preserveExistingSelection
is false, also remove selection from any other rows that are currently selected
{
type: “DESELECT_ROW”;
preserveExistingSelection: boolean;
rowKey: string;
vpId: string;
}
{
type: “SELECT_ROW_RANGE”;
preserveExistingSelection: boolean;
fromRowKey: string;
toRowKey: string;
vpId: string;
}
{
type: “SELECT_ALL”;
vpId: string;
}
{
type: “DESELECT_ALL”;
vpId: string;
}
The above would allow us to support the full set of extended selection
features that we currently have.
Note DESELECT_ROW with preserveExistingSelection:false would be identical to DESELECT_ALL, but API feels lopsided if we omit the preserveExistingSelection