From ca21c5b287e4034959ca8a0c06fe4d1f4ec16e78 Mon Sep 17 00:00:00 2001 From: Bu5hm4nn Date: Mon, 11 Mar 2024 10:48:22 -0600 Subject: [PATCH] Fix issue #2578 - `row_start_x` tracks a virtual position in the 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 #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 --- Cargo.lock | 9 ++++ crates/epaint/src/text/text_layout.rs | 75 +++++++++++++++++++-------- examples/wrapping-layout/Cargo.toml | 23 ++++++++ examples/wrapping-layout/README.md | 7 +++ examples/wrapping-layout/src/main.rs | 55 ++++++++++++++++++++ 5 files changed, 146 insertions(+), 23 deletions(-) create mode 100644 examples/wrapping-layout/Cargo.toml create mode 100644 examples/wrapping-layout/README.md create mode 100644 examples/wrapping-layout/src/main.rs diff --git a/Cargo.lock b/Cargo.lock index 92948c4589f..b13c66b4e02 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4607,6 +4607,15 @@ dependencies = [ "memchr", ] +[[package]] +name = "wrapping_layout" +version = "0.1.0" +dependencies = [ + "eframe", + "egui_extras", + "env_logger", +] + [[package]] name = "x11-dl" version = "2.21.0" diff --git a/crates/epaint/src/text/text_layout.rs b/crates/epaint/src/text/text_layout.rs index 9b055edc577..32febe6373c 100644 --- a/crates/epaint/src/text/text_layout.rs +++ b/crates/epaint/src/text/text_layout.rs @@ -147,6 +147,9 @@ fn layout_section( paragraph.empty_paragraph_height = line_height; // TODO(emilk): replace this hack with actually including `\n` in the glyphs? } + // TODO(bu5hm4nn): in a label widget, `leading_space` is used to adjust for existing text in a screen row, + // but the comment on `LayoutSection::leading_space` makes it clear it was originally intended for typographical + // indentation and not for screen layout paragraph.cursor_x += leading_space; let mut last_glyph_id = None; @@ -244,34 +247,20 @@ fn line_break(paragraph: &Paragraph, job: &LayoutJob, out_rows: &mut Vec, e let mut first_row_indentation = paragraph.glyphs[0].pos.x; let mut row_start_x = 0.0; let mut row_start_idx = 0; + let mut non_empty_rows = 0; for i in 0..paragraph.glyphs.len() { - if job.wrap.max_rows <= out_rows.len() { - *elided = true; + let potential_row_width = paragraph.glyphs[i].max_x() - row_start_x - first_row_indentation; + + if job.wrap.max_rows > 0 && non_empty_rows >= job.wrap.max_rows { break; } - let potential_row_width = paragraph.glyphs[i].max_x() - row_start_x; - - if job.wrap.max_width < potential_row_width { - // Row break: - - 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: - // TODO(emilk): this records the height of this first row as zero, though that is probably fine since first_row_indentation usually comes with a first_row_min_height. - out_rows.push(Row { - section_index_at_start: paragraph.section_index_at_start, - glyphs: vec![], - visuals: Default::default(), - rect: rect_from_x_range(first_row_indentation..=first_row_indentation), - ends_with_newline: false, - }); - row_start_x += first_row_indentation; - first_row_indentation = 0.0; - } else if let Some(last_kept_index) = row_break_candidates.get(job.wrap.break_anywhere) - { + // (bu5hm4nn): we want to actually allow as much text as possible on the first line so + // we don't need a special case for the first row, but we need to subtract + // the first_row_indentation from the allowed max width + if potential_row_width > (job.wrap.max_width - first_row_indentation) { + if let Some(last_kept_index) = row_break_candidates.get(job.wrap.break_anywhere) { let glyphs: Vec = paragraph.glyphs[row_start_idx..=last_kept_index] .iter() .copied() @@ -297,6 +286,12 @@ fn line_break(paragraph: &Paragraph, job: &LayoutJob, out_rows: &mut Vec, e row_start_idx = last_kept_index + 1; row_start_x = paragraph.glyphs[row_start_idx].pos.x; row_break_candidates = Default::default(); + non_empty_rows += 1; + + // (bu5hm4nn) first row indentation gets consumed the first time it's used + if first_row_indentation > 0.0 { + first_row_indentation = 0.0; + } } else { // Found no place to break, so we have to overrun wrap_width. } @@ -925,6 +920,7 @@ impl RowBreakCandidates { .flatten() } + #[allow(dead_code)] fn has_good_candidate(&self, break_anywhere: bool) -> bool { if break_anywhere { self.any.is_some() @@ -1062,3 +1058,36 @@ mod tests { ); } } + +#[test] +fn test_line_break_first_row_not_empty() { + let mut fonts = FontsImpl::new(1.0, 1024, super::FontDefinitions::default()); + let mut layout_job = LayoutJob::single_section( + "SomeSuperLongTextThatDoesNotHaveAnyGoodBreakCandidatesButStillNeedsToBeBroken".into(), + super::TextFormat::default(), + ); + + // a small area + layout_job.wrap.max_width = 110.0; + + // give the first row a leading space, simulating that there already is + // text in this visual row + layout_job.sections.first_mut().unwrap().leading_space = 50.0; + + let galley = super::layout(&mut fonts, layout_job.into()); + assert_eq!( + galley + .rows + .iter() + .map(|row| row.glyphs.iter().map(|g| g.chr).collect::()) + .collect::>(), + vec![ + "SomeSup", + "erLongTextThat", + "DoesNotHaveAn", + "yGoodBreakCand", + "idatesButStillNe", + "edsToBeBroken" + ] + ); +} diff --git a/examples/wrapping-layout/Cargo.toml b/examples/wrapping-layout/Cargo.toml new file mode 100644 index 00000000000..1fb196c313e --- /dev/null +++ b/examples/wrapping-layout/Cargo.toml @@ -0,0 +1,23 @@ +[package] +name = "wrapping_layout" +version = "0.1.0" +authors = ["Emil Ernerfeldt ", "Bu5hm4nn "] +license = "MIT OR Apache-2.0" +edition = "2021" +rust-version = "1.72" +publish = false + + +[dependencies] +eframe = { workspace = true, features = [ + "default", + "__screenshot", # __screenshot is so we can dump a screenshot using EFRAME_SCREENSHOT_TO +] } + +# For image support: +egui_extras = { workspace = true, features = ["default", "image"] } + +env_logger = { version = "0.10", default-features = false, features = [ + "auto-color", + "humantime", +] } diff --git a/examples/wrapping-layout/README.md b/examples/wrapping-layout/README.md new file mode 100644 index 00000000000..f93777f3fa9 --- /dev/null +++ b/examples/wrapping-layout/README.md @@ -0,0 +1,7 @@ +Example showing text wrapping with `ui.horizontal_wrapped()` + +```sh +cargo run -p wrapping_layout +``` + +![](screenshot.png) diff --git a/examples/wrapping-layout/src/main.rs b/examples/wrapping-layout/src/main.rs new file mode 100644 index 00000000000..6b357946356 --- /dev/null +++ b/examples/wrapping-layout/src/main.rs @@ -0,0 +1,55 @@ +use eframe::{ + egui::{self, WidgetText}, + emath::Align, + epaint::Stroke, +}; + +fn main() -> Result<(), eframe::Error> { + env_logger::init(); // Log to stderr (if you run with `RUST_LOG=debug`). + let options = eframe::NativeOptions { + viewport: egui::ViewportBuilder::default().with_inner_size([320.0, 240.0]), + ..Default::default() + }; + eframe::run_native( + "Horizontal Wrapped Layouts", + options, + Box::new(|cc| Box::new(MyEguiApp::new(cc))), + ) +} + +#[derive(Default)] +struct MyEguiApp {} + +impl MyEguiApp { + fn new(_cc: &eframe::CreationContext<'_>) -> Self { + Self::default() + } +} + +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 ); + }); + }); + } +}