Fix and simplify unjustify, remove LineData::alignment (#271)

`unjustify` doesn't mirror `align`, so it performs the wrong
calculations, and re-line-breaking or re-aligning currently lead to the
wrong result after a justified alignment.

As the alignment function requires further iteration (e.g. for floated
boxes), this patch removes the specialized `unjustify` implementation
and instead uses the same code for aligning and unjustifying. This is a
trade-off between efficiency and correctness. If this is merged in, we
can always split the two again in the future once alignment has
stabilized.

This stores the width the layout was aligned to, which is necessary for
unjustifying.

Author: tomcur

CHANGELOG.md

@@ -46,6 +46,7 @@ This release has an [MSRV] of 1.82.
 #### Parley
 
 - Fix alignment of right-to-left text. ([#250][], [#268][] by [@tomcur][])
+- Performing line breaking or aligning a layout again, after justified alignment had been applied previously, now lead to the correct results. ([#271][] by [@tomcur][])
 
 ## [0.2.0] - 2024-10-10
 
@@ -129,6 +130,7 @@ This release has an [MSRV] of 1.70.
 [#250]: https://github.com/linebender/parley/pull/250
 [#259]: https://github.com/linebender/parley/pull/259
 [#268]: https://github.com/linebender/parley/pull/268
+[#271]: https://github.com/linebender/parley/pull/271
 
 [Unreleased]: https://github.com/linebender/parley/compare/v0.2.0...HEAD
 [0.2.0]: https://github.com/linebender/parley/releases/tag/v0.2.0

parley/src/layout/alignment.rs

@@ -7,29 +7,61 @@ use super::{
 };
 use crate::style::Brush;
 
+/// Align the layout.
+///
+/// If [`Alignment::Justified`] is requested, clusters' [`ClusterData::advance`] will be adjusted.
+/// Prior to re-line-breaking or re-aligning, [`unjustify`] has to be called.
 pub(crate) fn align<B: Brush>(
     layout: &mut LayoutData<B>,
     alignment_width: Option<f32>,
     alignment: Alignment,
     align_when_overflowing: bool,
+) {
+    layout.alignment_width = alignment_width.unwrap_or(layout.width);
+    layout.is_aligned_justified = alignment == Alignment::Justified;
+
+    align_impl::<_, false>(layout, alignment, align_when_overflowing);
+}
+
+/// Removes previous justification applied to clusters.
+///
+/// This is part of resetting state in preparation for re-line-breaking or re-aligning the same
+/// layout.
+pub(crate) fn unjustify<B: Brush>(layout: &mut LayoutData<B>) {
+    if layout.is_aligned_justified {
+        align_impl::<_, true>(layout, Alignment::Justified, false);
+        layout.is_aligned_justified = false;
+    }
+}
+
+/// The actual alignment implementation.
+///
+/// This is const-generic over `UNDO_JUSTIFICATION`: justified alignment adjusts clusters'
+/// [`ClusterData::advance`], and this mutation has to be undone for re-line-breaking or
+/// re-aligning. `UNDO_JUSTIFICATION` indicates whether the adjustment has to be applied, or
+/// undone.
+///
+/// Writing a separate function for undoing justification would be faster, but not by much, and
+/// doing it this way we are sure the calculations performed are equivalent.
+fn align_impl<B: Brush, const UNDO_JUSTIFICATION: bool>(
+    layout: &mut LayoutData<B>,
+    alignment: Alignment,
+    align_when_overflowing: bool,
 ) {
     // Whether the text base direction is right-to-left.
     let is_rtl = layout.base_level & 1 == 1;
-    let alignment_width = alignment_width.unwrap_or(layout.width);
 
     // Apply alignment to line items
     for line in &mut layout.lines {
-        // TODO: remove this field
-        line.alignment = alignment;
-
         if is_rtl {
             // In RTL text, trailing whitespace is on the left. As we hang that whitespace, offset
             // the line to the left.
             line.metrics.offset = -line.metrics.trailing_whitespace;
         }
 
         // Compute free space.
-        let free_space = alignment_width - line.metrics.advance + line.metrics.trailing_whitespace;
+        let free_space =
+            layout.alignment_width - line.metrics.advance + line.metrics.trailing_whitespace;
 
         if !align_when_overflowing && free_space <= 0.0 {
             if is_rtl {
@@ -65,7 +97,8 @@ pub(crate) fn align<B: Brush>(
                     continue;
                 }
 
-                let adjustment = free_space / line.num_spaces as f32;
+                let adjustment =
+                    free_space / line.num_spaces as f32 * if UNDO_JUSTIFICATION { -1. } else { 1. };
                 let mut applied = 0;
                 // Iterate over text runs in the line and clusters in the text run
                 //   - Iterate forwards for even bidi levels (which represent LTR runs)
@@ -100,55 +133,3 @@ pub(crate) fn align<B: Brush>(
         }
     }
 }
-
-/// Removes previous justification applied to clusters.
-/// This is part of resetting state in preparation for re-linebreaking the same layout.
-pub(crate) fn unjustify<B: Brush>(layout: &mut LayoutData<B>) {
-    // Whether the text base direction is right-to-left.
-    let is_rtl = layout.base_level & 1 == 1;
-
-    for line in &layout.lines {
-        if line.alignment == Alignment::Justified
-            && line.max_advance.is_finite()
-            && line.max_advance < f32::MAX
-        {
-            let extra = line.max_advance - line.metrics.advance + line.metrics.trailing_whitespace;
-            if line.break_reason != BreakReason::None && line.num_spaces != 0 {
-                let adjustment = extra / line.num_spaces as f32;
-                let mut applied = 0;
-
-                let line_items: &mut dyn Iterator<Item = &LineItemData> = if is_rtl {
-                    &mut layout.line_items[line.item_range.clone()].iter().rev()
-                } else {
-                    &mut layout.line_items[line.item_range.clone()].iter()
-                };
-                for line_run in line_items.filter(|item| item.is_text_run()) {
-                    if line_run.bidi_level & 1 != 0 {
-                        for cluster in layout.clusters[line_run.cluster_range.clone()]
-                            .iter_mut()
-                            .rev()
-                        {
-                            if applied == line.num_spaces {
-                                break;
-                            }
-                            if cluster.info.whitespace().is_space_or_nbsp() {
-                                cluster.advance -= adjustment;
-                                applied += 1;
-                            }
-                        }
-                    } else {
-                        for cluster in layout.clusters[line_run.cluster_range.clone()].iter_mut() {
-                            if applied == line.num_spaces {
-                                break;
-                            }
-                            if cluster.info.whitespace().is_space_or_nbsp() {
-                                cluster.advance -= adjustment;
-                                applied += 1;
-                            }
-                        }
-                    }
-                }
-            }
-        }
-    }
-}

parley/src/layout/data.rs

@@ -2,7 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0 OR MIT
 
 use crate::inline_box::InlineBox;
-use crate::layout::{Alignment, ContentWidths, Glyph, LineMetrics, RunMetrics, Style};
+use crate::layout::{ContentWidths, Glyph, LineMetrics, RunMetrics, Style};
 use crate::style::Brush;
 use crate::util::nearly_zero;
 use crate::Font;
@@ -105,8 +105,10 @@ pub(crate) struct LineData {
     pub(crate) metrics: LineMetrics,
     /// The cause of the line break.
     pub(crate) break_reason: BreakReason,
-    /// Alignment.
-    pub(crate) alignment: Alignment,
+    #[expect(
+        dead_code,
+        reason = "needed in the future for aligning around floated boxes"
+    )]
     /// Maximum advance for the line.
     pub(crate) max_advance: f32,
     /// Number of justified clusters on the line.
@@ -220,6 +222,12 @@ pub(crate) struct LayoutData<B: Brush> {
     // Output of line breaking
     pub(crate) lines: Vec<LineData>,
     pub(crate) line_items: Vec<LineItemData>,
+
+    // Output of alignment
+    /// Whether the layout is aligned with [`crate::Alignment::Justified`].
+    pub(crate) is_aligned_justified: bool,
+    /// The width the layout was aligned to.
+    pub(crate) alignment_width: f32,
 }
 
 impl<B: Brush> Default for LayoutData<B> {
@@ -243,6 +251,8 @@ impl<B: Brush> Default for LayoutData<B> {
             glyphs: Vec::new(),
             lines: Vec::new(),
             line_items: Vec::new(),
+            is_aligned_justified: false,
+            alignment_width: 0.0,
         }
     }
 }

parley/src/layout/line/greedy.rs

@@ -10,10 +10,9 @@ use swash::text::cluster::Whitespace;
 #[allow(unused_imports)]
 use core_maths::CoreFloat;
 
-use crate::layout::alignment::unjustify;
 use crate::layout::{
-    Alignment, Boundary, BreakReason, Layout, LayoutData, LayoutItem, LayoutItemKind, LineData,
-    LineItemData, LineMetrics, Run,
+    Boundary, BreakReason, Layout, LayoutData, LayoutItem, LayoutItemKind, LineData, LineItemData,
+    LineMetrics, Run,
 };
 use crate::style::Brush;
 
@@ -108,7 +107,6 @@ pub struct BreakLines<'a, B: Brush> {
 
 impl<'a, B: Brush> BreakLines<'a, B> {
     pub(crate) fn new(layout: &'a mut Layout<B>) -> Self {
-        unjustify(&mut layout.data);
         layout.data.width = 0.;
         layout.data.height = 0.;
         let mut lines = LineLayout::default();
@@ -166,7 +164,6 @@ impl<'a, B: Brush> BreakLines<'a, B> {
                     &mut self.lines,
                     &mut self.state.line,
                     max_advance,
-                    Alignment::Start,
                     $break_reason,
                 )
             };
@@ -699,7 +696,6 @@ fn try_commit_line<B: Brush>(
     lines: &mut LineLayout,
     state: &mut LineState,
     max_advance: f32,
-    alignment: Alignment,
     break_reason: BreakReason,
 ) -> bool {
     // Ensure that the cluster and item endpoints are within range
@@ -820,7 +816,6 @@ fn try_commit_line<B: Brush>(
     lines.lines.push(LineData {
         item_range: start_item_idx..end_item_idx,
         max_advance,
-        alignment,
         break_reason,
         num_spaces,
         metrics: LineMetrics {

parley/src/layout/mod.rs

@@ -19,6 +19,7 @@ use super::style::Brush;
 use crate::{Font, InlineBox};
 #[cfg(feature = "accesskit")]
 use accesskit::{Node, NodeId, Role, TextDirection, TreeUpdate};
+use alignment::unjustify;
 #[cfg(feature = "accesskit")]
 use alloc::vec::Vec;
 use core::{cmp::Ordering, ops::Range};
@@ -164,6 +165,7 @@ impl<B: Brush> Layout<B> {
 
     /// Returns line breaker to compute lines for the layout.
     pub fn break_lines(&mut self) -> BreakLines<'_, B> {
+        unjustify(&mut self.data);
         BreakLines::new(self)
     }
 
@@ -189,6 +191,7 @@ impl<B: Brush> Layout<B> {
         alignment: Alignment,
         align_when_overflowing: bool,
     ) {
+        unjustify(&mut self.data);
         align(
             &mut self.data,
             container_width,

parley/src/tests/test_basic.rs

@@ -301,3 +301,25 @@ fn inbox_content_width() {
             .check_layout_snapshot(&layout);
     }
 }
+
+#[test]
+/// Layouts can be re-line-breaked and re-aligned.
+fn realign() {
+    let mut env = testenv!();
+
+    let text = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.";
+    let mut builder = env.ranged_builder(text);
+    let mut layout = builder.build(text);
+    layout.break_all_lines(Some(150.0));
+    for idx in 0..8 {
+        if [2, 3, 4].contains(&idx) {
+            layout.break_all_lines(Some(150.0));
+        }
+        layout.align(
+            Some(150.),
+            Alignment::Justified,
+            false, /* align_when_overflowing */
+        );
+    }
+    env.check_layout_snapshot(&layout);
+}