emilk / egui

egui: an easy-to-use immediate mode GUI in Rust that runs on both web and native
https://www.egui.rs/
Apache License 2.0
22.51k stars 1.61k forks source link

Add expand_bg to text layouting #5365

Open MeGaGiGaGon opened 1 week ago

MeGaGiGaGon commented 1 week ago

This removes the expand(1.0) on text background colors, since it makes translucent background colors have bad looking bleeding.

There is probably a smarter solution than disabling the highlighting entirely, but I don't see a way to do that while keeping the area consumed consistent between translucent/solid colors, or adding a decent step up in complexity.

Since this makes it impossible to tell if selected text is highlighted, this also adds a blanket 0.5 gamma multiply to the text selection background color. If that is undesirable because it's a bad arbitrary number choice, or if it's too much of an unexpected change and just the default values should be changed, please let me know.

These changes cause the tests that use screenshots with highlighted text to fail, though I am not sure how to update those tests to match the changes.

Comparison Images Current: ![image](https://github.com/user-attachments/assets/6dc85492-4f8e-4e7a-84b4-3ee10a48b8b3) After changes: ![image](https://github.com/user-attachments/assets/9b35bbd3-159d-42a9-b22f-80febb707cfa)
Code used to make comparison images ```rs fn color_text_format(ui: &Ui, color: Color32) -> TextFormat { TextFormat { font_id: FontId::monospace(ui.text_style_height(&egui::TextStyle::Monospace)), background: color, ..Default::default() } } fn color_sequence_galley(ui: &Ui, text: &str, colors: [Color32; 3]) -> Arc { let mut layout_job = LayoutJob::default(); for color in colors { layout_job.append(text, 0.0, color_text_format(ui, color)); } ui.fonts(|f| f.layout_job(layout_job)) } fn color_sequence_row(ui: &mut Ui, label_text: &str, text: &str, colors: [Color32; 3]) { ui.label(label_text); ui.label(color_sequence_galley(ui, text, colors)); ui.end_row(); } egui::Grid::new("comparison display").show(ui, |ui| { ui.ctx().set_pixels_per_point(2.0); let transparent = Color32::TRANSPARENT; let solid = Color32::RED; let solid_2 = Color32::GREEN; let translucent_1 = Color32::GRAY.gamma_multiply(0.5); let translucent_2 = Color32::GREEN.gamma_multiply(0.5); color_sequence_row(ui, "Transparent to Solid:", " ", [transparent, solid, transparent]); color_sequence_row(ui, "Translucent to Transparent:", " ", [transparent, translucent_1, transparent]); color_sequence_row(ui, "Solid to Transparent:", " ", [solid, solid_2, solid]); color_sequence_row(ui, "Solid to Solid:", " ", [solid, transparent, solid]); color_sequence_row(ui, "Solid to Translucent:", " ", [solid, translucent_1, solid]); color_sequence_row(ui, "Translucent to Translucent:", " ", [translucent_1, translucent_2, translucent_1]); color_sequence_row(ui, "Transparent to Solid:", "a", [transparent, solid, transparent]); color_sequence_row(ui, "Translucent to Transparent:", "a", [transparent, translucent_1, transparent]); color_sequence_row(ui, "Solid to Transparent:", "a", [solid, solid_2, solid]); color_sequence_row(ui, "Solid to Solid:", "a", [solid, transparent, solid]); color_sequence_row(ui, "Solid to Translucent:", "a", [solid, translucent_1, solid]); color_sequence_row(ui, "Translucent to Translucent:", "a", [translucent_1, translucent_2, translucent_1]); }) ```
github-actions[bot] commented 1 week ago

Preview available at https://egui-pr-preview.github.io/pr/5365-remove-text-bg-expand Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

lucasmerlin commented 1 week ago

These changes cause the tests that use screenshots with highlighted text to fail, though I am not sure how to update those tests to match the changes.

Run the tests with UPDATE_SNAPSHOTS=true, this should also be explained in contributing.md

MeGaGiGaGon commented 1 week ago

I've added the option, with the default how it currently is, so hopefully this should fix the screenshot tests failing anyways. (Per a discord discussion, I can't fix the tests failing/update them easily since my computer's too old for wgpu).

As usual, name subject to bikeshedding. I'm also not sure if I should have propagated the attribute to RichText as well as TextFormat, but I did.

This does allow for bad looking things/the original bleed issue by putting expanded and non-expanded backgrounds next to each other, but that should be fine.

MeGaGiGaGon commented 1 week ago

It looks like because of SelectableLabel, some of the tests are still failing :(

lucasmerlin commented 1 week ago

It looks like because of SelectableLabel, some of the tests are still failing :(

You changed the Selection color, so it makes sense that the selectable label looks different now. That's why I was suggesting to add an additional field to style, just for text selection 🤔 I think it'd be weird if the selectable label background is transparent

emilk commented 1 week ago

It would also be nice if that code was checked in, and run through kittest's snapshot test, so we don't regress on this again!

MeGaGiGaGon commented 1 week ago

It looks like because of SelectableLabel, some of the tests are still failing :(

You changed the Selection color, so it makes sense that the selectable label looks different now. That's why I was suggesting to add an additional field to style, just for text selection 🤔 I think it'd be weird if the selectable label background is transparent

The more I think on it, the more I realize dealing with the selection color issues will probably be a decently big thing itself, so I'm going to split it out from this PR so these changes don't get stalled by my indecision.


It would also be nice if that code was checked in, and run through kittest's snapshot test, so we don't regress on this again!

I'm a bit confused on what you mean by this, is it just relating to the selection changes? Since in that case, whenever I get an issue/pr for the selection changes opened those tests can be added there.