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 text wrapping too late #2791

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bu5hm4nn
Copy link
Contributor

@bu5hm4nn bu5hm4nn commented Mar 8, 2023

UPDATE

This PR now fixes both #2578 and #2793. These fixes have now been tested for many months in https://github.com/mikedilger/gossip app.

To reproduce the problem, paste this code into a hello_world example:

impl eframe::App for MyEguiApp {
    fn update(&mut self, ctx: &egui::Context, _frame: &mut eframe::Frame) {
        egui::CentralPanel::default().show(ctx, |ui| {
            ui.horizontal_wrapped(|ui| {
                ui.hyperlink_to("@npub1vdaeclr2mnntmyw...", "whocares");
                let text = " LotsOfTextPrecededByASpace5kgqfqqxwhkrkw60stn8aph4gm2h2053xvwvvlvjm3q9eqdpqxycrqvpqd3hhgar9wfujqarfvd4k2arncqzpgxqzz6sp5vfenc5l4uafsky0w069zs329edf608ggpjjveguwxfl3xlswg5vq9qyyssqj46d5x3gsnljffm79eqwszk4mk47lkxywdp8mxum7un3qm0ztwj9jf46cm4lw2un9hk4gttgtjdrk29h27xu4e3ume20sqsna8q7xwspqqkwq7";
                ui.label(text);
                ui.style_mut().visuals.widgets.noninteractive.fg_stroke = Stroke::new( 1.0, eframe::epaint::Color32::RED );
                ui.label("More text followed by two newlines\n\n");
                ui.style_mut().visuals.widgets.noninteractive.fg_stroke = Stroke::new( 1.0, eframe::epaint::Color32::GREEN );
                ui.label("more text, no newline");
                ui.reset_style();
            });
            ui.separator();
            ui.horizontal_wrapped(|ui| {
                ui.label("Hyperlink no newline:");
                let url = "https://i.nostrimg.com/c72f5e1a2e162fad2625e15651a654465c06016016f7743b496021cafa2a524e/file.jpeg";
                ui.hyperlink_to( url, url );
                ui.end_row();
                ui.label("Hyperlink break_anywhere=true");
                let mut job = WidgetText::from(url).into_layout_job(ui.style(), egui::FontSelection::Default, Align::LEFT);
                job.wrap.break_anywhere = true;
                ui.hyperlink_to( job, url );
            });
       });
    }
}

Description for #2578 fix:
There was confusion in the code over how to break when on a non-empty visual row (first_row_indentation > 0.0), causing text to be shifted left outside the ui frame. This is the case for example when another label has already been placed in this ui.horizontal_wrapped().

This fix will not create an empty row, essentially starting a newline, but rather try to fit as much text as possible on the existing row. IMO this is the desired use of a wrapping layout.

Description for #2793 fix:
Do not apply item_spacing to cursor when creating a new row

@bu5hm4nn bu5hm4nn marked this pull request as draft March 9, 2023 19:24
if first_row_indentation > 0.0
&& !row_break_candidates.has_good_candidate(job.wrap.break_anywhere)
{
// Allow the first row to be completely empty, because we know there will be more space on the next row:
Copy link
Owner

Choose a reason for hiding this comment

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

If first_row_indentation == 0.0 then we never want an empty first row. We would rather overflow max_width, because otherwise we will just run into the same problem on the next row. For instance if max_width == 0.0 we want a layout where we have exactly one glyph per row.

If first_row_indentation > 0.0 however we are fine with having an empty row, because there will be more space available on the next row, and that might be enough to fit the first word or whatever.

Copy link
Owner

Choose a reason for hiding this comment

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

Does your code handle this case (max_width == 0.0) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just ran this in examples/wrapped_layout and it seems to work as you describe (one glyph per row).

let mut chui = ui.child_ui(
    Rect::from_two_pos(ui.cursor().min, ui.cursor().min),
    Layout::left_to_right(Align::Center),
);
chui.horizontal_wrapped(|ui| {
    ui.label("Frick");
});

@emilk emilk changed the title Fix issue #2578 Fix text wrapping too late Mar 30, 2023
@emilk
Copy link
Owner

emilk commented Apr 18, 2023

Is there more work to be done here, or is it ready for review?

@bu5hm4nn
Copy link
Contributor Author

bu5hm4nn commented Apr 18, 2023

We've been using this patch for a few weeks now in https://github.com/mikedilger/gossip/. We did have to add another patch, to fix the case where the cursor is initialized to non-interactive default size and not to line height. It's a rough fix, I think the only way to fix it properly is to have a separate property for line height in the cursor (because you need to know line height for wrapping).

This is the problem (the URL should actually be on the second line):
screenshot

And this is our additional patch:
mikedilger#5

So in case you want to incorporate that patch as well, I would consider this one ready to merge.

… source paragraph (the one that is too long) for which length has already been processed. When creating an empty row, this position should not be updated as no glyphs were consumed from the source paragraph. - added example that would demonstrate the problem if the line was included, and that is fixed with this commit

Fix issue emilk#2578 There was confusion in the code over how to
break when on a non-empty visual row (`first_row_indentation > 0.0`), causing
text to be shifted left outside the ui frame. This is the case for example
when another label has already been placed in this `ui.horizontal_wrapped()`.
This fix will not create an empty row, essentially starting a newline, but
rather try to fit as much text as possible on the existing row. IMO this is
the desired use of a wrapping layout. I've also added an example that would
demonstrate the problem if the line was included, and that is fixed with this
commit
@bu5hm4nn
Copy link
Contributor Author

Since this bug in ui.horizontal_wrapped() persists in the latest release, I have updated this branch by cherry picking the fixes ontop of a fresh checkout from master. Therefore the force push.

@jb55
Copy link

jb55 commented Apr 27, 2024

since I'm hitting this issue on https://github.com/damus-io/notedeck I will try this and report back!

@jb55
Copy link

jb55 commented Nov 26, 2024

tried @bu5hm4nn 's fork for 0.28.1 at f40370c with no luck. whatever it fixed must have been specific to gossip? I'm still seeing wrapping issues in notedeck columns:

Screenshot 2024-11-26 at 11 49 03

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants