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: Columns not auto sizing correctly #2359

Merged

Conversation

ericlln
Copy link
Contributor

@ericlln ericlln commented Feb 6, 2025

  • 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

@ericlln ericlln self-assigned this Feb 6, 2025
Copy link

codecov bot commented Feb 6, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 39 lines in your changes missing coverage. Please review.

Project coverage is 46.80%. Comparing base (ae544fa) to head (da41aaf).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
packages/grid/src/GridMetricCalculator.ts 45.09% 28 Missing ⚠️
packages/iris-grid/src/IrisGridRenderer.ts 0.00% 6 Missing ⚠️
packages/grid/src/DataBarCellRenderer.ts 0.00% 2 Missing ⚠️
packages/iris-grid/src/IrisGridTextCellRenderer.ts 0.00% 2 Missing ⚠️
packages/grid/src/TextCellRenderer.ts 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2359      +/-   ##
==========================================
+ Coverage   46.76%   46.80%   +0.03%     
==========================================
  Files         710      710              
  Lines       39107    39141      +34     
  Branches     9773     9956     +183     
==========================================
+ Hits        18288    18318      +30     
- Misses      20808    20812       +4     
  Partials       11       11              
Flag Coverage Δ
unit 46.80% <50.00%> (+0.03%) ⬆️

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.

causes all snapshots to be updated which makes it hard to tell whether the difference is caused by the theme change or other changes
should be add padding in between the header text and sort arrow in a different way anyway
@ericlln ericlln requested review from a team and dgodinez-dh and removed request for a team February 7, 2025 18:41
@ericlln ericlln marked this pull request as ready for review February 7, 2025 18:42

context.font = font;
// Assume char `m` is the largest character
const textMetrics = context.measureText('m');
Copy link
Contributor

Choose a reason for hiding this comment

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

In the ticket, the complaint was about a capital 'M' overflowing the width. Is lowercase 'm' going to have the same width?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I was initially investigating this issue I measured all alphanumeric characters using our canvas context, and found that a lowercase ‘m’ is a wider than an upper case one by about 8%, so I ended up taking that as the largest value. This might not be true for all fonts but it seems to be more common based on my limited research.

The Width of Lowercase ´m´

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

const fontWidth = fontWidths?.get(context.font) ?? DEFAULT_FONT_WIDTH;
Copy link
Contributor

Choose a reason for hiding this comment

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

I see in a lot of places that we no longer check the default font. What happens if the font is not the map? Do we get a default somehow, or is it guaranteed to be calculated?

Copy link
Contributor Author

@ericlln ericlln Feb 7, 2025

Choose a reason for hiding this comment

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

I saw that this constant was being used in a lot of different places, since CellRenderer.getCachedTruncatedString doesn’t accept an undefined value for the font widths. The function GridRenderer.truncateToWidth that it calls does though, and already uses that constant for default values, so I made getCachedTruncatedString accept undefined for the font widths, and the constant can be removed from other places safely.

If you did happen to need the font widths for something else in those methods, you would have to add back that undefined check.

Copy link
Contributor

@dgodinez-dh dgodinez-dh left a comment

Choose a reason for hiding this comment

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

@ericlln ericlln merged commit 77c620f into deephaven:main Feb 10, 2025
11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 10, 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.

Column header names do not truncate properly with wide characters
2 participants