pt/pf_app
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix the depth steppers' off-by-one, and fit row labels on restore

Browse Source 
The stepper's depth and ViewConfig's group_by_depth/split_by_depth are
the same unit -- a count of levels to show -- but applyDepth and
applySplitDepth sent d + 1 and every readback subtracted the same 1.
Self-consistent, so the toolbar always agreed with itself, and off by
one against the grid at every position: picking partgroup showed one
level past it. server.cpp turns group_by_depth into set_depth(n - 1)
and t_traversal::set_depth adds the 1 back, so n asks for n levels.

The +1 existed to dodge depth 0, which reaches the engine as
set_depth(-1) on an unsigned t_depth, underflows, and clamps to fully
expanded. The value is floored at 1 instead, so the stepper's "Total"
slot goes on showing the first level as it always has -- the wart is
kept deliberately rather than traded for the underflow. A bare total
needs group_rollup_mode.

Separately: fitRowLabels() has been written and unused since it was
judged to read worse than the default spacing. That judgement has
changed -- row labels come back clipped after a restore -- so it is now
called after both restore paths, initViewer and applyLayout. It is not
routed through the Fit widths button on purpose: that path goes via
regular-table's auto-size, which measures the leaf header's rendered
box over viewport rows only, while fitRowLabels canvas-measures the
label text.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Paul Trowbridge 2026-09-22 23:44:07 -04:00
parent cf8b72d737
commit 87ad5b1997
2 changed files with 64 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
CLAUDE.md
View File

@ -277,11 +277,24 @@ levels a two-field `group_by` does not have.
`depth` is null until the first layout has been read back; the stepper shows an
em dash rather than guessing a level.
Both are **1-based**: they count the levels to show, where the imperative
`view.set_depth()` counts the boundary below them. `server.cpp` does
`ctx1->set_depth(row_pivot_depth - 1)` and
`ctx2->set_depth(HEADER_COLUMN, column_pivot_depth - 1)`, so the toolbar sends
`d + 1` and subtracts one again when reading a layout back.
Both are **1-based counts of the levels to show**, and so is the stepper's own
`depth` — `DepthButtons` labels `d` as `levels[d - 1]`. Same unit on both
sides, so the value crosses unchanged in each direction. The engine agrees:
`server.cpp` turns `group_by_depth` into `set_depth(n - 1)` and
`t_traversal::set_depth` adds the 1 back, so `n` renders `n` levels, on both
axes (`ctx2->set_depth(HEADER_COLUMN, column_pivot_depth - 1)` is the same
arithmetic).
The toolbar used to send `d + 1` and subtract the same 1 when reading a layout
back. That was self-consistent — the stepper always agreed with itself — and
off by one against the grid at every position: picking `partgroup` showed one
level past it. The `+1` was there to dodge `depth 0`, which reaches the engine
as `set_depth(-1)` on an unsigned `t_depth`, underflows, and clamps to *fully
expanded*. `applyDepth` now floors the value at 1 instead, so the stepper's
**"Total" slot still shows the first level rather than a bare total** — the
long-standing wart, kept deliberately rather than traded for the underflow. A
real total needs `group_rollup_mode: 'total'`, which is a field in
`ViewConfigUpdate` but is unverified against the local WASM engine.
Because a depth lives in the config, it survives every view rebuild, rides into
the persisted and named layouts through `viewer.save()`, and needs nothing

64
ui/src/views/Forecast.jsx
View File

