Skip to content

Commit

Permalink
fix: Columns not auto sizing correctly (#2359)
Browse files Browse the repository at this point in the history
- Improved logic for calculating the range to truncate a string, based
on the width of the assumed largest character and smallest character for
a given font. This fixes the observed issue of wide strings not being
truncated.
- Changed column width calculation to calculate text width by iterating
through the text and adding up individual character widths which are
cached. This approach size columns more precisely than estimating based
on a single character's width and the length of the string, and does not
appear to have a significant performance impact.

Test snippet used to see if column width calculation works as expected;
resize columns to observe truncation behaviour
```
from deephaven import new_table
from deephaven.column import string_col

a = new_table([string_col("allllllllllllllllllllllllllllllllllllllllllllllll", ["a"])])
b = new_table([string_col("ammmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmm", ["b"])])
c = new_table([string_col("c", ["allllllllllllllllllllllllllllllllllllllllllllllll"])])
d = new_table([string_col("d", ["ammmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmm"])])
```
 
Closes #2288
  • Loading branch information
ericlln authored Feb 10, 2025
1 parent e28a741 commit 77c620f
Show file tree
Hide file tree
Showing 246 changed files with 444 additions and 89 deletions.
6 changes: 4 additions & 2 deletions packages/grid/src/CellRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,16 @@ abstract class CellRenderer {
context: CanvasRenderingContext2D,
text: string,
width: number,
fontWidth: number,
fontWidthLower?: number,
fontWidthUpper?: number,
truncationChar?: string
): string =>
GridRenderer.truncateToWidth(
context,
text,
width,
fontWidth,
fontWidthLower,
fontWidthUpper,
truncationChar
),
{ max: 10000 }
Expand Down
11 changes: 7 additions & 4 deletions packages/grid/src/DataBarCellRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
import GridColorUtils from './GridColorUtils';
import GridUtils from './GridUtils';
import memoizeClear from './memoizeClear';
import { DEFAULT_FONT_WIDTH, type GridRenderState } from './GridRendererTypes';
import { type GridRenderState } from './GridRendererTypes';
import type GridModel from './GridModel';

interface DataBarRenderMetrics {
Expand Down Expand Up @@ -87,7 +87,8 @@ class DataBarCellRenderer extends CellRenderer {
allRowHeights,
allRowYs,
firstColumn,
fontWidths,
fontWidthsLower,
fontWidthsUpper,
} = metrics;

const isFirstColumn = column === firstColumn;
Expand All @@ -103,13 +104,15 @@ class DataBarCellRenderer extends CellRenderer {
width: textWidth,
} = GridUtils.getTextRenderMetrics(state, column, row);

const fontWidth = fontWidths?.get(context.font) ?? DEFAULT_FONT_WIDTH;
const fontWidthLower = fontWidthsLower.get(context.font);
const fontWidthUpper = fontWidthsUpper.get(context.font);
const truncationChar = model.truncationCharForCell(modelColumn, modelRow);
const truncatedText = this.getCachedTruncatedString(
context,
text,
textWidth,
fontWidth,
fontWidthLower,
fontWidthUpper,
truncationChar
);

Expand Down
90 changes: 89 additions & 1 deletion packages/grid/src/GridMetricCalculator.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { trimMap } from './GridMetricCalculator';
import GridMetricCalculator, { trimMap } from './GridMetricCalculator';

describe('trimMap', () => {
function makeMap(low = 0, high = 10): Map<number, number> {
Expand Down Expand Up @@ -41,3 +41,91 @@ describe('trimMap', () => {
expectResult(makeMap(0, 100), makeMap(51, 100), 100, 50);
});
});

describe('getTextWidth', () => {
const font = 'mock font';
const mockCharWidths = new Map([
['a', 10],
['b', 20],
['c', 30],
['d', 40],
['e', 50],
]);
let gridMetricCalculator;
let allCharWidths;

beforeEach(() => {
gridMetricCalculator = new GridMetricCalculator();
allCharWidths = new Map();
Object.assign(gridMetricCalculator, {
allCharWidths,
});
});

it('should calculate text width and cache char widths if not in cache', () => {
const input = 'abc';
const mockContext = {
measureText: jest.fn().mockImplementation(char => ({
width: mockCharWidths.get(char),
})),
};

const textWidth = gridMetricCalculator.getTextWidth(
mockContext,
font,
input
);

const charWidths = allCharWidths.get(font);
expect(charWidths.get('a')).toEqual(10);
expect(charWidths.get('b')).toEqual(20);
expect(charWidths.get('c')).toEqual(30);
expect(textWidth).toEqual(60);
});

it('should calculate text width using cached char widths if in cache', () => {
const input = 'abcd';
const mockContext = {
measureText: jest.fn().mockImplementation(char => ({
width: mockCharWidths.get(char),
})),
};
const dummyMockContext = {
measureText: jest.fn(),
};

const firstRun = gridMetricCalculator.getTextWidth(
mockContext,
font,
input
);
const secondRun = gridMetricCalculator.getTextWidth(
dummyMockContext,
font,
input
);

expect(mockContext.measureText).toHaveBeenCalledTimes(input.length);
expect(dummyMockContext.measureText).toHaveBeenCalledTimes(0);
expect(firstRun).toEqual(100);
expect(secondRun).toEqual(100);
});

it('should terminate early if max width is exceeded', () => {
const input = 'abcde';
const mockContext = {
measureText: jest.fn().mockImplementation(char => ({
width: mockCharWidths.get(char),
})),
};

const textWidth = gridMetricCalculator.getTextWidth(
mockContext,
font,
input,
50
);

expect(textWidth).toEqual(50);
});
});
Loading

0 comments on commit 77c620f

Please sign in to comment.