Skip to content

Commit fe15465

Browse files
authored
fix: DH-19693: Add null/empty formatting to constituent strings (deephaven#2495)
Cherry-pick deephaven#2464 and deephaven#2480 to `v0.85` Part of [DH-19693](https://deephaven.atlassian.net/browse/DH-19693). Anything that renders as "empty" for empty strings, and "null" for null strings, should still render the same when rolledup. This adds the missing null and empty string formatting logic for tree table constituent values. Changes: Checks if the result should show "null" or "empty" based on formatter test: DH-19614: e2e tests for constituent formatting Follow-up to deephaven#2461. This PR adds e2e tests for constituent formatting. [DH-19693]: https://deephaven.atlassian.net/browse/DH-19693?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
1 parent dd2ed32 commit fe15465

File tree

35 files changed

+187
-5
lines changed

35 files changed

+187
-5
lines changed

.github/workflows/e2e.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ on:
1414
- 'release/**'
1515
- 'feature/**'
1616
env:
17-
DHC_VERSION: 0.36.1
17+
DHC_VERSION: 0.39.5
1818

1919
jobs:
2020
build:

packages/iris-grid/src/IrisGridTreeTableModel.test.ts

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import dh from '@deephaven/jsapi-shim';
22
import IrisGridTestUtils from './IrisGridTestUtils';
3+
import IrisGridTableModelTemplate from './IrisGridTableModelTemplate';
34
import IrisGridTreeTableModel, {
45
type UITreeRow,
56
} from './IrisGridTreeTableModel';
@@ -416,3 +417,143 @@ describe('IrisGridTreeTableModel textAlignForCell', () => {
416417
expect(result).toBe('left');
417418
});
418419
});
420+
421+
describe('IrisGridTreeTableModel textForCell', () => {
422+
let model: IrisGridTreeTableModel;
423+
424+
const columns = {
425+
string: irisGridTestUtils.makeColumn('StrCol', 'java.lang.String', 0),
426+
stringColWithStringConstituent: {
427+
...irisGridTestUtils.makeColumn('StrCol', 'java.lang.String', 0),
428+
constituentType: 'java.lang.String',
429+
},
430+
stringColWithIntConstituent: {
431+
...irisGridTestUtils.makeColumn('IntCol', 'int', 0),
432+
constituentType: 'int',
433+
} as DhType.Column,
434+
};
435+
436+
const rows = {
437+
leaf: {
438+
data: new Map(),
439+
hasChildren: false,
440+
isExpanded: false,
441+
depth: 2,
442+
} as UITreeRow,
443+
parent: {
444+
data: new Map(),
445+
hasChildren: true,
446+
isExpanded: false,
447+
depth: 1,
448+
} as UITreeRow,
449+
};
450+
451+
beforeEach(() => {
452+
const testColumns = irisGridTestUtils.makeColumns();
453+
const table = irisGridTestUtils.makeTreeTable(
454+
testColumns,
455+
testColumns.slice(0, 1),
456+
100,
457+
[]
458+
);
459+
model = new IrisGridTreeTableModel(dh, table);
460+
});
461+
462+
const mockTextCell = (
463+
value: unknown,
464+
column: DhType.Column,
465+
row: UITreeRow,
466+
displayText?: string
467+
) => {
468+
jest.spyOn(model, 'sourceColumn').mockReturnValue(column);
469+
jest.spyOn(model, 'row').mockReturnValue(row);
470+
jest.spyOn(model, 'valueForCell').mockReturnValue(value);
471+
if (displayText !== undefined) {
472+
jest.spyOn(model, 'displayString').mockReturnValue(displayText);
473+
}
474+
};
475+
476+
describe('leaf nodes with non-string constituent type', () => {
477+
it('handles null values', () => {
478+
jest
479+
.spyOn(model, 'columns', 'get')
480+
.mockReturnValue([columns.stringColWithIntConstituent]);
481+
mockTextCell(null, columns.stringColWithIntConstituent, rows.leaf, '');
482+
const result = model.textForCell(0, 0);
483+
expect(result).toBe('');
484+
});
485+
486+
it('handles normal values', () => {
487+
jest
488+
.spyOn(model, 'columns', 'get')
489+
.mockReturnValue([columns.stringColWithIntConstituent]);
490+
mockTextCell(21, columns.stringColWithIntConstituent, rows.leaf, '21');
491+
const result = model.textForCell(0, 0);
492+
expect(result).toBe('21');
493+
});
494+
});
495+
496+
describe('leaf nodes with formatter settings', () => {
497+
beforeEach(() => {
498+
jest.spyOn(model, 'columns', 'get').mockReturnValue([columns.string]);
499+
});
500+
501+
it('shows "null" when showNullStrings is true', () => {
502+
model.formatter.showNullStrings = true;
503+
mockTextCell(null, columns.stringColWithIntConstituent, rows.leaf, '');
504+
const result = model.textForCell(0, 0);
505+
expect(result).toBe('null');
506+
});
507+
508+
it('shows empty string when showNullStrings is false', () => {
509+
model.formatter.showNullStrings = false;
510+
mockTextCell(null, columns.stringColWithIntConstituent, rows.leaf, '');
511+
const result = model.textForCell(0, 0);
512+
expect(result).toBe('');
513+
});
514+
515+
it('shows "empty" when showEmptyStrings is true', () => {
516+
model.formatter.showEmptyStrings = true;
517+
mockTextCell('', columns.stringColWithIntConstituent, rows.leaf, '');
518+
const result = model.textForCell(0, 0);
519+
expect(result).toBe('empty');
520+
});
521+
522+
it('shows empty string when showEmptyStrings is false', () => {
523+
model.formatter.showEmptyStrings = false;
524+
mockTextCell('', columns.stringColWithIntConstituent, rows.leaf, '');
525+
const result = model.textForCell(0, 0);
526+
expect(result).toBe('');
527+
});
528+
});
529+
530+
describe('grouping column', () => {
531+
it('shows empty string for null values in rollup grouping columns', () => {
532+
Object.defineProperty(model.table, 'groupedColumns', {
533+
value: [],
534+
});
535+
jest
536+
.spyOn(model, 'getCachedGroupedColumnSet')
537+
.mockReturnValue(new Set([0]));
538+
mockTextCell(null, columns.string, { ...rows.parent, depth: 1 });
539+
const result = model.textForCell(0, 0);
540+
expect(result).toBe('');
541+
});
542+
});
543+
544+
describe('non-leaf nodes', () => {
545+
it('delegates to superClass for parent nodes', () => {
546+
jest
547+
.spyOn(IrisGridTableModelTemplate.prototype, 'textForCell')
548+
.mockReturnValue('parentString');
549+
mockTextCell(
550+
'parentString',
551+
columns.stringColWithIntConstituent,
552+
rows.parent,
553+
'parentString'
554+
);
555+
const result = model.textForCell(0, 0);
556+
expect(result).toBe('parentString');
557+
});
558+
});
559+
});