@ -929,12 +929,15 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio
await viewer.restore(cfg)
// split_full is the legacy key, from when collapsing truncated split_by
adoptSplit(cfg.split_full?.length ? cfg.split_full : cfg.split_by,
cfg.split_by_depth != null ? cfg.split_by_depth - 1 : null)
cfg.split_by_depth != null ? cfg.split_by_depth : null)
setGroupFull(cfg.group_by || [])
// restore() has already applied group_by_depth; this only syncs the
// toolbar. expand_depth is the legacy key, from when depth was imperative
// toolbar. The config's count and the stepper's are the same unit, so
// it reads across as it is -- it used to subtract the 1 that applyDepth
// added, which kept the toolbar agreeing with itself and with nothing
// else. expand_depth is the legacy key, from when depth was imperative
// and had to be stored beside the config rather than in it.
if (cfg.group_by_depth != null) setExpandDepth(cfg.group_by_depth - 1)
if (cfg.group_by_depth != null) setExpandDepth(cfg.group_by_depth)
else if (cfg.expand_depth != null) await applyDepth(cfg.expand_depth)
} else {
// The default is a published layout flagged is_default. A version-scoped
@ -970,6 +973,7 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio
// throw here would abort the rest of initViewer silently.
try {
await ensureRowLabelWidth()
await fitRowLabelsNow()
} catch (err) {
console.error('[pf-layout] threw', err)
}
@ -982,7 +986,7 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio
// collapse apart from the user rearranging the pivot.
const live = await viewer.save()
adoptSplit(live.split_by || [],
live.split_by_depth != null ? live.split_by_depth - 1 : null)
live.split_by_depth != null ? live.split_by_depth : null)
setGroupFull(live.group_by || [])
// the plugin element is replaced when the plugin changes, so re-assert
hideSplitTotal()
@ -1097,9 +1101,9 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio
if (!levels) return
const depth = Math.max(0, Math.min(n, levels))
try {
// 1-based, as with group_by_depth: server.cpp does
// ctx2->set_depth(HEADER_COLUMN, column_pivot_depth - 1)
await viewer.restore({ ...cfg, split_by_depth: depth + 1 })
// A count of levels to show, exactly as group_by_depth is -- the engine
// treats both axes identically. See applyDepth for why 0 is floored to 1.
await viewer.restore({ ...cfg, split_by_depth: Math.max(1, depth) })
setSplitDepth(depth)
} catch (err) {
console.error('[applySplitDepth]', err)
@ -1348,6 +1352,20 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio
await grid.draw({ invalid_columns: true })
}
// fitRowLabels takes the grid; this finds it, so the restore paths can call it
// the same way they call ensureRowLabelWidth.
//
// Deliberately not routed through the Fit widths button: that one goes via
// regular-table's own auto-size, which measures the leaf header's rendered box
// over the rows currently in the viewport. fitRowLabels measures the label
// text on a canvas instead, so it is not blinded by a clipped cell and does
// not depend on what happens to be scrolled into view.
async function fitRowLabelsNow() {
const grid = (await viewerRef.current?.getPlugin())?.regular_table
if (!grid?.saveColumnSizes) return
await fitRowLabels(grid)
}
const GRID_CSS = `
/* Let headers and row labels size their own column.
*
@ -1426,18 +1444,25 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio
async function applyDepth(d) {
const viewer = viewerRef.current
if (!viewer) return
// group_by_depth counts levels, where view.set_depth() counts the boundary
// below them -- server.cpp does ctx->set_depth(row_pivot_depth - 1) for both
// the one- and two-sided contexts. So the toolbar's 0..3, which were written
// against the imperative call, are one less than the config wants: depth 0
// (grand total only) is group_by_depth 1, and passing 0 asked the engine for
// set_depth(-1).
// group_by_depth is a count of levels to show, and so is the stepper's own
// d -- DepthButtons labels d as levels[d - 1]. Same unit, so d goes across
// unchanged. It used to be sent as d + 1, which showed one level more than
// was picked at every position: server.cpp turns group_by_depth into
// set_depth(n - 1), and t_traversal::set_depth adds the 1 back, so n levels
// is what n asks for.
//
// d === 0 is the stepper's "Total" slot, which has never worked: 0 reaches
// the engine as set_depth(-1) on an unsigned depth, underflows, and clamps
// to fully expanded. Sending 1 keeps it showing the first level, which is
// what it has always shown -- still wrong, but wrong in the small way
// rather than the large one. A bare total needs group_rollup_mode.
//
// The whole config goes back rather than a partial update, which had no
// effect on its own. table is dropped: it is loaded by reference, and a stale
// name in a restored config fails the lookup.
const target = Math.max(1, d)
const { table: _t, ...cfg } = await viewer.save()
await viewer.restore({ ...cfg, group_by_depth: d + 1 })
await viewer.restore({ ...cfg, group_by_depth: target })
// Until the engine carries the patch in ui/vendor, restore() drops the depth:
// ViewConfig::apply_update applies ten fields and neither depth is among
@ -1452,9 +1477,11 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio
// a button, which is predictable, and it becomes durable for free once the
// engine is rebuilt.
const after = await viewer.save()
if (after.group_by_depth !== d + 1) {
if (after.group_by_depth !== target) {
const view = await viewer.getView()
await view.set_depth(d)
// set_depth counts the boundary below the levels shown, so it is one
// less than the config's count of them.
await view.set_depth(target - 1)
const plugin = await viewer.getPlugin()
await plugin.draw(view)
}
@ -1545,12 +1572,13 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio
cfg.plugin_config = { ...(cfg.plugin_config || {}), edit_mode: 'SELECT_REGION' }
await viewer.restore(cfg)
adoptSplit(cfg.split_full?.length ? cfg.split_full : cfg.split_by,
cfg.split_by_depth != null ? cfg.split_by_depth - 1 : null)
cfg.split_by_depth != null ? cfg.split_by_depth : null)
setGroupFull(cfg.group_by || [])
if (cfg.group_by_depth != null) setExpandDepth(cfg.group_by_depth - 1)
if (cfg.group_by_depth != null) setExpandDepth(cfg.group_by_depth)
else if (cfg.expand_depth != null) await applyDepth(cfg.expand_depth)
await ensureRowLabelWidth()
await fitRowLabelsNow()
// The persisted copy is taken after the restore, so it carries whatever
// cleanLayout dropped on the way in rather than the stale original -- and