Skip to content

Commit f2d6ced

Browse files
authored
fix: DH-21258: Fix column group separators (#2597)
Update `GridUtils.getColumnSeparatorIndex` to take column group depth into account. BREAKING CHANGE: `getColumnSeparatorIndex` now requires a `model` argument.
1 parent 20662ae commit f2d6ced

File tree

3 files changed

+205
-4
lines changed

3 files changed

+205
-4
lines changed

packages/grid/src/GridUtils.test.ts

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
1+
import { TestUtils } from '@deephaven/test-utils';
12
import { type AxisRange, type BoundedAxisRange } from './GridAxisRange';
23
import { type ModelIndex, type MoveOperation } from './GridMetrics';
34
import type GridMetrics from './GridMetrics';
5+
import type GridModel from './GridModel';
46
import GridRange, { type GridRangeIndex } from './GridRange';
7+
import GridTheme from './GridTheme';
58
import GridUtils, { type Token, type TokenBox } from './GridUtils';
69

710
function expectModelIndexes(
@@ -1067,3 +1070,145 @@ describe('translateTokenBox', () => {
10671070
expect(GridUtils.translateTokenBox(input, metrics)).toEqual(expectedValue);
10681071
});
10691072
});
1073+
1074+
describe('getColumnSeparatorIndex', () => {
1075+
const mockTheme = {
1076+
...GridTheme,
1077+
allowColumnResize: true,
1078+
headerSeparatorHandleSize: 10,
1079+
columnHeaderHeight: 30,
1080+
};
1081+
1082+
const createMockMetrics = (
1083+
columnHeaderMaxDepth = 1
1084+
): Partial<GridMetrics> => ({
1085+
rowHeaderWidth: 50,
1086+
columnHeaderHeight: 30,
1087+
columnHeaderMaxDepth,
1088+
floatingColumns: [],
1089+
floatingLeftWidth: 0,
1090+
visibleColumns: [0, 1, 2, 3],
1091+
allColumnXs: new Map([
1092+
[0, 0],
1093+
[1, 100],
1094+
[2, 200],
1095+
[3, 300],
1096+
]),
1097+
allColumnWidths: new Map([
1098+
[0, 100],
1099+
[1, 100],
1100+
[2, 100],
1101+
[3, 100],
1102+
]),
1103+
modelColumns: new Map([
1104+
[0, 0],
1105+
[1, 1],
1106+
[2, 2],
1107+
[3, 3],
1108+
]),
1109+
// Additional properties needed for getRowAtY (called by getColumnHeaderDepthAtY)
1110+
gridY: 30,
1111+
floatingTopRowCount: 0,
1112+
floatingBottomRowCount: 0,
1113+
rowCount: 100,
1114+
visibleRows: [],
1115+
allRowYs: new Map(),
1116+
allRowHeights: new Map(),
1117+
});
1118+
1119+
/**
1120+
* Creates a mock GridModel with grouped column headers for testing
1121+
*/
1122+
const createMockGroupedGridModel = (
1123+
headerGroups: Map<number, Map<number, string>>
1124+
): GridModel =>
1125+
TestUtils.createMockProxy<GridModel>({
1126+
columnCount: 4,
1127+
rowCount: 100,
1128+
columnHeaderMaxDepth: headerGroups.size,
1129+
textForColumnHeader: (column: ModelIndex, depth = 0) =>
1130+
headerGroups.get(depth)?.get(column) ?? '',
1131+
});
1132+
1133+
const singleLevelHeaderGroups = new Map([
1134+
[
1135+
0,
1136+
new Map([
1137+
[0, 'A'],
1138+
[1, 'B'],
1139+
[2, 'C'],
1140+
[3, 'D'],
1141+
]),
1142+
],
1143+
]);
1144+
1145+
const multiLevelHeaderGroups = new Map([
1146+
[
1147+
0,
1148+
new Map([
1149+
[0, 'A'],
1150+
[1, 'B'],
1151+
[2, 'C'],
1152+
[3, 'D'],
1153+
]),
1154+
],
1155+
[
1156+
1,
1157+
new Map([
1158+
[0, 'Group1'],
1159+
[1, 'Group1'],
1160+
[2, 'Group2'],
1161+
[3, 'Group2'],
1162+
]),
1163+
],
1164+
]);
1165+
1166+
it.each([
1167+
{
1168+
description: 'detects separator at column boundary',
1169+
x: 150, // At boundary between column 0 and 1 (100 + 50)
1170+
y: 15, // Middle of the top header (maxDepth - 1)
1171+
headerGroups: singleLevelHeaderGroups,
1172+
maxDepth: 1,
1173+
expected: 0,
1174+
},
1175+
{
1176+
description: 'detects there is no separator within the column',
1177+
x: 120, // Within column 1
1178+
y: 15,
1179+
headerGroups: singleLevelHeaderGroups,
1180+
maxDepth: 1,
1181+
expected: null,
1182+
},
1183+
{
1184+
description:
1185+
'should return null at depth 1 when no separator exists (columns in same group)',
1186+
x: 150, // Between column 0 and 1
1187+
y: 15, // Middle of the top header (maxDepth - 1)
1188+
headerGroups: multiLevelHeaderGroups,
1189+
maxDepth: 2,
1190+
expected: null,
1191+
},
1192+
{
1193+
description: 'should detect separator at depth 1 when groups differ',
1194+
x: 250, // Between Group1 and Group2
1195+
y: 15,
1196+
headerGroups: multiLevelHeaderGroups,
1197+
maxDepth: 2,
1198+
expected: 1,
1199+
},
1200+
])('$description', ({ x, y, headerGroups, maxDepth, expected }) => {
1201+
const metrics = createMockMetrics(maxDepth) as GridMetrics;
1202+
const model = createMockGroupedGridModel(headerGroups);
1203+
1204+
const result = GridUtils.getColumnSeparatorIndex(
1205+
x,
1206+
y,
1207+
metrics,
1208+
mockTheme,
1209+
model
1210+
);
1211+
1212+
expect(result).toBe(expected);
1213+
});
1214+
});

