-
Notifications
You must be signed in to change notification settings - Fork 1.9k
When justifying text, keep significant whitespace. #7379
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
base: main
Are you sure you want to change the base?
Conversation
for glyph in &mut row.glyphs { | ||
glyph.pos.x += translate_x; | ||
glyph.pos.x = point_scale.round_to_pixel(glyph.pos.x); | ||
translate_x += extra_x_per_glyph; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: there was a secondary minor bug here:
previously we were distributing the extra_x_per_glyph
among all glyphs, even those that we had chosen to be trimmed. We calculated extra_x_per_glyph
based on num_glyphs_in_range
, but were then multiplying it by total glyphs. We were probably getting a bit too much extra spacing.
Below, we only add more extra_x_*
when we're operating on glyphs within the visible_glyphs
range.
|
||
let glyph_range = if num_leading_spaces == row.glyphs.len() { | ||
// There is only whitespace | ||
(0, row.glyphs.len()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept this logic in my refactor below. And I see that it avoids possible underflow when/after we get to the assert!()
but I'm still a bit fuzzy on why we even continue in that case. Do we need to shift all of the spaces into a left/center/right-aligned location? So... what, they're in a useful place when selecting label texts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha. Now I bet I see why all text was being trimmed when right-justified.
Each "row" that's being justified may contain a trailing space because the rest of the text has been moved into the next row.
I'll look into this later today. Which would you prefer?
- A local solution within
halign_and_justify_row()
that fixes this case. - A higher-level fix, which will remove whitespace between rows as they get split? (I imagine this one might result in incorrect copy/paste though?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer a fix within halign_and_justify_row.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be awesome if you could add an explicit test for this case so we don't break it in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer a fix within halign_and_justify_row.
Preview available at https://egui-pr-preview.github.io/pr/7379-fix-justify |
Fixes #1272