Skip to content

Exclude \n when splitting Galleys#7316

Merged
lucasmerlin merged 6 commits intomainfrom
lucas/text-layout-quirk
Jul 9, 2025
Merged

Exclude \n when splitting Galleys#7316
lucasmerlin merged 6 commits intomainfrom
lucas/text-layout-quirk

Conversation

@lucasmerlin
Copy link
Collaborator

Previously when galleys were splitted, each exept the last had an extra empty row that had to be removed when they were concated. This changes it to remove the \n from the layout jobs when splitting.

@github-actions
Copy link

github-actions bot commented Jul 9, 2025

Preview available at https://egui-pr-preview.github.io/pr/7316-lucastext-layout-quirk
Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

@lucasmerlin lucasmerlin marked this pull request as ready for review July 9, 2025 09:12

for placed_row in &mut rows {
let mut max_row_height = first_row_min_height.max(placed_row.rect().height());
let mut max_row_height = first_row_min_height.at_least(placed_row.height());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This really got me, changing the position here to NaN:

pos: pos2(0.0, f32::NAN),

Would cause the max_row_height here to change to 0. Took me a bit to see that this was due to getting the height from the rect instead of looking at the size directly.

);
assert_eq!(
galley.intrinsic_size,
Vec2::new(0.0, 16.0),
Copy link
Owner

Choose a reason for hiding this comment

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

Where does 16.0 come from? FontId::default() is 14.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This also surprised me, font.row_height() returns 16.0. Seems like this comes from the font_scaling:

// Scale the font properly (see https://github.com/emilk/egui/issues/2068).
let units_per_em = ab_glyph_font.units_per_em().unwrap_or_else(|| {
panic!("The font unit size of {font_name:?} exceeds the expected range (16..=16384)")
});
let font_scaling = ab_glyph_font.height_unscaled() / units_per_em;
let scale_in_pixels = scale_in_pixels * font_scaling;
self.cache

For the default font, this seems to be 1.121, so the font gets scaled to 15.694.

Then here we get to ~16.125:

height_in_points: ascent - descent + line_gap,

Copy link
Owner

Choose a reason for hiding this comment

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

makes sense - let's be explicit in the tests though by replacing the hard-coded 16.0

let row_height = fonts.row_height(font_id):

This makes it less brittle if we change the default FontId. Also, self-documenting.

Comment on lines +859 to +861
merged_galley.mesh_bounds = merged_galley
.mesh_bounds
.union(placed_row.visuals.mesh_bounds.translate(new_pos.to_vec2()));
Copy link
Owner

Choose a reason for hiding this comment

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

What do you think about adding a BirOr override to Rect? We already have it for Response

Suggested change
merged_galley.mesh_bounds = merged_galley
.mesh_bounds
.union(placed_row.visuals.mesh_bounds.translate(new_pos.to_vec2()));
merged_galley.mesh_bounds |=
placed_row.visuals.mesh_bounds.translate(new_pos.to_vec2());

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That'd be neat!

@emilk emilk added this to the egui 0.32.0 milestone Jul 9, 2025
);
assert_eq!(
galley.intrinsic_size,
Vec2::new(0.0, 16.0),
Copy link
Owner

Choose a reason for hiding this comment

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

makes sense - let's be explicit in the tests though by replacing the hard-coded 16.0

let row_height = fonts.row_height(font_id):

This makes it less brittle if we change the default FontId. Also, self-documenting.

…getter to avoid accumulating rounding errors. Also improve tests
@lucasmerlin lucasmerlin merged commit 207e71c into main Jul 9, 2025
47 checks passed
@lucasmerlin lucasmerlin deleted the lucas/text-layout-quirk branch July 9, 2025 12:53
lucasmerlin added a commit that referenced this pull request Jul 9, 2025
* Fixes a bug introduced by #7316 

The last `\n` was ignored for texts ending with `\n` in the galley split
logic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants