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

Add expand_bg to text layouting #5365

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

MeGaGiGaGon
Copy link
Contributor

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

After changes:

image

Code used to make comparison images
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<Galley> {
    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]);
})
  • I have followed the instructions in the PR template

Copy link

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
lucasmerlin previously approved these changes Nov 12, 2024
Copy link
Collaborator

@lucasmerlin lucasmerlin left a comment

Choose a reason for hiding this comment

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

Unfortunately this also shrinks the background of code blocks (as you can see in the failing snapshot images) and I think those looks better with the expansion there. Maybe the expansion could be an option somewhere in the style struct?

crates/egui/src/text_selection/visuals.rs Outdated Show resolved Hide resolved
@lucasmerlin lucasmerlin dismissed their stale review November 12, 2024 17:43

Oops - didn't mean to approve

@lucasmerlin lucasmerlin added feature New feature or request egui labels Nov 12, 2024
@lucasmerlin
Copy link
Collaborator

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 MeGaGiGaGon changed the title Remove text bg expand Add expand_bg to text layouting and add selection transparency Nov 12, 2024
@MeGaGiGaGon
Copy link
Contributor Author

MeGaGiGaGon commented Nov 12, 2024

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
Copy link
Contributor Author

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

@lucasmerlin
Copy link
Collaborator

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

emilk commented Nov 13, 2024

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
Copy link
Contributor Author

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.

@MeGaGiGaGon MeGaGiGaGon changed the title Add expand_bg to text layouting and add selection transparency Add expand_bg to text layouting Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
egui feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants