Merge pull request #273 from fiddlerwoaroof/fix/pb30-col-toggle-follow

Pin the column toggle's cursor-follow; a group lookup stops at its row (pb30, 0r01)

context/code/core.md

@@ -302,7 +302,10 @@ previous survivor, pruned against unpruned. It runs over five fixtures,
 including a non-contiguous group, a level with no groups, and two adjacent
 outer groups. Without the non-contiguous fixture, a pruner that keyed runs by
 name instead of header position passed every test. `open_runs` is the one
-reading of the markers: `innermost_group` and the tests both use it.
+reading of the markers: `innermost_group` and the tests both use it. It is a
+lazy iterator, so a single lookup (`z`, which asks about one row) walks the axis
+only up to that row, rather than building and cloning a stack for every row of
+it (improvise-0r01).
 
 A header's level also names its category (`cats[level]`) on the wire, where
 `FrameGroup` carries `level` and not the category (improvise-7xyx removed that

context/test-evidence/command.md

@@ -6260,3 +6260,24 @@ ticket was written at `a4910eb4`, when every row test sat at `(0, 0)`. Since
 then, the grid tests seat the cursor on `Apr` (row 2). Injecting
 `row_group_for(0)` fails five of them, plus `z_on_a_row_past_an_inner_run_folds_the_outer_group`
 from improvise-e265.
+
+### a_column_collapse_lands_on_the_cursors_own_run_and_keeps_its_row
+
+improvise-pb30. `gz` (`ToggleGroupAtCursor { is_row: false }`) puts the cursor
+on the collapsed group afterwards, through `landing` and `place_cursor`. The
+column arm of both was untested: three rivals passed all 1948 tests. The
+fixture puts two column levels (`Region` over `Month`) and a two-item row
+category in place. The cursor sits at `(1, 7)`, which is South/May, the tail of
+South's `Q2` run, and off the origin on both axes. The test asserts:
+- the collapsed column frame;
+- that the cursor lands on `(1, 5)`, South's `Q2`, on the same row.
+
+Rivals, each injected and run over the whole workspace. Each fails this test
+and nothing else:
+- **`place_cursor`'s column arm swapped** (`(at, ctx.selected.0)`): the cursor
+  lands transposed.
+- **`landing` returning `None` for columns**: the cursor is left where the
+  toggle's reframe put it.
+- **`landing` reading `col_tuple(0)`**: the prefix is North's, so the cursor
+  lands on North's `Q2`. This one needs the two column levels. With one level,
+  the prefix is empty and every column reads the same.

context/test-evidence/core.md

@@ -1604,3 +1604,18 @@ folded:
 
 Failed at base 202e7eac on the second assertion: the flat reading said `None`.
 The flat-membership rival fails it again.
+
+### a_lookup_does_not_read_past_its_row
+
+improvise-0r01, which is ChatGPT Codex's P2 on #263. `innermost_group` built
+`open_runs` for the whole axis, cloning a level stack per row, to answer for one
+row on every `z` keypress. `open_runs` is now lazy and the lookup takes
+`.nth(idx)`.
+
+The walk's extent cannot be seen through a well-formed axis, so the test builds
+an axis whose second header is unreadable on purpose: `level: usize::MAX`, which
+`Vec::resize` cannot allocate. A lookup for row 0 must answer `Upper` without
+reaching it.
+
+Rival: collecting `open_runs` before `.nth(idx)`, which is the eager shape. It
+fails with `capacity overflow`, and the other 335 core tests pass.

crates/improvise-command/src/command/cmd/grid.rs

@@ -212,6 +212,65 @@ mod tests {
         );
     }
 
