From ee3126139fb162c1f051d8af227f3e88fa615931 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Wed, 6 Nov 2024 11:31:46 +0000 Subject: [PATCH] Resolve mismatch between label width and alignment (#727) If the alignment was set previously, and the `CrossAxisAlignment` was not `Start`, the width Parley believed we were in wouldn't match the width used for Masonry layout. This would lead to weird behaviour. See the diff for the test images in https://github.com/linebender/xilem/commit/0a68159869243b2d9b3a5e58a7454b572eba7b2d for more context --- Cargo.lock | 37 ++++++++---- masonry/src/text/text_layout.rs | 32 +++++++++- masonry/src/widget/label.rs | 40 +++++++++++-- masonry/src/widget/prose.rs | 60 ++++++++++++++++--- ...et__label__tests__label_alignment_flex.png | 3 + ...et__prose__tests__prose_alignment_flex.png | 3 + xilem/examples/http_cats.rs | 13 ++-- xilem/examples/mason.rs | 4 +- xilem/examples/variable_clock.rs | 5 +- xilem/src/view/prose.rs | 24 +++++++- xilem/src/view/variable_label.rs | 1 - 11 files changed, 180 insertions(+), 42 deletions(-) create mode 100644 masonry/src/widget/screenshots/masonry__widget__label__tests__label_alignment_flex.png create mode 100644 masonry/src/widget/screenshots/masonry__widget__prose__tests__prose_alignment_flex.png diff --git a/Cargo.lock b/Cargo.lock index f9438414..15322110 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -122,6 +122,12 @@ version = "1.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f26201604c87b1e01bd3d98f8d5d9a8fcbb815e8cedb41ffccbeb4bf593a35fe" +[[package]] +name = "adler2" +version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "512761e0bb2578dd7380c6baaa0f4ce03e84f95e960231d1dec8bf4d7d6e2627" + [[package]] name = "ahash" version = "0.8.11" @@ -473,7 +479,7 @@ dependencies = [ "cc", "cfg-if", "libc", - "miniz_oxide", + "miniz_oxide 0.7.4", "object", "rustc-demangle", ] @@ -556,9 +562,9 @@ checksum = "79296716171880943b8470b5f8d03aa55eb2e645a4874bdbb28adb49162e012c" [[package]] name = "bytemuck" -version = "1.16.3" +version = "1.19.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "102087e286b4677862ea56cf8fc58bb2cdfa8725c40ffb80fe3a008eb7f2fc83" +checksum = "8334215b81e418a0a7bdb8ef0849474f40bb10c8b71f1c4ed315cff49f32494d" dependencies = [ "bytemuck_derive", ] @@ -1104,7 +1110,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7f211bbe8e69bbd0cfdea405084f128ae8b4aaa6b0b522fc8f2b009084797920" dependencies = [ "crc32fast", - "miniz_oxide", + "miniz_oxide 0.7.4", ] [[package]] @@ -2140,6 +2146,15 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b8a240ddb74feaf34a79a7add65a741f3167852fba007066dcac1ca548d89c08" dependencies = [ "adler", +] + +[[package]] +name = "miniz_oxide" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e2d80299ef12ff69b16a84bb182e3b9df68b5a91574d3d4fa6e41b65deec4df1" +dependencies = [ + "adler2", "simd-adler32", ] @@ -2675,15 +2690,15 @@ checksum = "d231b230927b5e4ad203db57bbcbee2802f6bce620b1e4a9024a07d94e2907ec" [[package]] name = "png" -version = "0.17.13" +version = "0.17.14" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "06e4b0d3d1312775e782c86c91a111aa1f910cbb65e1337f9975b5f9a554b5e1" +checksum = "52f9d46a34a05a6a57566bc2bfae066ef07585a6e3fa30fbbdff5936380623f0" dependencies = [ "bitflags 1.3.2", "crc32fast", "fdeflate", "flate2", - "miniz_oxide", + "miniz_oxide 0.8.0", ] [[package]] @@ -3532,18 +3547,18 @@ dependencies = [ [[package]] name = "thiserror" -version = "1.0.63" +version = "1.0.65" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c0342370b38b6a11b6cc11d6a805569958d54cfa061a29969c3b5ce2ea405724" +checksum = "5d11abd9594d9b38965ef50805c5e469ca9cc6f197f883f717e0269a3057b3d5" dependencies = [ "thiserror-impl", ] [[package]] name = "thiserror-impl" -version = "1.0.63" +version = "1.0.65" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a4558b58466b9ad7ca0f102865eccc95938dca1a74a856f2b57b6629050da261" +checksum = "ae71770322cbd277e69d762a16c444af02aa0575ac0d174f0b9562d3b37f8602" dependencies = [ "proc-macro2", "quote", diff --git a/masonry/src/text/text_layout.rs b/masonry/src/text/text_layout.rs index 39b605cb..c9a15e8e 100644 --- a/masonry/src/text/text_layout.rs +++ b/masonry/src/text/text_layout.rs @@ -255,6 +255,9 @@ impl TextLayout { } /// Set the [`Alignment`] for this layout. + /// + /// This alignment can only be meaningful when a + /// [maximum width](Self::set_max_advance) is provided. pub fn set_text_alignment(&mut self, alignment: Alignment) { if self.alignment != alignment { self.alignment = alignment; @@ -313,20 +316,43 @@ impl TextLayout { &self.layout } - /// The size of the laid-out text, excluding any trailing whitespace. + /// The size of the region the text should be drawn in, + /// excluding any trailing whitespace if present. + /// + /// Should be used for the drawing of non-interactive text (as the + /// trailing whitespace is selectable for interactive text). /// /// This is not meaningful until [`Self::rebuild`] has been called. pub fn size(&self) -> Size { self.assert_rebuilt("size"); - Size::new(self.layout.width().into(), self.layout.height().into()) + Size::new( + self.layout_width(self.layout.width()).into(), + self.layout.height().into(), + ) } /// The size of the laid-out text, including any trailing whitespace. /// + /// Should be used for the drawing of interactive text. + /// /// This is not meaningful until [`Self::rebuild`] has been called. pub fn full_size(&self) -> Size { self.assert_rebuilt("full_size"); - Size::new(self.layout.full_width().into(), self.layout.height().into()) + Size::new( + self.layout_width(self.layout.full_width()).into(), + self.layout.height().into(), + ) + } + + /// If performing layout `max_advance` to calculate text alignment, the only + /// reasonable behaviour is to take up the entire available width. + /// + /// The coherent way to have multiple items laid out on the same line is for them to + /// be inside the same text layout object "region". This is currently deferred. + /// As an interim solution, we allow multiple items to be on the same line if the `max_advance` wasn't used + /// (and therefore the text alignment argument is effectively ignored). + fn layout_width(&self, width: f32) -> f32 { + self.max_advance.unwrap_or(width) } /// Return the text's [`LayoutMetrics`]. diff --git a/masonry/src/widget/label.rs b/masonry/src/widget/label.rs index 24f6dc8c..f0ca9e75 100644 --- a/masonry/src/widget/label.rs +++ b/masonry/src/widget/label.rs @@ -20,8 +20,9 @@ use crate::{ RegisterCtx, TextEvent, Update, UpdateCtx, Widget, WidgetId, }; -// added padding between the edges of the widget and the text. -pub(super) const LABEL_X_PADDING: f64 = 2.0; +/// Added padding between each horizontal edge of the widget +/// and the text in logical pixels. +const LABEL_X_PADDING: f64 = 2.0; /// Options for handling lines that are too wide for the label. #[derive(Debug, Clone, Copy, PartialEq)] @@ -202,7 +203,9 @@ impl Widget for Label { .rebuild(font_ctx, layout_ctx, &self.text, self.text_changed); self.text_changed = false; } - // We ignore trailing whitespace for a label + // We would like to ignore trailing whitespace for a label. + // However, Parley doesn't make that an option when using `max_advance`. + // If we aren't wrapping words, we can safely ignore this, however. let text_size = self.text_layout.size(); let label_size = Size { height: text_size.height, @@ -265,7 +268,7 @@ mod tests { use crate::assert_render_snapshot; use crate::testing::TestHarness; use crate::theme::{PRIMARY_DARK, PRIMARY_LIGHT}; - use crate::widget::{Flex, SizedBox}; + use crate::widget::{CrossAxisAlignment, Flex, SizedBox}; #[test] fn simple_label() { @@ -291,6 +294,35 @@ mod tests { assert_render_snapshot!(harness, "styled_label"); } + #[test] + /// A wrapping label's alignment should be respected, regardkess of + /// its parent's alignment. + fn label_alignment_flex() { + fn base_label() -> Label { + Label::new("Hello") + .with_text_size(10.0) + .with_line_break_mode(LineBreaking::WordWrap) + } + let label1 = base_label().with_text_alignment(Alignment::Start); + let label2 = base_label().with_text_alignment(Alignment::Middle); + let label3 = base_label().with_text_alignment(Alignment::End); + let label4 = base_label().with_text_alignment(Alignment::Start); + let label5 = base_label().with_text_alignment(Alignment::Middle); + let label6 = base_label().with_text_alignment(Alignment::End); + let flex = Flex::column() + .with_flex_child(label1, CrossAxisAlignment::Start) + .with_flex_child(label2, CrossAxisAlignment::Start) + .with_flex_child(label3, CrossAxisAlignment::Start) + .with_flex_child(label4, CrossAxisAlignment::Center) + .with_flex_child(label5, CrossAxisAlignment::Center) + .with_flex_child(label6, CrossAxisAlignment::Center) + .gap(0.0); + + let mut harness = TestHarness::create_with_size(flex, Size::new(80.0, 80.0)); + + assert_render_snapshot!(harness, "label_alignment_flex"); + } + #[test] fn line_break_modes() { let widget = Flex::column() diff --git a/masonry/src/widget/prose.rs b/masonry/src/widget/prose.rs index ecd3f153..aaae1fb6 100644 --- a/masonry/src/widget/prose.rs +++ b/masonry/src/widget/prose.rs @@ -11,13 +11,16 @@ use vello::peniko::BlendMode; use vello::Scene; use crate::text::{ArcStr, TextBrush, TextWithSelection}; -use crate::widget::label::LABEL_X_PADDING; use crate::widget::{LineBreaking, WidgetMut}; use crate::{ AccessCtx, AccessEvent, BoxConstraints, CursorIcon, EventCtx, LayoutCtx, PaintCtx, PointerEvent, QueryCtx, RegisterCtx, TextEvent, Update, UpdateCtx, Widget, WidgetId, }; +/// Added padding between each horizontal edge of the widget +/// and the text in logical pixels. +const PROSE_X_PADDING: f64 = 2.0; + /// The prose widget is a widget which displays text which can be /// selected with keyboard and mouse, and which can be copied from, /// but cannot be modified by the user. @@ -136,7 +139,7 @@ impl Prose { impl Widget for Prose { fn on_pointer_event(&mut self, ctx: &mut EventCtx, event: &PointerEvent) { let window_origin = ctx.widget_state.window_origin(); - let inner_origin = Point::new(window_origin.x + LABEL_X_PADDING, window_origin.y); + let inner_origin = Point::new(window_origin.x + PROSE_X_PADDING, window_origin.y); match event { PointerEvent::PointerDown(button, state) => { if !ctx.is_disabled() { @@ -223,7 +226,7 @@ impl Widget for Prose { None } else if bc.max().width.is_finite() { // TODO: Does Prose have different needs here? - Some(bc.max().width as f32 - 2. * LABEL_X_PADDING as f32) + Some(bc.max().width as f32 - 2. * PROSE_X_PADDING as f32) } else if bc.min().width.is_sign_negative() { Some(0.0) } else { @@ -234,11 +237,11 @@ impl Widget for Prose { let (font_ctx, layout_ctx) = ctx.text_contexts(); self.text_layout.rebuild(font_ctx, layout_ctx); } - // We ignore trailing whitespace for a label - let text_size = self.text_layout.size(); + // We include trailing whitespace for prose, as it can be selected. + let text_size = self.text_layout.full_size(); let label_size = Size { height: text_size.height, - width: text_size.width + 2. * LABEL_X_PADDING, + width: text_size.width + 2. * PROSE_X_PADDING, }; bc.constrain(label_size) } @@ -255,7 +258,7 @@ impl Widget for Prose { scene.push_layer(BlendMode::default(), 1., Affine::IDENTITY, &clip_rect); } self.text_layout - .draw(scene, Point::new(LABEL_X_PADDING, 0.0)); + .draw(scene, Point::new(PROSE_X_PADDING, 0.0)); if self.line_break_mode == LineBreaking::Clip { scene.pop_layer(); @@ -289,4 +292,45 @@ impl Widget for Prose { } } -// TODO - Add tests +// TODO - Add more tests +#[cfg(test)] +mod tests { + use parley::layout::Alignment; + use vello::kurbo::Size; + + use crate::{ + assert_render_snapshot, + testing::TestHarness, + widget::{CrossAxisAlignment, Flex, LineBreaking, Prose}, + }; + + #[test] + /// A wrapping prose's alignment should be respected, regardkess of + /// its parent's alignment. + fn prose_alignment_flex() { + fn base_label() -> Prose { + // Trailing whitespace is displayed when laying out prose. + Prose::new("Hello ") + .with_text_size(10.0) + .with_line_break_mode(LineBreaking::WordWrap) + } + let label1 = base_label().with_text_alignment(Alignment::Start); + let label2 = base_label().with_text_alignment(Alignment::Middle); + let label3 = base_label().with_text_alignment(Alignment::End); + let label4 = base_label().with_text_alignment(Alignment::Start); + let label5 = base_label().with_text_alignment(Alignment::Middle); + let label6 = base_label().with_text_alignment(Alignment::End); + let flex = Flex::column() + .with_flex_child(label1, CrossAxisAlignment::Start) + .with_flex_child(label2, CrossAxisAlignment::Start) + .with_flex_child(label3, CrossAxisAlignment::Start) + .with_flex_child(label4, CrossAxisAlignment::Center) + .with_flex_child(label5, CrossAxisAlignment::Center) + .with_flex_child(label6, CrossAxisAlignment::Center) + .gap(0.0); + + let mut harness = TestHarness::create_with_size(flex, Size::new(80.0, 80.0)); + + assert_render_snapshot!(harness, "prose_alignment_flex"); + } +} diff --git a/masonry/src/widget/screenshots/masonry__widget__label__tests__label_alignment_flex.png b/masonry/src/widget/screenshots/masonry__widget__label__tests__label_alignment_flex.png new file mode 100644 index 00000000..25423c2d --- /dev/null +++ b/masonry/src/widget/screenshots/masonry__widget__label__tests__label_alignment_flex.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:c4333c177b7c8911446cec6e9c0b3b9ce779fa1b45884845de89ff2cfd0683cd +size 2553 diff --git a/masonry/src/widget/screenshots/masonry__widget__prose__tests__prose_alignment_flex.png b/masonry/src/widget/screenshots/masonry__widget__prose__tests__prose_alignment_flex.png new file mode 100644 index 00000000..fa65754e --- /dev/null +++ b/masonry/src/widget/screenshots/masonry__widget__prose__tests__prose_alignment_flex.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:11a0972091eedf3fcdbf767dd9bb2da2bec93870baae03459c0b7623f12a14f9 +size 2534 diff --git a/xilem/examples/http_cats.rs b/xilem/examples/http_cats.rs index 60c6d810..83a3b163 100644 --- a/xilem/examples/http_cats.rs +++ b/xilem/examples/http_cats.rs @@ -13,8 +13,8 @@ use winit::window::Window; use xilem::core::fork; use xilem::core::one_of::OneOf3; use xilem::view::{ - button, flex, image, portal, prose, sized_box, spinner, worker, Axis, CrossAxisAlignment, - FlexExt, FlexSpacer, + button, flex, image, inline_prose, portal, prose, sized_box, spinner, worker, Axis, FlexExt, + FlexSpacer, }; use xilem::{Color, EventLoop, EventLoopBuilder, TextAlignment, WidgetView, Xilem}; @@ -102,12 +102,10 @@ impl HttpCats { portal(sized_box(info_area).expand_width()).flex(1.), )) .direction(Axis::Horizontal) - .cross_axis_alignment(CrossAxisAlignment::Fill) .must_fill_major_axis(true) .flex(1.), )) - .must_fill_major_axis(true) - .cross_axis_alignment(CrossAxisAlignment::Fill), + .must_fill_major_axis(true), worker( worker_value, |proxy, mut rx| async move { @@ -164,8 +162,8 @@ impl Status { let code = self.code; flex(( // TODO: Reduce allocations here? - prose(self.code.to_string()), - prose(self.message), + inline_prose(self.code.to_string()), + inline_prose(self.message), FlexSpacer::Flex(1.), // TODO: Spinner if image pending? // TODO: Tick if image loaded? @@ -199,7 +197,6 @@ impl Status { prose("Copyright ©️ https://http.cat ").alignment(TextAlignment::End), )) .main_axis_alignment(xilem::view::MainAxisAlignment::Start) - .cross_axis_alignment(CrossAxisAlignment::Fill) } } diff --git a/xilem/examples/mason.rs b/xilem/examples/mason.rs index f736436f..6cde4920 100644 --- a/xilem/examples/mason.rs +++ b/xilem/examples/mason.rs @@ -61,9 +61,7 @@ fn app_logic(data: &mut AppData) -> impl WidgetView { fork( flex(( flex(( - label("Label") - .brush(Color::REBECCA_PURPLE) - .alignment(TextAlignment::Start), + label("Label").brush(Color::REBECCA_PURPLE), label("Bold Label").weight(TextWeight::BOLD), // TODO masonry doesn't allow setting disabled manually anymore? // label("Disabled label").disabled(), diff --git a/xilem/examples/variable_clock.rs b/xilem/examples/variable_clock.rs index bb2e033e..5d22f8c9 100644 --- a/xilem/examples/variable_clock.rs +++ b/xilem/examples/variable_clock.rs @@ -13,7 +13,8 @@ use time::{OffsetDateTime, UtcOffset}; use winit::error::EventLoopError; use xilem::core::fork; use xilem::view::{ - button, flex, label, portal, prose, sized_box, task, variable_label, Axis, FlexExt, FlexSpacer, + button, flex, inline_prose, label, portal, prose, sized_box, task, variable_label, Axis, + FlexExt, FlexSpacer, }; use xilem::{Color, EventLoop, EventLoopBuilder, WidgetView, Xilem}; @@ -113,7 +114,7 @@ impl TimeZone { let date_time_in_self = data.now_utc.to_offset(self.offset); sized_box(flex(( flex(( - prose(self.region), + inline_prose(self.region), FlexSpacer::Flex(1.), label(format!("UTC{}", self.offset)).brush( if data.local_offset.is_ok_and(|it| it == self.offset) { diff --git a/xilem/src/view/prose.rs b/xilem/src/view/prose.rs index 47f699bd..c17bfad2 100644 --- a/xilem/src/view/prose.rs +++ b/xilem/src/view/prose.rs @@ -2,7 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 use masonry::text::{ArcStr, TextBrush}; -use masonry::widget; +use masonry::widget::{self, LineBreaking}; use crate::core::{DynMessage, Mut, ViewMarker}; use crate::{Color, MessageResult, Pod, TextAlignment, View, ViewCtx, ViewId}; @@ -13,15 +13,27 @@ pub fn prose(content: impl Into) -> Prose { text_brush: Color::WHITE.into(), alignment: TextAlignment::default(), text_size: masonry::theme::TEXT_SIZE_NORMAL as f32, + line_break_mode: LineBreaking::WordWrap, } } +/// A version of [`prose`] suitable for including in the same line +/// as other content. +/// +/// Note that setting [`alignment`](Prose::alignment) on the result +/// will be meaningless. +#[doc(alias = "span")] +pub fn inline_prose(content: impl Into) -> Prose { + prose(content).line_break_mode(LineBreaking::Overflow) +} + pub struct Prose { content: ArcStr, text_brush: TextBrush, alignment: TextAlignment, text_size: f32, + line_break_mode: LineBreaking, // TODO: disabled: bool, // TODO: add more attributes of `masonry::widget::Prose` } @@ -43,6 +55,10 @@ impl Prose { self.text_size = text_size; self } + pub fn line_break_mode(mut self, line_break_mode: LineBreaking) -> Self { + self.line_break_mode = line_break_mode; + self + } } impl ViewMarker for Prose {} @@ -55,7 +71,8 @@ impl View for Prose { widget::Prose::new(self.content.clone()) .with_text_brush(self.text_brush.clone()) .with_text_alignment(self.alignment) - .with_text_size(self.text_size), + .with_text_size(self.text_size) + .with_line_break_mode(self.line_break_mode), ); (widget_pod, ()) } @@ -79,6 +96,9 @@ impl View for Prose { if prev.text_size != self.text_size { widget::Prose::set_text_size(&mut element, self.text_size); } + if prev.line_break_mode != self.line_break_mode { + widget::Prose::set_line_break_mode(&mut element, self.line_break_mode); + } } fn teardown(&self, (): &mut Self::ViewState, _: &mut ViewCtx, _: Mut) {} diff --git a/xilem/src/view/variable_label.rs b/xilem/src/view/variable_label.rs index ab4a1f8f..fde12102 100644 --- a/xilem/src/view/variable_label.rs +++ b/xilem/src/view/variable_label.rs @@ -100,7 +100,6 @@ impl View for VariableLabel { let widget_pod = ctx.new_pod( widget::VariableLabel::new(self.label.clone()) .with_text_brush(self.text_brush.clone()) - .with_line_break_mode(widget::LineBreaking::WordWrap) .with_text_alignment(self.alignment) .with_font(self.font) .with_text_size(self.text_size)