4d71c019cbe377736f567afd99adac6e76069bcf
8 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Edward Langley
|
d00cea2477 |
fix: show decimals by default, and let a formula name its own format
improvise-e0ex. `Margin = Profit / Revenue` drew as `1`: `View::new` seeded
`,.0` — zero decimals — so a 0.66 ratio rounded half-away-from-zero. The cause
is the format, not the arithmetic; exact decimal math would not change the
rendering by one character, so nothing here touches the evaluator and gotcha 8
is unchanged.
Two halves, per the user's ruling:
* `View::new` seeds `,.2`. Revenue draws `18,400.00` and Margin `0.66`. The
noise on whole numbers is the cheaper error — a currency column with two
redundant zeroes is legible, a ratio with none is wrong — and a format cannot
be guessed from a value.
* A formula may carry its own format, which wins for its target; a raw measure
keeps the view's. `formula.pest` gains
`format_clause = ${ ^"FORMAT" ~ sp+ ~ format_spec }`, with `sp+` for `WHERE`'s
reason (a predicate is invisible to the grammar-walking generator) and
`FORMAT` added to `quote::KEYWORDS`.
`improv.pest` is deliberately unchanged: `formula_raw` already carries the
`## Formulas` line verbatim, so the format rides in the formula text with
nothing to add — and the generator, which reads `improv.pest`, sees no new
spelling to violate.
Resolution has one home. `FormatPlan::of(model, view)` replaces the
`(comma, decimals)` pair every render path threaded — `GridLayout::display_text`,
`viewport::grid_body`/`compute_col_widths`, `project::grid_in` and
`persistence/export.rs::render_csv` — rather than being added beside it, which
would be two answers to "what format is this cell". It is asked of the cell's
`CellKey`, so a cell aggregating over `_Measure` keeps the view's format by
construction. `GridBody::col_formats` carries the answer to the totals strip,
which is formatted on its own path.
Persistence is untouched except for metadata: `format_f64` is display,
`persistence/format.rs::format_number` writes round-trip precision, and the two
have no call between them. Newly created views write `format: ,.2`; existing
documents are unaffected, because `format_md` has always written the line
explicitly. `web/demo.improv` moves to `,.2` on both views.
The trap this sets is gotcha 34's: `format_md` writes `Formula::raw`, so a
`FORMAT` suffix round-trips whether or not the walker parsed it. The round-trip
test therefore asserts the parsed field — injecting a walker that drops the
clause fails that one test and nothing else in `improvise-io`, 242 tests
including the whole characterization suite and the grammar properties.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
|
||
|
Edward Langley
|
1dd6950ed3 |
A view-scoped ModelCommand names its view (improvise-lltt)
Nine ModelCommand variants configure a view and named it implicitly: the effect read App::view_mut(), the HOST's slice. Since improvise-jytm made ViewState::active_view authoritative per client, a client on another view moved the host's view instead of its own -- its grid did not change and every other client's did. Each of the nine now carries `view: String`; model_effects maps it to effect::ViewTarget::Named, and the nine effects carry a ViewTarget rather than reading an ambient one. There is no operandless form left: App::view_mut() is deleted and replaced by view_mut_of(&ViewTarget), so a future geometry effect has to say which view it writes. The host's key path passes ViewTarget::Active, which is right -- the TUI is the host and has no session to ask. A Named view the document has not got is a no-op rather than resolve_view's fallback onto the default. Second half, which the ticket did not predict: naming the view made a reframe of a view the host is NOT on reachable, and adopt_cursor_epoch could not see one -- cursor_epoch is bumped only by the host's own reset_cursor, so those clients were reset by nobody and told nothing (gotcha 24's strictly-worse-than-nothing state). App::last_reframed_elsewhere is the second trigger; its opening frames are taken lazily inside view_mut_of, the one door such a write goes through, so the held-key path still hashes one view and improvise-mcq4's declined cost is not paid. Evidence and the three injected faults: context/test-evidence/command.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH |
||
|
Edward Langley
|
822547a332 |
perf(project): one walk of the grid instead of three (improvise-jmjz)
grid_of evaluated the whole grid roughly three times per frame: compute_col_widths walked every cell for its text, then every cell again for the column totals it must size, and grid_of then evaluated the window a third time. Every result was discarded at the end of the frame. ui/viewport.rs::grid_body is now the one walk. It evaluates each cell once and derives all three answers from that value -- widen the column, add to the column's sum, and if the position is inside the requested window keep the CellValue for cell_display to dress. compute_col_widths survives as grid_body with an empty window, because the keypress path wants widths and no cells, and two width rules would be the disease improvise-edp names. MEASURED, orchestrator's machine, cargo bench --bench grid_projection, before and after: typical (400 cells) 1.840ms -> 0.687ms large (9,600) 172.6ms -> 50.0ms 18k (gotcha 41) 294.4ms -> 95.2ms pathological (90,000) 4.481s -> 1.320s About 3x, which is the number of passes removed -- the shape the acceptance criterion asked for rather than a speedup argued from the structure. Three things the ticket did not predict. The benchmark's col_widths column now measures something ELSE. It read 45-47% and now reads 64-82%, which looks like a regression and is the opposite: compute_col_widths IS the fused walk, so its share rose while the frame fell by two thirds. A ratio whose denominator moved is not comparable across the change, and the benchmark now says so. The windowing ratio did not improve, and that is correct: the pass that remains is exactly the one a viewport cannot narrow. improvise-i5tk's decline stands and the two findings are now cleanly separated. It is not solved, only three times cheaper -- 18k cells is still 95ms, 5.7 frames. The next lever is caching ACROSS frames. Guard: a_window_narrows_the_cells_and_neither_the_totals_nor_the_widths. Fusing three consumers into one walk creates a hazard the separate passes did not have, and the natural "optimisation" (iterate rows rather than 0..row_count) makes a column total the sum of the visible rows, silently. Watched fail with left: [1.0] against right: [300021.0]. All 1469 pre-existing tests unchanged, which is also the evidence the outputs did not move. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH |
||
|
Edward Langley
|
bcccfdef6a |
Harden the fixture that made improvise-mcq4 visible, after merging #81
Merge of `gh/main` at
|
||
|
Edward Langley
|
a808c3d7fd |
Merge #76: one cursor mechanism, not two, and the cost that exposes
`main` moved to #76 (improvise-748f, improvise-7kvw) while this branch was in
review. #76 approaches cursor invalidation from the per-effect side; this
branch (improvise-ni6k) replaced that table with an observation. The merge is
the interesting part.
TWO CONFLICTS, both resolved keeping both sides:
- `document_content` (#76's new O(cells) hash for the CellData arm) lands
beside `document_config`, which stays `pub(crate)` for improvise-1xf4's test.
- Three pre-loop bases now: #76's `content_basis` (writes_cells) and
`config_basis` (moves_geometry, their narrower gate), plus this branch's
`frame_basis`. Mine stays on `touches_model` and the comment says why: a
`CellData` write under `prune_empty` reframes.
THE ONE THAT DID NOT CONFLICT IS THE ONE THAT MATTERED. `effect.rs` auto-merged
clean and left #76's new per-effect `SetPageSelection` reset_cursor standing
beside the observation — two live mechanisms, 1435 tests green, nothing failing.
That is the failure mode, not a merge error. The stray is removed; one mechanism
remains, and `reset_cursor` now has exactly one caller in the crate.
#76's OWN TESTS ARE NOW THE GUARD, which is the outcome to want:
`a_page_flip_under_prune_empty_invalidates_the_cursor` and its negative twin
both pass against the observation with the per-effect call deleted, and
disabling the observation fails the positive arm while the negative one still
passes. So the observation subsumes the per-effect fix rather than merely
coexisting with it.
A MEASURED REGRESSION, FILED AS improvise-mcq4, NOT HIDDEN. 748f exists to keep
a no-cell batch off an O(cells) walk. `view_frame` builds through
`with_frozen_records`, which applies `prune_empty`, which walks cells. One
`CycleAxis` batch with pruning on, fingerprint off vs on: 3.0ms -> 12.4ms at
18,000 cells and 28.5ms -> 91.7ms at 90,000 — 3-4x, crossing the 16.7ms budget
where the tree before it sat at 3.0ms.
It was nearly missed twice, both times by a fixture that could not reach the
expensive branch: the SetCell bench shows +2-5% because #76's own walk dominates
that arm, and the geometry bench, written first against the existing fixture,
had `prune_empty` OFF and read 17-176us. `benches/reshape_observation.rs` now
carries the held-key case with pruning ON, and context/code/command.md carries
the argument.
Test function names diffed BY NAME across the merge, whole workspace including
harness/: nothing lost from either side. The seven present at
|
||
|
Edward Langley
|
585f25bf78 |
Invalidate the cursor by observing the grid's frame, not by a per-effect table
improvise-ni6k. Twelve effects called `app.reset_cursor()`; that table is
the shape gotcha 24 names as the disease, and it had a hole the day it
was written. The observation replaces it: `view_frames` fingerprints
every view's drawn frame, `apply_effects` diffs it per model effect, and
the cursor resets when the view it is in reframes. No effect declares
anything, and `adopt_cursor_epoch` reads the same set.
FOUR THINGS THE TICKET DID NOT PREDICT, each one a fixture or a design
question rather than a line:
1. The frame fingerprint and `layout_version` answer DIFFERENT questions,
so they are two observations and not one. `AddCategory` of an empty
category moves no row or column but does change the coordinates a
client's CellKey hangs off — layout_version must bump, the cursor need
not. `view_shape`/`last_reshaped_views` (config) is untouched and still
drives `layout_version`; `view_frame`/`last_reframed_views` (drawn) is
new and drives the cursor. Unifying them broke two tests in both
directions, which is how the distinction was found.
2. The observation must run INSIDE the effect loop, not after it. At
end-of-batch it clobbered deliberate cursor placement in the same batch
— `[AddItem(measure), <place cursor>]` sent the cursor home, failing
three records-mode tests. In-loop, a later effect wins, which is the
same ordering property gotcha 45 gives `document_config`.
3. `GridLayout::new` does NOT apply `prune_empty`; `with_frozen_records`
does. A frame hash built on `new` cannot see a pruned row, which is
exactly the class improvise-7kvw is in.
4. Two per-effect no-op guards were carrying the whole weight and are now
orphaned: `effective_items` and `item_is_drawn` are deleted, along with
six inline `moved`/`drawn`/`real`/`existed` predicates.
COST, measured as the ticket required, `benches/reshape_observation.rs`
(new), one SetCell batch, same fixtures as `grid_projection`:
typical 400 cells 172µs -> 206µs (+20%)
large 9.6k cells 3.70ms -> 4.17ms (+13%)
18k cells 7.09ms -> 8.41ms (+19%)
pathological 90k 35.2ms -> 39.8ms (+13%)
The batch is dominated by `rebuild_layout`/`recompute_formulas`, not by
the fingerprint.
WHAT CLOSES AS A CONSEQUENCE: improvise-7kvw, asserted by
`paging_across_a_prune_that_drops_a_row_invalidates_the_cursor`.
AddFormula/RemoveFormula join the shared `reshaping_commands` table (a
formula target is a `_Measure` item, so it is a column — gotcha 7), each
proved reshaping by the table's own `assert_ne!` on the grid shape.
WHAT DOES NOT CLOSE, and this is the honest part: `SortData`. A pure
record reorder changes no label and no count, and records-mode `row_items`
are positional, so a row_items fingerprint cannot see it. I wrote the
mechanism (hashing `layout.records`' coords) and then REMOVED it, because
I could not build a fixture where `SortData` actually reorders anything —
records came back in key order whatever order the cells went in. Shipping
a confident comment over a mechanism I could not demonstrate is this
repo's named defect, so it is reported instead. See the hand-back.
Negative arms unchanged and still passing, including the refused-command
arm and `SetNumberFormat`; disabling the observation fails the whole
reshaping table, which is the proof it is what does the work.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
|
||
|
Edward Langley
|
ed412a0256 |
fix: review round — the Ok-but-no-op resets, the measure regression, and a
loop-of-one test F1 (HIGH). Gating the reset on Ok is not the same as gating it on "something changed". Six accepted commands changed nothing and reset every cursor anyway: SetAxis to the axis already held, HideItem on an already-hidden item, ShowItem on a never-hidden one, AddItem of an existing item (Category::add_item dedupes and returns Ok), and RemoveItem/RemoveCategory of something absent. Each now asks the state before mutating. The case that made it urgent needs no second client: TileSelect binds `r` to set-axis-for-tile row, which emits SetAxis with no comparison against the current axis, so `r` on a Row category sent the user's own cursor to the origin. F2 (REGRESSION this PR introduced). Naming a measure in the trailing `+` column at the bottom-right of a records grid sent the cursor home: AddItem now resets, inserts_a_column matches only NewCategory, and EnterAdvance at the bottom-right returns AbortChain with no SetSelected. The restore moves to BEFORE the advance, where an aborting advance cannot skip it -- gotcha 21's rule applied to the cursor rather than the mode -- which also subsumes the +cat arm's copy. F3. a_refused_model_command_invalidates_no_cursor named a class and tested one member; injecting reset_cursor into AddCategory's Err arm left it green. Now a table over all three refusable effects, and that injection fails it by name. F5. The benchmark could not reproduce its own figures: it took the mean over a few iterations, and review measured the same share at 8% and at 131% -- a part slower than the whole containing it, printed flat. It also passed (false, 2) where grid_of passes the view's (true, 0), so it timed a call that never happens. Now min-of-N with the view's own format, and a share above 100% prints UNSTABLE. Figures are stable across runs at ratio 1.2-1.5x, col_widths 45-49%. F6. The drg counts omitted the 8 simple_effect! invocations and misclassified AddItem at base through a regex that cannot see `app` newline `.model_state` -- doctrine(derive_then_verify) catching the person citing it. Correct: 57 effects, narrow set 32 -> 25, not 25 -> 17 of 49. The direction holds and is still the argument; the headline was overstated by about half again. Corrected in context/code/command.md and on the bead, not quietly restated. F9. a_model_command_that_leaves_the_active_view_alone_broadcasts_nothing asserts a ResetCursor broadcast, so the name read as a lie: ..._broadcasts_no_switch. F4 filed as improvise-pdvm: AddItem/RemoveItem declare CellData so layout_version does not bump while cursor_epoch does, and this PR's own reshape assertion is the first evidence for it. Related to improvise-75ld, same question other direction. Faults injected for every new guard: SetAxis's moved check, AddItem's fresh check, AddCategory's Err arm, and the pre-fix advance ordering ((0,0) vs (0,3)). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
8a4ee06570 |
bench: measure grid_of's whole-grid cost, and decline the narrowing it was for
improvise-i5tk's acceptance criterion 1 was a benchmark, on the ground that "this can dominate TUI rendering" is exactly as unevidenced as "it is fine". benches/grid_projection.rs is that benchmark, and it says no. One grid_of call, dense pivot grids, this machine: typical 100 x 4 = 400 cells whole-grid 1.902ms ratio 1.2x col_widths 45% large 1200 x 8 = 9600 cells whole-grid 185.644ms ratio 1.3x col_widths 64% 18k cells (gotcha 41) 3000 x 6 = 18000 cells whole-grid 402.435ms ratio 1.5x col_widths 42% pathological 9000 x 10 = 90000 cells whole-grid 5.235s ratio 1.5x col_widths 44% ratio is whole-grid over a 45x12 terminal window: 1.2-1.5x, against a fix whose stated cost is changing what a cache index MEANS in the TUI and putting every scroll site in scope. Wrong trade, so i5tk is closed on the measurement. Why the ratio is so low is the bigger finding. Two of the three whole-grid passes a frame performs are viewport-independent by construction: ui/viewport.rs::compute_col_widths takes no viewport and walks every (row, col) twice -- once for cell text, once for the column totals it must also size -- and a column total must sum every row including scrolled-past ones (gotcha 44). grid_of's own cell_display loop is the third pass and the only one windowing would touch. So the whole grid is evaluated about three times per frame, every result discarded, and at 18,000 cells a keypress costs four tenths of a second. Filed as improvise-jmjz; its fix is caching or one fused pass, which this benchmark has now distinguished from the windowing it rules out. No criterion crate: plain Instant, and test = false so cargo test does not run a target that takes seconds by design. Closes improvise-i5tk Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |