From f9569efc4083851cd0355dce68a18d4d624c82f7 Mon Sep 17 00:00:00 2001 From: PgBiel <9021226+PgBiel@users.noreply.github.com> Date: Sun, 6 Apr 2025 01:30:27 -0300 Subject: [PATCH] adapt usages of header_height to repeating_header_height - Document unchanged usages --- crates/typst-layout/src/grid/layouter.rs | 16 +++++- crates/typst-layout/src/grid/rowspans.rs | 66 +++++++++++++++++++----- 2 files changed, 67 insertions(+), 15 deletions(-) diff --git a/crates/typst-layout/src/grid/layouter.rs b/crates/typst-layout/src/grid/layouter.rs index c75bf0d3e..cc957eb2c 100644 --- a/crates/typst-layout/src/grid/layouter.rs +++ b/crates/typst-layout/src/grid/layouter.rs @@ -1034,11 +1034,12 @@ impl<'a> GridLayouter<'a> { .skip(self.lrows.iter().any(|row| matches!(row, Row::Fr(..))) as usize) { // Subtract header and footer heights from the region height when - // it's not the first. + // it's not the first. Ignore non-repeating headers as they only + // appear on the first region by definition. target.set_max( region.y - if i > 0 { - self.header_height + self.footer_height + self.repeating_header_height + self.footer_height } else { Abs::zero() }, @@ -1258,6 +1259,10 @@ impl<'a> GridLayouter<'a> { // row group. We use 'in_last_with_offset' so our 'in_last' call // properly considers that a header and a footer would be added on each // region break. + // + // See 'check_for_unbreakable_rows' as for why we're using + // 'header_height' to predict header height and not + // 'repeating_header_height'. let height = frame.height(); while self.unbreakable_rows_left == 0 && !self.regions.size.y.fits(height) @@ -1427,6 +1432,10 @@ impl<'a> GridLayouter<'a> { && self.lrows.last().is_some_and(|row| row.index() < last_header_end) && !in_last_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.header_height + self.footer_height, ) { @@ -1446,6 +1455,9 @@ impl<'a> GridLayouter<'a> { self.lrows.is_empty() && !in_last_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.header_height + self.footer_height, ) && footer.start != 0 diff --git a/crates/typst-layout/src/grid/rowspans.rs b/crates/typst-layout/src/grid/rowspans.rs index 2c970784c..770d564d1 100644 --- a/crates/typst-layout/src/grid/rowspans.rs +++ b/crates/typst-layout/src/grid/rowspans.rs @@ -261,6 +261,11 @@ impl GridLayouter<'_> { while !self.regions.size.y.fits(row_group.height) && !in_last_with_offset( self.regions, + // Note that we consider that the exact same headers and footers will be + // added if we skip like this (blocking other rows from being laid out) + // due to orphan/widow prevention, which explains the usage of + // 'header_height' (include non-repeating but pending headers) rather + // than 'repeating_header_height'. self.header_height + self.footer_height, ) { @@ -409,8 +414,18 @@ impl GridLayouter<'_> { // // This will update the 'custom_backlog' vector with the // updated heights of the upcoming regions. + // + // We predict that header height will only include that of + // repeating headers, as we can assume non-repeating headers in + // the first region have been successfully placed, unless + // something didn't fit on the first region of the auto row, + // but we will only find that out after measurement, and if + // that happens, we discard the measurement and try again. let mapped_regions = self.regions.map(&mut custom_backlog, |size| { - Size::new(size.x, size.y - self.header_height - self.footer_height) + Size::new( + size.x, + size.y - self.repeating_header_height - self.footer_height, + ) }); // Callees must use the custom backlog instead of the current @@ -511,7 +526,26 @@ impl GridLayouter<'_> { .iter() .copied() .chain(std::iter::once(if breakable { - self.initial.y - self.header_height - self.footer_height + // Here we are calculating the available height for a + // rowspan from the top of the current region, so + // we have to use initial header heights (note that + // header height can change in the middle of the + // region). + // TODO: maybe cache this + // NOTE: it is safe to access 'lrows' here since + // 'breakable' can only be true outside of headers + // and unbreakable rows in general, so there is no risk + // of accessing an incomplete list of rows. + let initial_header_height = self.lrows + [..self.current_header_rows] + .iter() + .map(|row| match row { + Row::Frame(frame, _, _) => frame.height(), + Row::Fr(_, _, _) => Abs::zero(), + }) + .sum(); + + self.initial.y - initial_header_height - self.footer_height } else { // When measuring unbreakable auto rows, infinite // height is available for content to expand. @@ -523,11 +557,12 @@ impl GridLayouter<'_> { // rowspan's already laid out heights with the current // region's height and current backlog to ensure a good // level of accuracy in the measurements. - let backlog = self - .regions - .backlog - .iter() - .map(|&size| size - self.header_height - self.footer_height); + // + // Assume only repeating headers will survive starting at + // the next region. + let backlog = self.regions.backlog.iter().map(|&size| { + size - self.repeating_header_height - self.footer_height + }); heights_up_to_current_region.chain(backlog).collect::>() } else { @@ -544,7 +579,7 @@ impl GridLayouter<'_> { last = self .regions .last - .map(|size| size - self.header_height - self.footer_height); + .map(|size| size - self.repeating_header_height - self.footer_height); } else { // The rowspan started in the current region, as its vector // of heights in regions is currently empty. @@ -746,10 +781,10 @@ impl GridLayouter<'_> { simulated_regions.next(); disambiguator += 1; - // Subtract the initial header and footer height, since that's the - // height we used when subtracting from the region backlog's + // Subtract the repeating header and footer height, since that's + // the height we used when subtracting from the region backlog's // heights while measuring cells. - simulated_regions.size.y -= self.header_height + self.footer_height; + simulated_regions.size.y -= self.repeating_header_height + self.footer_height; } if let Some(original_last_resolved_size) = last_resolved_size { @@ -884,7 +919,11 @@ impl GridLayouter<'_> { let rowspan_simulator = RowspanSimulator::new( disambiguator, simulated_regions, - self.header_height, + // There can be no new headers or footers within a multi-page + // rowspan, since headers and footers are unbreakable, so + // assuming the repeating header height and footer height + // won't change is safe. + self.repeating_header_height, self.footer_height, ); @@ -968,7 +1007,8 @@ impl GridLayouter<'_> { { extra_amount_to_grow -= simulated_regions.size.y.max(Abs::zero()); simulated_regions.next(); - simulated_regions.size.y -= self.header_height + self.footer_height; + simulated_regions.size.y -= + self.repeating_header_height + self.footer_height; disambiguator += 1; } simulated_regions.size.y -= extra_amount_to_grow;