Skip to content

fix: Fix column width calculation logic in grid#2370

Merged
ericlln merged 10 commits intodeephaven:mainfrom
ericlln:DH-18734-fix-text-truncation-logic-in-grid
Mar 11, 2025
Merged

fix: Fix column width calculation logic in grid#2370
ericlln merged 10 commits intodeephaven:mainfrom
ericlln:DH-18734-fix-text-truncation-logic-in-grid

Conversation

@ericlln
Copy link
Copy Markdown
Contributor

@ericlln ericlln commented Feb 19, 2025

  • 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();

@ericlln ericlln requested a review from a team February 19, 2025 21:30
@ericlln ericlln self-assigned this Feb 19, 2025
@ericlln ericlln requested review from dgodinez-dh and removed request for a team February 19, 2025 21:30
dgodinez-dh
dgodinez-dh previously approved these changes Feb 19, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.81%. Comparing base (35fc599) to head (ab245f5).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2370      +/-   ##
==========================================
+ Coverage   46.80%   46.81%   +0.01%     
==========================================
  Files         710      710              
  Lines       39224    39243      +19     
  Branches     9789     9796       +7     
==========================================
+ Hits        18357    18370      +13     
- Misses      20856    20862       +6     
  Partials       11       11              
Flag Coverage Δ
unit 46.81% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread packages/grid/src/GridRenderer.ts Outdated
@ericlln ericlln changed the title fix: Fix text truncation logic in grid fix: Fix column width calculation logic in grid Feb 25, 2025
@ericlln ericlln requested a review from mofojed March 4, 2025 23:20
Comment thread packages/grid/src/GridMetricCalculator.test.ts Outdated
charWidths.set(char, charWidth);
width += charWidth;
if (!charWidths.has(char)) {
context.font = font;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just set the font at the start of this method... Interesting that we weren't setting the font before this change, I wonder if that caused any issues.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup missing that initially definitely caused issues and the header text width was not being calculated correctly as a result. Since the header font and cell font only differ slightly, it wasn’t able to be noticed visually. Again, it would be nice to be able to catch something like this through testing.

Comment thread packages/grid/src/GridMetricCalculator.ts Outdated
Comment thread packages/grid/src/GridMetricCalculator.ts Outdated
@ericlln ericlln requested a review from mofojed March 6, 2025 16:43
@ericlln ericlln requested a review from mattrunyon March 7, 2025 15:08
Copy link
Copy Markdown
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why we aren't just measuring the text with a cache like we already do in GridRenderer to determine if it needs to be truncated. I guess in theory this should reduce the number of measureText calls in the metric calculator (we will still measure the whole string in GridRenderer and might be faster if the values are constantly different where the cache isn't much help.

We could add a measured text cache to GridMetrics which is then accessible by GridRenderer and avoid double measuring the whole string. But that would be a separate ticket

@ericlln ericlln merged commit c88fc82 into deephaven:main Mar 11, 2025
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants