fix: a new view takes an unused name, derived once for both front ends
improvise-vw47. Create View 2 and View 3, configure View 3, delete View 2,
then ask for a new view: both resolvers spelled `format!("View {}", len + 1)`,
which is the count and not a free name. `Workbook::create_view` replaces
whatever holds the name, so View 3's format reset to defaults and no view was
added -- a symptom that looks like nothing happened.
The name is derived at two sites, `cmd/panel.rs::CreateAndSwitchView` and
`resolve.rs`'s "create-and-switch-view" arm, so fixing one would have left the
TUI and the browser naming different views. `view::unused_view_name` in
improvise-core is the one derivation both now call. It takes an iterator of
names because the browser holds no `Workbook`, only the `ViewListCache` that
`view_summaries` maps one-for-one off `workbook.views`.
`Workbook::create_view` is deliberately untouched. Its replacement semantics is
what the `_Records`/`_Drill` rebuilders need -- both reach it through
`effect::CreateView` on every entry -- so refusing or renaming there would turn
"a view was reset" into "the key does nothing". The argument is in
`context/code/core.md` section 11.
Three tests, written failing first. The helper's own cases pin that a run of
taken names is searched rather than bumped once. The `Cmd` test uses the
ticket's fixture and asserts the count is below the highest name before asking
anything, because the pre-existing sibling test's fixtures all leave the
derived name free -- which is how the bug lived under it. `lockstep.rs`
witnesses the agreement: note that `both_ends` PASSES against the unfixed
source, both ends being wrong identically, so that test carries the count and
format assertions too.
Rivals injected: fixing panel.rs alone fails lockstep on the document text;
starting the search at 1 fails the pre-existing incremental-naming test.
Baseline 1667 passed / 0 failed / 7 ignored over 37 result lines; now 1670.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
@@ -580,6 +580,43 @@ builds on demand. Both put the virtual `_Index`/`_Dim` categories on axes, and
|
||||
those categories are never persisted — so these views are rebuilt from scratch
|
||||
each time rather than saved (improvise-60z).
|
||||
|
||||
### `create_view` replaces, and `unused_view_name` is why it may keep doing so
|
||||
|
||||
`Workbook::create_view` inserts over whatever holds the name. That is not an
|
||||
oversight: "rebuilt from scratch each time" **is** that replacement, and both
|
||||
rebuilders reach it through `effect::CreateView` — `DrillIntoCell` and the
|
||||
records-mode command each emit a `CreateView` of their fixed name on every
|
||||
entry, so an entry into records mode would otherwise stack views or refuse.
|
||||
The third production caller, `parse.rs::build_view`, guards with
|
||||
`contains_key` and so cannot observe the difference.
|
||||
|
||||
So the two callers that need a **new** name are the user-facing ones, and they
|
||||
are the ones that must not name a view that exists. Before improvise-vw47 both
|
||||
spelled `format!("View {}", views.len() + 1)`, which is the count and not the
|
||||
name: delete `View 2` from a workbook holding `Default`, `View 2`, `View 3` and
|
||||
the count says `View 3`, which is taken. `create_view` then did exactly what
|
||||
its other callers need and replaced it — the user's configured view silently
|
||||
reset to defaults, and no view added. **A symptom that looks like nothing
|
||||
happened.**
|
||||
|
||||
Two fixes were rejected in favour of the helper, and both are worth naming
|
||||
because each turns a quiet wrong answer into a loud one at a caller that was
|
||||
never wrong. Making `create_view` **refuse** a name it holds breaks records
|
||||
mode outright — the key stops working, which reads as a better error and is a
|
||||
worse bug. Making it **rename** silently gives the rebuilders a `_Records 2`
|
||||
that `RECORDS_VIEW` does not name and nothing switches to.
|
||||
|
||||
`view::unused_view_name` therefore answers only the question the two user-facing
|
||||
resolvers ask, and `create_view` keeps its semantics untouched. It takes an
|
||||
iterator of names rather than a `&Workbook` because the browser resolver holds
|
||||
no workbook — it has `ViewListCache`, a `Vec<ViewSummary>` that
|
||||
`project::view_summaries` maps one-for-one off `workbook.views`, so the two
|
||||
front ends see the same list and derive the same name **by calling the same
|
||||
function**, which is what design-principles §5 asks for and what
|
||||
`tests/lockstep.rs` witnesses. It starts its search at the count rather than at
|
||||
1 so that the common no-gap case keeps its existing answer; starting at 1 would
|
||||
rename the first view a fresh workbook creates from `View 2` to `View 1`.
|
||||
|
||||
## 12. `axis_of` panics, `try_axis_of` does not
|
||||
|
||||
Registration is guaranteed for every model category: `Workbook::new`,
|
||||
|
||||
Reference in New Issue
Block a user