From cbbd7699e8c2832adb001d534c2c7e39ad7ed67e Mon Sep 17 00:00:00 2001 From: Bill Denney Date: Sat, 9 May 2026 13:55:47 -0400 Subject: [PATCH 1/5] Allow multi-line group labels to flow into suppressed rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a group column has a multi-line value (e.g. "B\nC") that is then suppressed on subsequent rows of the same group, the label now flows visually through the blanked cells like an HTML rowspan instead of forcing the first row of the group to be tall enough to fit every line on its own. Implementation: * `measure_row_heights_tbl()` now returns a per-cell height matrix. * New helper `.compute_page_row_heights()` resolves per-row heights for a page by initialising rows from the non-group max and walking group columns innermost-first, growing the first row of each span only when the label exceeds the span's cumulative height. Innermost-first means outer spans borrow inner-pass growth. * `paginate_rows()` now drives the page-fit decision with a per-page tentative recompute. This is required because span heights are non-monotone in row count (adding a row to an open span can leave the page total unchanged) and because the orphan case — the first row of a multi-row group landing alone on a page boundary — must size that row to fit the full label by itself. `committed_rh` snapshots heights after each successful append so the flush captures the orphan-correct values. * `drawDetails.tfl_table_grob()` reads per-page committed heights from the page spec, computes a span-end matrix, and passes the cumulative span height to `.draw_cell_text()` for non-suppressed group cells so the clip viewport extends across the span. `row_rule` lines that would slice through a flowing label are suppressed. Closes humanpred/writetfl/issues/29 Co-Authored-By: Claude Opus 4.7 (1M context) --- R/table_draw.R | 128 +++++++++---- R/table_pagelist.R | 26 +-- R/table_rows.R | 274 ++++++++++++++++++++-------- design/ARCHITECTURE.md | 64 +++++-- design/DECISIONS.md | 86 +++++++++ design/DESIGN.md | 47 ++++- man/build_table_grob.Rd | 2 +- man/measure_row_heights_tbl.Rd | 15 +- man/paginate_rows.Rd | 43 ++++- tests/testthat/test-row_span.R | 298 +++++++++++++++++++++++++++++++ tests/testthat/test-table_draw.R | 27 +-- tests/testthat/test-tfl_table.R | 35 ++-- 12 files changed, 876 insertions(+), 169 deletions(-) create mode 100644 tests/testthat/test-row_span.R diff --git a/R/table_draw.R b/R/table_draw.R index 9ae50b9..26e8edf 100644 --- a/R/table_draw.R +++ b/R/table_draw.R @@ -63,24 +63,25 @@ #' @keywords internal build_table_grob <- function(row_page, col_group_idx, n_group_cols, resolved_cols, tbl, - row_heights_in = NULL, - cont_row_h_in = NULL, - is_first_col_page = TRUE, - is_last_col_page = TRUE) { + cell_heights_in_mat = NULL, + cont_row_h_in = NULL, + is_first_col_page = TRUE, + is_last_col_page = TRUE) { # Subset to display columns for this page page_cols <- resolved_cols[col_group_idx] grid::gTree( - row_page = row_page, - col_group_idx = col_group_idx, - n_group_cols = n_group_cols, - page_cols = page_cols, - tbl = tbl, - row_heights_in = row_heights_in, # cached from paginate phase - cont_row_h_in = cont_row_h_in, # cached from paginate phase - is_first_col_page = is_first_col_page, # FALSE when prior col pages exist - is_last_col_page = is_last_col_page, # FALSE when more col pages follow - cl = "tfl_table_grob" + row_page = row_page, + col_group_idx = col_group_idx, + n_group_cols = n_group_cols, + page_cols = page_cols, + resolved_cols = resolved_cols, # full list, for span recompute + tbl = tbl, + cell_heights_in_mat = cell_heights_in_mat, # cached full matrix + cont_row_h_in = cont_row_h_in, # cached from paginate phase + is_first_col_page = is_first_col_page, # FALSE when prior col pages exist + is_last_col_page = is_last_col_page, # FALSE when more col pages follow + cl = "tfl_table_grob" ) } @@ -165,15 +166,38 @@ drawDetails.tfl_table_grob <- function(x, recording) { max(vapply(tbl$row_cont_msg, .cont_h, numeric(1L))) } - # Data row heights — prefer cached values - row_h_vec <- if (!is.null(x$row_heights_in) && - length(x$row_heights_in) >= (if (n_rows > 0L) max(rows) else 0L)) { - x$row_heights_in[rows] + group_vars <- tbl$group_vars + + # Precompute the per-page suppression matrix. Also used by the per-page + # row-height resolver and the row-rule suppression check below. + suppress_mat <- if (isTRUE(tbl$suppress_repeated_groups) && + length(group_vars) > 0L) { + .compute_cell_suppression(data, group_vars, rows) + } else NULL + + # Per-page row heights — prefer the heights that pagination committed; if + # absent, recompute from the cached cell-height matrix using the same + # span-aware algorithm pagination uses. As a final fallback, build a + # per-page matrix on the fly (covers grobs that were assembled outside + # the normal pipeline). + row_h_vec <- if (!is.null(row_page$row_heights_in) && + length(row_page$row_heights_in) == n_rows) { + row_page$row_heights_in + } else if (!is.null(x$cell_heights_in_mat) && !is.null(x$resolved_cols)) { + .compute_page_row_heights( + x$cell_heights_in_mat, rows, x$resolved_cols, group_vars, suppress_mat + ) } else { - vapply(rows, function(i) { - max(vapply(page_cols, function(cs) { - s <- .fmt_cell(data[[cs$col]][i], na_str) - gp_c <- .gp_with_lineheight(.resolve_table_cell_gp(gp_tbl, cs$is_group_col), lh) + # Per-page fallback: build a small matrix for just the rows on this page + # using page_cols, then apply the algorithm. + fallback_mat <- matrix(0, nrow = n_rows, ncol = length(page_cols)) + for (j in seq_along(page_cols)) { + cs <- page_cols[[j]] + gp_c <- .gp_with_lineheight( + .resolve_table_cell_gp(gp_tbl, cs$is_group_col), lh + ) + for (ri in seq_len(n_rows)) { + s <- .fmt_cell(data[[cs$col]][rows[[ri]]], na_str) disp_s <- if (cs$wrap && !is.null(cs$width_in)) { .wrap_text(s, cs$width_in - h_lft_in - h_rgt_in, gp_c) } else s @@ -181,17 +205,35 @@ drawDetails.tfl_table_grob <- function(x, recording) { grob <- grid::textGrob(disp_s, gp = gp_c) h1 <- .height_in(grid::grobHeight(grob)) h2 <- nlines * .height_in(grid::stringHeight("M")) - max(h1, h2) - }, numeric(1L))) + v_pad_in - }, numeric(1L)) + fallback_mat[ri, j] <- max(h1, h2) + v_pad_in + } + } + .compute_page_row_heights( + fallback_mat, seq_len(n_rows), page_cols, group_vars, suppress_mat + ) } # Precompute group sizes — group rules are suppressed for single-row groups - group_vars <- tbl$group_vars group_sizes <- if (tbl$group_rule && length(group_vars) > 0L) { .compute_group_sizes(data, group_vars) } else NULL + # Precompute span ends per group column on this page so non-suppressed group + # cells can be drawn with a clip viewport that covers the whole span. + # span_end_mat[ri, g] is the last row index in the same span as ri for + # group column g; only meaningful when suppress_mat[ri, g] == FALSE. + span_end_mat <- if (!is.null(suppress_mat)) { + se <- matrix(NA_integer_, nrow = n_rows, ncol = length(group_vars)) + for (g in seq_along(group_vars)) { + starts <- which(!suppress_mat[, g]) + if (length(starts) > 0L) { + ends <- c(starts[-1L] - 1L, n_rows) + se[starts, g] <- ends + } + } + se + } else NULL + # --- Build row y-positions (top-to-bottom, in inches from top of vp) --- # In grid: y=0 is bottom, y=1 is top. # We track y_top_in = distance from TOP of viewport (increasing downward). @@ -240,11 +282,7 @@ drawDetails.tfl_table_grob <- function(x, recording) { } # Group boundaries (track previous group key to detect changes) - grp_starts <- row_page$group_starts - # Precompute which group cells to suppress (hierarchical: outer change resets inner) - suppress_mat <- if (tbl$suppress_repeated_groups && length(group_vars) > 0L) { - .compute_cell_suppression(data, group_vars, rows) - } else NULL + grp_starts <- row_page$group_starts # Data row background fill setup data_row_gp <- .resolve_table_gp(gp_tbl, "data_row") @@ -294,10 +332,23 @@ drawDetails.tfl_table_grob <- function(x, recording) { raw_val <- data[[cs$col]][i] cell_str <- .fmt_cell(raw_val, na_str) - # Group repeat suppression + # Group repeat suppression and span detection + clip_h <- row_h if (!is.null(suppress_mat) && cs$is_group_col) { col_pos <- match(cs$col, group_vars, nomatch = 0L) - if (col_pos > 0L && suppress_mat[[ri, col_pos]]) cell_str <- "" + if (col_pos > 0L) { + if (suppress_mat[[ri, col_pos]]) { + cell_str <- "" + } else if (!is.null(span_end_mat)) { + # Non-suppressed group cell: clip to the full span height so the + # (possibly multi-line) label can flow into the suppressed rows + # below it (HTML rowspan-style). + ri_end <- span_end_mat[[ri, col_pos]] + if (!is.na(ri_end) && ri_end > ri) { + clip_h <- sum(row_h_vec[ri:ri_end]) + } + } + } } # Resolve cell gpar (with lineheight applied) @@ -312,15 +363,20 @@ drawDetails.tfl_table_grob <- function(x, recording) { .draw_cell_text(display_str, cs$align, col_x_left[[j]], col_x_right[[j]], - y_cursor, row_h, vp_w, vp_h, + y_cursor, clip_h, vp_w, vp_h, h_lft_in, h_rgt_in, v_top_in, cell_gp, cs$width_in) } y_cursor <- y_cursor + row_h - # Row rule between data rows (not after last) - if (tbl$row_rule && ri < n_rows) { + # Row rule between data rows (not after last). Suppress the rule when the + # next row is part of a multi-row group span starting at or before this + # row — drawing a horizontal line through a label that flows downward + # would visually slice the label. + rule_inside_span <- !is.null(suppress_mat) && ri < n_rows && + any(suppress_mat[ri + 1L, ]) + if (tbl$row_rule && ri < n_rows && !rule_inside_span) { rule_gp <- .resolve_table_gp(gp_tbl, "row_rule") y_rule_npc <- 1 - y_cursor / vp_h x_left_npc <- col_x_left[[1L]] / vp_w diff --git a/R/table_pagelist.R b/R/table_pagelist.R index f080273..d32065e 100644 --- a/R/table_pagelist.R +++ b/R/table_pagelist.R @@ -187,7 +187,7 @@ tfl_table_to_pagelist <- function(tbl, pg_width, pg_height, dots, tbl$line_height) } else 0 - row_heights <- measure_row_heights_tbl( + cell_h_mat <- measure_row_heights_tbl( tbl$data, resolved_cols, tbl$gp, tbl$cell_padding, tbl$na_string, tbl$line_height, tbl$max_measure_rows ) @@ -206,8 +206,10 @@ tfl_table_to_pagelist <- function(tbl, pg_width, pg_height, dots, # --- Step 6: Paginate rows --- row_pages <- paginate_rows( - tbl$data, row_heights, cont_row_h, header_row_h, ch, - tbl$group_vars, tbl$row_cont_msg, tbl$group_rule + tbl$data, cell_h_mat, resolved_cols, tbl$group_vars, + cont_row_h, header_row_h, ch, + tbl$row_cont_msg, tbl$group_rule, + suppress_repeated_groups = isTRUE(tbl$suppress_repeated_groups) ) # --- Step 7: Assemble page specs --- @@ -219,15 +221,15 @@ tfl_table_to_pagelist <- function(tbl, pg_width, pg_height, dots, for (rp in seq_len(n_rp)) { for (cg in seq_len(n_cg)) { grob <- build_table_grob( - row_page = row_pages[[rp]], - col_group_idx = col_groups[[cg]], - n_group_cols = n_group_cols, - resolved_cols = resolved_cols, - tbl = tbl, - row_heights_in = row_heights, - cont_row_h_in = cont_row_h, - is_first_col_page = (cg == 1L), - is_last_col_page = (cg == n_cg) + row_page = row_pages[[rp]], + col_group_idx = col_groups[[cg]], + n_group_cols = n_group_cols, + resolved_cols = resolved_cols, + tbl = tbl, + cell_heights_in_mat = cell_h_mat, + cont_row_h_in = cont_row_h, + is_first_col_page = (cg == 1L), + is_last_col_page = (cg == n_cg) ) page_spec <- list(content = grob) pages[[idx]] <- page_spec diff --git a/R/table_rows.R b/R/table_rows.R index a88fd38..7d15331 100644 --- a/R/table_rows.R +++ b/R/table_rows.R @@ -1,29 +1,39 @@ # table_rows.R — Row height measurement and group-aware row pagination # # Functions: -# measure_row_heights_tbl() — memoised per-row height measurement -# paginate_rows() — split rows into pages respecting group boundaries +# measure_row_heights_tbl() — per-cell height matrix +# .compute_page_row_heights() — resolve per-row heights with group spanning +# paginate_rows() — split rows into pages (span-aware) # --------------------------------------------------------------------------- -# measure_row_heights_tbl() — memoised row height measurement +# measure_row_heights_tbl() — per-cell height matrix # --------------------------------------------------------------------------- -#' Measure the rendered height of each data row in inches +#' Measure the rendered height of each table cell in inches #' #' Must be called while a viewport is active. #' Uses a memoised string-height function to avoid re-measuring repeated values. #' -#' @return Numeric vector of row heights in inches (length = nrow(data)). +#' Returns a matrix of cell heights (rows = data rows, cols = resolved_cols). +#' Each entry is the rendered height of that cell in inches, **including the +#' top + bottom cell padding (`v_pad_in`)** so that the per-row height is +#' simply `max(cell_h_mat[i, ])` without further adjustment. +#' +#' @param max_measure_rows Maximum number of rows to measure individually. +#' Non-sampled rows take the per-column max of the sampled rows (a +#' conservative estimate that mirrors prior behaviour). +#' @return Numeric matrix `[nrow(data) × length(resolved_cols)]` of inches. #' @keywords internal measure_row_heights_tbl <- function(data, resolved_cols, gp_tbl, cell_padding, na_string, line_height, max_measure_rows) { n_rows <- nrow(data) + n_cols <- length(resolved_cols) v_pad_in <- .height_in(cell_padding[["top"]]) + .height_in(cell_padding[["bottom"]]) h_lft_in <- .width_in(cell_padding[["left"]]) h_rgt_in <- .width_in(cell_padding[["right"]]) - # Memoised height function: (string, gp_key) -> height_in + # Memoised per-cell-text-height function: (string, gp_key) -> height_in memo <- new.env(hash = TRUE, parent = emptyenv()) .memo_str_height <- function(s, gp_key, gp) { key <- paste0(gp_key, "\x01", s) @@ -48,35 +58,140 @@ measure_row_heights_tbl <- function(data, resolved_cols, gp_tbl, cell_padding, seq_len(n_rows) } - # Measure sampled rows - sampled_heights <- vapply(sample_rows, function(i) { - max(vapply(resolved_cols, function(cs) { + # Build the matrix. Iterate column-major so we can resolve gpar once per + # column rather than once per cell. + cell_h_mat <- matrix(0, nrow = n_rows, ncol = n_cols) + for (j in seq_len(n_cols)) { + cs <- resolved_cols[[j]] + base_gp <- .resolve_table_cell_gp(gp_tbl, cs$is_group_col) + cell_gp <- .gp_with_lineheight(base_gp, line_height) + gp_key <- paste0(if (cs$is_group_col) "group_col" else "data_row", + "_lh", line_height) + avail_w <- if (!is.null(cs$width_in)) cs$width_in - h_lft_in - h_rgt_in + else NA_real_ + + for (i in sample_rows) { cell_str <- .fmt_cell(data[[cs$col]][i], na_string) - base_gp <- .resolve_table_cell_gp(gp_tbl, cs$is_group_col) - cell_gp <- .gp_with_lineheight(base_gp, line_height) - gp_key <- paste0(if (cs$is_group_col) "group_col" else "data_row", - "_lh", line_height) - # For wrap-eligible columns, wrap the text to the column width first display_str <- if (cs$wrap && !is.null(cs$width_in)) { - avail_w <- cs$width_in - h_lft_in - h_rgt_in .wrap_text(cell_str, avail_w, cell_gp) } else { cell_str } - # Count lines for conservative estimate - nlines <- max(1L, length(strsplit(display_str, "\n", fixed = TRUE)[[1L]])) - h_grob <- .memo_str_height(display_str, gp_key, cell_gp) - h_line <- nlines * .height_in(grid::stringHeight("M")) - max(h_grob, h_line) - }, numeric(1L))) + v_pad_in - }, numeric(1L)) - - max_sampled <- max(sampled_heights) - - # Build full height vector - heights <- rep(max_sampled, n_rows) - heights[sample_rows] <- sampled_heights - heights + nlines <- max(1L, length(strsplit(display_str, "\n", fixed = TRUE)[[1L]])) + h_grob <- .memo_str_height(display_str, gp_key, cell_gp) + h_line <- nlines * .height_in(grid::stringHeight("M")) + cell_h_mat[i, j] <- max(h_grob, h_line) + v_pad_in + } + } + + # For non-sampled rows, fill each column with the per-column max-of-sampled + # so that conservative heights are preserved. + if (length(sample_rows) < n_rows) { + not_sampled <- setdiff(seq_len(n_rows), sample_rows) + if (length(sample_rows) > 0L) { + col_max <- apply(cell_h_mat[sample_rows, , drop = FALSE], 2L, max) + } else { + col_max <- rep(0, n_cols) # nocov + } + for (j in seq_len(n_cols)) { + cell_h_mat[not_sampled, j] <- col_max[[j]] + } + } + + cell_h_mat +} + +# --------------------------------------------------------------------------- +# .compute_page_row_heights() — resolve per-row heights for one page +# --------------------------------------------------------------------------- + +# Compute the per-row heights for the rows on a single page, allowing a +# multi-line group label to flow through the suppressed cells below it. +# +# Algorithm: +# 1. Initialise row_h[ri] = max of cell_h_mat over non-group columns at row +# page_rows[ri]. This is the height the row needs *ignoring* the group +# column's potentially multi-line value. +# 2. Walk group_vars from innermost (last) to outermost (first). For each +# group column g, find spans of consecutive page rows where +# suppress_mat[ri_start, g] == FALSE and suppress_mat[ri, g] == TRUE for +# ri in (ri_start, ri_end]. For each span, if the label cell at +# ri_start is taller than sum(row_h[ri_start..ri_end]), grow row_h[ri_start] +# by the deficit (text is top-aligned so growth on the first row keeps +# the label visually anchored). +# +# The early-exit short-circuits when there are no group columns or +# suppression is disabled, returning the simple per-row max. In that case +# the algorithm is a no-op compared to today's behaviour. +# +# @param cell_h_mat Full matrix of cell heights including v_pad (inches). +# @param page_rows Integer vector of data-row indices visible on this page. +# @param resolved_cols List of resolved column specs (full list, not just page). +# @param group_vars Character vector of group column names. +# @param suppress_mat Logical matrix [length(page_rows) × length(group_vars)] +# from .compute_cell_suppression(), or NULL when suppression is disabled. +# @return Numeric vector of length(page_rows), heights in inches. +#' @keywords internal +.compute_page_row_heights <- function(cell_h_mat, page_rows, resolved_cols, + group_vars, suppress_mat) { + n_pr <- length(page_rows) + if (n_pr == 0L) return(numeric(0L)) + + n_grp <- length(group_vars) + + # Early exit when there are no group columns or suppression is disabled. + # In both cases, every cell in every row is rendered in full (no flow into + # blanked cells), so the row height is the per-row max over ALL columns. + if (n_grp == 0L || is.null(suppress_mat)) { + return(as.numeric(apply(cell_h_mat[page_rows, , drop = FALSE], 1L, max))) + } + + # Map each resolved column to its column index in cell_h_mat + col_names <- vapply(resolved_cols, function(cs) cs$col, character(1L)) + is_group <- vapply(resolved_cols, function(cs) isTRUE(cs$is_group_col), + logical(1L)) + + # Initial per-row heights: max over non-group columns. Group columns are + # excluded here because their height contribution is handled below in the + # span-aware growth pass. If every column is a group column (degenerate / + # no body data) fall back to the full row max so we still produce a sane + # height. + non_group_idx <- which(!is_group) + row_h <- if (length(non_group_idx) > 0L) { + apply(cell_h_mat[page_rows, non_group_idx, drop = FALSE], 1L, max) + } else { + apply(cell_h_mat[page_rows, , drop = FALSE], 1L, max) # nocov + } + # apply() drops to a length-n_pr vector; ensure shape for n_pr == 1 + row_h <- as.numeric(row_h) + + # Resolve each group_var to its column index in cell_h_mat + group_col_idx <- match(group_vars, col_names) + + # Process from innermost to outermost so outer-group spans can borrow space + # already contributed by inner-group growth. + for (g in rev(seq_len(n_grp))) { + j_mat <- group_col_idx[[g]] + if (is.na(j_mat)) next # safety; shouldn't happen + + # Span starts: rows where suppress_mat[, g] is FALSE. A span runs from + # one start (inclusive) to the next start - 1 (or n_pr at the end). + starts <- which(!suppress_mat[, g]) + if (length(starts) == 0L) next # entire column suppressed (shouldn't happen) + ends <- c(starts[-1L] - 1L, n_pr) + + for (s_idx in seq_along(starts)) { + ri_start <- starts[[s_idx]] + ri_end <- ends[[s_idx]] + label_h <- cell_h_mat[page_rows[[ri_start]], j_mat] + avail <- sum(row_h[ri_start:ri_end]) + if (label_h > avail + 1e-9) { + row_h[[ri_start]] <- row_h[[ri_start]] + (label_h - avail) + } + } + } + + row_h } # --------------------------------------------------------------------------- @@ -85,60 +200,79 @@ measure_row_heights_tbl <- function(data, resolved_cols, gp_tbl, cell_padding, #' Split rows into pages, respecting group boundaries #' +#' Uses a per-page tentative recompute of `.compute_page_row_heights()` so that +#' span-aware row heights drive the page-fit decision. Two non-obvious +#' properties of this scheme are preserved by the algorithm: +#' +#' * Adding a row to an existing group-span on the current page may leave the +#' total page height unchanged (the span absorbs deficit that previously +#' inflated earlier rows), so more rows can fit than a per-row scalar sum +#' would predict. +#' * When only the first row of a multi-row group lands on the current page +#' (group orphan), that row's span on the page is length 1 and the row is +#' grown to fit the full label height. `committed_rh` snapshots heights +#' after each successful append, so the orphan-correct heights are what gets +#' flushed to the page spec. +#' #' @param data Data frame. -#' @param row_heights_in Numeric vector of row heights in inches. +#' @param cell_h_mat Per-cell height matrix from `measure_row_heights_tbl()`. +#' @param resolved_cols Full list of resolved column specs (used to identify +#' non-group columns). +#' @param group_vars Character vector of group column names. #' @param cont_row_h Height of a continuation-marker row in inches. #' @param header_row_h Height of the column header row (0 if suppressed). #' @param content_height_in Available content height per page. -#' @param group_vars Character vector of group column names. #' @param row_cont_msg Text for continuation-marker rows. -#' @param group_rule Logical — are group rules drawn? -#' @return A list of row-page specs (see internal structure below). +#' @param group_rule Logical — are group rules drawn? (Reserved for future +#' use; currently does not affect pagination because rules are 0-height.) +#' @param suppress_repeated_groups Logical, from `tbl$suppress_repeated_groups`. +#' @return A list of row-page specs, each with `$rows`, `$is_cont_top`, +#' `$is_cont_bottom`, `$group_starts`, and `$row_heights_in` (the committed +#' per-row heights for that page in inches). #' @keywords internal -paginate_rows <- function(data, row_heights_in, cont_row_h, header_row_h, - content_height_in, group_vars, row_cont_msg, - group_rule) { - n_rows <- nrow(data) - n_grp <- length(group_vars) +paginate_rows <- function(data, cell_h_mat, resolved_cols, group_vars, + cont_row_h, header_row_h, content_height_in, + row_cont_msg, group_rule, + suppress_repeated_groups = TRUE) { + n_rows <- nrow(data) - # Identify group boundaries: rows that start a new group + # Group boundaries in the *full* data — used for the page-spec $group_starts + # field that the drawing code consults for group-rule placement. group_starts <- .compute_group_starts(data, group_vars) - pages <- list() - cur_rows <- integer(0L) - cur_h <- header_row_h - # Rule heights: rules are drawn within existing row boundaries, 0 extra height - # (they render at the row boundary, not consuming additional space) + pages <- list() + cur_rows <- integer(0L) + committed_rh <- numeric(0L) # heights for cur_rows after last successful add + is_cont_top <- FALSE - flush_page <- function(rows, is_cont_top, is_cont_bottom) { + flush_page <- function(rows, row_heights_in, is_cont_top, is_cont_bottom) { pages[[length(pages) + 1L]] <<- list( rows = rows, is_cont_top = is_cont_top, is_cont_bottom = is_cont_bottom, - group_starts = intersect(group_starts, rows) + group_starts = intersect(group_starts, rows), + row_heights_in = row_heights_in ) } i <- 1L - is_cont_top <- FALSE # does this page start with a (continued) row? - while (i <= n_rows) { - rh <- row_heights_in[[i]] - - # Would this row fit? - extra_h <- rh - if (is_cont_top) extra_h <- extra_h # cont row already counted below - - needs_group_rule <- group_rule && i %in% group_starts && length(cur_rows) > 0L - # Group rule uses no additional height (drawn at boundary) - - # Does adding this row overflow? - if (cur_h + extra_h + cont_row_h > content_height_in + 1e-6 && length(cur_rows) > 0L) { + candidate <- c(cur_rows, i) + sup <- if (suppress_repeated_groups && length(group_vars) > 0L) { + .compute_cell_suppression(data, group_vars, candidate) + } else NULL + rh <- .compute_page_row_heights(cell_h_mat, candidate, resolved_cols, + group_vars, sup) + total <- header_row_h + + (if (is_cont_top) cont_row_h else 0) + + sum(rh) + + cont_row_h # reserve bottom continuation marker + if (total > content_height_in + 1e-6 && length(cur_rows) > 0L) { # Warn whenever a group is split across pages (row i and the last row - # on the current page belong to the same group) - if (length(group_vars) > 0L && length(cur_rows) > 0L) { - last_in_page <- cur_rows[length(cur_rows)] + # on the current page belong to the same group). + if (length(group_vars) > 0L) { + last_in_page <- cur_rows[[length(cur_rows)]] same_group <- all(vapply(group_vars, function(gv) { identical(data[[gv]][last_in_page], data[[gv]][i]) }, logical(1L))) @@ -150,23 +284,21 @@ paginate_rows <- function(data, row_heights_in, cont_row_h, header_row_h, } } - flush_page(cur_rows, is_cont_top, is_cont_bottom = TRUE) + flush_page(cur_rows, committed_rh, is_cont_top, is_cont_bottom = TRUE) - # Re-init next page with cont_top marker cur_rows <- integer(0L) - cur_h <- header_row_h + cont_row_h # top cont row + committed_rh <- numeric(0L) is_cont_top <- TRUE - - next # restart loop iteration to re-add row i + next # re-process row i on a fresh page } - cur_rows <- c(cur_rows, i) - cur_h <- cur_h + rh - i <- i + 1L + cur_rows <- candidate + committed_rh <- rh + i <- i + 1L } if (length(cur_rows) > 0L) { - flush_page(cur_rows, is_cont_top, is_cont_bottom = FALSE) + flush_page(cur_rows, committed_rh, is_cont_top, is_cont_bottom = FALSE) } pages diff --git a/design/ARCHITECTURE.md b/design/ARCHITECTURE.md index 19a5984..f946d9c 100644 --- a/design/ARCHITECTURE.md +++ b/design/ARCHITECTURE.md @@ -148,27 +148,42 @@ export_tfl(x = tfl_table_obj, ...) [exported] │ paginate_cols(...) ├── [scratch device + outer_vp] measure heights: │ .measure_header_row_height() — table_utils.R - │ measure_row_heights_tbl() — table_rows.R + │ measure_row_heights_tbl() → cell_h_mat — table_rows.R + │ Per-cell height matrix [nrow × ncol]; each entry includes + │ the v_pad_in (top + bottom padding). This is the input to + │ the per-page row-height resolver below. │ .measure_cont_row_height() — table_utils.R ├── paginate_rows(...) — table_rows.R + │ Span-aware pagination: per-page tentative recompute via + │ .compute_page_row_heights() to drive page-fit decisions. Each + │ committed page spec carries $row_heights_in (orphan-correct). └── for rp × cg: build_table_grob(row_page, col_group_idx, — table_draw.R n_group_cols, resolved_cols, tbl, - row_heights_in, cont_row_h_in, + cell_heights_in_mat, cont_row_h_in, is_first_col_page, is_last_col_page) → gTree of class "tfl_table_grob" [grob passed as x$content to export_tfl_page()] drawDetails.tfl_table_grob(x, recording) — table_draw.R - ├── Reuse or recompute: header_row_h, cont_row_h, row_h_vec + ├── Reuse or recompute: header_row_h, cont_row_h, suppress_mat + ├── row_h_vec ← row_page$row_heights_in (committed by pagination), or + │ recompute via .compute_page_row_heights(cell_heights_in_mat, …). + ├── span_end_mat (per group column, per row) — last row index in the + │ span starting at each non-suppressed row; lets non-suppressed group + │ cells be drawn with a clip viewport spanning the full span height + │ so multi-line labels flow into suppressed rows below (rowspan-style). ├── Draw column header row (.draw_header_row) ├── Draw col_header_rule (grid.lines) ├── Draw top continuation row (.draw_cont_row) ├── for each data row: │ group rule before row (grid.lines) - │ draw each cell (.draw_cell_text) - │ row rule after row (grid.lines, if row_rule && not last) + │ draw each cell (.draw_cell_text); for non-suppressed + │ group cells whose span > 1 row, pass span_h instead of row_h + │ so the clip viewport extends over the whole span. + │ row rule after row (grid.lines), suppressed when row ri+1 + │ has any suppressed group column (rule would slice a label). ├── group_rule_after_last (grid.lines) ├── Draw bottom continuation row (.draw_cont_row) ├── Draw row_header_sep (grid.lines) @@ -330,8 +345,8 @@ export_tfl(x = list_of_table1, ...) [exported] | `R/reexports.R` | `%||%` from rlang | | `R/tfl_table.R` | `tfl_colspec()`, `tfl_table()`, `print.tfl_table()`, `.check_named_subset()` | | `R/table_columns.R` | `resolve_col_specs()`, `compute_col_widths()`, `.apply_col_wrapping()`, `paginate_cols()` | -| `R/table_rows.R` | `measure_row_heights_tbl()`, `paginate_rows()` | -| `R/table_draw.R` | `build_table_grob()`, `drawDetails.tfl_table_grob()`, `.draw_header_row()`, `.draw_cont_row()`, `.draw_cell_text()` | +| `R/table_rows.R` | `measure_row_heights_tbl()` (returns per-cell matrix), `.compute_page_row_heights()`, `paginate_rows()` | +| `R/table_draw.R` | `build_table_grob()`, `drawDetails.tfl_table_grob()`, `.compute_cell_suppression()`, `.draw_header_row()`, `.draw_cont_row()`, `.draw_cell_text()` | | `R/table_pagelist.R` | `tfl_table_to_pagelist()`, `compute_table_content_area()` | | `R/sub_tfl.R` | `.compute_sub_tfl_groups()`, `.format_sub_tfl_caption()`, `.apply_sub_tfl_caption()`, `.strip_sub_tfl_cols()`, `.resolve_col_label()` | | `R/table_utils.R` | `.make_outer_vp()`, `.width_in()`, `.height_in()`, `.measure_header_row_height()`, `.measure_cont_row_height()`, `.gp_with_lineheight()`, `.compute_group_starts()`, `.compute_group_sizes()`, `.collect_col_strings()`, `.fmt_cell()`, `.fmt_cell_vec()`, `.measure_max_string_width()`, `.resolve_table_gp()`, `.resolve_table_cell_gp()`, `.default_align()`, `.wrap_text()` | @@ -607,16 +622,37 @@ paginate_cols(col_indices, col_widths_in, group_col_indices, ``` measure_row_heights_tbl(data, resolved_cols, gp_tbl, cell_padding, na_string, line_height, max_measure_rows) - → numeric vector of length nrow(data), heights in inches + → numeric MATRIX [nrow(data) × length(resolved_cols)] of heights in inches + Each entry includes the v_pad_in (top + bottom padding) so that + per-row max(matrix[i, ]) is the row height when no spanning happens. Uses memoised string-height to avoid re-measuring repeated values. max_measure_rows: sample the longest rows to cap measurement cost. - -paginate_rows(data, row_heights, cont_row_h, header_row_h, - avail_h, group_vars, row_cont_msg, group_rule) + Non-sampled rows take the per-column max-of-sampled value. + +.compute_page_row_heights(cell_h_mat, page_rows, resolved_cols, + group_vars, suppress_mat) + → numeric vector of length(page_rows) + Span-aware per-page resolver. Initialises row_h[ri] = max over + non-group columns of cell_h_mat[page_rows[ri], col]. Then for each + group column from innermost (last) to outermost (first), finds spans + in suppress_mat and grows row_h[ri_start] by any deficit between the + label height (cell_h_mat[page_rows[ri_start], col_g]) and the sum of + the span's row heights. Innermost-first ensures outer groups can + borrow the space inner groups already pushed for. Early-exit when + no group columns or suppress_mat = NULL — returns per-row max over + *all* columns (no flow when suppression is disabled). + +paginate_rows(data, cell_h_mat, resolved_cols, group_vars, + cont_row_h, header_row_h, content_height_in, + row_cont_msg, group_rule, suppress_repeated_groups) → list of row_page structs: - { rows, is_cont_top, is_cont_bottom, group_starts } - Group-aware: tries to keep group blocks together; places - continuation-marker rows at page boundaries. + { rows, is_cont_top, is_cont_bottom, group_starts, row_heights_in } + Span-aware pagination via per-page tentative recompute: each candidate + row addition recomputes suppress_mat and .compute_page_row_heights for + c(cur_rows, i) and checks the total against content_height_in. When + overflow is detected, the previously committed (cur_rows, committed_rh) + pair is flushed — committed_rh captures the orphan-correct heights for + the row that landed alone at the page boundary. ``` --- diff --git a/design/DECISIONS.md b/design/DECISIONS.md index 19d9617..6a0c71f 100644 --- a/design/DECISIONS.md +++ b/design/DECISIONS.md @@ -900,3 +900,89 @@ bug being fixed. Users who want the old behavior can pass grob check), `R/table_columns.R` (`compute_col_widths()` per-column + total-width checks), `R/table_pagelist.R` (thread the arg through), `R/export_tfl.R` (`@inheritDotParams`). + +--- + +## D-40: Group-label rowspan-style flow (issue #29) + +**Decision:** When a group column's value is multi-line, do not force its +row to be tall enough to fit the whole label. Instead let the label flow +through the suppressed (blanked) cells in the rows below it the same way +HTML `` reserves a single visually-spanning cell. + +The implementation has three parts: + +1. **Per-cell height matrix.** `measure_row_heights_tbl()` now returns a + `[nrow(data) × length(resolved_cols)]` matrix of cell heights instead + of a per-row scalar vector. Each entry includes `v_pad_in` so the + per-row max equals the row height when no spanning happens. + +2. **Span-aware per-page resolver `.compute_page_row_heights()`.** Given + the matrix, the rows on a page, the resolved columns, the group + variable list, and the per-page suppression matrix, walk group columns + from innermost to outermost and grow the first row of each span by any + deficit between the label height and the sum of the span's row + heights. Innermost-first lets outer spans borrow inner-pass growth. + First-row growth matches the label's top-anchored alignment in + `.draw_cell_text()`. + +3. **Per-page tentative recompute in `paginate_rows()`.** The fit check + uses `sum(.compute_page_row_heights(c(cur_rows, i), …))` rather than a + running scalar accumulator. This is required because span heights are + non-monotone in row count (adding a row to an open span can leave the + total unchanged or even shrink earlier-row contributions as the span's + `avail` grows), and because the orphan case — when only the first row + of a multi-row group lands on the current page — must size that row + to fit the full label by itself. `committed_rh` snapshots heights + after each successful append so the flush at overflow uses the + orphan-correct heights. + +**User need (from issue #29):** "If there is a grouping column in a table +which will have empty rows under it, and the grouping column has multiple +rows of text, do not reserve space for the grouping column more than is +required. Allow it to flow into the empty space below like a rowspan." +Plus: "if there is a grouping column on one page and different behavior +on the next page... the handling of the reserved height for column A will +differ between the pages." + +**Suppression-aware row rule:** the existing `row_rule` between data rows +is suppressed when the next row is part of a multi-row group span (any +suppressed group column on row `ri+1`). A horizontal line slicing +through a label that flows downward would visually fragment it; HTML +rowspan also has no internal borders. Group rules and +`group_rule_after_last` are unaffected because they only fire at group +boundaries (which are also span boundaries). + +**Alternatives considered and rejected:** +- *Distribute deficit evenly across rows of the span* — leaves wasted + space below the (top-anchored) label in early rows. First-row growth + is both visually minimal and consistent with the alignment. +- *Vertically centre labels in the span* — aesthetic change orthogonal + to the height-management problem. Not in scope; can be added later + via a `tfl_table` argument if a use case appears. +- *Span-aware row-fill rectangles* — currently each row paints its own + background. Painting a multi-row block under a span would be visually + consistent with the label flow but conflicts with stripe shading + (`fill_by = "row"`). Out of scope; the per-row stripe is consistent + with the body cells still being one-row each. +- *Cache only the per-page committed heights, not the full matrix* — + pagination needs the matrix for its tentative recompute, drawing + needs it for the fallback path. Caching both the matrix on the grob + and the committed heights on each page spec is cheap (matrices are + small) and lets each consumer pick whichever is cheaper. + +**Files touched:** +- Modified: `R/table_rows.R` (matrix output, `.compute_page_row_heights()`, + span-aware `paginate_rows()`); `R/table_draw.R` (per-page row-h source, + span-end matrix, span-aware clipping height in `.draw_cell_text()` calls, + row-rule suppression predicate, renamed `cell_heights_in_mat` cache); + `R/table_pagelist.R` (pass the matrix and `suppress_repeated_groups`). +- New tests: `tests/testthat/test-row_span.R` exercising the algorithm, + pagination's free-row property, the orphan case, the per-page reset, and + end-to-end rendering. + +**Backward compatibility:** no exported API changes. Existing tables +that did not use multi-line group labels render identically (same row +heights, same pagination). Tables that did use them now render in less +vertical space, which may *increase* the number of rows on a page (and +correspondingly *decrease* the total page count). diff --git a/design/DESIGN.md b/design/DESIGN.md index 3d0e6fe..484b212 100644 --- a/design/DESIGN.md +++ b/design/DESIGN.md @@ -172,14 +172,57 @@ final output) without writing to disk. --- -## Why store `row_heights_in` and `cont_row_h_in` in the gTree? +## Why store `cell_heights_in_mat` and `cont_row_h_in` in the gTree? The `drawDetails` method is called by `grid` at render time, potentially long -after paginate time. Pre-computing row heights during pagination (when a +after paginate time. Pre-computing cell heights during pagination (when a scratch device is already open) and caching them in the grob avoids opening another device at draw time and ensures layout consistency: the heights used for pagination and the heights used for drawing are identical. +The grob caches the *full* per-cell height matrix rather than per-row +scalars because the per-row height for a given page depends on which other +rows are on that page (suppression resets per page; multi-row group spans +absorb deficit jointly). Each page spec separately carries its committed +`row_heights_in` so drawing reads the exact same heights pagination decided +on, while the matrix supports the fallback path if that cache is missing. + +--- + +## Why a per-cell height matrix instead of per-row scalars? + +Issue #29 introduced HTML-`rowspan`-style flow for multi-line group labels. +The label of a group column should be allowed to flow downward through the +suppressed cells beneath it, so a 2-line label spanning two single-line +rows costs 2 lines, not 3. Implementing that requires answering, for any +(row, column) pair, "what is the natural height of this cell ignoring its +neighbours?" — i.e. a per-cell measurement. Per-row scalars cannot +represent this without losing the column dimension. + +The matrix also lets the per-page row-height resolver recompute heights +when suppression boundaries shift between pages (e.g. when a group is +split across pages and the label re-appears on the second page), without +re-doing the expensive `grobHeight()` measurements. + +--- + +## Why innermost-group first in `.compute_page_row_heights()`? + +Outer-group spans are always supersets of (or equal to) inner-group spans +because `.compute_cell_suppression()` resets the inner `last_val` whenever +any outer column changes. Processing inner spans first means the row +heights already absorb whatever growth the inner labels demanded by the +time outer spans are evaluated; the outer label "borrows" any extra space +the inner pass added. Reversing the order would compute outer growth +against pre-inner heights and then over-grow when inner labels later need +more space. + +The deficit always lands on the *first row* of the span because +`.draw_cell_text()` anchors labels to the top-of-cell (`just = c(.., "top")`). +Growing a later row in the span would not make the top-anchored label any +more visible — extra space would simply appear below the label inside an +already-drawn row. + --- ## Why use rotated side labels for `col_cont_msg` instead of `footer_center`? diff --git a/man/build_table_grob.Rd b/man/build_table_grob.Rd index 862dc1a..eced8aa 100644 --- a/man/build_table_grob.Rd +++ b/man/build_table_grob.Rd @@ -10,7 +10,7 @@ build_table_grob( n_group_cols, resolved_cols, tbl, - row_heights_in = NULL, + cell_heights_in_mat = NULL, cont_row_h_in = NULL, is_first_col_page = TRUE, is_last_col_page = TRUE diff --git a/man/measure_row_heights_tbl.Rd b/man/measure_row_heights_tbl.Rd index 22313ce..e67ecc9 100644 --- a/man/measure_row_heights_tbl.Rd +++ b/man/measure_row_heights_tbl.Rd @@ -2,7 +2,7 @@ % Please edit documentation in R/table_rows.R \name{measure_row_heights_tbl} \alias{measure_row_heights_tbl} -\title{Measure the rendered height of each data row in inches} +\title{Measure the rendered height of each table cell in inches} \usage{ measure_row_heights_tbl( data, @@ -14,11 +14,22 @@ measure_row_heights_tbl( max_measure_rows ) } +\arguments{ +\item{max_measure_rows}{Maximum number of rows to measure individually. +Non-sampled rows take the per-column max of the sampled rows (a +conservative estimate that mirrors prior behaviour).} +} \value{ -Numeric vector of row heights in inches (length = nrow(data)). +Numeric matrix \verb{[nrow(data) × length(resolved_cols)]} of inches. } \description{ Must be called while a viewport is active. Uses a memoised string-height function to avoid re-measuring repeated values. } +\details{ +Returns a matrix of cell heights (rows = data rows, cols = resolved_cols). +Each entry is the rendered height of that cell in inches, \strong{including the +top + bottom cell padding (\code{v_pad_in})} so that the per-row height is +simply \code{max(cell_h_mat[i, ])} without further adjustment. +} \keyword{internal} diff --git a/man/paginate_rows.Rd b/man/paginate_rows.Rd index 501a9e5..8b47a7a 100644 --- a/man/paginate_rows.Rd +++ b/man/paginate_rows.Rd @@ -6,19 +6,26 @@ \usage{ paginate_rows( data, - row_heights_in, + cell_h_mat, + resolved_cols, + group_vars, cont_row_h, header_row_h, content_height_in, - group_vars, row_cont_msg, - group_rule + group_rule, + suppress_repeated_groups = TRUE ) } \arguments{ \item{data}{Data frame.} -\item{row_heights_in}{Numeric vector of row heights in inches.} +\item{cell_h_mat}{Per-cell height matrix from \code{measure_row_heights_tbl()}.} + +\item{resolved_cols}{Full list of resolved column specs (used to identify +non-group columns).} + +\item{group_vars}{Character vector of group column names.} \item{cont_row_h}{Height of a continuation-marker row in inches.} @@ -26,16 +33,34 @@ paginate_rows( \item{content_height_in}{Available content height per page.} -\item{group_vars}{Character vector of group column names.} - \item{row_cont_msg}{Text for continuation-marker rows.} -\item{group_rule}{Logical — are group rules drawn?} +\item{group_rule}{Logical — are group rules drawn? (Reserved for future +use; currently does not affect pagination because rules are 0-height.)} + +\item{suppress_repeated_groups}{Logical, from \code{tbl$suppress_repeated_groups}.} } \value{ -A list of row-page specs (see internal structure below). +A list of row-page specs, each with \verb{$rows}, \verb{$is_cont_top}, +\verb{$is_cont_bottom}, \verb{$group_starts}, and \verb{$row_heights_in} (the committed +per-row heights for that page in inches). } \description{ -Split rows into pages, respecting group boundaries +Uses a per-page tentative recompute of \code{.compute_page_row_heights()} so that +span-aware row heights drive the page-fit decision. Two non-obvious +properties of this scheme are preserved by the algorithm: +} +\details{ +\itemize{ +\item Adding a row to an existing group-span on the current page may leave the +total page height unchanged (the span absorbs deficit that previously +inflated earlier rows), so more rows can fit than a per-row scalar sum +would predict. +\item When only the first row of a multi-row group lands on the current page +(group orphan), that row's span on the page is length 1 and the row is +grown to fit the full label height. \code{committed_rh} snapshots heights +after each successful append, so the orphan-correct heights are what gets +flushed to the page spec. +} } \keyword{internal} diff --git a/tests/testthat/test-row_span.R b/tests/testthat/test-row_span.R new file mode 100644 index 0000000..b4d91b1 --- /dev/null +++ b/tests/testthat/test-row_span.R @@ -0,0 +1,298 @@ +# test-row_span.R — HTML-rowspan-style flow for multi-line group labels +# +# Issue #29: a multi-line value in a group column should not force its row to +# be tall enough to fit all label lines. Instead, the label should be +# allowed to flow into the suppressed (blanked) cells in the rows below it, +# the same way HTML reserves a single cell that visually +# spans N rows. +# +# Tests here exercise three layers: +# * .compute_page_row_heights() — the core algorithm (synthetic inputs) +# * paginate_rows() — span-aware pagination (per-page recompute) +# * export_tfl() — end-to-end rendering smoke tests +# +# The synthetic-input tests deliberately bypass grid::stringHeight() so the +# expected heights are exact rather than device-dependent. + +# ---- helpers --------------------------------------------------------------- + +# Build a minimal resolved_cols list for .compute_page_row_heights(). +.spec <- function(col, is_group_col = FALSE) { + list(col = col, is_group_col = is_group_col) +} + +# ---- .compute_page_row_heights() ------------------------------------------- + +test_that("two-row span fits in available height; no row grows", { + # data: A is a 2-line group label spanning two rows, B is single-line. + # cell_h_mat: A column heights = c(2, 0); B column heights = c(1, 1). + # The 0 in A row 2 is irrelevant (cell is suppressed); algorithm reads only + # the start-of-span value. + cell_h_mat <- matrix(c(2, 0, # column A + 1, 1), # column B + nrow = 2, byrow = FALSE) + resolved_cols <- list(.spec("A", is_group_col = TRUE), .spec("B")) + suppress_mat <- matrix(c(FALSE, TRUE), nrow = 2, ncol = 1, + dimnames = list(NULL, "A")) + + row_h <- writetfl:::.compute_page_row_heights( + cell_h_mat, page_rows = 1:2, resolved_cols, + group_vars = "A", suppress_mat = suppress_mat + ) + # Both rows stay at 1 (the non-group-cell height). Label fits in span = 2. + expect_equal(row_h, c(1, 1)) +}) + +test_that("single-row span grows to fit the multi-line label", { + cell_h_mat <- matrix(c(2, # A + 1), # B + nrow = 1, byrow = FALSE) + resolved_cols <- list(.spec("A", is_group_col = TRUE), .spec("B")) + suppress_mat <- matrix(FALSE, nrow = 1, ncol = 1, + dimnames = list(NULL, "A")) + + row_h <- writetfl:::.compute_page_row_heights( + cell_h_mat, page_rows = 1L, resolved_cols, + group_vars = "A", suppress_mat = suppress_mat + ) + # Available = 1, label = 2 → grow to 2. + expect_equal(row_h, 2) +}) + +test_that("nested groups: inner span absorbed first, outer borrows remainder", { + # Three rows, two group columns A (outer) and B (inner). + # A has label height 2 over all 3 rows (one big span). + # B has label height 2 over rows 1-2, then a new value at row 3. + # Non-group column C is 1 line per row. + # Expected: inner span (B 1-2) sees label=2 vs avail=2 → no grow. + # Outer span (A 1-3) sees label=2 vs avail=3 → no grow. + cell_h_mat <- matrix(c(2, 0, 0, # A + 2, 0, 1, # B + 1, 1, 1), # C + nrow = 3, byrow = FALSE) + resolved_cols <- list( + .spec("A", is_group_col = TRUE), + .spec("B", is_group_col = TRUE), + .spec("C") + ) + suppress_mat <- matrix(c(FALSE, TRUE, TRUE, + FALSE, TRUE, FALSE), + nrow = 3, ncol = 2, + dimnames = list(NULL, c("A", "B"))) + row_h <- writetfl:::.compute_page_row_heights( + cell_h_mat, page_rows = 1:3, resolved_cols, + group_vars = c("A", "B"), suppress_mat = suppress_mat + ) + expect_equal(row_h, c(1, 1, 1)) +}) + +test_that("nested groups: inner growth feeds outer span availability", { + # A is 5 lines tall over 3 rows, B has a 4-line label on its own (1 span). + # Non-group column C is 1 line per row. + # Inner pass: B span = rows 1-3, label = 4 vs avail = 3 → grow row 1 by 1. + # row_h becomes c(2, 1, 1). + # Outer pass: A span = rows 1-3, label = 5 vs avail = 4 → grow row 1 by 1. + # row_h becomes c(3, 1, 1). + cell_h_mat <- matrix(c(5, 0, 0, # A + 4, 0, 0, # B + 1, 1, 1), # C + nrow = 3, byrow = FALSE) + resolved_cols <- list( + .spec("A", is_group_col = TRUE), + .spec("B", is_group_col = TRUE), + .spec("C") + ) + suppress_mat <- matrix(c(FALSE, TRUE, TRUE, + FALSE, TRUE, TRUE), + nrow = 3, ncol = 2, + dimnames = list(NULL, c("A", "B"))) + row_h <- writetfl:::.compute_page_row_heights( + cell_h_mat, page_rows = 1:3, resolved_cols, + group_vars = c("A", "B"), suppress_mat = suppress_mat + ) + expect_equal(row_h, c(3, 1, 1)) +}) + +test_that("suppress_mat = NULL falls back to per-row max over all columns", { + # When suppression is disabled, every cell is rendered, so the row needs to + # accommodate its tallest cell — including the group cell. + cell_h_mat <- matrix(c(2, 2, # A (group, but not suppressed) + 1, 1), # B + nrow = 2, byrow = FALSE) + resolved_cols <- list(.spec("A", is_group_col = TRUE), .spec("B")) + row_h <- writetfl:::.compute_page_row_heights( + cell_h_mat, page_rows = 1:2, resolved_cols, + group_vars = "A", suppress_mat = NULL + ) + expect_equal(row_h, c(2, 2)) +}) + +test_that("no group_vars degenerates to per-row max", { + cell_h_mat <- matrix(c(3, 1, # A + 1, 4), # B + nrow = 2, byrow = FALSE) + resolved_cols <- list(.spec("A"), .spec("B")) + row_h <- writetfl:::.compute_page_row_heights( + cell_h_mat, page_rows = 1:2, resolved_cols, + group_vars = character(0L), suppress_mat = NULL + ) + expect_equal(row_h, c(3, 4)) +}) + +test_that("zero-row page returns numeric(0)", { + expect_equal( + writetfl:::.compute_page_row_heights( + matrix(numeric(0L), nrow = 0L, ncol = 2L), + page_rows = integer(0L), + resolved_cols = list(.spec("A", is_group_col = TRUE), .spec("B")), + group_vars = "A", suppress_mat = NULL + ), + numeric(0L) + ) +}) + +# ---- paginate_rows() — span-aware fit + orphan handling -------------------- + +test_that("3-row group with 3-line label fits on one page (free-row)", { + # Property (a) from the design: adding a row to an open span can leave the + # page total unchanged. 3-row group, label = 3 lines, non-group = 1 line. + # Per-row independent sum would be 3 + 1 + 1 = 5 lines. Span-aware total + # is max(3, 3) = 3 lines, so the page holds all three rows in a 3-line + # content height. + # + # Note: cell_h_mat carries the *natural* cell height regardless of + # suppression — pagination's per-page recompute reads the current span + # start row's cell value, which may differ between pages if the same group + # is split across pages (label re-shown on a new page). + data <- data.frame(grp = rep("L1\nL2\nL3", 3L), + val = c("a", "b", "c"), + stringsAsFactors = FALSE) + cell_h_mat <- matrix(c(3, 3, 3, # grp (label = 3 lines, regardless of suppression) + 1, 1, 1), # val + nrow = 3, byrow = FALSE) + resolved_cols <- list(.spec("grp", is_group_col = TRUE), .spec("val")) + + pages <- writetfl:::paginate_rows( + data, cell_h_mat, resolved_cols, + group_vars = "grp", + cont_row_h = 0, header_row_h = 0, + content_height_in = 3, + row_cont_msg = c("(continued above)", "(continued below)"), + group_rule = FALSE + ) + expect_length(pages, 1L) + expect_equal(pages[[1L]]$rows, 1:3) + # Each row is 1 line; the label flows. + expect_equal(pages[[1L]]$row_heights_in, c(1, 1, 1)) +}) + +test_that("group orphan: lone first-row on a page grows to fit full label", { + # Property (b): when the rest of a group is pushed to the next page, the + # row that lands alone on the new page must be tall enough for the full + # (re-shown) label by itself. + # + # Setup: 4-row group with label = 4 lines, val = 1 line for rows 1-3 and + # 2 lines for row 4. Adding row 4 to the [1..3] page would push the total + # over the budget (because val = 2), so row 4 gets flushed to its own page + # where, as a single-row span, it must grow to fit the 4-line label. + data <- data.frame(grp = rep("L1\nL2\nL3\nL4", 4L), + val = c("a", "b", "c", "d\ne"), + stringsAsFactors = FALSE) + cell_h_mat <- matrix(c(4, 4, 4, 4, # grp + 1, 1, 1, 2), # val (row 4 is 2 lines) + nrow = 4, byrow = FALSE) + resolved_cols <- list(.spec("grp", is_group_col = TRUE), .spec("val")) + + warns <- character(0) + pages <- withCallingHandlers( + writetfl:::paginate_rows( + data, cell_h_mat, resolved_cols, + group_vars = "grp", + cont_row_h = 1, header_row_h = 0, + content_height_in = 5, + row_cont_msg = c("(continued above)", "(continued below)"), + group_rule = FALSE + ), + warning = function(w) { + warns <<- c(warns, conditionMessage(w)); invokeRestart("muffleWarning") + } + ) + # The group split must trigger the (continued) warning. + expect_true(any(grepl("continued", warns))) + # Two pages: rows 1-3 on page 1 (label spans them with deficit on row 1), + # row 4 alone on page 2 with full label height. + expect_length(pages, 2L) + expect_equal(pages[[1L]]$rows, 1:3) + # Page 1: span = [1,2,3], avail = 3, label = 4 → deficit 1 on row 1. + expect_equal(pages[[1L]]$row_heights_in, c(2, 1, 1)) + expect_equal(pages[[2L]]$rows, 4L) + # Page 2 orphan: single-row span re-shows the 4-line label. + expect_equal(pages[[2L]]$row_heights_in, 4) +}) + +test_that("pagination reset per page: re-shown label on next page sized correctly", { + # Same setup as the orphan test but framed around "the same data renders + # at different row heights on different pages because suppression resets + # at every page boundary". Page 2's row 4 must be tall enough for the + # re-shown label (= 4 lines) even though it was 1 line on page 1's matrix. + data <- data.frame(grp = rep("L1\nL2\nL3\nL4", 4L), + val = c("a", "b", "c", "d\ne"), + stringsAsFactors = FALSE) + cell_h_mat <- matrix(c(4, 4, 4, 4, + 1, 1, 1, 2), + nrow = 4, byrow = FALSE) + resolved_cols <- list(.spec("grp", is_group_col = TRUE), .spec("val")) + + pages <- suppressWarnings(writetfl:::paginate_rows( + data, cell_h_mat, resolved_cols, + group_vars = "grp", + cont_row_h = 1, header_row_h = 0, + content_height_in = 5, + row_cont_msg = c("(continued above)", "(continued below)"), + group_rule = FALSE + )) + # Page 2 orphan: row 4 alone at 4 lines (label height). This is the + # "same row may render differently on different pages" property. + expect_equal(pages[[2L]]$row_heights_in, 4) +}) + +# ---- end-to-end via export_tfl() ------------------------------------------- + +test_that("end-to-end: user's two-page rowspan example renders without error", { + # The user's exact example from issue #29: + # page 1: A = c("B\nC", "B\nC"), D = c("E", "F") + # page 2 (after suppression reset): A = "B\nC", D = "F" + # We craft a single 3-row data frame and a tight content height that forces + # page 1 to hold rows 1-2 and page 2 to hold row 3 alone. + df <- dplyr::group_by( + data.frame(A = rep("B\nC", 3L), + D = c("E", "F", "G"), + stringsAsFactors = FALSE), + A + ) + tbl <- tfl_table(df) + f <- tempfile(fileext = ".pdf") + on.exit(unlink(f)) + expect_no_error( + suppressWarnings( # the deliberate group split fires the (continued) warn + export_tfl(tbl, file = f, pg_width = 11, pg_height = 8.5, + min_content_height = grid::unit(0.5, "inches")) + ) + ) + expect_true(file.exists(f)) + expect_gt(file.info(f)$size, 0) +}) + +test_that("end-to-end: row_rule does not error inside multi-row spans", { + # Smoke test for the row-rule suppression-within-span branch. + df <- dplyr::group_by( + data.frame(A = c("X\nY", "X\nY", "X\nY"), + B = c("p", "q", "r"), + stringsAsFactors = FALSE), + A + ) + tbl <- tfl_table(df, row_rule = TRUE) + f <- tempfile(fileext = ".pdf") + on.exit(unlink(f)) + expect_no_error(export_tfl(tbl, file = f)) +}) diff --git a/tests/testthat/test-table_draw.R b/tests/testthat/test-table_draw.R index 371fdc1..94b3a4f 100644 --- a/tests/testthat/test-table_draw.R +++ b/tests/testthat/test-table_draw.R @@ -2,12 +2,14 @@ library(dplyr, warn.conflicts = FALSE) -# drawDetails — uncached height fallback (lines 112-141) ---------------------- +# drawDetails — uncached height fallback -------------------------------------- # -# build_table_grob() accepts row_heights_in = NULL and cont_row_h_in = NULL. -# When the grob is drawn, drawDetails falls back to recomputing those heights -# on the fly. The test exercises both fallback branches and, via a wrap- -# eligible column, also the .wrap_text branch inside the row-height loop. +# build_table_grob() accepts cell_heights_in_mat = NULL and cont_row_h_in = NULL. +# When the grob is drawn, drawDetails falls back to building a per-page cell +# matrix on the fly and applying .compute_page_row_heights(). The row_page +# also lacks $row_heights_in, forcing the recompute path. The test exercises +# both fallback branches and, via a wrap-eligible column, also the .wrap_text +# branch inside the per-cell measurement loop. test_that("drawDetails recomputes cont_row_h and row_h_vec when not cached", { df <- data.frame( @@ -34,16 +36,17 @@ test_that("drawDetails recomputes cont_row_h and row_h_vec when not cached", { is_cont_top = TRUE, # forces the uncached cont_row_h branch is_cont_bottom = FALSE, group_starts = integer(0L) + # row_heights_in deliberately omitted to force the recompute path ) grob <- writetfl:::build_table_grob( - row_page = row_page, - col_group_idx = seq_along(resolved), - n_group_cols = 0L, - resolved_cols = resolved, - tbl = tbl, - row_heights_in = NULL, # force uncached row-height path - cont_row_h_in = NULL # force uncached cont-row-height path + row_page = row_page, + col_group_idx = seq_along(resolved), + n_group_cols = 0L, + resolved_cols = resolved, + tbl = tbl, + cell_heights_in_mat = NULL, # force the per-page fallback measurement + cont_row_h_in = NULL # force uncached cont-row-height path ) f <- tempfile(fileext = ".pdf") diff --git a/tests/testthat/test-tfl_table.R b/tests/testthat/test-tfl_table.R index ee72d7c..4704f9f 100644 --- a/tests/testthat/test-tfl_table.R +++ b/tests/testthat/test-tfl_table.R @@ -381,14 +381,22 @@ make_row_page_inputs <- function(n = 5, group_every = NULL) { value = seq_len(n), stringsAsFactors = FALSE ) - list(data = data, heights = rep(1, n)) + # New paginate_rows takes a per-cell matrix and resolved_cols. + # All-uniform 1-inch cells reproduce the prior "heights = rep(1, n)" behaviour. + cell_h_mat <- matrix(1, nrow = n, ncol = 2L) + resolved_cols <- list( + list(col = "grp", is_group_col = FALSE), + list(col = "value", is_group_col = FALSE) + ) + list(data = data, cell_h_mat = cell_h_mat, resolved_cols = resolved_cols) } test_that("paginate_rows fits all rows on one page", { inp <- make_row_page_inputs(3) - pages <- paginate_rows(inp$data, inp$heights, cont_row_h = 0.5, - header_row_h = 0.5, content_height_in = 10, + pages <- paginate_rows(inp$data, inp$cell_h_mat, inp$resolved_cols, group_vars = character(0L), + cont_row_h = 0.5, header_row_h = 0.5, + content_height_in = 10, row_cont_msg = "(continued)", group_rule = FALSE) expect_length(pages, 1L) expect_equal(pages[[1L]]$rows, 1:3) @@ -398,9 +406,10 @@ test_that("paginate_rows fits all rows on one page", { test_that("paginate_rows splits across pages", { inp <- make_row_page_inputs(5) - pages <- paginate_rows(inp$data, inp$heights, cont_row_h = 0.2, - header_row_h = 0.5, content_height_in = 3, + pages <- paginate_rows(inp$data, inp$cell_h_mat, inp$resolved_cols, group_vars = character(0L), + cont_row_h = 0.2, header_row_h = 0.5, + content_height_in = 3, row_cont_msg = "(continued)", group_rule = FALSE) expect_gt(length(pages), 1L) # All row indices covered exactly once @@ -410,9 +419,10 @@ test_that("paginate_rows splits across pages", { test_that("paginate_rows marks cont_top/cont_bottom on splits", { inp <- make_row_page_inputs(5) - pages <- paginate_rows(inp$data, inp$heights, cont_row_h = 0.2, - header_row_h = 0.5, content_height_in = 2.5, + pages <- paginate_rows(inp$data, inp$cell_h_mat, inp$resolved_cols, group_vars = character(0L), + cont_row_h = 0.2, header_row_h = 0.5, + content_height_in = 2.5, row_cont_msg = "(continued)", group_rule = FALSE) if (length(pages) >= 2L) { expect_true(pages[[1L]]$is_cont_bottom) @@ -424,14 +434,19 @@ test_that("paginate_rows warns when a group spans multiple pages", { data <- data.frame(grp = c("A","A","A","A","A"), val = 1:5, stringsAsFactors = FALSE) gdf <- dplyr::group_by(data, grp) - heights <- rep(1, 5) + cell_h_mat <- matrix(1, nrow = 5, ncol = 2L) + resolved_cols <- list( + list(col = "grp", is_group_col = TRUE), + list(col = "val", is_group_col = FALSE) + ) # Multiple page breaks may fire multiple warnings; capture all and check # at least one matches warns <- character(0) withCallingHandlers( - paginate_rows(gdf, heights, cont_row_h = 0.2, - header_row_h = 0.5, content_height_in = 3, + paginate_rows(gdf, cell_h_mat, resolved_cols, group_vars = "grp", + cont_row_h = 0.2, header_row_h = 0.5, + content_height_in = 3, row_cont_msg = "(continued)", group_rule = FALSE), warning = function(w) { warns <<- c(warns, conditionMessage(w)) From 615cf7266c8e5bc57c361ca51471f7aa8e75589f Mon Sep 17 00:00:00 2001 From: Bill Denney Date: Sat, 9 May 2026 15:19:46 -0400 Subject: [PATCH 2/5] Draw group rule at outer-group boundaries even when inner group has size 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With nested group_vars, the previous logic suppressed the group rule at any transition where the new innermost group had only one row. That hid legitimate outer-level boundaries — for example, with group_vars = c("Cohort", "Visit"), the transition from the last Cohort 1 row to the first Cohort 2 row was suppressed whenever Cohort 2 happened to start with a one-row Visit, leaving no visual separator at the Cohort change. New helper .compute_group_rule_sizes() returns, for each group_start row, the size of the group at the *outermost* changing level (group_vars[1..k] where k is the outermost level whose value changed vs. the preceding row). drawDetails.tfl_table_grob() now uses this size to decide whether to draw the rule. Single-level grouping is unchanged: the outermost (and only) level is the innermost level, so the size is the same as .compute_group_sizes(). Nested grouping where the outer level changes now correctly draws a rule when the outer-level group has multiple rows, regardless of how many rows the new innermost block happens to have. Co-Authored-By: Claude Opus 4.7 (1M context) --- R/table_draw.R | 15 ++++++--- R/table_utils.R | 52 ++++++++++++++++++++++++++++++- tests/testthat/test-table_utils.R | 45 ++++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 5 deletions(-) diff --git a/R/table_draw.R b/R/table_draw.R index 26e8edf..e4805fb 100644 --- a/R/table_draw.R +++ b/R/table_draw.R @@ -213,9 +213,16 @@ drawDetails.tfl_table_grob <- function(x, recording) { ) } - # Precompute group sizes — group rules are suppressed for single-row groups - group_sizes <- if (tbl$group_rule && length(group_vars) > 0L) { - .compute_group_sizes(data, group_vars) + # Per-transition group size used for group-rule visibility. For a + # transition where only the innermost level changes, this is the size of + # the new innermost group (rule suppressed when the new group has just + # one row, since there is no actual block to delineate). For a + # transition where an *outer* level changes, this is the size of the + # outermost changing level's group, so cohort-level boundaries still + # render a rule even when the first inner block at the new cohort + # happens to be a single row. + group_rule_sizes <- if (tbl$group_rule && length(group_vars) > 0L) { + .compute_group_rule_sizes(data, group_vars) } else NULL # Precompute span ends per group column on this page so non-suppressed group @@ -298,7 +305,7 @@ drawDetails.tfl_table_grob <- function(x, recording) { # and the group has more than one row in the full data) if (i %in% grp_starts && ri > 1L) group_fill_idx <- group_fill_idx + 1L if (tbl$group_rule && i %in% grp_starts && y_cursor > header_row_h + 1e-6) { - gs <- if (!is.null(group_sizes)) group_sizes[as.character(i)] else NA_integer_ + gs <- if (!is.null(group_rule_sizes)) group_rule_sizes[as.character(i)] else NA_integer_ if (is.na(gs) || gs > 1L) { rule_gp <- .resolve_table_gp(gp_tbl, "group_rule") y_rule_npc <- 1 - y_cursor / vp_h diff --git a/R/table_utils.R b/R/table_utils.R index 85359a7..af5ef6b 100644 --- a/R/table_utils.R +++ b/R/table_utils.R @@ -106,7 +106,8 @@ # Compute group sizes (number of rows per group) from data and group vars. # Returns a named integer vector: name = row index of group start (as string), -# value = number of rows in that group. +# value = number of rows in that group. Group is defined by the *full* +# group_vars vector — i.e. the most-specific (innermost) grouping. .compute_group_sizes <- function(data, group_vars) { if (length(group_vars) == 0L || nrow(data) == 0L) return(integer(0L)) all_starts <- .compute_group_starts(data, group_vars) @@ -115,6 +116,55 @@ stats::setNames(sizes, as.character(all_starts)) } +# Per-group-start "effective size" for group-rule visibility. +# +# Returns a named integer vector keyed by group_start row index (as string). +# The first group_start has size NA_integer_ (no transition before it). +# For each subsequent group_start row i, the value is the size of the group +# at the *outermost* level k that changed between rows i-1 and i, defined as +# the count of rows in `data` whose group_vars[1..k] all match row i's +# values. +# +# This differs from .compute_group_sizes(), which always uses the full +# group_vars vector. When a transition crosses an outer-group boundary but +# the new innermost group has only a single row (e.g. Cohort changes from 1 +# to 2 and Cohort 2 happens to start with a one-row Visit), .compute_group_ +# sizes() returns 1 and the rule gets suppressed; this helper returns the +# outer Cohort group size, so a meaningful boundary still gets a rule. +.compute_group_rule_sizes <- function(data, group_vars) { + if (length(group_vars) == 0L || nrow(data) == 0L) return(integer(0L)) + starts <- .compute_group_starts(data, group_vars) + sizes <- rep(NA_integer_, length(starts)) + names(sizes) <- as.character(starts) + if (length(starts) <= 1L) return(sizes) + + for (idx in seq_along(starts)[-1L]) { + i <- starts[[idx]] + i_prev <- i - 1L + for (k in seq_along(group_vars)) { + if (!identical(data[[group_vars[[k]]]][[i_prev]], + data[[group_vars[[k]]]][[i]])) { + # Outermost changing level is k; count rows whose group_vars[1..k] + # all equal row i's values. + cols <- group_vars[seq_len(k)] + mask <- rep(TRUE, nrow(data)) + for (gv in cols) { + v <- data[[gv]] + target <- data[[gv]][[i]] + if (is.na(target)) { + mask <- mask & is.na(v) + } else { + mask <- mask & !is.na(v) & v == target + } + } + sizes[[idx]] <- sum(mask) + break + } + } + } + sizes +} + # --------------------------------------------------------------------------- # String / cell formatting helpers # --------------------------------------------------------------------------- diff --git a/tests/testthat/test-table_utils.R b/tests/testthat/test-table_utils.R index bffd6eb..35576ef 100644 --- a/tests/testthat/test-table_utils.R +++ b/tests/testthat/test-table_utils.R @@ -50,6 +50,51 @@ test_that(".compute_group_sizes returns integer(0) when group_vars is empty", { expect_equal(result, integer(0L)) }) +# .compute_group_rule_sizes() ------------------------------------------------- + +test_that(".compute_group_rule_sizes returns NA for the first group_start", { + df <- data.frame(g = c("A", "A", "B"), stringsAsFactors = FALSE) + res <- writetfl:::.compute_group_rule_sizes(df, "g") + expect_equal(unname(res[[1L]]), NA_integer_) +}) + +test_that(".compute_group_rule_sizes uses outer-level size when outer changes", { + # Two-level group: Cohort, Visit. Cohort 1 has 2 visits (each 2 rows); + # Cohort 2 has 2 visits (each 1 row). At the boundary between the last + # Cohort 1 row and the first Cohort 2 row, the inner (Cohort=2, Baseline) + # group has 1 row but the outer Cohort=2 group has 2 rows. The rule + # size at that transition should be 2 (outer), not 1 (inner). + df <- data.frame( + Cohort = c(1, 1, 1, 1, 2, 2), + Visit = c("A", "A", "B", "B", "A", "B"), + stringsAsFactors = FALSE + ) + res <- writetfl:::.compute_group_rule_sizes(df, c("Cohort", "Visit")) + # group_starts are rows c(1, 3, 5, 6). + expect_equal(names(res), c("1", "3", "5", "6")) + expect_equal(unname(res), + c(NA_integer_, # row 1: no transition before + 2L, # row 3: Visit changed within Cohort 1 → (Cohort=1, Visit=B) has 2 rows + 2L, # row 5: Cohort changed → (Cohort=2) outer group has 2 rows + 1L)) # row 6: Visit changed within Cohort 2 → (Cohort=2, Visit=B) has 1 row +}) + +test_that(".compute_group_rule_sizes single-level: matches innermost size", { + df <- data.frame(g = c("A", "A", "B", "C", "C", "C"), stringsAsFactors = FALSE) + res <- writetfl:::.compute_group_rule_sizes(df, "g") + expect_equal(names(res), c("1", "3", "4")) + expect_equal(unname(res), c(NA_integer_, 1L, 3L)) +}) + +test_that(".compute_group_rule_sizes returns integer(0) for empty inputs", { + expect_equal(writetfl:::.compute_group_rule_sizes( + data.frame(g = character(0L), stringsAsFactors = FALSE), "g" + ), integer(0L)) + expect_equal(writetfl:::.compute_group_rule_sizes( + data.frame(a = 1:3), character(0L) + ), integer(0L)) +}) + # .collect_col_strings() ------------------------------------------------------ test_that(".collect_col_strings truncates to max_rows unique strings", { From 7cdfc63774685991cda0aabb9664dec1b10342c9 Mon Sep 17 00:00:00 2001 From: Bill Denney Date: Sat, 9 May 2026 15:37:26 -0400 Subject: [PATCH 3/5] Make rowspan flow opt-in via simplify_rowspan; add partial-width group rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a new tfl_table() argument simplify_rowspan, defaulting to FALSE so existing tables render byte-for-byte identically to the pre-issue-29 release. When simplify_rowspan = TRUE the four behaviour changes introduced in this branch all switch on together: * Span-aware row heights — multi-line group labels flow through the suppressed cells below them instead of inflating the first row. * Row-rule suppression within a multi-row span — a horizontal line that would slice a flowing label is skipped. * Partial-width group rules — the rule line starts at the column for the outermost group_var level that actually changed at the transition, so unchanged outer columns through which the label is flowing aren't visually divided. * Group rules drawn at every transition. The historical "skip rule when the new innermost group has size 1" check is bypassed; with partial widths and label-flow alignment, single-row transitions are visually unambiguous. Implementation: * tfl_table() gets the new argument plus checkmate flag validation; the print method reports it. * paginate_rows() gains simplify_rowspan and passes NULL to .compute_page_row_heights() when FALSE so the resolver short-circuits to the historical per-row max. * drawDetails.tfl_table_grob() reads tbl$simplify_rowspan and gates: the row-height resolver fallback path, span clipping, the row-rule suppression predicate, and the group-rule visibility/width logic. * New helper .compute_group_rule_info() returns both the outermost- changing-level size and the level index per group_start row; drawing reads the level to set the rule's left-edge column. Tests: existing row-span tests now pass simplify_rowspan = TRUE; new default-behaviour test verifies historical per-row max heights still apply when the flag is FALSE. .compute_group_rule_info() unit tests cover NA, single-level, multi-level, and empty inputs. Co-Authored-By: Claude Opus 4.7 (1M context) --- R/table_draw.R | 109 ++++++++++++++++++++---------- R/table_pagelist.R | 3 +- R/table_rows.R | 16 ++++- R/table_utils.R | 49 +++++++++----- R/tfl_table.R | 16 +++++ design/ARCHITECTURE.md | 2 +- design/DECISIONS.md | 44 ++++++++++-- man/paginate_rows.Rd | 8 ++- man/tfl_table.Rd | 14 ++++ tests/testthat/test-row_span.R | 61 ++++++++++++++--- tests/testthat/test-table_utils.R | 53 +++++++++------ 11 files changed, 277 insertions(+), 98 deletions(-) diff --git a/R/table_draw.R b/R/table_draw.R index e4805fb..4d473a5 100644 --- a/R/table_draw.R +++ b/R/table_draw.R @@ -166,26 +166,33 @@ drawDetails.tfl_table_grob <- function(x, recording) { max(vapply(tbl$row_cont_msg, .cont_h, numeric(1L))) } - group_vars <- tbl$group_vars + group_vars <- tbl$group_vars + simplify_rowspan <- isTRUE(tbl$simplify_rowspan) - # Precompute the per-page suppression matrix. Also used by the per-page - # row-height resolver and the row-rule suppression check below. + # Precompute the per-page suppression matrix. Used both for cell-content + # blanking (always, when suppress_repeated_groups is set) and — only when + # simplify_rowspan = TRUE — for span-aware row heights, span clipping, + # and within-span row-rule suppression. suppress_mat <- if (isTRUE(tbl$suppress_repeated_groups) && length(group_vars) > 0L) { .compute_cell_suppression(data, group_vars, rows) } else NULL + # The row-height resolver returns the per-row max over all columns when + # given suppress_mat = NULL (the historical layout). Pass NULL when + # simplify_rowspan = FALSE so behaviour matches the pre-#29 release. + row_h_suppress <- if (simplify_rowspan) suppress_mat else NULL + # Per-page row heights — prefer the heights that pagination committed; if # absent, recompute from the cached cell-height matrix using the same - # span-aware algorithm pagination uses. As a final fallback, build a - # per-page matrix on the fly (covers grobs that were assembled outside - # the normal pipeline). + # algorithm pagination uses. As a final fallback, build a per-page + # matrix on the fly (covers grobs assembled outside the normal pipeline). row_h_vec <- if (!is.null(row_page$row_heights_in) && length(row_page$row_heights_in) == n_rows) { row_page$row_heights_in } else if (!is.null(x$cell_heights_in_mat) && !is.null(x$resolved_cols)) { .compute_page_row_heights( - x$cell_heights_in_mat, rows, x$resolved_cols, group_vars, suppress_mat + x$cell_heights_in_mat, rows, x$resolved_cols, group_vars, row_h_suppress ) } else { # Per-page fallback: build a small matrix for just the rows on this page @@ -209,27 +216,33 @@ drawDetails.tfl_table_grob <- function(x, recording) { } } .compute_page_row_heights( - fallback_mat, seq_len(n_rows), page_cols, group_vars, suppress_mat + fallback_mat, seq_len(n_rows), page_cols, group_vars, row_h_suppress ) } - # Per-transition group size used for group-rule visibility. For a - # transition where only the innermost level changes, this is the size of - # the new innermost group (rule suppressed when the new group has just - # one row, since there is no actual block to delineate). For a - # transition where an *outer* level changes, this is the size of the - # outermost changing level's group, so cohort-level boundaries still - # render a rule even when the first inner block at the new cohort - # happens to be a single row. - group_rule_sizes <- if (tbl$group_rule && length(group_vars) > 0L) { - .compute_group_rule_sizes(data, group_vars) - } else NULL + # Group-rule metadata. The two helpers differ: + # .compute_group_sizes() — innermost-group size (historical). + # Suppresses the rule whenever the new innermost group has 1 row, + # even if an outer level changed at this transition. + # .compute_group_rule_info() — outermost-changing-level size + level. + # Used when simplify_rowspan = TRUE to (a) draw rules at outer-level + # boundaries that the historical helper missed and (b) start the + # rule line at the changing column instead of at column 1. + group_sizes <- NULL + group_rule_info <- NULL + if (tbl$group_rule && length(group_vars) > 0L) { + if (simplify_rowspan) { + group_rule_info <- .compute_group_rule_info(data, group_vars) + } else { + group_sizes <- .compute_group_sizes(data, group_vars) + } + } - # Precompute span ends per group column on this page so non-suppressed group - # cells can be drawn with a clip viewport that covers the whole span. - # span_end_mat[ri, g] is the last row index in the same span as ri for - # group column g; only meaningful when suppress_mat[ri, g] == FALSE. - span_end_mat <- if (!is.null(suppress_mat)) { + # Precompute span ends per group column on this page so non-suppressed + # group cells can be drawn with a clip viewport that covers the whole + # span. span_end_mat[ri, g] is the last row index in the same span as ri + # for group column g; only meaningful when simplify_rowspan = TRUE. + span_end_mat <- if (simplify_rowspan && !is.null(suppress_mat)) { se <- matrix(NA_integer_, nrow = n_rows, ncol = length(group_vars)) for (g in seq_along(group_vars)) { starts <- which(!suppress_mat[, g]) @@ -301,16 +314,38 @@ drawDetails.tfl_table_grob <- function(x, recording) { i <- rows[[ri]] row_h <- row_h_vec[[ri]] - # Group rule before this row (if it starts a group, not the first visible row, - # and the group has more than one row in the full data) + # Group rule before this row (if it starts a group and is not the first + # visible row). Visibility and width depend on simplify_rowspan: + # * FALSE (default) — historical: rule is full table width and is + # suppressed when the *innermost* group at this start has size 1. + # * TRUE — rule is drawn at every transition (no size suppression); + # it starts at the column corresponding to the outermost level + # that actually changed at this boundary, so unchanged outer + # columns through which the label is flowing aren't sliced. if (i %in% grp_starts && ri > 1L) group_fill_idx <- group_fill_idx + 1L if (tbl$group_rule && i %in% grp_starts && y_cursor > header_row_h + 1e-6) { - gs <- if (!is.null(group_rule_sizes)) group_rule_sizes[as.character(i)] else NA_integer_ - if (is.na(gs) || gs > 1L) { + draw_rule <- FALSE + rule_start_col <- 1L + if (simplify_rowspan && !is.null(group_rule_info)) { + # Always draw at every transition; pick start column from the + # outermost-changing level reported by the helper. + gk <- group_rule_info$levels[as.character(i)] + if (!is.na(gk)) { + draw_rule <- TRUE + rule_start_col <- min(as.integer(gk), n_disp_cols) + } + } else if (!is.null(group_sizes)) { + # Historical visibility check. + gs <- group_sizes[as.character(i)] + draw_rule <- is.na(gs) || gs > 1L + } else { + draw_rule <- TRUE + } + if (draw_rule) { rule_gp <- .resolve_table_gp(gp_tbl, "group_rule") y_rule_npc <- 1 - y_cursor / vp_h - x_left_npc <- col_x_left[[1L]] / vp_w - x_right_npc <- col_x_right[[n_disp_cols]] / vp_w + x_left_npc <- col_x_left[[rule_start_col]] / vp_w + x_right_npc <- col_x_right[[n_disp_cols]] / vp_w grid::grid.lines(x = grid::unit(c(x_left_npc, x_right_npc), "npc"), y = grid::unit(c(y_rule_npc, y_rule_npc), "npc"), gp = rule_gp) @@ -377,12 +412,14 @@ drawDetails.tfl_table_grob <- function(x, recording) { y_cursor <- y_cursor + row_h - # Row rule between data rows (not after last). Suppress the rule when the - # next row is part of a multi-row group span starting at or before this - # row — drawing a horizontal line through a label that flows downward - # would visually slice the label. - rule_inside_span <- !is.null(suppress_mat) && ri < n_rows && - any(suppress_mat[ri + 1L, ]) + # Row rule between data rows (not after last). When simplify_rowspan + # is TRUE, suppress the rule if the next row is part of a multi-row + # group span starting at or before this row — drawing a horizontal + # line through a label that flows downward would visually slice it. + # When simplify_rowspan is FALSE (default), draw rules between every + # pair of data rows (the historical behaviour). + rule_inside_span <- simplify_rowspan && !is.null(suppress_mat) && + ri < n_rows && any(suppress_mat[ri + 1L, ]) if (tbl$row_rule && ri < n_rows && !rule_inside_span) { rule_gp <- .resolve_table_gp(gp_tbl, "row_rule") y_rule_npc <- 1 - y_cursor / vp_h diff --git a/R/table_pagelist.R b/R/table_pagelist.R index d32065e..5bd2326 100644 --- a/R/table_pagelist.R +++ b/R/table_pagelist.R @@ -209,7 +209,8 @@ tfl_table_to_pagelist <- function(tbl, pg_width, pg_height, dots, tbl$data, cell_h_mat, resolved_cols, tbl$group_vars, cont_row_h, header_row_h, ch, tbl$row_cont_msg, tbl$group_rule, - suppress_repeated_groups = isTRUE(tbl$suppress_repeated_groups) + suppress_repeated_groups = isTRUE(tbl$suppress_repeated_groups), + simplify_rowspan = isTRUE(tbl$simplify_rowspan) ) # --- Step 7: Assemble page specs --- diff --git a/R/table_rows.R b/R/table_rows.R index 7d15331..5cfdf42 100644 --- a/R/table_rows.R +++ b/R/table_rows.R @@ -226,6 +226,10 @@ measure_row_heights_tbl <- function(data, resolved_cols, gp_tbl, cell_padding, #' @param group_rule Logical — are group rules drawn? (Reserved for future #' use; currently does not affect pagination because rules are 0-height.) #' @param suppress_repeated_groups Logical, from `tbl$suppress_repeated_groups`. +#' @param simplify_rowspan Logical, from `tbl$simplify_rowspan`. When `FALSE` +#' (default) the per-row height is the per-row max over all cells (the +#' historical behaviour); the span-aware grow-into-suppressed-cells flow +#' only kicks in when this is `TRUE`. #' @return A list of row-page specs, each with `$rows`, `$is_cont_top`, #' `$is_cont_bottom`, `$group_starts`, and `$row_heights_in` (the committed #' per-row heights for that page in inches). @@ -233,7 +237,8 @@ measure_row_heights_tbl <- function(data, resolved_cols, gp_tbl, cell_padding, paginate_rows <- function(data, cell_h_mat, resolved_cols, group_vars, cont_row_h, header_row_h, content_height_in, row_cont_msg, group_rule, - suppress_repeated_groups = TRUE) { + suppress_repeated_groups = TRUE, + simplify_rowspan = FALSE) { n_rows <- nrow(data) # Group boundaries in the *full* data — used for the page-spec $group_starts @@ -261,8 +266,13 @@ paginate_rows <- function(data, cell_h_mat, resolved_cols, group_vars, sup <- if (suppress_repeated_groups && length(group_vars) > 0L) { .compute_cell_suppression(data, group_vars, candidate) } else NULL - rh <- .compute_page_row_heights(cell_h_mat, candidate, resolved_cols, - group_vars, sup) + # Pass NULL to disable the span-aware grow when simplify_rowspan = FALSE. + # The resolver's early-exit then returns the per-row max over all + # columns — the historical behaviour. + rh <- .compute_page_row_heights( + cell_h_mat, candidate, resolved_cols, group_vars, + if (simplify_rowspan) sup else NULL + ) total <- header_row_h + (if (is_cont_top) cont_row_h else 0) + sum(rh) + diff --git a/R/table_utils.R b/R/table_utils.R index af5ef6b..1884093 100644 --- a/R/table_utils.R +++ b/R/table_utils.R @@ -116,27 +116,39 @@ stats::setNames(sizes, as.character(all_starts)) } -# Per-group-start "effective size" for group-rule visibility. +# Per-group-start metadata for group-rule visibility and width. # -# Returns a named integer vector keyed by group_start row index (as string). -# The first group_start has size NA_integer_ (no transition before it). -# For each subsequent group_start row i, the value is the size of the group -# at the *outermost* level k that changed between rows i-1 and i, defined as -# the count of rows in `data` whose group_vars[1..k] all match row i's -# values. +# Returns a list with two named integer vectors keyed by group_start row +# index (as string): +# +# $sizes — for each group_start row i, the size of the group at the +# *outermost* level that changed between rows i-1 and i (the +# count of rows in `data` whose group_vars[1..k] all match +# row i's values, where k is the outermost changing level). +# The first group_start is NA (no transition before it). +# $levels — the column index k (1-based, into group_vars) of the +# outermost changing level. NA for the first group_start. # # This differs from .compute_group_sizes(), which always uses the full -# group_vars vector. When a transition crosses an outer-group boundary but -# the new innermost group has only a single row (e.g. Cohort changes from 1 -# to 2 and Cohort 2 happens to start with a one-row Visit), .compute_group_ -# sizes() returns 1 and the rule gets suppressed; this helper returns the -# outer Cohort group size, so a meaningful boundary still gets a rule. -.compute_group_rule_sizes <- function(data, group_vars) { - if (length(group_vars) == 0L || nrow(data) == 0L) return(integer(0L)) +# group_vars vector for sizing. When a transition crosses an outer-group +# boundary but the new innermost group has only a single row (e.g. Cohort +# changes from 1 to 2 and Cohort 2 happens to start with a one-row Visit), +# .compute_group_sizes() returns 1 and the rule gets suppressed; here the +# outer Cohort group size is returned so a meaningful boundary still gets +# a rule. $levels lets callers draw a partial-width rule that starts at +# the changing column instead of always spanning the full table width. +# +# Used only when tbl$simplify_rowspan is TRUE. +.compute_group_rule_info <- function(data, group_vars) { + if (length(group_vars) == 0L || nrow(data) == 0L) { + return(list(sizes = integer(0L), levels = integer(0L))) + } starts <- .compute_group_starts(data, group_vars) sizes <- rep(NA_integer_, length(starts)) - names(sizes) <- as.character(starts) - if (length(starts) <= 1L) return(sizes) + levels <- rep(NA_integer_, length(starts)) + names(sizes) <- as.character(starts) + names(levels) <- as.character(starts) + if (length(starts) <= 1L) return(list(sizes = sizes, levels = levels)) for (idx in seq_along(starts)[-1L]) { i <- starts[[idx]] @@ -157,12 +169,13 @@ mask <- mask & !is.na(v) & v == target } } - sizes[[idx]] <- sum(mask) + sizes[[idx]] <- sum(mask) + levels[[idx]] <- k break } } } - sizes + list(sizes = sizes, levels = levels) } # --------------------------------------------------------------------------- diff --git a/R/tfl_table.R b/R/tfl_table.R index b14ed51..14f81ba 100644 --- a/R/tfl_table.R +++ b/R/tfl_table.R @@ -114,6 +114,18 @@ tfl_colspec <- function(col, #' cells whose value equals the immediately preceding rendered row on the #' same page are left blank. The first data row on each page always shows #' the group value. +#' @param simplify_rowspan Logical. When `TRUE`, multi-line group-column +#' labels are allowed to flow into the suppressed (blanked) cells below +#' them — HTML-`rowspan`-style — instead of forcing the first row of the +#' group to be tall enough to fit every line. Row heights are resolved +#' per page so a row may render at different heights on different pages +#' when a group is split (the first row on a page re-shows the label and +#' may need to grow to fit it alone). Also: `row_rule` lines that would +#' slice through a flowing label are suppressed within the span, and +#' `group_rule` lines start at the first column whose group value is +#' actually changing at that boundary (so an outer label flowing through +#' the boundary is not visually divided). Default `FALSE`, preserving +#' the historical per-row layout exactly. #' @param sub_tfl Character vector of column names in `x`, or `NULL` (default). #' When non-NULL, the table is split into one sub-table per unique combination #' of values in these columns. Each sub-table's caption gains a suffix of the @@ -240,6 +252,7 @@ tfl_table <- function(x, allow_col_split = TRUE, balance_col_pages = FALSE, suppress_repeated_groups = TRUE, + simplify_rowspan = FALSE, sub_tfl = NULL, sub_tfl_sep = ": ", sub_tfl_collapse = "; ", @@ -364,6 +377,7 @@ tfl_table <- function(x, checkmate::assert_flag(allow_col_split, .var.name = "allow_col_split") checkmate::assert_flag(balance_col_pages, .var.name = "balance_col_pages") checkmate::assert_flag(suppress_repeated_groups, .var.name = "suppress_repeated_groups") + checkmate::assert_flag(simplify_rowspan, .var.name = "simplify_rowspan") checkmate::assert_flag(show_col_names, .var.name = "show_col_names") checkmate::assert_flag(col_header_rule, .var.name = "col_header_rule") checkmate::assert_flag(group_rule, .var.name = "group_rule") @@ -407,6 +421,7 @@ tfl_table <- function(x, allow_col_split = allow_col_split, balance_col_pages = balance_col_pages, suppress_repeated_groups = suppress_repeated_groups, + simplify_rowspan = simplify_rowspan, sub_tfl = sub_tfl, sub_tfl_sep = sub_tfl_sep, sub_tfl_collapse = sub_tfl_collapse, @@ -495,6 +510,7 @@ print.tfl_table <- function(x, ...) { x$allow_col_split, x$suppress_repeated_groups, x$show_col_names)) cat(sprintf(" col_header_rule=%s group_rule=%s row_rule=%s row_header_sep=%s\n", x$col_header_rule, x$group_rule, x$row_rule, x$row_header_sep)) + cat(sprintf(" simplify_rowspan=%s\n", isTRUE(x$simplify_rowspan))) if (!is.null(x$col_cont_msg)) { cat(sprintf(" col_cont_msg: left=\"%s\" right=\"%s\"\n", x$col_cont_msg[[1L]], x$col_cont_msg[[2L]])) diff --git a/design/ARCHITECTURE.md b/design/ARCHITECTURE.md index f946d9c..d4f4574 100644 --- a/design/ARCHITECTURE.md +++ b/design/ARCHITECTURE.md @@ -349,7 +349,7 @@ export_tfl(x = list_of_table1, ...) [exported] | `R/table_draw.R` | `build_table_grob()`, `drawDetails.tfl_table_grob()`, `.compute_cell_suppression()`, `.draw_header_row()`, `.draw_cont_row()`, `.draw_cell_text()` | | `R/table_pagelist.R` | `tfl_table_to_pagelist()`, `compute_table_content_area()` | | `R/sub_tfl.R` | `.compute_sub_tfl_groups()`, `.format_sub_tfl_caption()`, `.apply_sub_tfl_caption()`, `.strip_sub_tfl_cols()`, `.resolve_col_label()` | -| `R/table_utils.R` | `.make_outer_vp()`, `.width_in()`, `.height_in()`, `.measure_header_row_height()`, `.measure_cont_row_height()`, `.gp_with_lineheight()`, `.compute_group_starts()`, `.compute_group_sizes()`, `.collect_col_strings()`, `.fmt_cell()`, `.fmt_cell_vec()`, `.measure_max_string_width()`, `.resolve_table_gp()`, `.resolve_table_cell_gp()`, `.default_align()`, `.wrap_text()` | +| `R/table_utils.R` | `.make_outer_vp()`, `.width_in()`, `.height_in()`, `.measure_header_row_height()`, `.measure_cont_row_height()`, `.gp_with_lineheight()`, `.compute_group_starts()`, `.compute_group_sizes()`, `.compute_group_rule_info()` (used when `simplify_rowspan = TRUE` for outer-level rule visibility and partial-width start column), `.collect_col_strings()`, `.fmt_cell()`, `.fmt_cell_vec()`, `.measure_max_string_width()`, `.resolve_table_gp()`, `.resolve_table_cell_gp()`, `.default_align()`, `.wrap_text()` | --- diff --git a/design/DECISIONS.md b/design/DECISIONS.md index 6a0c71f..3eacce3 100644 --- a/design/DECISIONS.md +++ b/design/DECISIONS.md @@ -945,13 +945,43 @@ Plus: "if there is a grouping column on one page and different behavior on the next page... the handling of the reserved height for column A will differ between the pages." -**Suppression-aware row rule:** the existing `row_rule` between data rows -is suppressed when the next row is part of a multi-row group span (any -suppressed group column on row `ri+1`). A horizontal line slicing -through a label that flows downward would visually fragment it; HTML -rowspan also has no internal borders. Group rules and -`group_rule_after_last` are unaffected because they only fire at group -boundaries (which are also span boundaries). +**Opt-in via `simplify_rowspan` (default `FALSE`).** All of the +behavioural changes in this decision are gated on a new `tfl_table()` +argument `simplify_rowspan`, defaulting to `FALSE` so existing tables +render byte-for-byte identically to the pre-#29 release. When `TRUE`, +all four pieces below switch on together: +1. span-aware row heights (the rowspan flow); +2. row-rule suppression within a multi-row span; +3. partial-width group rules (rule starts at the outermost changing + group-var column); +4. group rules drawn at *every* transition, even when the new innermost + block has only one row (the historical size-suppression check is + bypassed because partial widths and label-flow alignment make + single-row transitions visually unambiguous). + +**Suppression-aware row rule:** when `simplify_rowspan = TRUE`, the +existing `row_rule` between data rows is suppressed if the next row is +part of a multi-row group span (any suppressed group column on row +`ri+1`). A horizontal line slicing through a label that flows downward +would visually fragment it; HTML rowspan also has no internal borders. +Group rules and `group_rule_after_last` are unaffected because they +only fire at group boundaries (which are also span boundaries). + +**Partial-width group rules.** When `simplify_rowspan = TRUE`, the +group rule line starts at the column corresponding to the *outermost* +group-var level that actually changed at the transition, not at column +1. A new helper `.compute_group_rule_info()` returns both the size and +the outermost-changing level per group_start; drawing reads the level +to set the line's left edge. Concrete result for nested +`group_vars = c("Cohort", "Visit")`: + +| transition | outermost changer | rule columns | +| ---------- | ----------------- | ------------------- | +| Visit only | Visit (level 2) | Visit, Value | +| Cohort | Cohort (level 1) | Cohort, Visit, Value| + +Without `simplify_rowspan`, group rules remain full-width and the +historical `.compute_group_sizes()` suppression continues to apply. **Alternatives considered and rejected:** - *Distribute deficit evenly across rows of the span* — leaves wasted diff --git a/man/paginate_rows.Rd b/man/paginate_rows.Rd index 8b47a7a..c713e8f 100644 --- a/man/paginate_rows.Rd +++ b/man/paginate_rows.Rd @@ -14,7 +14,8 @@ paginate_rows( content_height_in, row_cont_msg, group_rule, - suppress_repeated_groups = TRUE + suppress_repeated_groups = TRUE, + simplify_rowspan = FALSE ) } \arguments{ @@ -39,6 +40,11 @@ non-group columns).} use; currently does not affect pagination because rules are 0-height.)} \item{suppress_repeated_groups}{Logical, from \code{tbl$suppress_repeated_groups}.} + +\item{simplify_rowspan}{Logical, from \code{tbl$simplify_rowspan}. When \code{FALSE} +(default) the per-row height is the per-row max over all cells (the +historical behaviour); the span-aware grow-into-suppressed-cells flow +only kicks in when this is \code{TRUE}.} } \value{ A list of row-page specs, each with \verb{$rows}, \verb{$is_cont_top}, diff --git a/man/tfl_table.Rd b/man/tfl_table.Rd index 580d884..38504a3 100644 --- a/man/tfl_table.Rd +++ b/man/tfl_table.Rd @@ -15,6 +15,7 @@ tfl_table( allow_col_split = TRUE, balance_col_pages = FALSE, suppress_repeated_groups = TRUE, + simplify_rowspan = FALSE, sub_tfl = NULL, sub_tfl_sep = ": ", sub_tfl_collapse = "; ", @@ -77,6 +78,19 @@ cells whose value equals the immediately preceding rendered row on the same page are left blank. The first data row on each page always shows the group value.} +\item{simplify_rowspan}{Logical. When \code{TRUE}, multi-line group-column +labels are allowed to flow into the suppressed (blanked) cells below +them — HTML-\code{rowspan}-style — instead of forcing the first row of the +group to be tall enough to fit every line. Row heights are resolved +per page so a row may render at different heights on different pages +when a group is split (the first row on a page re-shows the label and +may need to grow to fit it alone). Also: \code{row_rule} lines that would +slice through a flowing label are suppressed within the span, and +\code{group_rule} lines start at the first column whose group value is +actually changing at that boundary (so an outer label flowing through +the boundary is not visually divided). Default \code{FALSE}, preserving +the historical per-row layout exactly.} + \item{sub_tfl}{Character vector of column names in \code{x}, or \code{NULL} (default). When non-NULL, the table is split into one sub-table per unique combination of values in these columns. Each sub-table's caption gains a suffix of the diff --git a/tests/testthat/test-row_span.R b/tests/testthat/test-row_span.R index b4d91b1..be2abb3 100644 --- a/tests/testthat/test-row_span.R +++ b/tests/testthat/test-row_span.R @@ -178,7 +178,8 @@ test_that("3-row group with 3-line label fits on one page (free-row)", { cont_row_h = 0, header_row_h = 0, content_height_in = 3, row_cont_msg = c("(continued above)", "(continued below)"), - group_rule = FALSE + group_rule = FALSE, + simplify_rowspan = TRUE ) expect_length(pages, 1L) expect_equal(pages[[1L]]$rows, 1:3) @@ -211,7 +212,8 @@ test_that("group orphan: lone first-row on a page grows to fit full label", { cont_row_h = 1, header_row_h = 0, content_height_in = 5, row_cont_msg = c("(continued above)", "(continued below)"), - group_rule = FALSE + group_rule = FALSE, + simplify_rowspan = TRUE ), warning = function(w) { warns <<- c(warns, conditionMessage(w)); invokeRestart("muffleWarning") @@ -249,28 +251,51 @@ test_that("pagination reset per page: re-shown label on next page sized correctl cont_row_h = 1, header_row_h = 0, content_height_in = 5, row_cont_msg = c("(continued above)", "(continued below)"), - group_rule = FALSE + group_rule = FALSE, + simplify_rowspan = TRUE )) # Page 2 orphan: row 4 alone at 4 lines (label height). This is the # "same row may render differently on different pages" property. expect_equal(pages[[2L]]$row_heights_in, 4) }) +test_that("paginate_rows defaults to historical per-row max (simplify_rowspan = FALSE)", { + # Without the opt-in, the same input that produces flowing rows of 1 line + # under TRUE produces fat rows of label height under the historical layout. + data <- data.frame(grp = rep("L1\nL2\nL3", 3L), + val = c("a", "b", "c"), + stringsAsFactors = FALSE) + cell_h_mat <- matrix(c(3, 3, 3, # grp + 1, 1, 1), # val + nrow = 3, byrow = FALSE) + resolved_cols <- list(.spec("grp", is_group_col = TRUE), .spec("val")) + + pages <- writetfl:::paginate_rows( + data, cell_h_mat, resolved_cols, + group_vars = "grp", + cont_row_h = 0, header_row_h = 0, + content_height_in = 9, # generous so all rows fit + row_cont_msg = c("(continued above)", "(continued below)"), + group_rule = FALSE + # simplify_rowspan defaults to FALSE + ) + # Historical: each row's height = max over ALL cells (incl. group label). + expect_equal(pages[[1L]]$row_heights_in, c(3, 3, 3)) +}) + # ---- end-to-end via export_tfl() ------------------------------------------- -test_that("end-to-end: user's two-page rowspan example renders without error", { +test_that("end-to-end: simplify_rowspan = TRUE renders user's example without error", { # The user's exact example from issue #29: # page 1: A = c("B\nC", "B\nC"), D = c("E", "F") # page 2 (after suppression reset): A = "B\nC", D = "F" - # We craft a single 3-row data frame and a tight content height that forces - # page 1 to hold rows 1-2 and page 2 to hold row 3 alone. df <- dplyr::group_by( data.frame(A = rep("B\nC", 3L), D = c("E", "F", "G"), stringsAsFactors = FALSE), A ) - tbl <- tfl_table(df) + tbl <- tfl_table(df, simplify_rowspan = TRUE) f <- tempfile(fileext = ".pdf") on.exit(unlink(f)) expect_no_error( @@ -283,7 +308,7 @@ test_that("end-to-end: user's two-page rowspan example renders without error", { expect_gt(file.info(f)$size, 0) }) -test_that("end-to-end: row_rule does not error inside multi-row spans", { +test_that("end-to-end: row_rule with simplify_rowspan = TRUE does not error in spans", { # Smoke test for the row-rule suppression-within-span branch. df <- dplyr::group_by( data.frame(A = c("X\nY", "X\nY", "X\nY"), @@ -291,8 +316,26 @@ test_that("end-to-end: row_rule does not error inside multi-row spans", { stringsAsFactors = FALSE), A ) - tbl <- tfl_table(df, row_rule = TRUE) + tbl <- tfl_table(df, row_rule = TRUE, simplify_rowspan = TRUE) f <- tempfile(fileext = ".pdf") on.exit(unlink(f)) expect_no_error(export_tfl(tbl, file = f)) }) + +test_that("end-to-end: default (simplify_rowspan = FALSE) renders historically", { + # Same input as the user-example test but without the opt-in. The + # default behaviour (label inflates first row of group) must keep working + # without errors and produce a non-empty PDF. + df <- dplyr::group_by( + data.frame(A = rep("B\nC", 3L), + D = c("E", "F", "G"), + stringsAsFactors = FALSE), + A + ) + tbl <- tfl_table(df) # simplify_rowspan defaults to FALSE + f <- tempfile(fileext = ".pdf") + on.exit(unlink(f)) + expect_no_error(export_tfl(tbl, file = f, pg_width = 11, pg_height = 8.5)) + expect_true(file.exists(f)) + expect_gt(file.info(f)$size, 0) +}) diff --git a/tests/testthat/test-table_utils.R b/tests/testthat/test-table_utils.R index 35576ef..89129f4 100644 --- a/tests/testthat/test-table_utils.R +++ b/tests/testthat/test-table_utils.R @@ -50,49 +50,58 @@ test_that(".compute_group_sizes returns integer(0) when group_vars is empty", { expect_equal(result, integer(0L)) }) -# .compute_group_rule_sizes() ------------------------------------------------- +# .compute_group_rule_info() -------------------------------------------------- -test_that(".compute_group_rule_sizes returns NA for the first group_start", { - df <- data.frame(g = c("A", "A", "B"), stringsAsFactors = FALSE) - res <- writetfl:::.compute_group_rule_sizes(df, "g") - expect_equal(unname(res[[1L]]), NA_integer_) +test_that(".compute_group_rule_info returns NA size and level for the first group_start", { + df <- data.frame(g = c("A", "A", "B"), stringsAsFactors = FALSE) + res <- writetfl:::.compute_group_rule_info(df, "g") + expect_equal(unname(res$sizes[[1L]]), NA_integer_) + expect_equal(unname(res$levels[[1L]]), NA_integer_) }) -test_that(".compute_group_rule_sizes uses outer-level size when outer changes", { +test_that(".compute_group_rule_info reports outer level + size when outer changes", { # Two-level group: Cohort, Visit. Cohort 1 has 2 visits (each 2 rows); # Cohort 2 has 2 visits (each 1 row). At the boundary between the last # Cohort 1 row and the first Cohort 2 row, the inner (Cohort=2, Baseline) - # group has 1 row but the outer Cohort=2 group has 2 rows. The rule - # size at that transition should be 2 (outer), not 1 (inner). + # group has 1 row but the outer Cohort=2 group has 2 rows. df <- data.frame( Cohort = c(1, 1, 1, 1, 2, 2), Visit = c("A", "A", "B", "B", "A", "B"), stringsAsFactors = FALSE ) - res <- writetfl:::.compute_group_rule_sizes(df, c("Cohort", "Visit")) + res <- writetfl:::.compute_group_rule_info(df, c("Cohort", "Visit")) # group_starts are rows c(1, 3, 5, 6). - expect_equal(names(res), c("1", "3", "5", "6")) - expect_equal(unname(res), + expect_equal(names(res$sizes), c("1", "3", "5", "6")) + expect_equal(names(res$levels), c("1", "3", "5", "6")) + expect_equal(unname(res$sizes), c(NA_integer_, # row 1: no transition before 2L, # row 3: Visit changed within Cohort 1 → (Cohort=1, Visit=B) has 2 rows 2L, # row 5: Cohort changed → (Cohort=2) outer group has 2 rows 1L)) # row 6: Visit changed within Cohort 2 → (Cohort=2, Visit=B) has 1 row + expect_equal(unname(res$levels), + c(NA_integer_, # row 1: no transition before + 2L, # Visit (level 2) is outermost changer + 1L, # Cohort (level 1) is outermost changer + 2L)) # Visit (level 2) is outermost changer }) -test_that(".compute_group_rule_sizes single-level: matches innermost size", { - df <- data.frame(g = c("A", "A", "B", "C", "C", "C"), stringsAsFactors = FALSE) - res <- writetfl:::.compute_group_rule_sizes(df, "g") - expect_equal(names(res), c("1", "3", "4")) - expect_equal(unname(res), c(NA_integer_, 1L, 3L)) +test_that(".compute_group_rule_info single-level: level = 1 always", { + df <- data.frame(g = c("A", "A", "B", "C", "C", "C"), stringsAsFactors = FALSE) + res <- writetfl:::.compute_group_rule_info(df, "g") + expect_equal(names(res$sizes), c("1", "3", "4")) + expect_equal(unname(res$sizes), c(NA_integer_, 1L, 3L)) + expect_equal(unname(res$levels), c(NA_integer_, 1L, 1L)) }) -test_that(".compute_group_rule_sizes returns integer(0) for empty inputs", { - expect_equal(writetfl:::.compute_group_rule_sizes( +test_that(".compute_group_rule_info returns integer(0) fields for empty inputs", { + res1 <- writetfl:::.compute_group_rule_info( data.frame(g = character(0L), stringsAsFactors = FALSE), "g" - ), integer(0L)) - expect_equal(writetfl:::.compute_group_rule_sizes( - data.frame(a = 1:3), character(0L) - ), integer(0L)) + ) + expect_equal(res1$sizes, integer(0L)) + expect_equal(res1$levels, integer(0L)) + res2 <- writetfl:::.compute_group_rule_info(data.frame(a = 1:3), character(0L)) + expect_equal(res2$sizes, integer(0L)) + expect_equal(res2$levels, integer(0L)) }) # .collect_col_strings() ------------------------------------------------------ From 9c0ed3ca887bc4a6147d3f0a8e874e3f823d8996 Mon Sep 17 00:00:00 2001 From: Bill Denney Date: Sat, 9 May 2026 16:16:55 -0400 Subject: [PATCH 4/5] Suppressed group cells contribute zero height to their row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With simplify_rowspan = FALSE (the default), a multi-line group label inflates only the row that displays it; the trailing rows where the group cell is blanked by suppression are now sized only by their non-group content. Previously each row's height was the per-row max over every cell, including blanked group cells, so a 3-row group with a 2-line label rendered as 3 rows of 2 lines each (6 lines) instead of the new 2 + 1 + 1 = 4 lines. Implementation: .compute_page_row_heights() gains a simplify_rowspan parameter and three modes: * No suppression — per-row max over all cells (unchanged). * Suppression on, simplify_rowspan = FALSE — per-row max over the non-suppressed cells (group cells contribute 0 when suppressed). * Suppression on, simplify_rowspan = TRUE — span-aware flow as introduced in the previous commit on this branch. paginate_rows() and drawDetails.tfl_table_grob() now always pass suppress_mat to the resolver and forward simplify_rowspan as an explicit flag, so the resolver picks the right mode without callers having to massage the suppress matrix. Tables that don't use multi-line group labels render identically to the pre-issue-29 release. Tables that did use them now produce strictly more compact output and never cause overlap or clipping. Co-Authored-By: Claude Opus 4.7 (1M context) --- R/table_draw.R | 11 +-- R/table_rows.R | 131 ++++++++++++++++------------ R/tfl_table.R | 10 ++- design/DECISIONS.md | 24 ++++-- man/tfl_table.Rd | 10 ++- tests/testthat/test-row_span.R | 151 ++++++++++++++++++++++++--------- 6 files changed, 225 insertions(+), 112 deletions(-) diff --git a/R/table_draw.R b/R/table_draw.R index 4d473a5..d13baa4 100644 --- a/R/table_draw.R +++ b/R/table_draw.R @@ -178,11 +178,6 @@ drawDetails.tfl_table_grob <- function(x, recording) { .compute_cell_suppression(data, group_vars, rows) } else NULL - # The row-height resolver returns the per-row max over all columns when - # given suppress_mat = NULL (the historical layout). Pass NULL when - # simplify_rowspan = FALSE so behaviour matches the pre-#29 release. - row_h_suppress <- if (simplify_rowspan) suppress_mat else NULL - # Per-page row heights — prefer the heights that pagination committed; if # absent, recompute from the cached cell-height matrix using the same # algorithm pagination uses. As a final fallback, build a per-page @@ -192,7 +187,8 @@ drawDetails.tfl_table_grob <- function(x, recording) { row_page$row_heights_in } else if (!is.null(x$cell_heights_in_mat) && !is.null(x$resolved_cols)) { .compute_page_row_heights( - x$cell_heights_in_mat, rows, x$resolved_cols, group_vars, row_h_suppress + x$cell_heights_in_mat, rows, x$resolved_cols, group_vars, suppress_mat, + simplify_rowspan = simplify_rowspan ) } else { # Per-page fallback: build a small matrix for just the rows on this page @@ -216,7 +212,8 @@ drawDetails.tfl_table_grob <- function(x, recording) { } } .compute_page_row_heights( - fallback_mat, seq_len(n_rows), page_cols, group_vars, row_h_suppress + fallback_mat, seq_len(n_rows), page_cols, group_vars, suppress_mat, + simplify_rowspan = simplify_rowspan ) } diff --git a/R/table_rows.R b/R/table_rows.R index 5cfdf42..480e4f7 100644 --- a/R/table_rows.R +++ b/R/table_rows.R @@ -105,24 +105,31 @@ measure_row_heights_tbl <- function(data, resolved_cols, gp_tbl, cell_padding, # .compute_page_row_heights() — resolve per-row heights for one page # --------------------------------------------------------------------------- -# Compute the per-row heights for the rows on a single page, allowing a -# multi-line group label to flow through the suppressed cells below it. +# Compute the per-row heights for the rows on a single page. # -# Algorithm: -# 1. Initialise row_h[ri] = max of cell_h_mat over non-group columns at row -# page_rows[ri]. This is the height the row needs *ignoring* the group -# column's potentially multi-line value. -# 2. Walk group_vars from innermost (last) to outermost (first). For each -# group column g, find spans of consecutive page rows where -# suppress_mat[ri_start, g] == FALSE and suppress_mat[ri, g] == TRUE for -# ri in (ri_start, ri_end]. For each span, if the label cell at -# ri_start is taller than sum(row_h[ri_start..ri_end]), grow row_h[ri_start] -# by the deficit (text is top-aligned so growth on the first row keeps -# the label visually anchored). +# Three modes, selected by `simplify_rowspan` and the presence of +# `suppress_mat`: # -# The early-exit short-circuits when there are no group columns or -# suppression is disabled, returning the simple per-row max. In that case -# the algorithm is a no-op compared to today's behaviour. +# * **No-suppression** (`suppress_mat` is `NULL`). Every group cell is +# rendered in every row, so the row height is the per-row max over +# every cell. Same result regardless of `simplify_rowspan`. +# +# * **Suppression on, `simplify_rowspan = FALSE`** (the historical +# default). Each row's height is the per-row max over its +# *non-suppressed* cells: non-group cells always count, but a group +# cell that's blanked by suppression contributes height 0. This is +# the regime where empty trailing rows of a multi-line group are +# sized only by their non-group content (issue #29 follow-up: a +# multi-line group label inflates only the row that displays it, +# not the suppressed rows below). +# +# * **Suppression on, `simplify_rowspan = TRUE`**. The label is allowed +# to flow downward across the suppressed cells (HTML-rowspan-style). +# Initialise row_h from non-group cells only, then walk group_vars +# innermost-first and, for each span, grow row_h[span_start] by the +# amount the label exceeds `sum(row_h[span])`. Innermost-first so +# outer spans can borrow space inner spans already pushed for. +# First-row growth matches the label's top-aligned drawing. # # @param cell_h_mat Full matrix of cell heights including v_pad (inches). # @param page_rows Integer vector of data-row indices visible on this page. @@ -130,67 +137,79 @@ measure_row_heights_tbl <- function(data, resolved_cols, gp_tbl, cell_padding, # @param group_vars Character vector of group column names. # @param suppress_mat Logical matrix [length(page_rows) × length(group_vars)] # from .compute_cell_suppression(), or NULL when suppression is disabled. +# @param simplify_rowspan Logical from `tbl$simplify_rowspan`. See above. # @return Numeric vector of length(page_rows), heights in inches. #' @keywords internal .compute_page_row_heights <- function(cell_h_mat, page_rows, resolved_cols, - group_vars, suppress_mat) { + group_vars, suppress_mat, + simplify_rowspan = FALSE) { n_pr <- length(page_rows) if (n_pr == 0L) return(numeric(0L)) n_grp <- length(group_vars) - # Early exit when there are no group columns or suppression is disabled. - # In both cases, every cell in every row is rendered in full (no flow into - # blanked cells), so the row height is the per-row max over ALL columns. - if (n_grp == 0L || is.null(suppress_mat)) { - return(as.numeric(apply(cell_h_mat[page_rows, , drop = FALSE], 1L, max))) - } - # Map each resolved column to its column index in cell_h_mat col_names <- vapply(resolved_cols, function(cs) cs$col, character(1L)) is_group <- vapply(resolved_cols, function(cs) isTRUE(cs$is_group_col), logical(1L)) - # Initial per-row heights: max over non-group columns. Group columns are - # excluded here because their height contribution is handled below in the - # span-aware growth pass. If every column is a group column (degenerate / - # no body data) fall back to the full row max so we still produce a sane - # height. + # Per-row max over non-group cells. Non-group cells always contribute + # to the row height in every mode. Falls back to the full-matrix max if + # every column is somehow a group column (degenerate input). non_group_idx <- which(!is_group) - row_h <- if (length(non_group_idx) > 0L) { + ng_max <- if (length(non_group_idx) > 0L) { apply(cell_h_mat[page_rows, non_group_idx, drop = FALSE], 1L, max) } else { apply(cell_h_mat[page_rows, , drop = FALSE], 1L, max) # nocov } - # apply() drops to a length-n_pr vector; ensure shape for n_pr == 1 - row_h <- as.numeric(row_h) + row_h <- as.numeric(ng_max) - # Resolve each group_var to its column index in cell_h_mat + if (n_grp == 0L) return(row_h) group_col_idx <- match(group_vars, col_names) - # Process from innermost to outermost so outer-group spans can borrow space - # already contributed by inner-group growth. - for (g in rev(seq_len(n_grp))) { - j_mat <- group_col_idx[[g]] - if (is.na(j_mat)) next # safety; shouldn't happen + # Span-aware growth (simplify_rowspan = TRUE). Operates on row_h + # initialised from non-group cells only, so the label is amortised + # across the span and only grows the start row when the deficit is + # positive. Falls back to the suppression-aware-max path below when + # suppress_mat is NULL (which means suppression is off — every cell is + # always rendered, so there is nothing to flow through). + if (simplify_rowspan && !is.null(suppress_mat)) { + for (g in rev(seq_len(n_grp))) { + j_mat <- group_col_idx[[g]] + if (is.na(j_mat)) next # safety; shouldn't happen - # Span starts: rows where suppress_mat[, g] is FALSE. A span runs from - # one start (inclusive) to the next start - 1 (or n_pr at the end). - starts <- which(!suppress_mat[, g]) - if (length(starts) == 0L) next # entire column suppressed (shouldn't happen) - ends <- c(starts[-1L] - 1L, n_pr) + starts <- which(!suppress_mat[, g]) + if (length(starts) == 0L) next # entire column suppressed (shouldn't happen) + ends <- c(starts[-1L] - 1L, n_pr) - for (s_idx in seq_along(starts)) { - ri_start <- starts[[s_idx]] - ri_end <- ends[[s_idx]] - label_h <- cell_h_mat[page_rows[[ri_start]], j_mat] - avail <- sum(row_h[ri_start:ri_end]) - if (label_h > avail + 1e-9) { - row_h[[ri_start]] <- row_h[[ri_start]] + (label_h - avail) + for (s_idx in seq_along(starts)) { + ri_start <- starts[[s_idx]] + ri_end <- ends[[s_idx]] + label_h <- cell_h_mat[page_rows[[ri_start]], j_mat] + avail <- sum(row_h[ri_start:ri_end]) + if (label_h > avail + 1e-9) { + row_h[[ri_start]] <- row_h[[ri_start]] + (label_h - avail) + } } } + return(row_h) } + # Suppression-aware per-row max (simplify_rowspan = FALSE). For each + # group column, fold its cell heights into row_h, but treat suppressed + # cells as contributing zero — so empty trailing rows of a multi-line + # group label are sized purely by their non-group content. When + # suppress_mat is NULL (suppression disabled) every group cell counts, + # which degenerates to the historical per-row max over all cells. + for (g in seq_len(n_grp)) { + j_mat <- group_col_idx[[g]] + if (is.na(j_mat)) next # safety; shouldn't happen + g_heights <- cell_h_mat[page_rows, j_mat] + if (!is.null(suppress_mat)) { + g_heights[suppress_mat[, g]] <- 0 + } + row_h <- pmax(row_h, g_heights) + } row_h } @@ -266,12 +285,14 @@ paginate_rows <- function(data, cell_h_mat, resolved_cols, group_vars, sup <- if (suppress_repeated_groups && length(group_vars) > 0L) { .compute_cell_suppression(data, group_vars, candidate) } else NULL - # Pass NULL to disable the span-aware grow when simplify_rowspan = FALSE. - # The resolver's early-exit then returns the per-row max over all - # columns — the historical behaviour. + # Pass simplify_rowspan to the resolver explicitly. When FALSE, the + # resolver returns the per-row max over non-suppressed cells (so a + # blank trailing row in a multi-line-label group is sized only by its + # non-group content). When TRUE, the resolver does the span-aware + # flow that allows the label to extend across the span. rh <- .compute_page_row_heights( - cell_h_mat, candidate, resolved_cols, group_vars, - if (simplify_rowspan) sup else NULL + cell_h_mat, candidate, resolved_cols, group_vars, sup, + simplify_rowspan = simplify_rowspan ) total <- header_row_h + (if (is_cont_top) cont_row_h else 0) + diff --git a/R/tfl_table.R b/R/tfl_table.R index 14f81ba..ac4da41 100644 --- a/R/tfl_table.R +++ b/R/tfl_table.R @@ -124,8 +124,14 @@ tfl_colspec <- function(col, #' slice through a flowing label are suppressed within the span, and #' `group_rule` lines start at the first column whose group value is #' actually changing at that boundary (so an outer label flowing through -#' the boundary is not visually divided). Default `FALSE`, preserving -#' the historical per-row layout exactly. +#' the boundary is not visually divided). Default `FALSE`. +#' +#' When `simplify_rowspan = FALSE`, suppressed (blanked) group cells +#' contribute zero height to their row — only the row that actually +#' displays the label inflates, the trailing rows take their height +#' from their non-group content. When suppression is itself disabled +#' (`suppress_repeated_groups = FALSE`), every group cell is rendered +#' on every row so the per-row max over all cells is used. #' @param sub_tfl Character vector of column names in `x`, or `NULL` (default). #' When non-NULL, the table is split into one sub-table per unique combination #' of values in these columns. Each sub-table's caption gains a suffix of the diff --git a/design/DECISIONS.md b/design/DECISIONS.md index 3eacce3..6d42b15 100644 --- a/design/DECISIONS.md +++ b/design/DECISIONS.md @@ -946,11 +946,12 @@ on the next page... the handling of the reserved height for column A will differ between the pages." **Opt-in via `simplify_rowspan` (default `FALSE`).** All of the -behavioural changes in this decision are gated on a new `tfl_table()` -argument `simplify_rowspan`, defaulting to `FALSE` so existing tables -render byte-for-byte identically to the pre-#29 release. When `TRUE`, -all four pieces below switch on together: -1. span-aware row heights (the rowspan flow); +behavioural changes that *visibly redistribute* multi-line group labels +(the flow, partial-width rules, span-aware row-rule suppression) are +gated on a new `tfl_table()` argument `simplify_rowspan`, defaulting to +`FALSE`. When `TRUE`, four pieces switch on together: +1. span-aware row heights (the rowspan flow — label flows downward + across suppressed cells); 2. row-rule suppression within a multi-row span; 3. partial-width group rules (rule starts at the outermost changing group-var column); @@ -959,6 +960,19 @@ all four pieces below switch on together: bypassed because partial widths and label-flow alignment make single-row transitions visually unambiguous). +**`simplify_rowspan = FALSE` row-height refinement.** Even with the +flow turned off, suppressed (blanked) group cells now contribute zero +to their row's height — so a multi-line group label inflates only the +row that actually displays it, not the trailing rows below. This is a +small backward-incompatible change relative to the strict pre-#29 +layout (where every row's height was the per-row max over every cell, +including blanked ones), but it produces strictly more compact output +and never causes overlap or clipping. Tables that don't have +multi-line group labels render identically to the pre-#29 release. +When suppression is itself disabled (`suppress_repeated_groups = FALSE`), +every group cell is rendered in every row and the per-row max over all +cells is restored. + **Suppression-aware row rule:** when `simplify_rowspan = TRUE`, the existing `row_rule` between data rows is suppressed if the next row is part of a multi-row group span (any suppressed group column on row diff --git a/man/tfl_table.Rd b/man/tfl_table.Rd index 38504a3..aadef28 100644 --- a/man/tfl_table.Rd +++ b/man/tfl_table.Rd @@ -88,8 +88,14 @@ may need to grow to fit it alone). Also: \code{row_rule} lines that would slice through a flowing label are suppressed within the span, and \code{group_rule} lines start at the first column whose group value is actually changing at that boundary (so an outer label flowing through -the boundary is not visually divided). Default \code{FALSE}, preserving -the historical per-row layout exactly.} +the boundary is not visually divided). Default \code{FALSE}. + +When \code{simplify_rowspan = FALSE}, suppressed (blanked) group cells +contribute zero height to their row — only the row that actually +displays the label inflates, the trailing rows take their height +from their non-group content. When suppression is itself disabled +(\code{suppress_repeated_groups = FALSE}), every group cell is rendered +on every row so the per-row max over all cells is used.} \item{sub_tfl}{Character vector of column names in \code{x}, or \code{NULL} (default). When non-NULL, the table is split into one sub-table per unique combination diff --git a/tests/testthat/test-row_span.R b/tests/testthat/test-row_span.R index be2abb3..7291725 100644 --- a/tests/testthat/test-row_span.R +++ b/tests/testthat/test-row_span.R @@ -23,12 +23,11 @@ # ---- .compute_page_row_heights() ------------------------------------------- -test_that("two-row span fits in available height; no row grows", { - # data: A is a 2-line group label spanning two rows, B is single-line. - # cell_h_mat: A column heights = c(2, 0); B column heights = c(1, 1). - # The 0 in A row 2 is irrelevant (cell is suppressed); algorithm reads only - # the start-of-span value. - cell_h_mat <- matrix(c(2, 0, # column A +test_that("simplify_rowspan = TRUE: two-row span fits in available height; no row grows", { + # cell_h_mat carries the *natural* cell height for every cell. Even + # though A is suppressed at row 2, we set its matrix value to its full + # 2-line label height because the matrix is suppression-agnostic. + cell_h_mat <- matrix(c(2, 2, # column A (label height = 2 lines) 1, 1), # column B nrow = 2, byrow = FALSE) resolved_cols <- list(.spec("A", is_group_col = TRUE), .spec("B")) @@ -37,13 +36,14 @@ test_that("two-row span fits in available height; no row grows", { row_h <- writetfl:::.compute_page_row_heights( cell_h_mat, page_rows = 1:2, resolved_cols, - group_vars = "A", suppress_mat = suppress_mat + group_vars = "A", suppress_mat = suppress_mat, + simplify_rowspan = TRUE ) # Both rows stay at 1 (the non-group-cell height). Label fits in span = 2. expect_equal(row_h, c(1, 1)) }) -test_that("single-row span grows to fit the multi-line label", { +test_that("simplify_rowspan = TRUE: single-row span grows to fit the multi-line label", { cell_h_mat <- matrix(c(2, # A 1), # B nrow = 1, byrow = FALSE) @@ -53,21 +53,16 @@ test_that("single-row span grows to fit the multi-line label", { row_h <- writetfl:::.compute_page_row_heights( cell_h_mat, page_rows = 1L, resolved_cols, - group_vars = "A", suppress_mat = suppress_mat + group_vars = "A", suppress_mat = suppress_mat, + simplify_rowspan = TRUE ) # Available = 1, label = 2 → grow to 2. expect_equal(row_h, 2) }) -test_that("nested groups: inner span absorbed first, outer borrows remainder", { - # Three rows, two group columns A (outer) and B (inner). - # A has label height 2 over all 3 rows (one big span). - # B has label height 2 over rows 1-2, then a new value at row 3. - # Non-group column C is 1 line per row. - # Expected: inner span (B 1-2) sees label=2 vs avail=2 → no grow. - # Outer span (A 1-3) sees label=2 vs avail=3 → no grow. - cell_h_mat <- matrix(c(2, 0, 0, # A - 2, 0, 1, # B +test_that("simplify_rowspan = TRUE: nested groups, inner absorbed first", { + cell_h_mat <- matrix(c(2, 2, 2, # A + 2, 2, 1, # B 1, 1, 1), # C nrow = 3, byrow = FALSE) resolved_cols <- list( @@ -81,20 +76,15 @@ test_that("nested groups: inner span absorbed first, outer borrows remainder", { dimnames = list(NULL, c("A", "B"))) row_h <- writetfl:::.compute_page_row_heights( cell_h_mat, page_rows = 1:3, resolved_cols, - group_vars = c("A", "B"), suppress_mat = suppress_mat + group_vars = c("A", "B"), suppress_mat = suppress_mat, + simplify_rowspan = TRUE ) expect_equal(row_h, c(1, 1, 1)) }) -test_that("nested groups: inner growth feeds outer span availability", { - # A is 5 lines tall over 3 rows, B has a 4-line label on its own (1 span). - # Non-group column C is 1 line per row. - # Inner pass: B span = rows 1-3, label = 4 vs avail = 3 → grow row 1 by 1. - # row_h becomes c(2, 1, 1). - # Outer pass: A span = rows 1-3, label = 5 vs avail = 4 → grow row 1 by 1. - # row_h becomes c(3, 1, 1). - cell_h_mat <- matrix(c(5, 0, 0, # A - 4, 0, 0, # B +test_that("simplify_rowspan = TRUE: nested groups, inner growth feeds outer span", { + cell_h_mat <- matrix(c(5, 5, 5, # A + 4, 4, 4, # B 1, 1, 1), # C nrow = 3, byrow = FALSE) resolved_cols <- list( @@ -108,23 +98,79 @@ test_that("nested groups: inner growth feeds outer span availability", { dimnames = list(NULL, c("A", "B"))) row_h <- writetfl:::.compute_page_row_heights( cell_h_mat, page_rows = 1:3, resolved_cols, - group_vars = c("A", "B"), suppress_mat = suppress_mat + group_vars = c("A", "B"), suppress_mat = suppress_mat, + simplify_rowspan = TRUE ) expect_equal(row_h, c(3, 1, 1)) }) -test_that("suppress_mat = NULL falls back to per-row max over all columns", { - # When suppression is disabled, every cell is rendered, so the row needs to - # accommodate its tallest cell — including the group cell. +test_that("simplify_rowspan = FALSE: suppressed group cells contribute 0 to row height", { + # The user's follow-up: with the rowspan flow OFF, blank trailing rows + # of a multi-line-label group must not inflate to label height. Row 1 + # shows the 2-line A label and rises to 2; rows 2 and 3 are 1 line each + # because their A cell is suppressed and contributes 0. + cell_h_mat <- matrix(c(2, 2, 2, # A + 1, 1, 1), # B + nrow = 3, byrow = FALSE) + resolved_cols <- list(.spec("A", is_group_col = TRUE), .spec("B")) + suppress_mat <- matrix(c(FALSE, TRUE, TRUE), nrow = 3, ncol = 1, + dimnames = list(NULL, "A")) + + row_h <- writetfl:::.compute_page_row_heights( + cell_h_mat, page_rows = 1:3, resolved_cols, + group_vars = "A", suppress_mat = suppress_mat, + simplify_rowspan = FALSE + ) + expect_equal(row_h, c(2, 1, 1)) +}) + +test_that("simplify_rowspan = FALSE: nested groups also zero out suppressed cells", { + # Outer A is 3 lines, inner B is 2 lines. Row 1: both shown → row_h = 3. + # Row 2: both suppressed → row_h = 1 (just C). + # Row 3: A still suppressed, B re-shows (different value) → row_h = max(1, 2) = 2. + cell_h_mat <- matrix(c(3, 3, 3, # A + 2, 2, 2, # B + 1, 1, 1), # C + nrow = 3, byrow = FALSE) + resolved_cols <- list( + .spec("A", is_group_col = TRUE), + .spec("B", is_group_col = TRUE), + .spec("C") + ) + suppress_mat <- matrix(c(FALSE, TRUE, TRUE, + FALSE, TRUE, FALSE), + nrow = 3, ncol = 2, + dimnames = list(NULL, c("A", "B"))) + row_h <- writetfl:::.compute_page_row_heights( + cell_h_mat, page_rows = 1:3, resolved_cols, + group_vars = c("A", "B"), suppress_mat = suppress_mat, + simplify_rowspan = FALSE + ) + expect_equal(row_h, c(3, 1, 2)) +}) + +test_that("suppress_mat = NULL: every cell counts (per-row max over all)", { + # When suppression is disabled, every cell is rendered, so the row + # needs to accommodate its tallest cell — including the group cell. + # Result is the same regardless of simplify_rowspan. cell_h_mat <- matrix(c(2, 2, # A (group, but not suppressed) 1, 1), # B nrow = 2, byrow = FALSE) resolved_cols <- list(.spec("A", is_group_col = TRUE), .spec("B")) - row_h <- writetfl:::.compute_page_row_heights( - cell_h_mat, page_rows = 1:2, resolved_cols, - group_vars = "A", suppress_mat = NULL + expect_equal( + writetfl:::.compute_page_row_heights( + cell_h_mat, page_rows = 1:2, resolved_cols, + group_vars = "A", suppress_mat = NULL, simplify_rowspan = FALSE + ), + c(2, 2) + ) + expect_equal( + writetfl:::.compute_page_row_heights( + cell_h_mat, page_rows = 1:2, resolved_cols, + group_vars = "A", suppress_mat = NULL, simplify_rowspan = TRUE + ), + c(2, 2) ) - expect_equal(row_h, c(2, 2)) }) test_that("no group_vars degenerates to per-row max", { @@ -259,13 +305,14 @@ test_that("pagination reset per page: re-shown label on next page sized correctl expect_equal(pages[[2L]]$row_heights_in, 4) }) -test_that("paginate_rows defaults to historical per-row max (simplify_rowspan = FALSE)", { - # Without the opt-in, the same input that produces flowing rows of 1 line - # under TRUE produces fat rows of label height under the historical layout. +test_that("simplify_rowspan = FALSE: only the labelled row inflates (suppressed → 0)", { + # Without the opt-in, multi-line group labels still inflate the labelled + # row, but the suppressed (blanked) trailing rows are sized only by their + # non-group content — they do *not* inherit the label height. data <- data.frame(grp = rep("L1\nL2\nL3", 3L), val = c("a", "b", "c"), stringsAsFactors = FALSE) - cell_h_mat <- matrix(c(3, 3, 3, # grp + cell_h_mat <- matrix(c(3, 3, 3, # grp (label = 3 lines, every row's natural value) 1, 1, 1), # val nrow = 3, byrow = FALSE) resolved_cols <- list(.spec("grp", is_group_col = TRUE), .spec("val")) @@ -279,7 +326,29 @@ test_that("paginate_rows defaults to historical per-row max (simplify_rowspan = group_rule = FALSE # simplify_rowspan defaults to FALSE ) - # Historical: each row's height = max over ALL cells (incl. group label). + expect_equal(pages[[1L]]$row_heights_in, c(3, 1, 1)) +}) + +test_that("simplify_rowspan = FALSE without suppression: every cell counts (mode A)", { + # If suppression is also off, every group cell is fully rendered on + # every row, so each row inflates to label height. + data <- data.frame(grp = rep("L1\nL2\nL3", 3L), + val = c("a", "b", "c"), + stringsAsFactors = FALSE) + cell_h_mat <- matrix(c(3, 3, 3, + 1, 1, 1), + nrow = 3, byrow = FALSE) + resolved_cols <- list(.spec("grp", is_group_col = TRUE), .spec("val")) + + pages <- writetfl:::paginate_rows( + data, cell_h_mat, resolved_cols, + group_vars = "grp", + cont_row_h = 0, header_row_h = 0, + content_height_in = 99, + row_cont_msg = c("(continued above)", "(continued below)"), + group_rule = FALSE, + suppress_repeated_groups = FALSE + ) expect_equal(pages[[1L]]$row_heights_in, c(3, 3, 3)) }) From c1cc85964ef5f856dca6a050b242879a4f694e8c Mon Sep 17 00:00:00 2001 From: Bill Denney Date: Sat, 9 May 2026 16:34:27 -0400 Subject: [PATCH 5/5] Make rowspan flow the default whenever suppression is on; drop simplify_rowspan flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Suppression and the rowspan flow are conceptually one thing: when a user asks for suppress_repeated_groups = TRUE they're asking for the visual where the multi-line label spans the suppressed cells beneath it, the row heights aren't inflated by the labelled-but-flowable column, the row rule doesn't slice through the flowing label, and the group rule starts at the column whose value actually changed. An earlier iteration of this branch added a simplify_rowspan flag to gate the flow + visual changes, but that left the package with three modes (suppression off / suppression on with flag off / suppression on with flag on) when one suffices. This commit removes the simplify_rowspan flag and makes the flow the sole behaviour whenever suppress_repeated_groups is TRUE. Setting suppress_repeated_groups = FALSE remains the way to opt out — every group cell renders on every row and the row max is taken over all cells (the strict per-row layout). * tfl_table() loses the simplify_rowspan parameter (and the validation, storage, and print-method line that went with it). * paginate_rows() and .compute_page_row_heights() lose the parameter; the resolver dispatches purely on whether suppress_mat is non-NULL. * drawDetails.tfl_table_grob() drops every isTRUE(tbl$simplify_rowspan) check; the span clipping, span-aware row-rule suppression, and the partial-width / always-drawn group rules all apply unconditionally when suppression is active. * Tests stop passing simplify_rowspan; the dedicated FALSE / TRUE test cases collapse into single-behaviour assertions. * Demo simplifies to one PDF per scenario (no off/on toggle). Co-Authored-By: Claude Opus 4.7 (1M context) --- R/table_draw.R | 102 +++++--------- R/table_pagelist.R | 3 +- R/table_rows.R | 116 ++++++---------- R/tfl_table.R | 34 ++--- design/DECISIONS.md | 92 +++++++------ man/paginate_rows.Rd | 8 +- man/tfl_table.Rd | 32 ++--- tests/testthat/test-row_span.R | 238 +++++++-------------------------- 8 files changed, 198 insertions(+), 427 deletions(-) diff --git a/R/table_draw.R b/R/table_draw.R index d13baa4..8e4faf4 100644 --- a/R/table_draw.R +++ b/R/table_draw.R @@ -166,13 +166,11 @@ drawDetails.tfl_table_grob <- function(x, recording) { max(vapply(tbl$row_cont_msg, .cont_h, numeric(1L))) } - group_vars <- tbl$group_vars - simplify_rowspan <- isTRUE(tbl$simplify_rowspan) + group_vars <- tbl$group_vars - # Precompute the per-page suppression matrix. Used both for cell-content - # blanking (always, when suppress_repeated_groups is set) and — only when - # simplify_rowspan = TRUE — for span-aware row heights, span clipping, - # and within-span row-rule suppression. + # Precompute the per-page suppression matrix. Drives cell-content + # blanking, span-aware row heights, span clipping for non-suppressed + # group cells, and within-span row-rule suppression. suppress_mat <- if (isTRUE(tbl$suppress_repeated_groups) && length(group_vars) > 0L) { .compute_cell_suppression(data, group_vars, rows) @@ -187,8 +185,7 @@ drawDetails.tfl_table_grob <- function(x, recording) { row_page$row_heights_in } else if (!is.null(x$cell_heights_in_mat) && !is.null(x$resolved_cols)) { .compute_page_row_heights( - x$cell_heights_in_mat, rows, x$resolved_cols, group_vars, suppress_mat, - simplify_rowspan = simplify_rowspan + x$cell_heights_in_mat, rows, x$resolved_cols, group_vars, suppress_mat ) } else { # Per-page fallback: build a small matrix for just the rows on this page @@ -212,34 +209,22 @@ drawDetails.tfl_table_grob <- function(x, recording) { } } .compute_page_row_heights( - fallback_mat, seq_len(n_rows), page_cols, group_vars, suppress_mat, - simplify_rowspan = simplify_rowspan + fallback_mat, seq_len(n_rows), page_cols, group_vars, suppress_mat ) } - # Group-rule metadata. The two helpers differ: - # .compute_group_sizes() — innermost-group size (historical). - # Suppresses the rule whenever the new innermost group has 1 row, - # even if an outer level changed at this transition. - # .compute_group_rule_info() — outermost-changing-level size + level. - # Used when simplify_rowspan = TRUE to (a) draw rules at outer-level - # boundaries that the historical helper missed and (b) start the - # rule line at the changing column instead of at column 1. - group_sizes <- NULL - group_rule_info <- NULL - if (tbl$group_rule && length(group_vars) > 0L) { - if (simplify_rowspan) { - group_rule_info <- .compute_group_rule_info(data, group_vars) - } else { - group_sizes <- .compute_group_sizes(data, group_vars) - } - } + # Group-rule metadata: outermost-changing-level size + level for each + # group_start. Drawing reads $levels to set the rule's left-edge column + # (so unchanged outer columns aren't sliced through by the rule line). + group_rule_info <- if (tbl$group_rule && length(group_vars) > 0L) { + .compute_group_rule_info(data, group_vars) + } else NULL # Precompute span ends per group column on this page so non-suppressed # group cells can be drawn with a clip viewport that covers the whole - # span. span_end_mat[ri, g] is the last row index in the same span as ri - # for group column g; only meaningful when simplify_rowspan = TRUE. - span_end_mat <- if (simplify_rowspan && !is.null(suppress_mat)) { + # span. span_end_mat[ri, g] is the last row index in the same span as + # ri for group column g. + span_end_mat <- if (!is.null(suppress_mat)) { se <- matrix(NA_integer_, nrow = n_rows, ncol = length(group_vars)) for (g in seq_along(group_vars)) { starts <- which(!suppress_mat[, g]) @@ -312,37 +297,20 @@ drawDetails.tfl_table_grob <- function(x, recording) { row_h <- row_h_vec[[ri]] # Group rule before this row (if it starts a group and is not the first - # visible row). Visibility and width depend on simplify_rowspan: - # * FALSE (default) — historical: rule is full table width and is - # suppressed when the *innermost* group at this start has size 1. - # * TRUE — rule is drawn at every transition (no size suppression); - # it starts at the column corresponding to the outermost level - # that actually changed at this boundary, so unchanged outer - # columns through which the label is flowing aren't sliced. + # visible row). The rule starts at the column corresponding to the + # outermost group_var level that actually changed at this transition, + # so unchanged outer columns through which the label is flowing + # aren't sliced. Drawn at every transition. if (i %in% grp_starts && ri > 1L) group_fill_idx <- group_fill_idx + 1L - if (tbl$group_rule && i %in% grp_starts && y_cursor > header_row_h + 1e-6) { - draw_rule <- FALSE - rule_start_col <- 1L - if (simplify_rowspan && !is.null(group_rule_info)) { - # Always draw at every transition; pick start column from the - # outermost-changing level reported by the helper. - gk <- group_rule_info$levels[as.character(i)] - if (!is.na(gk)) { - draw_rule <- TRUE - rule_start_col <- min(as.integer(gk), n_disp_cols) - } - } else if (!is.null(group_sizes)) { - # Historical visibility check. - gs <- group_sizes[as.character(i)] - draw_rule <- is.na(gs) || gs > 1L - } else { - draw_rule <- TRUE - } - if (draw_rule) { - rule_gp <- .resolve_table_gp(gp_tbl, "group_rule") - y_rule_npc <- 1 - y_cursor / vp_h - x_left_npc <- col_x_left[[rule_start_col]] / vp_w - x_right_npc <- col_x_right[[n_disp_cols]] / vp_w + if (tbl$group_rule && i %in% grp_starts && y_cursor > header_row_h + 1e-6 && + !is.null(group_rule_info)) { + gk <- group_rule_info$levels[as.character(i)] + if (!is.na(gk)) { + rule_start_col <- min(as.integer(gk), n_disp_cols) + rule_gp <- .resolve_table_gp(gp_tbl, "group_rule") + y_rule_npc <- 1 - y_cursor / vp_h + x_left_npc <- col_x_left[[rule_start_col]] / vp_w + x_right_npc <- col_x_right[[n_disp_cols]] / vp_w grid::grid.lines(x = grid::unit(c(x_left_npc, x_right_npc), "npc"), y = grid::unit(c(y_rule_npc, y_rule_npc), "npc"), gp = rule_gp) @@ -409,14 +377,12 @@ drawDetails.tfl_table_grob <- function(x, recording) { y_cursor <- y_cursor + row_h - # Row rule between data rows (not after last). When simplify_rowspan - # is TRUE, suppress the rule if the next row is part of a multi-row - # group span starting at or before this row — drawing a horizontal - # line through a label that flows downward would visually slice it. - # When simplify_rowspan is FALSE (default), draw rules between every - # pair of data rows (the historical behaviour). - rule_inside_span <- simplify_rowspan && !is.null(suppress_mat) && - ri < n_rows && any(suppress_mat[ri + 1L, ]) + # Row rule between data rows (not after last). Suppress the rule if + # the next row is part of a multi-row group span starting at or + # before this row — drawing a horizontal line through a label that + # flows downward would visually slice it. + rule_inside_span <- !is.null(suppress_mat) && ri < n_rows && + any(suppress_mat[ri + 1L, ]) if (tbl$row_rule && ri < n_rows && !rule_inside_span) { rule_gp <- .resolve_table_gp(gp_tbl, "row_rule") y_rule_npc <- 1 - y_cursor / vp_h diff --git a/R/table_pagelist.R b/R/table_pagelist.R index 5bd2326..d32065e 100644 --- a/R/table_pagelist.R +++ b/R/table_pagelist.R @@ -209,8 +209,7 @@ tfl_table_to_pagelist <- function(tbl, pg_width, pg_height, dots, tbl$data, cell_h_mat, resolved_cols, tbl$group_vars, cont_row_h, header_row_h, ch, tbl$row_cont_msg, tbl$group_rule, - suppress_repeated_groups = isTRUE(tbl$suppress_repeated_groups), - simplify_rowspan = isTRUE(tbl$simplify_rowspan) + suppress_repeated_groups = isTRUE(tbl$suppress_repeated_groups) ) # --- Step 7: Assemble page specs --- diff --git a/R/table_rows.R b/R/table_rows.R index 480e4f7..66e2341 100644 --- a/R/table_rows.R +++ b/R/table_rows.R @@ -107,29 +107,21 @@ measure_row_heights_tbl <- function(data, resolved_cols, gp_tbl, cell_padding, # Compute the per-row heights for the rows on a single page. # -# Three modes, selected by `simplify_rowspan` and the presence of -# `suppress_mat`: +# A single rule, dispatched on whether suppression is active: # -# * **No-suppression** (`suppress_mat` is `NULL`). Every group cell is -# rendered in every row, so the row height is the per-row max over -# every cell. Same result regardless of `simplify_rowspan`. +# * **`suppress_mat` is `NULL`** — suppression is off, so every group +# cell renders on every row. Row height is the per-row max over +# every cell (group and non-group alike). # -# * **Suppression on, `simplify_rowspan = FALSE`** (the historical -# default). Each row's height is the per-row max over its -# *non-suppressed* cells: non-group cells always count, but a group -# cell that's blanked by suppression contributes height 0. This is -# the regime where empty trailing rows of a multi-line group are -# sized only by their non-group content (issue #29 follow-up: a -# multi-line group label inflates only the row that displays it, -# not the suppressed rows below). -# -# * **Suppression on, `simplify_rowspan = TRUE`**. The label is allowed -# to flow downward across the suppressed cells (HTML-rowspan-style). -# Initialise row_h from non-group cells only, then walk group_vars -# innermost-first and, for each span, grow row_h[span_start] by the -# amount the label exceeds `sum(row_h[span])`. Innermost-first so -# outer spans can borrow space inner spans already pushed for. -# First-row growth matches the label's top-aligned drawing. +# * **`suppress_mat` is non-NULL** — suppression is on. Group columns +# never inflate row heights. Initialise row_h from non-group cells +# only, then walk group_vars innermost-first and, for each span, +# grow row_h[span_start] only if the label exceeds the cumulative +# span height — which lets multi-line labels flow downward through +# the blanked cells (HTML-`rowspan` behaviour) instead of inflating +# just the labelled row. Innermost-first so outer spans can borrow +# space inner spans already pushed for. First-row growth matches +# the label's top-aligned drawing. # # @param cell_h_mat Full matrix of cell heights including v_pad (inches). # @param page_rows Integer vector of data-row indices visible on this page. @@ -137,25 +129,21 @@ measure_row_heights_tbl <- function(data, resolved_cols, gp_tbl, cell_padding, # @param group_vars Character vector of group column names. # @param suppress_mat Logical matrix [length(page_rows) × length(group_vars)] # from .compute_cell_suppression(), or NULL when suppression is disabled. -# @param simplify_rowspan Logical from `tbl$simplify_rowspan`. See above. # @return Numeric vector of length(page_rows), heights in inches. #' @keywords internal .compute_page_row_heights <- function(cell_h_mat, page_rows, resolved_cols, - group_vars, suppress_mat, - simplify_rowspan = FALSE) { + group_vars, suppress_mat) { n_pr <- length(page_rows) if (n_pr == 0L) return(numeric(0L)) n_grp <- length(group_vars) - # Map each resolved column to its column index in cell_h_mat col_names <- vapply(resolved_cols, function(cs) cs$col, character(1L)) is_group <- vapply(resolved_cols, function(cs) isTRUE(cs$is_group_col), logical(1L)) - # Per-row max over non-group cells. Non-group cells always contribute - # to the row height in every mode. Falls back to the full-matrix max if - # every column is somehow a group column (degenerate input). + # Per-row max over non-group cells. Falls back to the full-matrix max + # if every column is somehow a group column (degenerate input). non_group_idx <- which(!is_group) ng_max <- if (length(non_group_idx) > 0L) { apply(cell_h_mat[page_rows, non_group_idx, drop = FALSE], 1L, max) @@ -167,48 +155,39 @@ measure_row_heights_tbl <- function(data, resolved_cols, gp_tbl, cell_padding, if (n_grp == 0L) return(row_h) group_col_idx <- match(group_vars, col_names) - # Span-aware growth (simplify_rowspan = TRUE). Operates on row_h - # initialised from non-group cells only, so the label is amortised - # across the span and only grows the start row when the deficit is - # positive. Falls back to the suppression-aware-max path below when - # suppress_mat is NULL (which means suppression is off — every cell is - # always rendered, so there is nothing to flow through). - if (simplify_rowspan && !is.null(suppress_mat)) { - for (g in rev(seq_len(n_grp))) { + if (is.null(suppress_mat)) { + # No suppression: every group cell is rendered fully on its row, so + # group cells contribute to the row max alongside non-group cells. + for (g in seq_len(n_grp)) { j_mat <- group_col_idx[[g]] if (is.na(j_mat)) next # safety; shouldn't happen - - starts <- which(!suppress_mat[, g]) - if (length(starts) == 0L) next # entire column suppressed (shouldn't happen) - ends <- c(starts[-1L] - 1L, n_pr) - - for (s_idx in seq_along(starts)) { - ri_start <- starts[[s_idx]] - ri_end <- ends[[s_idx]] - label_h <- cell_h_mat[page_rows[[ri_start]], j_mat] - avail <- sum(row_h[ri_start:ri_end]) - if (label_h > avail + 1e-9) { - row_h[[ri_start]] <- row_h[[ri_start]] + (label_h - avail) - } - } + row_h <- pmax(row_h, cell_h_mat[page_rows, j_mat]) } return(row_h) } - # Suppression-aware per-row max (simplify_rowspan = FALSE). For each - # group column, fold its cell heights into row_h, but treat suppressed - # cells as contributing zero — so empty trailing rows of a multi-line - # group label are sized purely by their non-group content. When - # suppress_mat is NULL (suppression disabled) every group cell counts, - # which degenerates to the historical per-row max over all cells. - for (g in seq_len(n_grp)) { + # Suppression on: group columns never inflate rows. For each span the + # label is amortised across the span and only grows the start row when + # the deficit is positive (so a multi-line label flows downward into + # the suppressed cells). Innermost-first lets outer spans borrow + # whatever growth inner spans already produced. + for (g in rev(seq_len(n_grp))) { j_mat <- group_col_idx[[g]] if (is.na(j_mat)) next # safety; shouldn't happen - g_heights <- cell_h_mat[page_rows, j_mat] - if (!is.null(suppress_mat)) { - g_heights[suppress_mat[, g]] <- 0 + + starts <- which(!suppress_mat[, g]) + if (length(starts) == 0L) next # entire column suppressed (shouldn't happen) + ends <- c(starts[-1L] - 1L, n_pr) + + for (s_idx in seq_along(starts)) { + ri_start <- starts[[s_idx]] + ri_end <- ends[[s_idx]] + label_h <- cell_h_mat[page_rows[[ri_start]], j_mat] + avail <- sum(row_h[ri_start:ri_end]) + if (label_h > avail + 1e-9) { + row_h[[ri_start]] <- row_h[[ri_start]] + (label_h - avail) + } } - row_h <- pmax(row_h, g_heights) } row_h } @@ -245,10 +224,6 @@ measure_row_heights_tbl <- function(data, resolved_cols, gp_tbl, cell_padding, #' @param group_rule Logical — are group rules drawn? (Reserved for future #' use; currently does not affect pagination because rules are 0-height.) #' @param suppress_repeated_groups Logical, from `tbl$suppress_repeated_groups`. -#' @param simplify_rowspan Logical, from `tbl$simplify_rowspan`. When `FALSE` -#' (default) the per-row height is the per-row max over all cells (the -#' historical behaviour); the span-aware grow-into-suppressed-cells flow -#' only kicks in when this is `TRUE`. #' @return A list of row-page specs, each with `$rows`, `$is_cont_top`, #' `$is_cont_bottom`, `$group_starts`, and `$row_heights_in` (the committed #' per-row heights for that page in inches). @@ -256,8 +231,7 @@ measure_row_heights_tbl <- function(data, resolved_cols, gp_tbl, cell_padding, paginate_rows <- function(data, cell_h_mat, resolved_cols, group_vars, cont_row_h, header_row_h, content_height_in, row_cont_msg, group_rule, - suppress_repeated_groups = TRUE, - simplify_rowspan = FALSE) { + suppress_repeated_groups = TRUE) { n_rows <- nrow(data) # Group boundaries in the *full* data — used for the page-spec $group_starts @@ -285,14 +259,8 @@ paginate_rows <- function(data, cell_h_mat, resolved_cols, group_vars, sup <- if (suppress_repeated_groups && length(group_vars) > 0L) { .compute_cell_suppression(data, group_vars, candidate) } else NULL - # Pass simplify_rowspan to the resolver explicitly. When FALSE, the - # resolver returns the per-row max over non-suppressed cells (so a - # blank trailing row in a multi-line-label group is sized only by its - # non-group content). When TRUE, the resolver does the span-aware - # flow that allows the label to extend across the span. rh <- .compute_page_row_heights( - cell_h_mat, candidate, resolved_cols, group_vars, sup, - simplify_rowspan = simplify_rowspan + cell_h_mat, candidate, resolved_cols, group_vars, sup ) total <- header_row_h + (if (is_cont_top) cont_row_h else 0) + diff --git a/R/tfl_table.R b/R/tfl_table.R index ac4da41..b67eac0 100644 --- a/R/tfl_table.R +++ b/R/tfl_table.R @@ -113,25 +113,17 @@ tfl_colspec <- function(col, #' @param suppress_repeated_groups Logical. When `TRUE` (default), group column #' cells whose value equals the immediately preceding rendered row on the #' same page are left blank. The first data row on each page always shows -#' the group value. -#' @param simplify_rowspan Logical. When `TRUE`, multi-line group-column -#' labels are allowed to flow into the suppressed (blanked) cells below -#' them — HTML-`rowspan`-style — instead of forcing the first row of the -#' group to be tall enough to fit every line. Row heights are resolved -#' per page so a row may render at different heights on different pages -#' when a group is split (the first row on a page re-shows the label and -#' may need to grow to fit it alone). Also: `row_rule` lines that would -#' slice through a flowing label are suppressed within the span, and -#' `group_rule` lines start at the first column whose group value is -#' actually changing at that boundary (so an outer label flowing through -#' the boundary is not visually divided). Default `FALSE`. -#' -#' When `simplify_rowspan = FALSE`, suppressed (blanked) group cells -#' contribute zero height to their row — only the row that actually -#' displays the label inflates, the trailing rows take their height -#' from their non-group content. When suppression is itself disabled -#' (`suppress_repeated_groups = FALSE`), every group cell is rendered -#' on every row so the per-row max over all cells is used. +#' the group value. When suppression is active, multi-line group labels +#' render HTML-`rowspan`-style: the label's full vertical extent flows +#' downward through the blanked cells below it instead of inflating the +#' labelled row alone. Row heights are computed per page (a row may +#' render at different heights on different pages when a group is split, +#' because the first row on a page re-shows the label and may need to +#' grow to fit it alone). `row_rule` lines that would slice through a +#' flowing label are suppressed, and `group_rule` lines start at the +#' first column whose group value is actually changing at that +#' boundary. Set `suppress_repeated_groups = FALSE` to render every +#' group cell on every row instead. #' @param sub_tfl Character vector of column names in `x`, or `NULL` (default). #' When non-NULL, the table is split into one sub-table per unique combination #' of values in these columns. Each sub-table's caption gains a suffix of the @@ -258,7 +250,6 @@ tfl_table <- function(x, allow_col_split = TRUE, balance_col_pages = FALSE, suppress_repeated_groups = TRUE, - simplify_rowspan = FALSE, sub_tfl = NULL, sub_tfl_sep = ": ", sub_tfl_collapse = "; ", @@ -383,7 +374,6 @@ tfl_table <- function(x, checkmate::assert_flag(allow_col_split, .var.name = "allow_col_split") checkmate::assert_flag(balance_col_pages, .var.name = "balance_col_pages") checkmate::assert_flag(suppress_repeated_groups, .var.name = "suppress_repeated_groups") - checkmate::assert_flag(simplify_rowspan, .var.name = "simplify_rowspan") checkmate::assert_flag(show_col_names, .var.name = "show_col_names") checkmate::assert_flag(col_header_rule, .var.name = "col_header_rule") checkmate::assert_flag(group_rule, .var.name = "group_rule") @@ -427,7 +417,6 @@ tfl_table <- function(x, allow_col_split = allow_col_split, balance_col_pages = balance_col_pages, suppress_repeated_groups = suppress_repeated_groups, - simplify_rowspan = simplify_rowspan, sub_tfl = sub_tfl, sub_tfl_sep = sub_tfl_sep, sub_tfl_collapse = sub_tfl_collapse, @@ -516,7 +505,6 @@ print.tfl_table <- function(x, ...) { x$allow_col_split, x$suppress_repeated_groups, x$show_col_names)) cat(sprintf(" col_header_rule=%s group_rule=%s row_rule=%s row_header_sep=%s\n", x$col_header_rule, x$group_rule, x$row_rule, x$row_header_sep)) - cat(sprintf(" simplify_rowspan=%s\n", isTRUE(x$simplify_rowspan))) if (!is.null(x$col_cont_msg)) { cat(sprintf(" col_cont_msg: left=\"%s\" right=\"%s\"\n", x$col_cont_msg[[1L]], x$col_cont_msg[[2L]])) diff --git a/design/DECISIONS.md b/design/DECISIONS.md index 6d42b15..bf2d2e2 100644 --- a/design/DECISIONS.md +++ b/design/DECISIONS.md @@ -945,48 +945,51 @@ Plus: "if there is a grouping column on one page and different behavior on the next page... the handling of the reserved height for column A will differ between the pages." -**Opt-in via `simplify_rowspan` (default `FALSE`).** All of the -behavioural changes that *visibly redistribute* multi-line group labels -(the flow, partial-width rules, span-aware row-rule suppression) are -gated on a new `tfl_table()` argument `simplify_rowspan`, defaulting to -`FALSE`. When `TRUE`, four pieces switch on together: -1. span-aware row heights (the rowspan flow — label flows downward - across suppressed cells); -2. row-rule suppression within a multi-row span; -3. partial-width group rules (rule starts at the outermost changing - group-var column); -4. group rules drawn at *every* transition, even when the new innermost - block has only one row (the historical size-suppression check is - bypassed because partial widths and label-flow alignment make - single-row transitions visually unambiguous). - -**`simplify_rowspan = FALSE` row-height refinement.** Even with the -flow turned off, suppressed (blanked) group cells now contribute zero -to their row's height — so a multi-line group label inflates only the -row that actually displays it, not the trailing rows below. This is a -small backward-incompatible change relative to the strict pre-#29 -layout (where every row's height was the per-row max over every cell, -including blanked ones), but it produces strictly more compact output -and never causes overlap or clipping. Tables that don't have -multi-line group labels render identically to the pre-#29 release. -When suppression is itself disabled (`suppress_repeated_groups = FALSE`), -every group cell is rendered in every row and the per-row max over all -cells is restored. - -**Suppression-aware row rule:** when `simplify_rowspan = TRUE`, the -existing `row_rule` between data rows is suppressed if the next row is -part of a multi-row group span (any suppressed group column on row -`ri+1`). A horizontal line slicing through a label that flows downward -would visually fragment it; HTML rowspan also has no internal borders. -Group rules and `group_rule_after_last` are unaffected because they -only fire at group boundaries (which are also span boundaries). - -**Partial-width group rules.** When `simplify_rowspan = TRUE`, the -group rule line starts at the column corresponding to the *outermost* -group-var level that actually changed at the transition, not at column -1. A new helper `.compute_group_rule_info()` returns both the size and -the outermost-changing level per group_start; drawing reads the level -to set the line's left edge. Concrete result for nested +**Default behaviour whenever suppression is active.** The four +behavioural changes below all turn on together whenever +`suppress_repeated_groups = TRUE` (the package default). No separate +opt-in flag — if the user has asked for suppression, they have asked +for the behaviour that makes suppression visually coherent: +1. span-aware row heights — group columns never inflate row heights; + multi-line labels flow downward through the blanked cells below; +2. row-rule suppression within a multi-row span — a horizontal line + that would slice a flowing label is skipped; +3. partial-width group rules — the rule line starts at the column for + the outermost group_var level that actually changed at the + transition, so unchanged outer columns through which the label is + flowing aren't visually divided; +4. group rules drawn at *every* transition — the historical "skip rule + when the new innermost group has size 1" check is bypassed because + partial widths and label-flow alignment make single-row transitions + visually unambiguous. + +**Opt-out via `suppress_repeated_groups = FALSE`.** Setting suppression +itself to `FALSE` reverts to the strict per-row layout: every group +cell renders fully on every row and each row's height is the per-row +max over every cell. Group rules also revert to full-width. This is +the only "off switch" — the design treats the rowspan flow as the +natural visual rendering of suppression, not a separate feature. + +An earlier iteration of this branch added a `simplify_rowspan` flag +defaulting to `FALSE` (opt-in for the flow). After review feedback +that the row-height behaviour should be the default whenever +suppression is on, the flag was removed: keeping it bifurcated the +mental model into three modes when one suffices. + +**Suppression-aware row rule.** The `row_rule` between data rows is +suppressed when the next row is part of a multi-row group span (any +suppressed group column on row `ri+1`). A horizontal line slicing +through a label that flows downward would visually fragment it; HTML +rowspan also has no internal borders. Group rules and +`group_rule_after_last` are unaffected because they only fire at group +boundaries (which are also span boundaries). + +**Partial-width group rules.** The group rule line starts at the +column corresponding to the *outermost* group-var level that actually +changed at the transition, not at column 1. A new helper +`.compute_group_rule_info()` returns both the size and the +outermost-changing level per group_start; drawing reads the level to +set the line's left edge. Concrete result for nested `group_vars = c("Cohort", "Visit")`: | transition | outermost changer | rule columns | @@ -994,8 +997,9 @@ to set the line's left edge. Concrete result for nested | Visit only | Visit (level 2) | Visit, Value | | Cohort | Cohort (level 1) | Cohort, Visit, Value| -Without `simplify_rowspan`, group rules remain full-width and the -historical `.compute_group_sizes()` suppression continues to apply. +Partial-width rules apply regardless of `suppress_repeated_groups`, +because the rule semantically marks a change at the outermost-changing +level whether or not the unchanged levels' cells are suppressed. **Alternatives considered and rejected:** - *Distribute deficit evenly across rows of the span* — leaves wasted diff --git a/man/paginate_rows.Rd b/man/paginate_rows.Rd index c713e8f..8b47a7a 100644 --- a/man/paginate_rows.Rd +++ b/man/paginate_rows.Rd @@ -14,8 +14,7 @@ paginate_rows( content_height_in, row_cont_msg, group_rule, - suppress_repeated_groups = TRUE, - simplify_rowspan = FALSE + suppress_repeated_groups = TRUE ) } \arguments{ @@ -40,11 +39,6 @@ non-group columns).} use; currently does not affect pagination because rules are 0-height.)} \item{suppress_repeated_groups}{Logical, from \code{tbl$suppress_repeated_groups}.} - -\item{simplify_rowspan}{Logical, from \code{tbl$simplify_rowspan}. When \code{FALSE} -(default) the per-row height is the per-row max over all cells (the -historical behaviour); the span-aware grow-into-suppressed-cells flow -only kicks in when this is \code{TRUE}.} } \value{ A list of row-page specs, each with \verb{$rows}, \verb{$is_cont_top}, diff --git a/man/tfl_table.Rd b/man/tfl_table.Rd index aadef28..85c8079 100644 --- a/man/tfl_table.Rd +++ b/man/tfl_table.Rd @@ -15,7 +15,6 @@ tfl_table( allow_col_split = TRUE, balance_col_pages = FALSE, suppress_repeated_groups = TRUE, - simplify_rowspan = FALSE, sub_tfl = NULL, sub_tfl_sep = ": ", sub_tfl_collapse = "; ", @@ -76,26 +75,17 @@ Default \code{FALSE}.} \item{suppress_repeated_groups}{Logical. When \code{TRUE} (default), group column cells whose value equals the immediately preceding rendered row on the same page are left blank. The first data row on each page always shows -the group value.} - -\item{simplify_rowspan}{Logical. When \code{TRUE}, multi-line group-column -labels are allowed to flow into the suppressed (blanked) cells below -them — HTML-\code{rowspan}-style — instead of forcing the first row of the -group to be tall enough to fit every line. Row heights are resolved -per page so a row may render at different heights on different pages -when a group is split (the first row on a page re-shows the label and -may need to grow to fit it alone). Also: \code{row_rule} lines that would -slice through a flowing label are suppressed within the span, and -\code{group_rule} lines start at the first column whose group value is -actually changing at that boundary (so an outer label flowing through -the boundary is not visually divided). Default \code{FALSE}. - -When \code{simplify_rowspan = FALSE}, suppressed (blanked) group cells -contribute zero height to their row — only the row that actually -displays the label inflates, the trailing rows take their height -from their non-group content. When suppression is itself disabled -(\code{suppress_repeated_groups = FALSE}), every group cell is rendered -on every row so the per-row max over all cells is used.} +the group value. When suppression is active, multi-line group labels +render HTML-\code{rowspan}-style: the label's full vertical extent flows +downward through the blanked cells below it instead of inflating the +labelled row alone. Row heights are computed per page (a row may +render at different heights on different pages when a group is split, +because the first row on a page re-shows the label and may need to +grow to fit it alone). \code{row_rule} lines that would slice through a +flowing label are suppressed, and \code{group_rule} lines start at the +first column whose group value is actually changing at that +boundary. Set \code{suppress_repeated_groups = FALSE} to render every +group cell on every row instead.} \item{sub_tfl}{Character vector of column names in \code{x}, or \code{NULL} (default). When non-NULL, the table is split into one sub-table per unique combination diff --git a/tests/testthat/test-row_span.R b/tests/testthat/test-row_span.R index 7291725..47e768e 100644 --- a/tests/testthat/test-row_span.R +++ b/tests/testthat/test-row_span.R @@ -1,18 +1,14 @@ # test-row_span.R — HTML-rowspan-style flow for multi-line group labels # # Issue #29: a multi-line value in a group column should not force its row to -# be tall enough to fit all label lines. Instead, the label should be -# allowed to flow into the suppressed (blanked) cells in the rows below it, -# the same way HTML reserves a single cell that visually -# spans N rows. +# be tall enough to fit all label lines. Instead, the label flows into the +# suppressed (blanked) cells in the rows below it, the same way HTML +# reserves a single cell that visually spans N rows. # -# Tests here exercise three layers: -# * .compute_page_row_heights() — the core algorithm (synthetic inputs) -# * paginate_rows() — span-aware pagination (per-page recompute) -# * export_tfl() — end-to-end rendering smoke tests -# -# The synthetic-input tests deliberately bypass grid::stringHeight() so the -# expected heights are exact rather than device-dependent. +# This is the default behaviour whenever suppression is active +# (suppress_repeated_groups = TRUE). When suppression is itself off, every +# group cell renders fully on every row and the per-row max over all cells +# is used (the historical layout). # ---- helpers --------------------------------------------------------------- @@ -23,12 +19,9 @@ # ---- .compute_page_row_heights() ------------------------------------------- -test_that("simplify_rowspan = TRUE: two-row span fits in available height; no row grows", { - # cell_h_mat carries the *natural* cell height for every cell. Even - # though A is suppressed at row 2, we set its matrix value to its full - # 2-line label height because the matrix is suppression-agnostic. - cell_h_mat <- matrix(c(2, 2, # column A (label height = 2 lines) - 1, 1), # column B +test_that("two-row span: label flows; both rows stay at non-group height", { + cell_h_mat <- matrix(c(2, 2, # column A (label = 2 lines) + 1, 1), # column B (1 line each) nrow = 2, byrow = FALSE) resolved_cols <- list(.spec("A", is_group_col = TRUE), .spec("B")) suppress_mat <- matrix(c(FALSE, TRUE), nrow = 2, ncol = 1, @@ -36,33 +29,27 @@ test_that("simplify_rowspan = TRUE: two-row span fits in available height; no ro row_h <- writetfl:::.compute_page_row_heights( cell_h_mat, page_rows = 1:2, resolved_cols, - group_vars = "A", suppress_mat = suppress_mat, - simplify_rowspan = TRUE + group_vars = "A", suppress_mat = suppress_mat ) - # Both rows stay at 1 (the non-group-cell height). Label fits in span = 2. expect_equal(row_h, c(1, 1)) }) -test_that("simplify_rowspan = TRUE: single-row span grows to fit the multi-line label", { - cell_h_mat <- matrix(c(2, # A - 1), # B - nrow = 1, byrow = FALSE) +test_that("single-row span: row grows to fit the multi-line label", { + cell_h_mat <- matrix(c(2, 1), nrow = 1, byrow = FALSE) resolved_cols <- list(.spec("A", is_group_col = TRUE), .spec("B")) suppress_mat <- matrix(FALSE, nrow = 1, ncol = 1, dimnames = list(NULL, "A")) row_h <- writetfl:::.compute_page_row_heights( cell_h_mat, page_rows = 1L, resolved_cols, - group_vars = "A", suppress_mat = suppress_mat, - simplify_rowspan = TRUE + group_vars = "A", suppress_mat = suppress_mat ) - # Available = 1, label = 2 → grow to 2. expect_equal(row_h, 2) }) -test_that("simplify_rowspan = TRUE: nested groups, inner absorbed first", { - cell_h_mat <- matrix(c(2, 2, 2, # A - 2, 2, 1, # B +test_that("nested groups: inner span absorbed first, outer borrows remainder", { + cell_h_mat <- matrix(c(2, 2, 2, # A (outer) + 2, 2, 1, # B (inner) 1, 1, 1), # C nrow = 3, byrow = FALSE) resolved_cols <- list( @@ -76,13 +63,12 @@ test_that("simplify_rowspan = TRUE: nested groups, inner absorbed first", { dimnames = list(NULL, c("A", "B"))) row_h <- writetfl:::.compute_page_row_heights( cell_h_mat, page_rows = 1:3, resolved_cols, - group_vars = c("A", "B"), suppress_mat = suppress_mat, - simplify_rowspan = TRUE + group_vars = c("A", "B"), suppress_mat = suppress_mat ) expect_equal(row_h, c(1, 1, 1)) }) -test_that("simplify_rowspan = TRUE: nested groups, inner growth feeds outer span", { +test_that("nested groups: inner growth feeds outer span availability", { cell_h_mat <- matrix(c(5, 5, 5, # A 4, 4, 4, # B 1, 1, 1), # C @@ -98,84 +84,30 @@ test_that("simplify_rowspan = TRUE: nested groups, inner growth feeds outer span dimnames = list(NULL, c("A", "B"))) row_h <- writetfl:::.compute_page_row_heights( cell_h_mat, page_rows = 1:3, resolved_cols, - group_vars = c("A", "B"), suppress_mat = suppress_mat, - simplify_rowspan = TRUE + group_vars = c("A", "B"), suppress_mat = suppress_mat ) expect_equal(row_h, c(3, 1, 1)) }) -test_that("simplify_rowspan = FALSE: suppressed group cells contribute 0 to row height", { - # The user's follow-up: with the rowspan flow OFF, blank trailing rows - # of a multi-line-label group must not inflate to label height. Row 1 - # shows the 2-line A label and rises to 2; rows 2 and 3 are 1 line each - # because their A cell is suppressed and contributes 0. - cell_h_mat <- matrix(c(2, 2, 2, # A - 1, 1, 1), # B - nrow = 3, byrow = FALSE) - resolved_cols <- list(.spec("A", is_group_col = TRUE), .spec("B")) - suppress_mat <- matrix(c(FALSE, TRUE, TRUE), nrow = 3, ncol = 1, - dimnames = list(NULL, "A")) - - row_h <- writetfl:::.compute_page_row_heights( - cell_h_mat, page_rows = 1:3, resolved_cols, - group_vars = "A", suppress_mat = suppress_mat, - simplify_rowspan = FALSE - ) - expect_equal(row_h, c(2, 1, 1)) -}) - -test_that("simplify_rowspan = FALSE: nested groups also zero out suppressed cells", { - # Outer A is 3 lines, inner B is 2 lines. Row 1: both shown → row_h = 3. - # Row 2: both suppressed → row_h = 1 (just C). - # Row 3: A still suppressed, B re-shows (different value) → row_h = max(1, 2) = 2. - cell_h_mat <- matrix(c(3, 3, 3, # A - 2, 2, 2, # B - 1, 1, 1), # C - nrow = 3, byrow = FALSE) - resolved_cols <- list( - .spec("A", is_group_col = TRUE), - .spec("B", is_group_col = TRUE), - .spec("C") - ) - suppress_mat <- matrix(c(FALSE, TRUE, TRUE, - FALSE, TRUE, FALSE), - nrow = 3, ncol = 2, - dimnames = list(NULL, c("A", "B"))) - row_h <- writetfl:::.compute_page_row_heights( - cell_h_mat, page_rows = 1:3, resolved_cols, - group_vars = c("A", "B"), suppress_mat = suppress_mat, - simplify_rowspan = FALSE - ) - expect_equal(row_h, c(3, 1, 2)) -}) - test_that("suppress_mat = NULL: every cell counts (per-row max over all)", { - # When suppression is disabled, every cell is rendered, so the row - # needs to accommodate its tallest cell — including the group cell. - # Result is the same regardless of simplify_rowspan. - cell_h_mat <- matrix(c(2, 2, # A (group, but not suppressed) + # Suppression disabled — every group cell renders fully on every row, so + # the row max is taken over all cells (group and non-group alike). + cell_h_mat <- matrix(c(2, 2, # A (group, but rendered every row) 1, 1), # B nrow = 2, byrow = FALSE) resolved_cols <- list(.spec("A", is_group_col = TRUE), .spec("B")) expect_equal( writetfl:::.compute_page_row_heights( cell_h_mat, page_rows = 1:2, resolved_cols, - group_vars = "A", suppress_mat = NULL, simplify_rowspan = FALSE - ), - c(2, 2) - ) - expect_equal( - writetfl:::.compute_page_row_heights( - cell_h_mat, page_rows = 1:2, resolved_cols, - group_vars = "A", suppress_mat = NULL, simplify_rowspan = TRUE + group_vars = "A", suppress_mat = NULL ), c(2, 2) ) }) -test_that("no group_vars degenerates to per-row max", { - cell_h_mat <- matrix(c(3, 1, # A - 1, 4), # B +test_that("no group_vars: per-row max over all columns", { + cell_h_mat <- matrix(c(3, 1, + 1, 4), nrow = 2, byrow = FALSE) resolved_cols <- list(.spec("A"), .spec("B")) row_h <- writetfl:::.compute_page_row_heights( @@ -199,21 +131,13 @@ test_that("zero-row page returns numeric(0)", { # ---- paginate_rows() — span-aware fit + orphan handling -------------------- -test_that("3-row group with 3-line label fits on one page (free-row)", { - # Property (a) from the design: adding a row to an open span can leave the - # page total unchanged. 3-row group, label = 3 lines, non-group = 1 line. - # Per-row independent sum would be 3 + 1 + 1 = 5 lines. Span-aware total - # is max(3, 3) = 3 lines, so the page holds all three rows in a 3-line - # content height. - # - # Note: cell_h_mat carries the *natural* cell height regardless of - # suppression — pagination's per-page recompute reads the current span - # start row's cell value, which may differ between pages if the same group - # is split across pages (label re-shown on a new page). +test_that("3-row group, 3-line label fits on one page (free-row property)", { + # Adding rows 2 and 3 to the open span doesn't change the page total — + # the span absorbs the deficit that initially inflated row 1. data <- data.frame(grp = rep("L1\nL2\nL3", 3L), val = c("a", "b", "c"), stringsAsFactors = FALSE) - cell_h_mat <- matrix(c(3, 3, 3, # grp (label = 3 lines, regardless of suppression) + cell_h_mat <- matrix(c(3, 3, 3, # grp 1, 1, 1), # val nrow = 3, byrow = FALSE) resolved_cols <- list(.spec("grp", is_group_col = TRUE), .spec("val")) @@ -224,29 +148,23 @@ test_that("3-row group with 3-line label fits on one page (free-row)", { cont_row_h = 0, header_row_h = 0, content_height_in = 3, row_cont_msg = c("(continued above)", "(continued below)"), - group_rule = FALSE, - simplify_rowspan = TRUE + group_rule = FALSE ) expect_length(pages, 1L) expect_equal(pages[[1L]]$rows, 1:3) - # Each row is 1 line; the label flows. expect_equal(pages[[1L]]$row_heights_in, c(1, 1, 1)) }) test_that("group orphan: lone first-row on a page grows to fit full label", { - # Property (b): when the rest of a group is pushed to the next page, the - # row that lands alone on the new page must be tall enough for the full - # (re-shown) label by itself. - # - # Setup: 4-row group with label = 4 lines, val = 1 line for rows 1-3 and - # 2 lines for row 4. Adding row 4 to the [1..3] page would push the total - # over the budget (because val = 2), so row 4 gets flushed to its own page - # where, as a single-row span, it must grow to fit the 4-line label. + # 4-row group with label = 4 lines, val = 1 line for rows 1-3 and 2 lines + # for row 4. Adding row 4 to the [1..3] page would push the total over + # the budget, so row 4 gets flushed to its own page where, as a single- + # row span, it must grow to fit the 4-line label. data <- data.frame(grp = rep("L1\nL2\nL3\nL4", 4L), val = c("a", "b", "c", "d\ne"), stringsAsFactors = FALSE) cell_h_mat <- matrix(c(4, 4, 4, 4, # grp - 1, 1, 1, 2), # val (row 4 is 2 lines) + 1, 1, 1, 2), # val nrow = 4, byrow = FALSE) resolved_cols <- list(.spec("grp", is_group_col = TRUE), .spec("val")) @@ -258,31 +176,23 @@ test_that("group orphan: lone first-row on a page grows to fit full label", { cont_row_h = 1, header_row_h = 0, content_height_in = 5, row_cont_msg = c("(continued above)", "(continued below)"), - group_rule = FALSE, - simplify_rowspan = TRUE + group_rule = FALSE ), warning = function(w) { warns <<- c(warns, conditionMessage(w)); invokeRestart("muffleWarning") } ) - # The group split must trigger the (continued) warning. expect_true(any(grepl("continued", warns))) - # Two pages: rows 1-3 on page 1 (label spans them with deficit on row 1), - # row 4 alone on page 2 with full label height. expect_length(pages, 2L) expect_equal(pages[[1L]]$rows, 1:3) - # Page 1: span = [1,2,3], avail = 3, label = 4 → deficit 1 on row 1. expect_equal(pages[[1L]]$row_heights_in, c(2, 1, 1)) expect_equal(pages[[2L]]$rows, 4L) - # Page 2 orphan: single-row span re-shows the 4-line label. expect_equal(pages[[2L]]$row_heights_in, 4) }) -test_that("pagination reset per page: re-shown label on next page sized correctly", { - # Same setup as the orphan test but framed around "the same data renders - # at different row heights on different pages because suppression resets - # at every page boundary". Page 2's row 4 must be tall enough for the - # re-shown label (= 4 lines) even though it was 1 line on page 1's matrix. +test_that("pagination reset per page: orphan re-shows the label and grows", { + # The same data renders at different row heights on different pages + # because suppression resets at every page boundary. data <- data.frame(grp = rep("L1\nL2\nL3\nL4", 4L), val = c("a", "b", "c", "d\ne"), stringsAsFactors = FALSE) @@ -297,41 +207,15 @@ test_that("pagination reset per page: re-shown label on next page sized correctl cont_row_h = 1, header_row_h = 0, content_height_in = 5, row_cont_msg = c("(continued above)", "(continued below)"), - group_rule = FALSE, - simplify_rowspan = TRUE + group_rule = FALSE )) - # Page 2 orphan: row 4 alone at 4 lines (label height). This is the - # "same row may render differently on different pages" property. expect_equal(pages[[2L]]$row_heights_in, 4) }) -test_that("simplify_rowspan = FALSE: only the labelled row inflates (suppressed → 0)", { - # Without the opt-in, multi-line group labels still inflate the labelled - # row, but the suppressed (blanked) trailing rows are sized only by their - # non-group content — they do *not* inherit the label height. - data <- data.frame(grp = rep("L1\nL2\nL3", 3L), - val = c("a", "b", "c"), - stringsAsFactors = FALSE) - cell_h_mat <- matrix(c(3, 3, 3, # grp (label = 3 lines, every row's natural value) - 1, 1, 1), # val - nrow = 3, byrow = FALSE) - resolved_cols <- list(.spec("grp", is_group_col = TRUE), .spec("val")) - - pages <- writetfl:::paginate_rows( - data, cell_h_mat, resolved_cols, - group_vars = "grp", - cont_row_h = 0, header_row_h = 0, - content_height_in = 9, # generous so all rows fit - row_cont_msg = c("(continued above)", "(continued below)"), - group_rule = FALSE - # simplify_rowspan defaults to FALSE - ) - expect_equal(pages[[1L]]$row_heights_in, c(3, 1, 1)) -}) - -test_that("simplify_rowspan = FALSE without suppression: every cell counts (mode A)", { - # If suppression is also off, every group cell is fully rendered on - # every row, so each row inflates to label height. +test_that("paginate_rows: suppress_repeated_groups = FALSE inflates every row", { + # If suppression is itself disabled, every group cell renders on every + # row, so each row inflates to label height. This is the strict + # historical layout and the only way to opt out of the rowspan flow. data <- data.frame(grp = rep("L1\nL2\nL3", 3L), val = c("a", "b", "c"), stringsAsFactors = FALSE) @@ -354,17 +238,14 @@ test_that("simplify_rowspan = FALSE without suppression: every cell counts (mode # ---- end-to-end via export_tfl() ------------------------------------------- -test_that("end-to-end: simplify_rowspan = TRUE renders user's example without error", { - # The user's exact example from issue #29: - # page 1: A = c("B\nC", "B\nC"), D = c("E", "F") - # page 2 (after suppression reset): A = "B\nC", D = "F" +test_that("end-to-end: user's two-page rowspan example renders without error", { df <- dplyr::group_by( data.frame(A = rep("B\nC", 3L), D = c("E", "F", "G"), stringsAsFactors = FALSE), A ) - tbl <- tfl_table(df, simplify_rowspan = TRUE) + tbl <- tfl_table(df) f <- tempfile(fileext = ".pdf") on.exit(unlink(f)) expect_no_error( @@ -377,34 +258,15 @@ test_that("end-to-end: simplify_rowspan = TRUE renders user's example without er expect_gt(file.info(f)$size, 0) }) -test_that("end-to-end: row_rule with simplify_rowspan = TRUE does not error in spans", { - # Smoke test for the row-rule suppression-within-span branch. +test_that("end-to-end: row_rule with a multi-row span renders without error", { df <- dplyr::group_by( data.frame(A = c("X\nY", "X\nY", "X\nY"), B = c("p", "q", "r"), stringsAsFactors = FALSE), A ) - tbl <- tfl_table(df, row_rule = TRUE, simplify_rowspan = TRUE) + tbl <- tfl_table(df, row_rule = TRUE) f <- tempfile(fileext = ".pdf") on.exit(unlink(f)) expect_no_error(export_tfl(tbl, file = f)) }) - -test_that("end-to-end: default (simplify_rowspan = FALSE) renders historically", { - # Same input as the user-example test but without the opt-in. The - # default behaviour (label inflates first row of group) must keep working - # without errors and produce a non-empty PDF. - df <- dplyr::group_by( - data.frame(A = rep("B\nC", 3L), - D = c("E", "F", "G"), - stringsAsFactors = FALSE), - A - ) - tbl <- tfl_table(df) # simplify_rowspan defaults to FALSE - f <- tempfile(fileext = ".pdf") - on.exit(unlink(f)) - expect_no_error(export_tfl(tbl, file = f, pg_width = 11, pg_height = 8.5)) - expect_true(file.exists(f)) - expect_gt(file.info(f)$size, 0) -})