+    /// The column twin of the row landing tests: two column levels, the cursor off
+    /// both origins (improvise-pb30). Evidence: `context/test-evidence/command.md`.
+    #[test]
+    fn a_column_collapse_lands_on_the_cursors_own_run_and_keeps_its_row() {
+        use crate::command::key::{KeyCode, KeyModifiers};
+        use crate::ui::app::App;
+        use crate::view::Axis;
+
+        let mut wb = crate::workbook::Workbook::new("T");
+        wb.add_category("Region").unwrap();
+        wb.add_category("Month").unwrap();
+        wb.add_category("Type").unwrap();
+        wb.add_item("Region", "North").unwrap();
+        wb.add_item("Region", "South").unwrap();
+        for (month, quarter) in [("Jan", "Q1"), ("Feb", "Q1"), ("Apr", "Q2"), ("May", "Q2")] {
+            wb.add_item_in_group("Month", month, quarter).unwrap();
+        }
+        wb.add_item("Type", "Food").unwrap();
+        wb.add_item("Type", "Tools").unwrap();
+        wb.active_view_mut().set_axis("Region", Axis::Column);
+        wb.active_view_mut().set_axis("Month", Axis::Column);
+        wb.active_view_mut().set_axis("Type", Axis::Row);
+        let mut app = App::new(wb, None);
+        app.rebuild_layout();
+        assert_eq!(app.layout.col_cats, ["Region", "Month"], "Month is inner");
+        app.cursor_mut().selected = (1, 7);
+        assert_eq!(
+            app.layout.col_tuple(7),
+            Some(&["South".into(), "May".into()][..]),
+            "the cursor sits on South/May, the tail of South's Q2 run"
+        );
+
+        let effects = {
+            let ctx = app.cmd_context(KeyCode::Char('z'), KeyModifiers::NONE);
+            ToggleGroupAtCursor { is_row: false }.execute(&ctx)
+        };
+        app.apply_effects(effects);
+
+        let cols: Vec<String> = (0..app.layout.col_count())
+            .map(|c| app.layout.col_label(c))
+            .collect();
+        assert_eq!(
+            cols,
+            [
+                "North/Jan",
+                "North/Feb",
+                "North/Q2",
+                "South/Jan",
+                "South/Feb",
+                "South/Q2"
+            ]
+        );
+        assert_eq!(
+            app.cursor().selected,
+            (1, 5),
+            "South's Q2 column, on the row the cursor was on"
+        );
+    }
+
     /// Evidence: `context/test-evidence/command.md`.
     #[test]
     fn toggling_a_group_collapses_that_group_and_the_cursor_follows_it() {

crates/improvise-core/src/view/layout.rs

@@ -349,29 +349,35 @@ fn remark(input: &OpenRuns, output: &mut Vec<Option<usize>>, kept: &mut Vec<Axis
 }
 
 /// Each data item's open runs, by level, as the position of the header that
-/// opened each: the one reading of the markers (improvise-e265).
-fn open_runs(items: &[AxisEntry]) -> Vec<Vec<Option<usize>>> {
+/// opened each: the one reading of the markers, lazy so a lookup stops (0r01).
+fn open_runs(items: &[AxisEntry]) -> impl Iterator<Item = Vec<Option<usize>>> + '_ {
     let mut open: Vec<Option<usize>> = Vec::new();
-    let mut out = Vec::new();
-    for (at, entry) in items.iter().enumerate() {
-        match entry {
+    items
+        .iter()
+        .enumerate()
+        .filter_map(move |(at, entry)| match entry {
             AxisEntry::GroupHeader { level, .. } => {
                 open.truncate(*level);
                 open.resize(*level, None);
                 open.push(Some(at));
+                None
             }
-            AxisEntry::GroupEnd { level } => open.truncate(*level),
-            AxisEntry::DataItem { .. } => out.push(open.clone()),
-        }
-    }
-    out
+            AxisEntry::GroupEnd { level } => {
+                open.truncate(*level);
+                None
+            }
+            AxisEntry::DataItem { .. } => Some(open.clone()),
+        })
 }
 
 /// The innermost run open at the data item `idx` — the group nearest it, which
 /// is what `z` toggles (improvise-e265's ruling).
 fn innermost_group(items: &[AxisEntry], idx: usize) -> Option<(String, String)> {
-    let runs = open_runs(items);
-    let at = runs.get(idx)?.iter().rev().find_map(|run| *run)?;
+    let at = open_runs(items)
+        .nth(idx)?
+        .into_iter()
+        .rev()
+        .find_map(|run| run)?;
     match &items[at] {
         AxisEntry::GroupHeader {
             cat_name,
@@ -2228,7 +2234,8 @@ mod group_membership {
             let kept: Vec<usize> = (0..data_items(full).count())
                 .filter(|i| mask & (1 << i) != 0)
                 .collect();
-            let (full_runs, pruned_runs) = (open_runs(full), open_runs(pruned));
+            let full_runs: Vec<_> = open_runs(full).collect();
+            let pruned_runs: Vec<_> = open_runs(pruned).collect();
             assert_eq!(pruned_runs.len(), kept.len(), "{name}: the survivors");
             if kept.len() == full_runs.len() {
                 assert_eq!(pruned, full, "{name}: nothing pruned");
@@ -2301,6 +2308,30 @@ mod group_membership {
         assert_eq!(l.row_group_for(0), Some(("Region".into(), "Upper".into())));
     }
 
+    /// A lookup reads only up to its own row (improvise-0r01). The header after
+    /// row 0 is unreadable on purpose: reaching it would try to allocate usize::MAX.
+    #[test]
+    fn a_lookup_does_not_read_past_its_row() {
+        let items = vec![
+            AxisEntry::GroupHeader {
+                level: 0,
+                cat_name: "Region".into(),
+                group_name: "Upper".into(),
+            },
+            AxisEntry::item(vec!["North".into()]),
+            AxisEntry::GroupHeader {
+                level: usize::MAX,
+                cat_name: "Nowhere".into(),
+                group_name: "Unreachable".into(),
+            },
+            AxisEntry::item(vec!["South".into()]),
+        ];
+        assert_eq!(
+            super::innermost_group(&items, 0),
+            Some(("Region".into(), "Upper".into()))
+        );
+    }
+
     /// The user's ruling on improvise-e265: membership is per level, and `z`
     /// names the innermost open group — the one nearest the cursor.
     #[test]