Skip to content

Commit c88fc82

Browse files
authored
fix: Fix column width calculation logic in grid (#2370)
- Noticed this when testing some things on Enterprise. The column header “SPet500” in the stocks table is truncated when it shouldn’t be. For some reason this case didn’t occur on Core, perhaps due to slight differences in canvas configurations. - Rounded down text measurement done in `GridRenderer.binaryTruncateToWidth()` as the measured text width with can differ slightly from the passed in estimated column width. This will prevent cases where text is truncated when it shouldn't be. Stocks table for testing: ``` import deephaven.plot.express as dx stocks = dx.data.stocks(); ```
1 parent 141bc13 commit c88fc82

File tree

72 files changed

+90
-24
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

72 files changed

+90
-24
lines changed

packages/grid/src/GridMetricCalculator.test.ts

+47-7
Original file line numberDiff line numberDiff line change
@@ -42,14 +42,18 @@ describe('trimMap', () => {
4242
});
4343
});
4444

45-
describe('getTextWidth', () => {
46-
const font = 'mock font';
45+
describe('calculateTextWidth', () => {
46+
let font = 'mock font';
4747
const mockCharWidths = new Map([
4848
['a', 10],
4949
['b', 20],
5050
['c', 30],
5151
['d', 40],
5252
['e', 50],
53+
['ab', 30],
54+
['bc', 50],
55+
['cd', 70],
56+
['de', 90],
5357
]);
5458
let gridMetricCalculator;
5559
let allCharWidths;
@@ -70,7 +74,7 @@ describe('getTextWidth', () => {
7074
})),
7175
};
7276

73-
const textWidth = gridMetricCalculator.getTextWidth(
77+
const textWidth = gridMetricCalculator.calculateTextWidth(
7478
mockContext,
7579
font,
7680
input
@@ -94,18 +98,17 @@ describe('getTextWidth', () => {
9498
measureText: jest.fn(),
9599
};
96100

97-
const firstRun = gridMetricCalculator.getTextWidth(
101+
const firstRun = gridMetricCalculator.calculateTextWidth(
98102
mockContext,
99103
font,
100104
input
101105
);
102-
const secondRun = gridMetricCalculator.getTextWidth(
106+
const secondRun = gridMetricCalculator.calculateTextWidth(
103107
dummyMockContext,
104108
font,
105109
input
106110
);
107111

108-
expect(mockContext.measureText).toHaveBeenCalledTimes(input.length);
109112
expect(dummyMockContext.measureText).toHaveBeenCalledTimes(0);
110113
expect(firstRun).toEqual(100);
111114
expect(secondRun).toEqual(100);
@@ -119,7 +122,7 @@ describe('getTextWidth', () => {
119122
})),
120123
};
121124

122-
const textWidth = gridMetricCalculator.getTextWidth(
125+
const textWidth = gridMetricCalculator.calculateTextWidth(
123126
mockContext,
124127
font,
125128
input,
@@ -128,4 +131,41 @@ describe('getTextWidth', () => {
128131

129132
expect(textWidth).toEqual(50);
130133
});
134+
135+
// This test checks that the calculation logic is equivalent to the native canvas one.
136+
// It doesn’t verify that it addresses font kerning properly, since it doesn’t actually render the text and context.measureText() just returns the text length
137+
it.each([
138+
[''],
139+
['a'],
140+
['a b'],
141+
[
142+
'Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.',
143+
],
144+
[
145+
'iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii',
146+
],
147+
[
148+
'mmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmm',
149+
],
150+
[
151+
'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa',
152+
],
153+
])(
154+
'should match the value calculated by context.measureText(): "%s"',
155+
text => {
156+
const canvas = global.document.createElement('canvas');
157+
const context = canvas.getContext('2d');
158+
font = '20px Arial';
159+
160+
if (context === null) {
161+
throw new Error('Could not get canvas context');
162+
}
163+
164+
context.font = font;
165+
const a = gridMetricCalculator.calculateTextWidth(context, font, text);
166+
const b = context.measureText(text).width;
167+
168+
expect(a).toEqual(b);
169+
}
170+
);
131171
});

packages/grid/src/GridMetricCalculator.ts

+43-17
Original file line numberDiff line numberDiff line change
@@ -1752,7 +1752,7 @@ export class GridMetricCalculator {
17521752
const headerText = model.textForColumnHeader(modelColumn, 0);
17531753
if (headerText !== undefined && headerText !== '') {
17541754
return (
1755-
this.getTextWidth(
1755+
this.calculateTextWidth(
17561756
context,
17571757
headerFont,
17581758
headerText,
@@ -1809,7 +1809,7 @@ export class GridMetricCalculator {
18091809
let cellWidth = 0;
18101810
if (text) {
18111811
cellWidth =
1812-
this.getTextWidth(
1812+
this.calculateTextWidth(
18131813
context,
18141814
font,
18151815
text,
@@ -1837,36 +1837,62 @@ export class GridMetricCalculator {
18371837
return columnWidth;
18381838
}
18391839

1840-
getTextWidth(
1840+
/**
1841+
* Calculates the width of a string using widths of individual and pairs of characters to take into account font kerning
1842+
* @param context The canvas rendering context
1843+
* @param font The font to get the width for
1844+
* @param text The text to calculate the width for
1845+
* @param maxWidth The maximum width to calculate to
1846+
*/
1847+
calculateTextWidth(
18411848
context: CanvasRenderingContext2D,
18421849
font: string,
18431850
text: string,
18441851
maxWidth?: number
18451852
): number {
1853+
if (text.length === 0) return 0;
1854+
context.font = font;
1855+
18461856
if (!this.allCharWidths.has(font)) {
18471857
this.allCharWidths.set(font, new Map());
18481858
}
1849-
18501859
const charWidths = getOrThrow(this.allCharWidths, font);
18511860

1852-
let width = 0;
1861+
let result = 0;
18531862
for (let i = 0; i < text.length; i += 1) {
18541863
const char = text[i];
1864+
const nextChar = text[i + 1];
18551865

1856-
if (charWidths.has(char)) {
1857-
width += getOrThrow(charWidths, char);
1858-
} else {
1859-
const textMetrics = context.measureText(char);
1860-
const { width: charWidth } = textMetrics;
1861-
charWidths.set(char, charWidth);
1862-
width += charWidth;
1866+
if (!charWidths.has(char)) {
1867+
charWidths.set(char, context.measureText(char).width);
18631868
}
18641869

1865-
if (maxWidth !== undefined && width > maxWidth) {
1866-
return maxWidth;
1870+
if (nextChar !== undefined) {
1871+
if (!charWidths.has(nextChar)) {
1872+
charWidths.set(nextChar, context.measureText(nextChar).width);
1873+
}
1874+
1875+
const pair = char + nextChar;
1876+
if (!charWidths.has(pair)) {
1877+
charWidths.set(pair, context.measureText(pair).width);
1878+
}
1879+
1880+
result += getOrThrow(charWidths, pair);
1881+
if (i > 0) {
1882+
// Need to remove the current character that was already counted in the previous pair
1883+
result -= getOrThrow(charWidths, char);
1884+
}
1885+
1886+
if (maxWidth !== undefined && result > maxWidth) {
1887+
return maxWidth;
1888+
}
1889+
} else if (result === 0) {
1890+
// On last char and no pair found before that => Only one char in string
1891+
result = getOrThrow(charWidths, char);
18671892
}
18681893
}
1869-
return width;
1894+
1895+
return result;
18701896
}
18711897

18721898
/**
@@ -1896,7 +1922,7 @@ export class GridMetricCalculator {
18961922
/**
18971923
* Calculates the lower bound width of a character of the provided font.
18981924
* @param font The font to get the width for
1899-
* @param state The grid metric state
1925+
* @param context The canvas rendering context
19001926
*/
19011927
calculateLowerFontWidth(
19021928
font: GridFont,
@@ -1920,7 +1946,7 @@ export class GridMetricCalculator {
19201946
/**
19211947
* Calculates the upper bound width of a character of the provided font.
19221948
* @param font The font to get the width for
1923-
* @param state The grid metric state
1949+
* @param context The canvas rendering context
19241950
*/
19251951
calculateUpperFontWidth(
19261952
font: GridFont,

0 commit comments

Comments
 (0)