packages/iris-grid/src/IrisGridTreeTableModel.ts

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,8 +130,25 @@ class IrisGridTreeTableModel extends IrisGridTableModelTemplate<
130130
if (row != null && column != null) {
131131
if (!row.hasChildren && column.constituentType != null) {
132132
const value = this.valueForCell(x, y);
133-
return this.displayString(value, column.constituentType, column.name);
133+
const text = this.displayString(
134+
value,
135+
column.constituentType,
136+
column.name
137+
);
138+
139+
if (TableUtils.isTextType(this.columns[x]?.type)) {
140+
if (value === null) {
141+
return this.formatter.showNullStrings ? 'null' : '';
142+
}
143+
144+
if (text === '') {
145+
return this.formatter.showEmptyStrings ? 'empty' : '';
146+
}
147+
}
148+
149+
return text;
134150
}
151+
135152
// Show empty string instead of null in rollup grouping columns (issue #1483)
136153
if (
137154
row.hasChildren &&

tests/docker-scripts/docker-compose.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
services:
22
dhc-server:
33
container_name: dhc-server
4-
image: ghcr.io/deephaven/server:${DHC_VERSION:-0.36.1}
4+
image: ghcr.io/deephaven/server:${DHC_VERSION:-0.39.5}
55
pull_policy: always
66
volumes:
77
- ./data:/data

tests/table-operations.spec.ts

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -439,8 +439,9 @@ test('rollup rows and aggregrate columns', async ({ page }) => {
439439
await expect(page.locator('.iris-grid-column')).toHaveScreenshot();
440440
});
441441

442-
await test.step('Toggle constituents', async () => {
443-
await page.getByText('Constituents').click();
442+
await test.step('Expand constituent with non-aggregated columns visible', async () => {
443+
const gridCanvas = page.locator('.iris-grid .grid-wrapper');
444+
await gridCanvas.click({ position: { x: 10, y: 80 } });
444445

445446
await waitForLoadingDone(page);
446447
await expect(page.locator('.iris-grid-column')).toHaveScreenshot();
@@ -453,6 +454,21 @@ test('rollup rows and aggregrate columns', async ({ page }) => {
453454
await expect(page.locator('.iris-grid-column')).toHaveScreenshot();
454455
});
455456

457+
await test.step('Expand constituent with non-aggregated columns hidden', async () => {
458+
const gridCanvas = page.locator('.iris-grid .grid-wrapper');
459+
await gridCanvas.click({ position: { x: 10, y: 80 } });
460+
461+
await waitForLoadingDone(page);
462+
await expect(page.locator('.iris-grid-column')).toHaveScreenshot();
463+
});
464+
465+
await test.step('Toggle constituents', async () => {
466+
await page.getByText('Constituents').click();
467+
468+
await waitForLoadingDone(page);
469+
await expect(page.locator('.iris-grid-column')).toHaveScreenshot();
470+
});
471+
456472
await test.step('Rollup another column', async () => {
457473
const intColumn = page.getByRole('button', { name: 'Int', exact: true });
458474
expect(intColumn).toBeTruthy();
@@ -486,6 +502,14 @@ test('rollup rows and aggregrate columns', async ({ page }) => {
486502
await expect(page.locator('.iris-grid-column')).toHaveScreenshot();
487503
});
488504

505+
await test.step('Expand constituent with aggregations applied', async () => {
506+
const gridCanvas = page.locator('.iris-grid .grid-wrapper');
507+
await gridCanvas.click({ position: { x: 10, y: 80 } });
508+
509+
await waitForLoadingDone(page);
510+
await expect(page.locator('.iris-grid-column')).toHaveScreenshot();
511+
});
512+
489513
await test.step('Edit aggregated columns', async () => {
490514
await page
491515
.getByRole('button', { name: 'Edit Columns', exact: true })
37.5 KB
Loading
67.5 KB
Loading
23.1 KB
Loading
38.1 KB
Loading
68.6 KB
Loading

0 commit comments

Comments
 (0)