Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix column width calculation logic in grid #2370

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ericlln
Copy link
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
Copy link

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.

@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
charWidths.set(char, charWidth);
width += charWidth;
if (!charWidths.has(char)) {
context.font = font;
Copy link
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
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.

@ericlln ericlln requested a review from mofojed March 6, 2025 16:43
@ericlln ericlln requested a review from mattrunyon March 7, 2025 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants