From 8045c72d28ba6fc0250e6b3834a78a488c7c1ab9 Mon Sep 17 00:00:00 2001 From: PgBiel <9021226+PgBiel@users.noreply.github.com> Date: Wed, 30 Apr 2025 20:09:36 -0300 Subject: [PATCH] switch to only snapshotting for orphan prevention --- crates/typst-layout/src/grid/layouter.rs | 86 ++++++++++-------------- crates/typst-layout/src/grid/repeated.rs | 39 +++++++---- 2 files changed, 59 insertions(+), 66 deletions(-) diff --git a/crates/typst-layout/src/grid/layouter.rs b/crates/typst-layout/src/grid/layouter.rs index 01b478fac..410e3c66c 100644 --- a/crates/typst-layout/src/grid/layouter.rs +++ b/crates/typst-layout/src/grid/layouter.rs @@ -104,10 +104,14 @@ pub(super) struct Current { pub(super) last_repeated_header_end: usize, /// Stores the length of `lrows` before a sequence of trailing rows /// equipped with orphan prevention were laid out. In this case, if no more - /// rows are laid out after those rows before the region ends, the rows - /// will be removed. For new headers in particular, which use this, those - /// headers will have been moved to the `pending_headers` vector and so - /// will automatically be placed again until they fit. + /// rows without orphan prevention are laid out after those rows before the + /// region ends, the rows will be removed. + /// + /// At the moment, this is only used by repeated headers (they aren't laid + /// out if alone in the region) and by new headers, which are moved to the + /// `pending_headers` vector and so will automatically be placed again + /// until they fit and are not orphans in at least one region (or exactly + /// one, for non-repeated headers). pub(super) lrows_orphan_snapshot: Option, /// The total simulated height for all headers currently in /// `repeating_headers` and `pending_headers`. @@ -1517,6 +1521,11 @@ impl<'a> GridLayouter<'a> { self.lrows.truncate(orphan_snapshot); self.current.repeated_header_rows = self.current.repeated_header_rows.min(orphan_snapshot); + + if orphan_snapshot == 0 { + // Removed all repeated headers. + self.current.last_repeated_header_end = 0; + } } } @@ -1531,55 +1540,28 @@ impl<'a> GridLayouter<'a> { self.current.repeated_header_rows.min(self.lrows.len()); } + // If no rows other than the footer have been laid out so far + // (e.g. due to header orphan prevention), and there are rows + // beside the footer, then don't lay it out at all. + // + // It is worth noting that the footer is made non-repeatable at + // the grid resolving stage if it is short-lived, that is, if + // it is at the start of the table (or right after headers at + // the start of the table). + // TODO(subfooters): explicitly check for short-lived footers. + // TODO(subfooters): widow prevention for non-repeated footers with a + // similar mechanism / when implementing multiple footers. let footer_would_be_widow = - if !self.lrows.is_empty() && self.current.repeated_header_rows > 0 { - // If headers are repeating, then we already know they are not - // short-lived as that is checked, so they have orphan prevention. - if self.lrows.len() == self.current.repeated_header_rows - && may_progress_with_offset( - self.regions, - // Since we're trying to find a region where to place all - // repeating + pending headers, it makes sense to use - // 'header_height' and include even non-repeating pending - // headers for this check. - self.current.header_height + self.current.footer_height, - ) - { - // Header and footer would be alone in this region, but - // there are more rows beyond the headers and the footer. - // Push an empty region. - self.lrows.clear(); - self.current.last_repeated_header_end = 0; - self.current.repeated_header_rows = 0; - true - } else { - false - } - } else if let Some(Repeatable::Repeated(_)) = &self.grid.footer { - // If no rows other than the footer have been laid out so far, - // and there are rows beside the footer, then don't lay it out - // at all. (Similar check from above, but for the case without - // headers.) - // - // It is worth noting that the footer is made non-repeatable at - // the grid resolving stage if it is short-lived, that is, if - // it is at the start of the table (or right after headers at - // the start of the table). - // TODO(subfooters): explicitly check for short-lived footers. - // TODO(subfooters): widow prevention for non-repeated footers with a - // similar mechanism / when implementing multiple footers. - self.lrows.is_empty() - && may_progress_with_offset( - self.regions, - // This header height isn't doing much as we just - // confirmed that there are no headers in this region, - // but let's keep it here for correctness. It will add - // zero anyway. - self.current.header_height + self.current.footer_height, - ) - } else { - false - }; + matches!(self.grid.footer, Some(Repeatable::Repeated(_))) + && self.lrows.is_empty() + && may_progress_with_offset( + self.regions, + // This header height isn't doing much as we just + // confirmed that there are no headers in this region, + // but let's keep it here for correctness. It will add + // zero anyway. + self.current.header_height + self.current.footer_height, + ); let mut laid_out_footer_start = None; if !footer_would_be_widow { diff --git a/crates/typst-layout/src/grid/repeated.rs b/crates/typst-layout/src/grid/repeated.rs index 62a8e578a..4e3004967 100644 --- a/crates/typst-layout/src/grid/repeated.rs +++ b/crates/typst-layout/src/grid/repeated.rs @@ -262,6 +262,18 @@ impl<'a> GridLayouter<'a> { self.current.repeating_header_height = Abs::zero(); self.current.repeating_header_heights.clear(); + debug_assert!(self.lrows.is_empty()); + debug_assert!(self.current.lrows_orphan_snapshot.is_none()); + if may_progress_with_offset(self.regions, self.current.footer_height) { + // Enable orphan prevention for headers at the top of the region. + // + // It is very rare for this to make a difference as we're usually + // at the 'last' region after the first skip, at which the snapshot + // is handled by 'layout_new_headers'. Either way, we keep this + // here for correctness. + self.current.lrows_orphan_snapshot = Some(self.lrows.len()); + } + // Use indices to avoid double borrow. We don't mutate headers in // 'layout_row' so this is fine. let mut i = 0; @@ -295,13 +307,6 @@ impl<'a> GridLayouter<'a> { } self.current.repeated_header_rows = self.lrows.len(); - - if !self.pending_headers.is_empty() { - // Restore snapshot: if pending headers placed again turn out to be - // orphans, remove their rows again. - self.current.lrows_orphan_snapshot = Some(self.lrows.len()); - } - for header in self.pending_headers { let header_height = self.layout_header_rows(header.unwrap(), engine, disambiguator, false)?; @@ -357,10 +362,22 @@ impl<'a> GridLayouter<'a> { self.finish_region(engine, false)?; } + // Remove new headers at the end of the region if the upcoming row + // doesn't fit. + // TODO(subfooters): what if there is a footer right after it? + if !short_lived + && self.current.lrows_orphan_snapshot.is_none() + && may_progress_with_offset( + self.regions, + self.current.header_height + self.current.footer_height, + ) + { + self.current.lrows_orphan_snapshot = Some(self.lrows.len()); + } + self.unbreakable_rows_left += total_header_row_count(headers.iter().map(Repeatable::unwrap)); - let initial_row_count = self.lrows.len(); for header in headers { let header_height = self.layout_header_rows(header.unwrap(), engine, 0, false)?; @@ -380,12 +397,6 @@ impl<'a> GridLayouter<'a> { } } - // Remove new headers at the end of the region if upcoming child doesn't fit. - // TODO: Short lived if footer comes afterwards - if !short_lived { - self.current.lrows_orphan_snapshot = Some(initial_row_count); - } - Ok(()) }