Skip to content

Commit c43f306

Browse files
authored
fix: Context menu using stale selection (#2407)
- Although we handle the case where the grid is right-clicked outside of the current selection in `GridSelectionHandler.ts`, where the new selection will be updated to the row the cursor is on, state changes are batched in event handlers so `IrisGridContextMenuHandler.tsx` will be using a stale value for `selectedRanges` when it runs. - To fix this we can implement the exact same logic in both components, such that the selected ranges used in `onContextMenu` in `IrisGridContextMenuHandler.tsx` will be the same as what `onContextMenu` in `GridSelectionHandler.ts` will set it to after all handlers have ran. Test Snippet (any table will work though) ``` from deephaven import empty_table, input_table source = empty_table(10).update(["X = i", "Y = i"]) result = input_table(init_table=source) ``` Before this change, it was seen that right clicking any cell when the table first opens will not produce the full context menu as expected, and only subsequent clicks will open the full context menu (probably for the wrong cell).
1 parent 1909fac commit c43f306

File tree

3 files changed

+87
-2
lines changed

3 files changed

+87
-2
lines changed

Diff for: packages/grid/src/mouse-handlers/GridSelectionMouseHandler.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,7 @@ class GridSelectionMouseHandler extends GridMouseHandler {
246246
);
247247

248248
// only change the selected range if the selected cell is not in the selected range
249-
if (!isInRange && gridPoint.row !== null) {
249+
if (!isInRange && gridPoint.row !== null && gridPoint.column !== null) {
250250
this.startPoint = undefined;
251251
this.stopTimer();
252252
grid.clearSelectedRanges();
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
import { GridRange } from '@deephaven/grid';
2+
import IrisGridContextMenuHandler from './IrisGridContextMenuHandler';
3+
4+
describe('getLatestSelection', () => {
5+
it('should return the original selection if the clicked cell is within the original selection', () => {
6+
const originalSelection = [new GridRange(1, 1, 2, 2)];
7+
const result = IrisGridContextMenuHandler.getLatestSelection(
8+
originalSelection,
9+
1,
10+
1
11+
);
12+
13+
expect(result).toBe(originalSelection);
14+
});
15+
16+
it('should return a new selection with the clicked cell if it is outside the original selection', () => {
17+
const originalSelection = [new GridRange(1, 1, 2, 2)];
18+
const columnIndex = 3;
19+
const rowIndex = 3;
20+
21+
const result = IrisGridContextMenuHandler.getLatestSelection(
22+
originalSelection,
23+
columnIndex,
24+
rowIndex
25+
);
26+
27+
expect(result).toEqual([GridRange.makeCell(columnIndex, rowIndex)]);
28+
});
29+
30+
it('should return the original selection if columnIndex is null', () => {
31+
const originalSelection = [new GridRange(1, 1, 2, 2)];
32+
33+
const result = IrisGridContextMenuHandler.getLatestSelection(
34+
originalSelection,
35+
null,
36+
1
37+
);
38+
39+
expect(result).toBe(originalSelection);
40+
});
41+
42+
it('should return the original selection if rowIndex is null', () => {
43+
const originalSelection = [new GridRange(1, 1, 2, 2)];
44+
45+
const result = IrisGridContextMenuHandler.getLatestSelection(
46+
originalSelection,
47+
null,
48+
1
49+
);
50+
51+
expect(result).toBe(originalSelection);
52+
});
53+
});

Diff for: packages/iris-grid/src/mousehandlers/IrisGridContextMenuHandler.tsx

+33-1
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import {
2222
GridMouseHandler,
2323
type GridPoint,
2424
GridRange,
25+
type GridRangeIndex,
2526
GridRenderer,
2627
isDeletableGridModel,
2728
isEditableGridModel,
@@ -149,6 +150,31 @@ class IrisGridContextMenuHandler extends GridMouseHandler {
149150
)}"`;
150151
}
151152

153+
/**
154+
* Returns the latest grid selection based on the current grid selection and where the user clicked
155+
* This code is dependent on the behavior of GridSelectionMouseHandler.onContextMenu
156+
* @param originalSelection The selection from the current grid state which may be stale
157+
* @param columnIndex The column index where the user clicked
158+
* @param rowIndex The row index where the user clicked
159+
*/
160+
static getLatestSelection(
161+
originalSelection: readonly GridRange[],
162+
columnIndex: GridRangeIndex,
163+
rowIndex: GridRangeIndex
164+
): readonly GridRange[] {
165+
const clickedInOriginalSelection = GridRange.containsCell(
166+
originalSelection,
167+
columnIndex,
168+
rowIndex
169+
);
170+
171+
// If the user clicked in a valid cell outside of the original selection,
172+
// the selection will be changed to just that cell.
173+
return clickedInOriginalSelection || columnIndex == null || rowIndex == null
174+
? originalSelection
175+
: [GridRange.makeCell(columnIndex, rowIndex)];
176+
}
177+
152178
irisGrid: IrisGrid;
153179

154180
debouncedUpdateCustomFormat: DebouncedFunc<
@@ -886,9 +912,15 @@ class IrisGridContextMenuHandler extends GridMouseHandler {
886912
isFilterBarShown,
887913
quickFilters,
888914
advancedFilters,
889-
selectedRanges,
915+
selectedRanges: stateSelectedRanges,
890916
} = irisGrid.state;
891917

918+
const selectedRanges = IrisGridContextMenuHandler.getLatestSelection(
919+
stateSelectedRanges,
920+
columnIndex,
921+
rowIndex
922+
);
923+
892924
assertNotNull(metrics);
893925

894926
const { columnHeaderHeight, gridY, columnHeaderMaxDepth } = metrics;

0 commit comments

Comments
 (0)