packages/grid/src/GridUtils.ts

Lines changed: 58 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import {
2525
} from './GridAxisRange';
2626
import { isExpandableGridModel } from './ExpandableGridModel';
2727
import { type GridRenderState } from './GridRendererTypes';
28+
import type GridModel from './GridModel';
2829

2930
export type GridPoint = {
3031
x: Coordinate;
@@ -453,19 +454,53 @@ export class GridUtils {
453454
);
454455
}
455456

457+
/**
458+
* Check if a separator exists between a column and the next column at a given depth.
459+
* A separator exists if adjacent columns have different header text at the specified depth.
460+
*
461+
* @param model The grid model
462+
* @param depth The header depth to check at
463+
* @param columnIndex The current model column index
464+
* @param nextColumnIndex The next model column index (undefined for last column)
465+
* @returns true if a separator should be shown, false otherwise
466+
*/
467+
static hasColumnSeparatorAtDepth(
468+
model: GridModel,
469+
depth: number | undefined,
470+
columnIndex: ModelIndex | undefined,
471+
nextColumnIndex: ModelIndex | undefined
472+
): boolean {
473+
if (depth == null || columnIndex == null) {
474+
return false;
475+
}
476+
477+
// Always show separator for the last column
478+
if (nextColumnIndex == null) {
479+
return true;
480+
}
481+
482+
// A separator exists if adjacent columns have different header text at this depth
483+
return (
484+
model.textForColumnHeader(columnIndex, depth) !==
485+
model.textForColumnHeader(nextColumnIndex, depth)
486+
);
487+
}
488+
456489
/**
457490
* Gets the column index if the x/y coordinates provided are close enough to the separator, otherwise null
458491
* @param x Mouse x coordinate
459492
* @param y Mouse y coordinate
460493
* @param metrics The grid metrics
461494
* @param theme The grid theme with potential user overrides
495+
* @param model The grid model
462496
* @returns Index of the column separator at the coordinates provided, or null if none match
463497
*/
464498
static getColumnSeparatorIndex(
465499
x: Coordinate,
466500
y: Coordinate,
467501
metrics: GridMetrics,
468-
theme: GridTheme
502+
theme: GridTheme,
503+
model: GridModel
469504
): VisibleIndex | null {
470505
const {
471506
rowHeaderWidth,
@@ -476,6 +511,7 @@ export class GridUtils {
476511
allColumnXs,
477512
allColumnWidths,
478513
columnHeaderMaxDepth,
514+
modelColumns,
479515
} = metrics;
480516
const { allowColumnResize, headerSeparatorHandleSize } = theme;
481517

@@ -489,6 +525,7 @@ export class GridUtils {
489525

490526
const gridX = x - rowHeaderWidth;
491527
const halfSeparatorSize = headerSeparatorHandleSize * 0.5;
528+
const depth = GridUtils.getColumnHeaderDepthAtY(y, metrics);
492529

493530
// Iterate through the floating columns first since they're on top
494531
let isPreviousColumnHidden = false;
@@ -507,7 +544,16 @@ export class GridUtils {
507544

508545
const minX = midX - halfSeparatorSize;
509546
const maxX = midX + halfSeparatorSize;
510-
if (minX <= gridX && gridX <= maxX) {
547+
if (
548+
minX <= gridX &&
549+
gridX <= maxX &&
550+
GridUtils.hasColumnSeparatorAtDepth(
551+
model,
552+
depth,
553+
modelColumns.get(column),
554+
modelColumns.get(column + 1)
555+
)
556+
) {
511557
return column;
512558
}
513559

@@ -538,7 +584,16 @@ export class GridUtils {
538584

539585
const minX = midX - halfSeparatorSize;
540586
const maxX = midX + halfSeparatorSize;
541-
if (minX <= gridX && gridX <= maxX) {
587+
if (
588+
minX <= gridX &&
589+
gridX <= maxX &&
590+
GridUtils.hasColumnSeparatorAtDepth(
591+
model,
592+
depth,
593+
modelColumns.get(column),
594+
modelColumns.get(column + 1)
595+
)
596+
) {
542597
return column;
543598
}
544599

packages/grid/src/mouse-handlers/GridColumnSeparatorMouseHandler.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,8 @@ class GridColumnSeparatorMouseHandler extends GridSeparatorMouseHandler {
2828
x,
2929
y,
3030
metrics,
31-
theme
31+
theme,
32+
model
3233
);
3334

3435
if (separatorIndex == null || depth == null) {

0 commit comments

Comments
 (0)