diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index f51186c9..c51bf735 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -939,7 +939,12 @@ the regression was live whenever `prune_empty` was on. The fix distinguishes the two emptinesses **without consulting the `View`**, which `GridLayout` has already dropped: a run that contained no `DataItem` at all was never expanded, i.e. collapsed, and its header survives; a run that -contained items the predicate rejected is genuinely empty and goes. A +contained items the predicate rejected is genuinely empty and goes. **Superseded +twice** (see `an_outer_group_whose_rows_all_prune_loses_its_header`). Since +improvise-6qet a collapsed group emits its fold as a `DataItem`, so no flat run +is item-less. The one run that still was, an outer group's header followed +directly by an inner group's header, was kept by that arm when it should have +gone (improvise-b5zh). The arm is deleted. A `GroupEnd` is kept whenever a run is open in the output, so a re-opened earlier header is closed even though the header between them was pruned. That keeps gotcha 1's shape — still a predicate over the data index, no `&View` @@ -1362,6 +1367,29 @@ fold is the row `z` names the group from. Note the narrowing this implies: an its header goes with it. lhi4 kept that header because it was the only way back to the rows; the fold is now that way back. +### an_outer_group_whose_rows_all_prune_loses_its_header + +improvise-b5zh asked whether `close_pending_run`'s "a header whose run saw no item +is collapsed, and survives" arm was dead since improvise-6qet, and proposed +deleting it. It was **not dead**, and where it ran it was wrong. With two grouped +categories on one axis (`Region` with `North[Upper]`, `South[Lower]`, outside +`Month` with `Jan`, `Feb` in `Q1`), `cross_product` puts each outer header +directly before an inner one: `[Upper, Q1, North/Jan, …]`. The inner header +closed the outer header's run as "saw no item", so `Upper` was kept even with +every North row pruned. At `1a6b877` the pruned axis was +`[Upper, GroupEnd, Lower, Q1, South/Jan, South/Feb]`, an orphan header drawn +over nothing. The fix keeps the pending headers as a stack: a header closes the +pending header of its **own** category and everything nested inside it, a kept +item flushes the whole stack, and a `GroupEnd` drops the innermost. Rival: a +header clears every pending header, which is the flat reading of "a header ends +the previous run". Only this test fails, because `Lower` is lost when `Q1` +arrives. The flat-case tests (lhi4, u6gc, the partly-pruned group) pass +unchanged under both versions, which is why none of them could see this. + +One pre-existing shape is left alone and stated: two consecutive `GroupEnd`s +(an inner and an outer run ending together) keep only the first, because +`run_open` is a single flag. That was true before this change, pruned or not. + ### records_steps_are_the_spine_then_every_non_index_dim_category_on_none improvise-t0ma. Before this, `records_steps` was guarded by one incidental test: the diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index fbb3e312..a7a82ff1 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -282,60 +282,53 @@ fn data_index_to_visual(items: &[AxisEntry], data_idx: usize) -> Option { .nth(data_idx) } -/// The run a header opened has ended. A header still pending saw either no -/// item at all — it is collapsed, and survives — or only rejected ones. -fn close_pending_run( - kept: &mut Vec, - pending: Option, - had_item: bool, -) -> bool { - pending - .filter(|_| !had_item) - .map(|h| kept.push(h)) - .is_some() +/// A header ends the pending run of its OWN category and every run nested inside +/// it; an outer category's header is still waiting for an item (improvise-b5zh). +fn close_runs_of(pending: &mut Vec, header: &AxisEntry) { + let cat_of = |e: &AxisEntry| match e { + AxisEntry::GroupHeader { cat_name, .. } => Some(cat_name.clone()), + AxisEntry::GroupEnd | AxisEntry::DataItem { .. } => None, + }; + if let Some(at) = pending.iter().position(|h| cat_of(h) == cat_of(header)) { + pending.truncate(at); + } } /// Drop the data items `keep` rejects, keeping a header only if an item under -/// it survives or the group is collapsed. A predicate — why: core.md §6. +/// it survives. A predicate — why: core.md §6. fn prune_axis_items(items: &mut Vec, keep: impl Fn(usize) -> bool) { let mut kept: Vec = Vec::new(); - let mut pending_header: Option = None; - let mut run_had_item = false; + // Headers no kept item has reached yet, outermost first. + let mut pending: Vec = Vec::new(); let mut run_open = false; let mut data_idx = 0usize; for entry in items.drain(..) { match &entry { - // A header ends the previous run: another group's header is where - // `expand_category` stops emitting a `GroupEnd` (improvise-gd83). + // Another group's header is where `expand_category` stops emitting a + // `GroupEnd` (improvise-gd83), so it closes that group's run. AxisEntry::GroupHeader { .. } => { - run_open |= close_pending_run(&mut kept, pending_header.take(), run_had_item); - pending_header = Some(entry); - run_had_item = false; + close_runs_of(&mut pending, &entry); + pending.push(entry); } // Kept whenever a run is open in `kept`, even where the header it // closed was pruned: an earlier one is then re-opened (u6gc). AxisEntry::GroupEnd => { - run_open |= close_pending_run(&mut kept, pending_header.take(), run_had_item); - run_had_item = false; + pending.pop(); if run_open { kept.push(entry); run_open = false; } } AxisEntry::DataItem { .. } => { - run_had_item = true; if keep(data_idx) { - if let Some(h) = pending_header.take() { - kept.push(h); - run_open = true; - } + run_open |= !pending.is_empty(); + kept.append(&mut pending); kept.push(entry); } data_idx += 1; } } } - close_pending_run(&mut kept, pending_header.take(), run_had_item); *items = kept; } @@ -1697,6 +1690,46 @@ mod tests { ); } + /// improvise-b5zh: an OUTER header is followed by an inner one, not by an item + /// — see context/test-evidence/core.md. + #[test] + fn an_outer_group_whose_rows_all_prune_loses_its_header() { + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_category("Month").unwrap(); + wb.add_category("Type").unwrap(); + wb.add_item_in_group("Region", "North", "Upper").unwrap(); + wb.add_item_in_group("Region", "South", "Lower").unwrap(); + wb.add_item_in_group("Month", "Jan", "Q1").unwrap(); + wb.add_item_in_group("Month", "Feb", "Q1").unwrap(); + wb.add_item("Type", "Food").unwrap(); + for month in ["Jan", "Feb"] { + wb.model.set_cell( + CellKey::new(vec![ + ("Region".into(), "South".into()), + ("Month".into(), month.into()), + ("Type".into(), "Food".into()), + ]), + CellValue::Number(1.0), + ); + } + wb.active_view_mut().set_axis("Region", Axis::Row); + wb.active_view_mut().set_axis("Month", Axis::Row); + wb.active_view_mut().set_axis("Type", Axis::Column); + wb.active_view_mut().prune_empty = true; + let layout = GridLayout::with_frozen_records(&wb.model, wb.active_view(), None); + assert_eq!(layout.row_count(), 2, "only South's rows are funded"); + assert!( + has_header(&layout.row_items, "Lower"), + "South's group stays" + ); + assert!( + !has_header(&layout.row_items, "Upper"), + "every North row pruned, so Upper has nothing under it: {:?}", + layout.row_items + ); + } + /// The commonest case and the one the other three cannot see: SOME of a /// group's items prune. context/test-evidence/core.md. #[test]