Merge pull request #27 from fiddlerwoaroof/wave/g-command-layer
WU-G: the keymap and command layer (improvise-th7m, lae, ct5, 2zq; git/aaw declined with evidence; 0hb deferred with its design note). Includes the ejection fix the 2zq ruling called for — a virtual on an axis is a configuration, not a vacancy — which also closed a live bug where adding a second regular category evicted _Measure from Column and made every measure vanish from the demo's own view.
This commit is contained in:
Ed Langley
GitHub
@@ -202,6 +202,55 @@ mod tests {
|
||||
assert!(dbg.contains("ExportCsv"), "Expected ExportCsv, got: {dbg}");
|
||||
assert!(dbg.contains("Normal"), "Expected Normal mode, got: {dbg}");
|
||||
}
|
||||
|
||||
/// improvise-lae: the save prompt has to commit to `SaveAs` and not to
|
||||
/// `ExportCsv`, which is what makes it a separate mode from
|
||||
/// `ExportPrompt` rather than a second binding onto it — `ExportCsv`
|
||||
/// would write CSV under whatever `.improv` name the user typed.
|
||||
///
|
||||
/// `SaveAs` and not `Save`, because `Save` writes the path the document
|
||||
/// already has, and the whole reason this prompt exists is that there
|
||||
/// isn't one.
|
||||
#[test]
|
||||
fn commit_save_writes_the_model_to_the_typed_path() {
|
||||
let m = two_cat_model();
|
||||
let layout = make_layout(&m);
|
||||
let reg = make_registry();
|
||||
let mut bufs = HashMap::new();
|
||||
bufs.insert("save".to_string(), "/tmp/typed.improv".to_string());
|
||||
let mut ctx = make_ctx(&m, &layout, ®);
|
||||
ctx.buffers = &bufs;
|
||||
let dbg = effects_debug(&CommitSave.execute(&ctx));
|
||||
assert!(
|
||||
dbg.contains("SaveAs(\"/tmp/typed.improv\")"),
|
||||
"Expected SaveAs at the typed path, got: {dbg}"
|
||||
);
|
||||
assert!(
|
||||
!dbg.contains("ExportCsv"),
|
||||
"the save prompt must not export CSV, got: {dbg}"
|
||||
);
|
||||
assert!(dbg.contains("Normal"), "Expected Normal mode, got: {dbg}");
|
||||
}
|
||||
|
||||
/// An empty prompt must not reach `SaveAs`: `SaveAs(PathBuf::from(""))`
|
||||
/// asks the backend to write to `""`, and the user then reads whatever
|
||||
/// I/O error that produces instead of "you typed nothing".
|
||||
#[test]
|
||||
fn commit_save_on_an_empty_path_writes_nothing() {
|
||||
let m = two_cat_model();
|
||||
let layout = make_layout(&m);
|
||||
let reg = make_registry();
|
||||
let mut bufs = HashMap::new();
|
||||
bufs.insert("save".to_string(), " ".to_string());
|
||||
let mut ctx = make_ctx(&m, &layout, ®);
|
||||
ctx.buffers = &bufs;
|
||||
let dbg = effects_debug(&CommitSave.execute(&ctx));
|
||||
assert!(
|
||||
!dbg.contains("SaveAs"),
|
||||
"an empty path must not reach SaveAs, got: {dbg}"
|
||||
);
|
||||
assert!(dbg.contains("SetStatus"), "Expected a status, got: {dbg}");
|
||||
}
|
||||
}
|
||||
|
||||
// ── Commit commands (mode-specific buffer consumers) ────────────────────────
|
||||
@@ -294,6 +343,7 @@ fn commit_plain_records_edit(
|
||||
commit_regular_cell_value(&row.cell_key(measure), value, effects);
|
||||
}
|
||||
RecordCol::NewMeasure => add_measure(value, effects),
|
||||
RecordCol::NewCategory => add_category(value, effects),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -312,6 +362,29 @@ fn add_measure(value: &str, effects: &mut Vec<Box<dyn Effect>>) {
|
||||
effects.push(effect::mark_dirty());
|
||||
}
|
||||
|
||||
/// Typing a name into the `+cat` column creates that category, which gives
|
||||
/// the row a coordinate column to type into (improvise-2zq).
|
||||
///
|
||||
/// It creates the column and **does not set the coordinate on the current
|
||||
/// row**, which is the deliberate answer to the ticket's third question.
|
||||
/// Two reasons. It is symmetric with the measure `+`, which creates an empty
|
||||
/// column and writes no value. And setting it would move *every* measure
|
||||
/// cell in the row onto the new coordinate (gotcha 1) — a data migration
|
||||
/// across N cells attached to the same Enter that made a schema change, in
|
||||
/// an app with no undo. The follow-up edit is one `l` away in the column
|
||||
/// that just appeared.
|
||||
///
|
||||
/// Like `add_measure` this is a schema change, so it applies immediately
|
||||
/// even inside a drill rather than being staged.
|
||||
fn add_category(value: &str, effects: &mut Vec<Box<dyn Effect>>) {
|
||||
let name = value.trim();
|
||||
if name.is_empty() {
|
||||
return;
|
||||
}
|
||||
effects.push(Box::new(effect::AddCategory(name.to_string())));
|
||||
effects.push(effect::mark_dirty());
|
||||
}
|
||||
|
||||
fn commit_cell_value(
|
||||
ctx: &CmdContext,
|
||||
key: &CellKey,
|
||||
@@ -323,8 +396,10 @@ fn commit_cell_value(
|
||||
return;
|
||||
};
|
||||
match (&col, ctx.has_drill_state) {
|
||||
// A new measure is a schema change, never a staged cell edit.
|
||||
// A new measure or category is a schema change, never a staged
|
||||
// cell edit: there is no cell to roll back with the drill.
|
||||
(RecordCol::NewMeasure, _) => add_measure(value, effects),
|
||||
(RecordCol::NewCategory, _) => add_category(value, effects),
|
||||
(_, true) => effects.push(stage_drill_edit(record_idx, col, value)),
|
||||
(_, false) => commit_plain_records_edit(ctx, record_idx, &col, value, effects),
|
||||
}
|
||||
@@ -334,6 +409,19 @@ fn commit_cell_value(
|
||||
commit_regular_cell_value(key, value, effects);
|
||||
}
|
||||
|
||||
/// Whether committing `key` inserts a column, invalidating every column
|
||||
/// index to its right.
|
||||
///
|
||||
/// Asked of the *layout* — `record_col` resolves the synthetic `_Dim`
|
||||
/// position into what the column actually is — rather than re-derived from
|
||||
/// the value or the cursor, so there is one answer to "what is this column"
|
||||
/// (gotcha 1).
|
||||
fn inserts_a_column(ctx: &CmdContext, key: &CellKey) -> bool {
|
||||
crate::view::synthetic_record_info(key)
|
||||
.and_then(|(_, col_idx)| ctx.layout.record_col(col_idx))
|
||||
.is_some_and(|col| matches!(col, RecordCol::NewCategory))
|
||||
}
|
||||
|
||||
/// Direction to advance after committing a cell edit.
|
||||
#[derive(Debug, Clone, Copy)]
|
||||
pub enum AdvanceDir {
|
||||
@@ -377,14 +465,43 @@ impl Cmd for CommitAndAdvance {
|
||||
}
|
||||
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
|
||||
let mut effects: Vec<Box<dyn Effect>> = Vec::new();
|
||||
commit_cell_value(ctx, &self.key, &self.value, &mut effects);
|
||||
// Pre-emptively drop to the normal counterpart of edit_mode. If the
|
||||
// advance succeeds, the trailing `EnterEditAtCursor` below will lift
|
||||
// us back into editing on the new cell. If the advance aborts
|
||||
// (e.g. already at bottom-right on Enter), `EnterEditAtCursor` is
|
||||
// skipped and we land in normal mode — which is the desired
|
||||
// "Enter at bottom-right commits and exits" behavior.
|
||||
//
|
||||
// It goes **before** the commit, not after (improvise-2zq). The
|
||||
// commit can abort the batch itself — `AddCategory` does so when the
|
||||
// document is at `MAX_CATEGORIES` — and an abort before this ran
|
||||
// left the app in the editing mode, where `draw_bottom_bar` renders
|
||||
// the minibuffer instead of the status line and the refusal the
|
||||
// effect had just written was never drawn. Tidy-up goes where an
|
||||
// abort cannot reach it; same rule as `ExecuteCommand`'s.
|
||||
effects.push(effect::change_mode(exit_mode_for(&self.edit_mode)));
|
||||
commit_cell_value(ctx, &self.key, &self.value, &mut effects);
|
||||
if inserts_a_column(ctx, &self.key) {
|
||||
// A schema change that inserts a column moves every index to its
|
||||
// right, and the advance below is computed from `ctx` — the
|
||||
// *pre-commit* layout, because `apply_effects` rebuilds only
|
||||
// after the whole batch (gotcha 23). On the last row `Enter`
|
||||
// wraps to `col + 1`, which after the insert is `+cat` again, so
|
||||
// the editor reopened category creation and a user naming two
|
||||
// categories in a row never reached the column they had made.
|
||||
// `Tab` reached it the same way.
|
||||
//
|
||||
// Not advancing is also the *useful* answer rather than merely
|
||||
// the safe one: `add_category` inserts the new column exactly
|
||||
// where the cursor already is, so staying put lands on it.
|
||||
//
|
||||
// The measure `+` shares none of this, and not by luck — it is
|
||||
// the last column, so there is no column to its right for an
|
||||
// index to shift into, and both advances are already special-
|
||||
// cased at the bottom-right edge.
|
||||
effects.push(Box::new(effect::RebuildLayout));
|
||||
return effects;
|
||||
}
|
||||
match self.advance {
|
||||
AdvanceDir::Down => {
|
||||
let adv = EnterAdvance {
|
||||
@@ -546,6 +663,54 @@ impl Cmd for CommitBuffer {
|
||||
}
|
||||
}
|
||||
|
||||
/// Commit a save path from the save buffer.
|
||||
///
|
||||
/// The sibling of [`CommitExport`], and deliberately not the same command:
|
||||
/// the export prompt writes CSV, this one writes the *model* through
|
||||
/// `SaveAs`, which also adopts the path so a later plain `Save` has one
|
||||
/// (improvise-lae).
|
||||
///
|
||||
/// An empty path bails rather than reaching `SaveAs`, which would otherwise
|
||||
/// ask the storage backend to write to `""` and report its error instead of
|
||||
/// the one the user can act on.
|
||||
#[derive(Debug)]
|
||||
pub struct CommitSave;
|
||||
impl Cmd for CommitSave {
|
||||
fn name(&self) -> &'static str {
|
||||
"commit-save"
|
||||
}
|
||||
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
|
||||
let path = read_buffer(ctx, "save").trim().to_string();
|
||||
if path.is_empty() {
|
||||
return vec![
|
||||
effect::set_status("No path given — not saved"),
|
||||
effect::change_mode(AppMode::Normal),
|
||||
];
|
||||
}
|
||||
// The mode change comes **first**, and that ordering is the whole
|
||||
// reason the user sees a failed save (found by Copilot on PR #27).
|
||||
//
|
||||
// `SaveAs` aborts the batch when the write fails (improvise-8tk's
|
||||
// other arm), so anything after it is skipped. With the order
|
||||
// reversed the batch stopped while still in `SavePrompt` — and
|
||||
// `draw_bottom_bar` renders the minibuffer *instead of* the status
|
||||
// line, so the "Save error:" set by the effect was never drawn. The
|
||||
// save appeared to do nothing at all.
|
||||
//
|
||||
// Leading with the mode change means the error lands in `Normal`,
|
||||
// where the status bar is what the bottom row draws. It costs the
|
||||
// typed path, which the welded `clear-buffer` would have taken on a
|
||||
// success anyway and which `EnterMode` clears on the next Ctrl-W
|
||||
// regardless (gotcha 11) — so nothing durable is lost, and the
|
||||
// convention that the side effect runs last and its message wins is
|
||||
// the one gotcha 20 already states.
|
||||
vec![
|
||||
effect::change_mode(AppMode::Normal),
|
||||
Box::new(effect::SaveAs(std::path::PathBuf::from(path))),
|
||||
]
|
||||
}
|
||||
}
|
||||
|
||||
/// Commit an export from the export buffer.
|
||||
#[derive(Debug)]
|
||||
pub struct CommitExport;
|
||||
|
||||
@@ -276,6 +276,15 @@ impl Cmd for Quit {
|
||||
/// Save then quit. The `Quit` only lands if the `Save` succeeds — a failed
|
||||
/// save aborts the effect chain, so `wq`/`ZZ` leaves the user in the app
|
||||
/// with the error visible rather than discarding the model (improvise-8tk).
|
||||
///
|
||||
/// **This looks like an `AAndB` command and is not one** (improvise-git).
|
||||
/// `wq` is the *user's* name for the composition, not an invented internal
|
||||
/// one: it is registered, so `:wq` reaches it, and `Quit`'s unsaved-changes
|
||||
/// message advertises that spelling. A keymap `Binding::Sequence` cannot be
|
||||
/// reached from the `:` line, so replacing this with one would either delete
|
||||
/// the ex-command or leave two independently-written spellings of "save then
|
||||
/// quit". One definition behind two surfaces is what this already is.
|
||||
/// Pinned by `text_buffer::tests::colon_wq_saves_then_quits`.
|
||||
#[derive(Debug)]
|
||||
pub struct SaveAndQuit;
|
||||
impl Cmd for SaveAndQuit {
|
||||
@@ -301,6 +310,30 @@ impl Cmd for SaveAndQuit {
|
||||
pub struct EditOrDrill {
|
||||
pub edit_mode: AppMode,
|
||||
}
|
||||
impl EditOrDrill {
|
||||
/// What to say when there is no cell under the cursor at all.
|
||||
///
|
||||
/// improvise-ct5: this used to say "cannot drill — no cell at cursor" in
|
||||
/// both modes. Drilling is a pivot-mode concept, and in records mode —
|
||||
/// where every regular category sits on `Axis::None`, so this branch is
|
||||
/// the one an empty grid always takes — the user's model is "a table I
|
||||
/// type into", with `o` one key away to add a row. Naming drilling there
|
||||
/// described a mechanism the user is not using instead of the key that
|
||||
/// would help.
|
||||
///
|
||||
/// The choice reads `edit_mode` — the argument the *keymap* supplies —
|
||||
/// rather than asking `ctx` what mode it is in, so the command stays as
|
||||
/// mode-agnostic as its doc comment claims and no runtime `is_records()`
|
||||
/// check comes back (improvise-4ju).
|
||||
fn nothing_at_cursor(&self) -> &'static str {
|
||||
if self.edit_mode.is_records() {
|
||||
"No record at cursor — press o to add a row"
|
||||
} else {
|
||||
"No cell at cursor"
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl Cmd for EditOrDrill {
|
||||
fn name(&self) -> &'static str {
|
||||
"edit-or-drill"
|
||||
@@ -321,7 +354,7 @@ impl Cmd for EditOrDrill {
|
||||
let is_aggregated = !is_synthetic && regular_none;
|
||||
if is_aggregated {
|
||||
let Some(key) = ctx.cell_key().clone() else {
|
||||
return vec![effect::set_status("cannot drill — no cell at cursor")];
|
||||
return vec![effect::set_status(self.nothing_at_cursor())];
|
||||
};
|
||||
return DrillIntoCell { key }.execute(ctx);
|
||||
}
|
||||
|
||||
@@ -301,26 +301,48 @@ mod tests {
|
||||
assert!(effects.is_empty());
|
||||
}
|
||||
|
||||
/// improvise-aaw asked for this command to be replaced by a keymap
|
||||
/// `Binding::Sequence [create-view X, switch-view X, enter-mode normal]`.
|
||||
/// It cannot be: a `Sequence` carries **literal** args, and the name here
|
||||
/// is computed from `ctx` — so a Sequence would hard-code one name and
|
||||
/// the second `n` in the view panel would collide with the first.
|
||||
///
|
||||
/// The test was called `..._names_incrementally` and asserted only that
|
||||
/// `CreateView`/`SwitchView`/`Normal` appeared — it never looked at the
|
||||
/// name, which is the one thing that makes this command a command and
|
||||
/// not a composition. Now it does, at two different view counts, because
|
||||
/// a single count cannot tell "derived from the workbook" from "constant".
|
||||
#[test]
|
||||
fn create_and_switch_view_names_incrementally() {
|
||||
let m = two_cat_model();
|
||||
let mut m = two_cat_model();
|
||||
let layout = make_layout(&m);
|
||||
let reg = make_registry();
|
||||
let ctx = make_ctx(&m, &layout, ®);
|
||||
let effects = CreateAndSwitchView.execute(&ctx);
|
||||
let dbg = effects_debug(&effects);
|
||||
let dbg = effects_debug(&CreateAndSwitchView.execute(&ctx));
|
||||
assert!(
|
||||
dbg.contains("CreateView"),
|
||||
"Expected CreateView, got: {dbg}"
|
||||
dbg.contains("CreateView(\"View 2\")"),
|
||||
"Expected the name derived from the one existing view, got: {dbg}"
|
||||
);
|
||||
assert!(
|
||||
dbg.contains("SwitchView"),
|
||||
"Expected SwitchView, got: {dbg}"
|
||||
dbg.contains("SwitchView(\"View 2\")"),
|
||||
"Expected a switch to the view just created, got: {dbg}"
|
||||
);
|
||||
assert!(
|
||||
dbg.contains("Normal"),
|
||||
"Expected return to Normal, got: {dbg}"
|
||||
);
|
||||
drop(ctx);
|
||||
|
||||
// A second view in the workbook must move the name. A literal arg in
|
||||
// a keymap Sequence could not, which is why aaw was declined.
|
||||
m.create_view("View 2");
|
||||
let layout = make_layout(&m);
|
||||
let ctx = make_ctx(&m, &layout, ®);
|
||||
let dbg = effects_debug(&CreateAndSwitchView.execute(&ctx));
|
||||
assert!(
|
||||
dbg.contains("CreateView(\"View 3\")"),
|
||||
"the name must follow the workbook, got: {dbg}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -642,6 +664,18 @@ impl Cmd for SwitchViewAtCursor {
|
||||
}
|
||||
|
||||
/// Create a new view, switch to it, and return to Normal mode.
|
||||
///
|
||||
/// **Not a shim, despite the name** (improvise-aaw). The new view's name is
|
||||
/// derived from `ctx` — `format!("View {}", views.len() + 1)` — and a keymap
|
||||
/// `Binding::Sequence` can only carry literal args, so the proposed
|
||||
/// `[create-view X, switch-view X, enter-mode normal]` would hard-code one
|
||||
/// name and the second `n` in the view panel would collide with the first.
|
||||
/// The derivation is the decision that makes this a command.
|
||||
///
|
||||
/// The ticket's two fallbacks are both behaviour changes wanting a product
|
||||
/// decision, not refactors: prompting for a name is a new feature, and
|
||||
/// making `create-view` switch by default changes `:create-view` for scripts
|
||||
/// and duplicates what `grid.rs` does by hand for `_Drill`/`_Records`.
|
||||
#[derive(Debug)]
|
||||
pub struct CreateAndSwitchView;
|
||||
impl Cmd for CreateAndSwitchView {
|
||||
|
||||
@@ -600,6 +600,7 @@ pub fn default_registry() -> CmdRegistry {
|
||||
},
|
||||
);
|
||||
r.register_nullary(|| Box::new(CommitExport));
|
||||
r.register_nullary(|| Box::new(CommitSave));
|
||||
r.register_nullary(|| Box::new(ExecuteCommand));
|
||||
|
||||
// ── Wizard ───────────────────────────────────────────────────────────
|
||||
|
||||
@@ -94,6 +94,35 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// improvise-git proposed deleting the `SaveAndQuit` command and binding
|
||||
/// `ZZ` to a `Binding::Sequence [w, force-quit]` instead — the repo's
|
||||
/// "do not name a command AAndB" principle.
|
||||
///
|
||||
/// That premise is incomplete, and this test is why. `wq` is not only a
|
||||
/// keybinding: it is a **registered command name**, so `:wq` reaches it
|
||||
/// through the registry, and `Quit`'s own unsaved-changes message
|
||||
/// advertises it ("Use :q! to force quit or :wq to save+quit"). A keymap
|
||||
/// `Sequence` is not reachable from the `:` line, so deleting the command
|
||||
/// deletes the ex-command with it.
|
||||
///
|
||||
/// Keeping the command *and* adding the Sequence would leave two
|
||||
/// independently-written spellings of "save then quit", which is the
|
||||
/// duplication this codebase keeps curing rather than creating. So this
|
||||
/// pins the surface, and the conversion is declined on it.
|
||||
#[test]
|
||||
fn colon_wq_saves_then_quits() {
|
||||
let m = two_cat_model();
|
||||
let layout = make_layout(&m);
|
||||
let reg = make_registry();
|
||||
let mut bufs = HashMap::new();
|
||||
bufs.insert("command".to_string(), "wq".to_string());
|
||||
let mut ctx = make_ctx(&m, &layout, ®);
|
||||
ctx.buffers = &bufs;
|
||||
let dbg = effects_debug(&ExecuteCommand.execute(&ctx));
|
||||
assert!(dbg.contains("Save"), "`:wq` must save, got: {dbg}");
|
||||
assert!(dbg.contains("Quit"), "`:wq` must then quit, got: {dbg}");
|
||||
}
|
||||
|
||||
/// improvise-6sq: `ExecuteCommand` appends `change_mode(Normal)` unless
|
||||
/// some effect reports `changes_mode()`. `EnterEditAtCursor` sets
|
||||
/// `view_state.mode` but did not override it, so
|
||||
@@ -364,17 +393,34 @@ impl Cmd for ExecuteCommand {
|
||||
|
||||
match crate::command::parse::parse_line_with(ctx.registry, &raw) {
|
||||
Ok(cmds) => {
|
||||
let mut effects: Vec<Box<dyn Effect>> = Vec::new();
|
||||
// Leave the command line **first**, before anything that can
|
||||
// fail runs.
|
||||
//
|
||||
// Still decided at apply time rather than here, because
|
||||
// ViewBack, ViewForward and StartImportWizard move the mode
|
||||
// only on some runs and no up-front answer is right
|
||||
// (improvise-toc). Running it at the head does not weaken
|
||||
// that: whatever a later effect sets, it sets *after* this,
|
||||
// so the mode a `:` line ends in is unchanged in every case
|
||||
// — the only difference is what happens when the batch never
|
||||
// reaches the end.
|
||||
//
|
||||
// Which is the point. A failed `Save`/`SaveAs` sets
|
||||
// `abort_effects` (gotcha 21), and an abort at the tail ate
|
||||
// this restoration too, stranding the user in `CommandMode`
|
||||
// with the stale command line drawn — where
|
||||
// `draw_bottom_bar` renders the minibuffer *instead of* the
|
||||
// status line, so the error the effect had just set was
|
||||
// invisible. The abort must stop the *chain* (`:w bad . q`
|
||||
// must not quit) and must not stop the *bookkeeping*, and
|
||||
// ordering answers that without giving any effect a "runs
|
||||
// anyway" flag: tidy-up goes where an abort cannot reach it.
|
||||
let mut effects: Vec<Box<dyn Effect>> = vec![Box::new(
|
||||
effect::ReturnToNormalUnlessMoved(ctx.mode.clone()),
|
||||
)];
|
||||
for cmd in cmds {
|
||||
effects.extend(cmd.execute(ctx));
|
||||
}
|
||||
// Return to Normal unless one of the effects above moves the
|
||||
// mode. Decided at apply time, not here: ViewBack,
|
||||
// ViewForward and StartImportWizard change it only on some
|
||||
// runs, so no up-front answer is right (improvise-toc).
|
||||
effects.push(Box::new(effect::ReturnToNormalUnlessMoved(
|
||||
ctx.mode.clone(),
|
||||
)));
|
||||
effects
|
||||
}
|
||||
Err(msg) => {
|
||||
|
||||
@@ -54,6 +54,51 @@ fn run(
|
||||
|
||||
/// Turn a binding into effects. The one place a keymap lookup becomes a
|
||||
/// mutation, and the reason `Binding` itself can be registry-free data.
|
||||
///
|
||||
/// # Why a `Sequence` flattens, and why it stays that way (improvise-0hb)
|
||||
///
|
||||
/// Every step is executed against the **same pre-dispatch `ctx`**, and the
|
||||
/// effects are concatenated into one vec that `App::apply_effects` runs as
|
||||
/// one batch. So a later *command* cannot observe an earlier one's effects —
|
||||
/// only a later *effect* can. improvise-0hb proposed closing that: apply
|
||||
/// effects so far between steps, rebuild the layout, build a fresh
|
||||
/// `CmdContext`, dispatch the next. The design pass says **no**, on three
|
||||
/// findings.
|
||||
///
|
||||
/// **It would silently reopen improvise-8tk, a P1 data loss.**
|
||||
/// `apply_effects` resets `abort_effects` at entry *and* again when it
|
||||
/// breaks, because the flag is per batch. Applying between steps therefore
|
||||
/// dispatches step 2 into a fresh batch with the flag cleared, so the `Quit`
|
||||
/// after a failed `Save` would land — the exact silent discard 8tk fixed.
|
||||
/// Gotcha 21's claim that the abort "carries across sequence steps" is true
|
||||
/// *because* of this flattening, not in spite of it. It was also untested
|
||||
/// until now: no shipped `Sequence` has a first step that can abort, and
|
||||
/// `ZZ` is a prefix onto the single `wq` command.
|
||||
/// `app.rs::a_sequence_whose_first_step_aborts_does_not_run_the_second` is
|
||||
/// the guard, and it fails when the abort is made per-step.
|
||||
///
|
||||
/// **Almost nothing would benefit.** Of the shipped `Sequence` bindings,
|
||||
/// all but one come from `TextEntry` and are either `[<commit>, clear-buffer
|
||||
/// <buf>]` or `[clear-buffer <buf>, enter-mode <exit>]` — `clear-buffer` and
|
||||
/// `enter-mode` take their operand as a literal arg and read nothing from
|
||||
/// `ctx`, so a refreshed `ctx` would change no outcome. The single genuine
|
||||
/// composition is records `o` = `[add-record-row, enter-edit-at-cursor]`,
|
||||
/// and it already works, because `EnterEditAtCursorCmd` fronts its effect
|
||||
/// with `RebuildLayout` and the effect re-reads the layout itself (gotcha
|
||||
/// 23). Every Esc and every commit keypress would pay a layout rebuild and
|
||||
/// a `CmdContext` construction — which recomputes column widths — to buy
|
||||
/// that nothing.
|
||||
///
|
||||
/// **And the ticket's motivating rewrite does not typecheck as a binding
|
||||
/// anyway.** improvise-git's `[(w, []), (force-quit, [])]` cannot dispatch:
|
||||
/// `w` is `register_pure`, whose interactive constructor rejects an empty
|
||||
/// arg list, so `run` returns `None` and the whole binding is dead. The
|
||||
/// nullary spelling is `save`. `every_bound_command_name_is_registered`
|
||||
/// checks exactly this (gotcha 15) and would have caught it.
|
||||
///
|
||||
/// If command-level transactionality is ever wanted, it wants its own
|
||||
/// campaign: the abort must become a property of the *sequence* rather than
|
||||
/// of the batch before any step is applied separately.
|
||||
fn effects_of(
|
||||
binding: &Binding,
|
||||
registry: &CmdRegistry,
|
||||
@@ -385,6 +430,27 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// improvise-th7m: `F1` was bound twice on consecutive lines — first to a
|
||||
/// bare `enter-mode` with no args, then to `enter-mode ["help"]`. The
|
||||
/// first was dead, because `Keymap::bind` *inserts* and the later binding
|
||||
/// wins; had it ever been the surviving one it would have failed
|
||||
/// `require_args` at dispatch and made `F1` do nothing.
|
||||
///
|
||||
/// The risk in deleting a line of a duplicated pair is deleting the wrong
|
||||
/// one, so this pins which binding `F1` is meant to carry: the same one
|
||||
/// `?` carries, which is what identified it as the intended survivor.
|
||||
#[test]
|
||||
fn f1_and_question_mark_both_open_help() {
|
||||
let ks = KeymapSet::default_keymaps();
|
||||
for key in [KeyCode::F(1), KeyCode::Char('?')] {
|
||||
assert_eq!(
|
||||
bound(&ks, &ModeKey::Normal, key),
|
||||
("enter-mode".to_string(), vec!["help".to_string()]),
|
||||
"{key:?} should open the help overlay"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn editing_mode_has_any_char_and_esc() {
|
||||
let ks = KeymapSet::default_keymaps();
|
||||
@@ -582,6 +648,7 @@ mod tests {
|
||||
ModeKey::CategoryAdd => AppMode::category_add(),
|
||||
ModeKey::ItemAdd => AppMode::item_add("Type".to_string()),
|
||||
ModeKey::ExportPrompt => AppMode::export_prompt(),
|
||||
ModeKey::SavePrompt => AppMode::save_prompt(),
|
||||
ModeKey::CommandMode => AppMode::command_mode(),
|
||||
ModeKey::ImportWizard => AppMode::ImportWizard,
|
||||
ModeKey::RecordsNormal => AppMode::RecordsNormal,
|
||||
@@ -784,6 +851,7 @@ mod tests {
|
||||
(AppMode::category_add(), COMMIT_STAY),
|
||||
(AppMode::item_add("Region".into()), COMMIT_STAY),
|
||||
(AppMode::export_prompt(), COMMIT_LEAVE),
|
||||
(AppMode::save_prompt(), COMMIT_LEAVE),
|
||||
(AppMode::command_mode(), COMMIT_LEAVE),
|
||||
]
|
||||
}
|
||||
@@ -1002,7 +1070,7 @@ mod binding_dump {
|
||||
#[cfg(test)]
|
||||
mod tests_support {
|
||||
use super::*;
|
||||
pub const ALL_MODES: [ModeKey; 16] = [
|
||||
pub const ALL_MODES: [ModeKey; 17] = [
|
||||
ModeKey::Normal,
|
||||
ModeKey::Help,
|
||||
ModeKey::FormulaPanel,
|
||||
@@ -1014,6 +1082,7 @@ mod tests_support {
|
||||
ModeKey::CategoryAdd,
|
||||
ModeKey::ItemAdd,
|
||||
ModeKey::ExportPrompt,
|
||||
ModeKey::SavePrompt,
|
||||
ModeKey::CommandMode,
|
||||
ModeKey::SearchMode,
|
||||
ModeKey::ImportWizard,
|
||||
|
||||
@@ -1391,6 +1391,299 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// improvise-2zq step 2: the `+cat` column creates a category and its
|
||||
/// column, without leaving the grid.
|
||||
///
|
||||
/// Driven through real keys. The assertion that earns its place is the
|
||||
/// last one: the new column is **usable immediately**, which is the whole
|
||||
/// point — `records_columns` filters on `kind.is_regular()` and never
|
||||
/// requires items, so a category with no items is a column the moment it
|
||||
/// exists.
|
||||
#[test]
|
||||
fn the_new_category_column_creates_a_category_and_its_column() {
|
||||
let mut app = records_model_with_two_rows();
|
||||
let plus_cat = (0..app.layout.col_count())
|
||||
.find(|i| app.layout.col_label(*i) == crate::view::NEW_CATEGORY_COL)
|
||||
.expect("the records grid offers a +cat column");
|
||||
app.cursor_mut().selected = (0, plus_cat);
|
||||
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
for c in "Product".chars() {
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
}
|
||||
app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
app.rebuild_layout();
|
||||
|
||||
assert!(
|
||||
app.model_state.workbook.model.category("Product").is_some(),
|
||||
"the category is created"
|
||||
);
|
||||
assert!(
|
||||
app.layout.is_records_mode(),
|
||||
"and creating it does not eject the user (step 1)"
|
||||
);
|
||||
let cols: Vec<String> = (0..app.layout.col_count())
|
||||
.map(|i| app.layout.col_label(i))
|
||||
.collect();
|
||||
assert_eq!(
|
||||
cols,
|
||||
vec![
|
||||
"Region",
|
||||
"Product",
|
||||
crate::view::NEW_CATEGORY_COL,
|
||||
"meas1",
|
||||
"meas2",
|
||||
crate::view::NEW_MEASURE_COL
|
||||
],
|
||||
"the new coordinate column appears immediately, before the \
|
||||
still-available +cat, got {cols:?}"
|
||||
);
|
||||
}
|
||||
|
||||
/// improvise-2zq, found by Copilot on PR #27: creating a category from
|
||||
/// `+cat` shifted the grid under the advance that followed it.
|
||||
///
|
||||
/// `add_category` inserts the new coordinate column **immediately before
|
||||
/// `+cat`**, so every column index from `+cat` rightwards moves by one.
|
||||
/// `CommitAndAdvance` then computed its advance from `ctx`, whose layout
|
||||
/// is the pre-commit one (gotcha 23: `apply_effects` rebuilds only after
|
||||
/// the whole batch). On the last row `Enter` wraps to the next column —
|
||||
/// and the index it wrapped to was `+cat` *again* after the rebuild.
|
||||
///
|
||||
/// So the failure is not "lands one column off". It **reopens category
|
||||
/// creation**: a user naming two categories in a row never reaches the
|
||||
/// column they just made, and gets an unbounded `+cat` loop instead.
|
||||
///
|
||||
/// The last row is the arm that wraps, so it is the arm tested.
|
||||
#[test]
|
||||
fn enter_on_plus_cat_in_the_last_row_does_not_reopen_category_creation() {
|
||||
let mut app = records_model_with_two_rows();
|
||||
let plus_cat = (0..app.layout.col_count())
|
||||
.find(|i| app.layout.col_label(*i) == crate::view::NEW_CATEGORY_COL)
|
||||
.expect("+cat column");
|
||||
let last_row = app.layout.row_count() - 1;
|
||||
app.cursor_mut().selected = (last_row, plus_cat);
|
||||
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
for c in "Product".chars() {
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
}
|
||||
app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
|
||||
let (_, col) = app.cursor().selected;
|
||||
let landed = app.layout.col_label(col);
|
||||
assert_ne!(
|
||||
landed,
|
||||
crate::view::NEW_CATEGORY_COL,
|
||||
"the advance must not land back on +cat — that is the loop, and \
|
||||
it is what a stale column index produces"
|
||||
);
|
||||
assert_eq!(
|
||||
landed, "Product",
|
||||
"and it lands on the column just created, which is the useful \
|
||||
answer rather than merely a safe one: the insert happens at the \
|
||||
cursor, so staying put arrives there"
|
||||
);
|
||||
}
|
||||
|
||||
/// The same stale index reached `Tab`, which advances right rather than
|
||||
/// wrapping. Both directions read the pre-commit layout, so both are
|
||||
/// covered — one arm of a two-arm rule is the shape
|
||||
/// `doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)`
|
||||
/// names.
|
||||
#[test]
|
||||
fn tab_on_plus_cat_does_not_reopen_category_creation() {
|
||||
let mut app = records_model_with_two_rows();
|
||||
let plus_cat = (0..app.layout.col_count())
|
||||
.find(|i| app.layout.col_label(*i) == crate::view::NEW_CATEGORY_COL)
|
||||
.expect("+cat column");
|
||||
app.cursor_mut().selected = (0, plus_cat);
|
||||
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
for c in "Product".chars() {
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
}
|
||||
app.handle_key(KeyEvent::new(KeyCode::Tab, KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
|
||||
let (_, col) = app.cursor().selected;
|
||||
assert_ne!(
|
||||
app.layout.col_label(col),
|
||||
crate::view::NEW_CATEGORY_COL,
|
||||
"Tab must not land back on +cat either"
|
||||
);
|
||||
assert_eq!(app.layout.col_label(col), "Product");
|
||||
}
|
||||
|
||||
/// Committing the `+cat` name creates the **column** and does not write
|
||||
/// the coordinate on the row the cursor was on.
|
||||
///
|
||||
/// The deliberate answer to the ticket's third question, and the reason
|
||||
/// is gotcha 1: setting a coordinate moves *every* measure cell in the
|
||||
/// row onto it. That is a data migration across N cells, and attaching it
|
||||
/// to the same Enter that made a schema change — in an app with no undo —
|
||||
/// is a bigger single keystroke than the measure `+` asks for. The
|
||||
/// follow-up edit is one column away.
|
||||
#[test]
|
||||
fn the_new_category_column_creates_the_column_but_writes_no_coordinate() {
|
||||
let mut app = records_model_with_two_rows();
|
||||
let plus_cat = (0..app.layout.col_count())
|
||||
.find(|i| app.layout.col_label(*i) == crate::view::NEW_CATEGORY_COL)
|
||||
.expect("+cat column");
|
||||
app.cursor_mut().selected = (0, plus_cat);
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
for c in "Product".chars() {
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
}
|
||||
app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
app.rebuild_layout();
|
||||
|
||||
let product_col = (0..app.layout.col_count())
|
||||
.find(|i| app.layout.col_label(*i) == "Product")
|
||||
.expect("the new column exists");
|
||||
for row in 0..app.layout.row_count() {
|
||||
assert_eq!(
|
||||
app.layout.records_display(row, product_col).as_deref(),
|
||||
Some(""),
|
||||
"row {row} must have no coordinate for a category just created"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// `MAX_CATEGORIES` is a reachable refusal on this path, and the refusal
|
||||
/// has to be **visible**.
|
||||
///
|
||||
/// `draw_bottom_bar` renders the minibuffer instead of the status line,
|
||||
/// so a message written while an editor is open is never drawn. The
|
||||
/// commit path therefore drops out of editing *before* running the add,
|
||||
/// and `AddCategory` aborts the batch on refusal so the trailing
|
||||
/// `EnterEditAtCursor` cannot re-open the editor over the message.
|
||||
///
|
||||
/// The limit itself is checked in exactly one place — `Model::add_category`
|
||||
/// — and nothing here restates it; this fills the document to the limit
|
||||
/// by asking for categories until it refuses.
|
||||
#[test]
|
||||
fn the_new_category_column_refuses_past_the_limit_where_the_user_can_see_it() {
|
||||
let mut app = records_model_with_two_rows();
|
||||
let mut n = 0;
|
||||
while app
|
||||
.model_state
|
||||
.workbook
|
||||
.add_category(format!("Filler{n}"))
|
||||
.is_ok()
|
||||
{
|
||||
n += 1;
|
||||
assert!(n < 100, "add_category must refuse eventually");
|
||||
}
|
||||
app.rebuild_layout();
|
||||
app.view_state.status_msg.clear();
|
||||
|
||||
let plus_cat = (0..app.layout.col_count())
|
||||
.find(|i| app.layout.col_label(*i) == crate::view::NEW_CATEGORY_COL)
|
||||
.expect("+cat column");
|
||||
app.cursor_mut().selected = (0, plus_cat);
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
for c in "TooMany".chars() {
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
}
|
||||
app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
|
||||
assert!(
|
||||
app.model_state.workbook.model.category("TooMany").is_none(),
|
||||
"the category is refused"
|
||||
);
|
||||
assert!(
|
||||
!app.view_state.status_msg.is_empty(),
|
||||
"the refusal is reported"
|
||||
);
|
||||
assert!(
|
||||
app.view_state.mode.minibuffer().is_none(),
|
||||
"and reported where it is drawn: a mode with a minibuffer hides \
|
||||
the status line, so the user would see nothing at all. mode is \
|
||||
{:?}, message {:?}",
|
||||
app.view_state.mode,
|
||||
app.view_state.status_msg
|
||||
);
|
||||
}
|
||||
|
||||
/// improvise-2zq step 1: naming a category in records mode used to eject
|
||||
/// the user from the grid they were standing in.
|
||||
///
|
||||
/// Records mode is `_Index` on Row and `_Dim` on Column, with every
|
||||
/// regular category on `Axis::None`. `View::on_category_added` seated a
|
||||
/// new regular category on the first axis no *regular* category held —
|
||||
/// Row — and **bumped the virtual out of the way** to get there. So `N`,
|
||||
/// a name, `Enter` moved `_Index` off Row, records detection stopped
|
||||
/// matching, and the grid silently became a pivot.
|
||||
///
|
||||
/// Driven through the real keys rather than through `on_category_added`,
|
||||
/// because that is how the bug was found and it is what makes this
|
||||
/// evidence rather than a restatement of the fix.
|
||||
#[test]
|
||||
fn naming_a_category_in_records_mode_does_not_eject_the_user() {
|
||||
let mut wb = Workbook::new("T");
|
||||
wb.add_category("Region").unwrap();
|
||||
wb.add_item("Region", "East").unwrap();
|
||||
let mut app = App::new(wb, None);
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
assert!(app.layout.is_records_mode(), "setup: in records mode");
|
||||
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char('N'), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
for c in "Product".chars() {
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
}
|
||||
app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
app.rebuild_layout();
|
||||
|
||||
assert!(
|
||||
app.model_state.workbook.model.category("Product").is_some(),
|
||||
"setup: the category really was created"
|
||||
);
|
||||
assert!(
|
||||
app.layout.is_records_mode(),
|
||||
"naming a category must not leave records mode; axes are {:?}",
|
||||
app.model_state
|
||||
.workbook
|
||||
.active_view()
|
||||
.category_axes
|
||||
.iter()
|
||||
.collect::<Vec<_>>()
|
||||
);
|
||||
assert_eq!(
|
||||
app.model_state.workbook.active_view().axis_of("Product"),
|
||||
crate::view::Axis::None,
|
||||
"records mode keeps every regular category on None, and \
|
||||
`records_columns` picks it up as a column from `is_regular` \
|
||||
alone — never from an axis"
|
||||
);
|
||||
let cols: Vec<String> = (0..app.layout.col_count())
|
||||
.map(|i| app.layout.col_label(i))
|
||||
.collect();
|
||||
assert!(
|
||||
cols.iter().any(|c| c == "Product"),
|
||||
"the new category must appear as a records column immediately, \
|
||||
got {cols:?}"
|
||||
);
|
||||
}
|
||||
|
||||
/// improvise-6sq: `i` and `o` want opposite behavior on an empty records
|
||||
/// grid, which is why `edit-or-drill` and `enter-edit-at-cursor` cannot
|
||||
/// be merged into one command. Entering records mode puts every regular
|
||||
@@ -1399,6 +1692,17 @@ mod tests {
|
||||
/// there is none, so `i` reports "cannot drill". `o` must still open an
|
||||
/// edit there (see `add_record_row_in_empty_records_view_creates_first_row`),
|
||||
/// so routing it through the same drill check would break it.
|
||||
///
|
||||
/// improvise-ct5 changed the *message* and deliberately not the
|
||||
/// divergence. The outcome — `i` opens no editor where there is nothing
|
||||
/// to edit — is right, and `o` one key away is the affordance for adding
|
||||
/// a row. What was wrong is that the message said "cannot drill", a
|
||||
/// pivot-mode concept in a mode whose model is "a table I type into".
|
||||
/// Making `i` behave like `o` here (the ticket's option (b)) would
|
||||
/// collapse exactly the divergence improvise-6sq exists to record, so
|
||||
/// the assertion below still pins that `i` does not enter editing; only
|
||||
/// the wording assertion moved, and it now also names what must *not*
|
||||
/// appear.
|
||||
#[test]
|
||||
fn i_and_o_diverge_on_an_empty_records_grid() {
|
||||
let mut wb = Workbook::new("T");
|
||||
@@ -1418,16 +1722,68 @@ mod tests {
|
||||
"i must not enter editing when there is no cell to edit"
|
||||
);
|
||||
assert!(
|
||||
app.view_state.status_msg.contains("cannot drill"),
|
||||
!app.view_state.status_msg.to_lowercase().contains("drill"),
|
||||
"records mode must not talk about drilling, got status {:?}",
|
||||
app.view_state.status_msg
|
||||
);
|
||||
assert!(
|
||||
app.view_state.status_msg.contains("press o to add a row"),
|
||||
"the message should name the key that does add a row, got {:?}",
|
||||
app.view_state.status_msg
|
||||
);
|
||||
}
|
||||
|
||||
/// The negative arm of improvise-ct5, and the reason the wording is
|
||||
/// keyed off the mode the keymap supplies rather than reworded once.
|
||||
///
|
||||
/// Without this, "say the records sentence everywhere" passes the test
|
||||
/// above — and it would promise `o`, which adds no row in pivot mode,
|
||||
/// trading one misleading message for another.
|
||||
///
|
||||
/// The fixture is an empty pivot grid that still counts as aggregated:
|
||||
/// `Region` on Row with no items makes `cell_key()` `None`, and `Extra`
|
||||
/// cycled to `Axis::None` is the regular category that makes
|
||||
/// `EditOrDrill` take the drill branch.
|
||||
#[test]
|
||||
fn the_empty_grid_message_is_not_the_records_one_in_pivot_mode() {
|
||||
let mut wb = Workbook::new("T");
|
||||
wb.add_category("Region").unwrap();
|
||||
wb.add_category("Extra").unwrap();
|
||||
wb.active_view_mut()
|
||||
.set_axis("Extra", crate::view::Axis::None);
|
||||
let mut app = App::new(wb, None);
|
||||
app.rebuild_layout();
|
||||
assert_eq!(app.layout.row_count(), 0, "setup: empty pivot grid");
|
||||
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
assert_eq!(
|
||||
app.view_state.mode,
|
||||
AppMode::Normal,
|
||||
"i must not enter editing when there is no cell to edit"
|
||||
);
|
||||
assert!(
|
||||
!app.view_state.status_msg.contains("add a row"),
|
||||
"`o` adds no row in pivot mode; it must not be offered, got {:?}",
|
||||
app.view_state.status_msg
|
||||
);
|
||||
assert!(
|
||||
app.view_state.status_msg.contains("No cell at cursor"),
|
||||
"got status {:?}",
|
||||
app.view_state.status_msg
|
||||
);
|
||||
}
|
||||
|
||||
/// A blank model has no measures, so wide format shows only the trailing
|
||||
/// new-measure column. Typing a name there creates the measure, which
|
||||
/// gives it a real column to type values into — records mode stays
|
||||
/// self-sufficient for onboarding (improvise-c5v).
|
||||
/// A blank model has no categories and no measures, so wide format shows
|
||||
/// exactly the two creation columns. Typing a name into the new-measure
|
||||
/// one creates the measure, which gives it a real column to type values
|
||||
/// into — records mode stays self-sufficient for onboarding
|
||||
/// (improvise-c5v).
|
||||
///
|
||||
/// improvise-2zq widened that: a blank grid now offers `+cat` beside `+`,
|
||||
/// so both halves of a schema can be bootstrapped without leaving the
|
||||
/// grid. This test still pins the measure half; the category half is
|
||||
/// `the_new_category_column_creates_a_category_and_its_column`.
|
||||
#[test]
|
||||
fn blank_model_bootstraps_its_first_measure_from_the_new_measure_column() {
|
||||
use crate::model::cell::CellKey;
|
||||
@@ -1440,8 +1796,8 @@ mod tests {
|
||||
.collect();
|
||||
assert_eq!(
|
||||
cols,
|
||||
vec![crate::view::NEW_MEASURE_COL],
|
||||
"a blank model offers only the new-measure column"
|
||||
vec![crate::view::NEW_CATEGORY_COL, crate::view::NEW_MEASURE_COL],
|
||||
"a blank model offers exactly the two creation columns"
|
||||
);
|
||||
|
||||
// `o` seeds a row and opens an edit on it.
|
||||
@@ -1449,7 +1805,17 @@ mod tests {
|
||||
.unwrap();
|
||||
assert_eq!(app.layout.row_count(), 1);
|
||||
|
||||
// Naming a measure in the + column creates it.
|
||||
// The cursor lands at column 0, which in a blank grid is now `+cat`
|
||||
// rather than `+` (improvise-2zq) — a record needs coordinates
|
||||
// before it needs measures, so that order is deliberate. Move to the
|
||||
// new-measure column to exercise the measure half.
|
||||
// `o` left us editing column 0, so leave that edit before moving.
|
||||
app.handle_key(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
let new_measure_col = app.layout.col_count() - 1;
|
||||
app.cursor_mut().selected = (0, new_measure_col);
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
for c in "Rev".chars() {
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
@@ -1462,12 +1828,17 @@ mod tests {
|
||||
.collect();
|
||||
assert_eq!(
|
||||
cols,
|
||||
vec!["Rev", crate::view::NEW_MEASURE_COL],
|
||||
"the named measure becomes a real column"
|
||||
vec![
|
||||
crate::view::NEW_CATEGORY_COL,
|
||||
"Rev",
|
||||
crate::view::NEW_MEASURE_COL
|
||||
],
|
||||
"the named measure becomes a real column, between the two \
|
||||
creation columns"
|
||||
);
|
||||
|
||||
// Typing into that column writes the value.
|
||||
let rev_col = 0;
|
||||
let rev_col = 1;
|
||||
app.cursor_mut().selected = (0, rev_col);
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
@@ -1529,9 +1900,15 @@ mod tests {
|
||||
.collect();
|
||||
assert_eq!(
|
||||
cols,
|
||||
vec!["Region", "meas1", "meas2", crate::view::NEW_MEASURE_COL],
|
||||
"setup: category column, one column per measure, then the \
|
||||
new-measure column"
|
||||
vec![
|
||||
"Region",
|
||||
crate::view::NEW_CATEGORY_COL,
|
||||
"meas1",
|
||||
"meas2",
|
||||
crate::view::NEW_MEASURE_COL
|
||||
],
|
||||
"setup: category columns, the new-category column, one column \
|
||||
per measure, then the new-measure column (improvise-2zq)"
|
||||
);
|
||||
app
|
||||
}
|
||||
@@ -1558,6 +1935,208 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// improvise-lae end to end: `Ctrl-W`, a path, `Enter`.
|
||||
///
|
||||
/// The failing case is the interesting one, and it pins an **ordering**
|
||||
/// (found by Copilot on PR #27). `SaveAs` aborts the batch on a failed
|
||||
/// write, so whatever `CommitSave` emits after it is skipped. A first
|
||||
/// version emitted `[SaveAs, ChangeMode(Normal)]`, which left the app in
|
||||
/// `SavePrompt` — and `draw_bottom_bar` renders the minibuffer *instead
|
||||
/// of* the status line, so the "Save error:" the effect had just set was
|
||||
/// never drawn. The save appeared to do nothing whatsoever.
|
||||
///
|
||||
/// Leading with the mode change puts the error in `Normal`, where the
|
||||
/// bottom row is the status bar. This test therefore asserts the mode
|
||||
/// *left* the prompt, which is the opposite of what it asserted before —
|
||||
/// deliberately, and it is the only thing keeping the two effects in
|
||||
/// that order.
|
||||
#[test]
|
||||
fn ctrl_w_prompts_for_a_path_and_a_failed_write_reports_it_where_it_shows() {
|
||||
let mut app = App::new(Workbook::new("T"), None);
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char('w'), KeyModifiers::CONTROL))
|
||||
.unwrap();
|
||||
assert!(
|
||||
matches!(app.view_state.mode, AppMode::SavePrompt { .. }),
|
||||
"Ctrl-W should open the save prompt, got {:?}",
|
||||
app.view_state.mode
|
||||
);
|
||||
|
||||
let path = "/nonexistent-dir-improvise-lae/m.improv";
|
||||
for c in path.chars() {
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
}
|
||||
assert_eq!(
|
||||
app.view_state.buffers.get("save").map(String::as_str),
|
||||
Some(path),
|
||||
"typing must reach the buffer the prompt names"
|
||||
);
|
||||
|
||||
app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
assert!(
|
||||
app.view_state.status_msg.starts_with("Save error:"),
|
||||
"got {:?}",
|
||||
app.view_state.status_msg
|
||||
);
|
||||
assert_eq!(
|
||||
app.view_state.mode,
|
||||
AppMode::Normal,
|
||||
"the error must be reported in a mode whose bottom row is the \
|
||||
status bar, not behind a minibuffer that hides it"
|
||||
);
|
||||
assert!(
|
||||
app.view_state.mode.minibuffer().is_none(),
|
||||
"setup for the assertion above: only a mode with no minibuffer \
|
||||
draws the status line at all"
|
||||
);
|
||||
}
|
||||
|
||||
/// Copilot round 2 reported that `Ctrl-W` from a records grid returns to
|
||||
/// `Normal`, losing the records keymap while the layout is still a
|
||||
/// records layout. It is right, and it is a **class**: every minibuffer
|
||||
/// mode reachable from `RecordsNormal` exits to `Normal`, because the
|
||||
/// exit is a constant baked into `TextEntry::new(..., "normal")` and
|
||||
/// into each commit command.
|
||||
///
|
||||
/// The point of this test is that the class **predates** `SavePrompt`:
|
||||
/// `:` and `Ctrl-E` do exactly the same thing and always have. That is
|
||||
/// why the fix is not "make `SavePrompt` remember its origin" — that
|
||||
/// would leave two of the three arms broken, which is the shape
|
||||
/// `doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)`
|
||||
/// names.
|
||||
///
|
||||
/// The binding *table* is not involved and was checked rather than
|
||||
/// reasoned about: an `IMPROVISE_BINDING_DUMP` diff shows
|
||||
/// `RecordsNormal` gains `Ctrl-W` and loses nothing, so `with_parent`
|
||||
/// inheritance (gotcha 13) is intact. What is lost is the runtime mode.
|
||||
#[test]
|
||||
fn every_minibuffer_mode_entered_from_records_returns_to_plain_normal() {
|
||||
for key in [
|
||||
KeyEvent::new(KeyCode::Char(':'), KeyModifiers::NONE),
|
||||
KeyEvent::new(KeyCode::Char('e'), KeyModifiers::CONTROL),
|
||||
KeyEvent::new(KeyCode::Char('w'), KeyModifiers::CONTROL),
|
||||
] {
|
||||
let mut wb = Workbook::new("T");
|
||||
wb.add_category("Region").unwrap();
|
||||
wb.add_item("Region", "East").unwrap();
|
||||
let mut app = App::new(wb, None);
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
assert_eq!(
|
||||
app.view_state.mode,
|
||||
AppMode::RecordsNormal,
|
||||
"setup: R enters records mode"
|
||||
);
|
||||
|
||||
app.handle_key(key).unwrap();
|
||||
assert!(
|
||||
app.view_state.mode.minibuffer().is_some(),
|
||||
"setup: {:?} opens a minibuffer, got {:?}",
|
||||
key.code,
|
||||
app.view_state.mode
|
||||
);
|
||||
|
||||
app.handle_key(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
assert_eq!(
|
||||
app.view_state.mode,
|
||||
AppMode::Normal,
|
||||
"{:?} cancels back to plain Normal, not RecordsNormal — the \
|
||||
layout is still a records layout, so this is the bug, and \
|
||||
it is the same for all three keys",
|
||||
key.code
|
||||
);
|
||||
assert!(
|
||||
app.layout.is_records_mode(),
|
||||
"...and the layout really is still records, which is what \
|
||||
makes the mode wrong rather than merely different"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// A failing `:` command must not strand the user in `CommandMode`.
|
||||
///
|
||||
/// Found by Copilot on PR #27, and it is the sharper form of gotcha 21.
|
||||
/// The abort is *supposed* to stop the chain — `:w /bad . q` must not
|
||||
/// quit. What it must not also stop is the **bookkeeping**: the mode
|
||||
/// restoration that `ExecuteCommand` appends. Aborting before that ran
|
||||
/// left the app in `CommandMode` with the stale command line still
|
||||
/// drawn, and `draw_bottom_bar` renders the minibuffer *instead of* the
|
||||
/// status line — so the "Save error:" set one effect earlier was
|
||||
/// invisible. A failed save looked exactly like a key that did nothing.
|
||||
///
|
||||
/// The answer is **ordering, not classification**: a batch puts its
|
||||
/// tidy-up where an abort cannot reach it, which is in front. No effect
|
||||
/// needs a new "runs anyway" flag, and the rule generalises to any
|
||||
/// command composing bookkeeping with something that can fail.
|
||||
///
|
||||
/// Both arms are here because they fail in different places: no path at
|
||||
/// all is `Save`'s guard (aborting since improvise-8tk, so this arm was
|
||||
/// broken before this branch), and a bad path is `SaveAs`'s write
|
||||
/// (aborting since f0f400d on this branch).
|
||||
#[test]
|
||||
fn a_failed_colon_write_lands_in_normal_where_its_error_is_drawn() {
|
||||
for (line, expect) in [
|
||||
("w", "No file path"),
|
||||
("w /nonexistent-dir-improvise-lae/m.improv", "Save error:"),
|
||||
] {
|
||||
let mut app = App::new(Workbook::new("T"), None);
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char(':'), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
for c in line.chars() {
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
}
|
||||
app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
|
||||
assert!(
|
||||
app.view_state.status_msg.contains(expect),
|
||||
":{line} should report {expect:?}, got {:?}",
|
||||
app.view_state.status_msg
|
||||
);
|
||||
assert_eq!(
|
||||
app.view_state.mode,
|
||||
AppMode::Normal,
|
||||
":{line} must leave the command line, or the status it just \
|
||||
set is hidden behind the minibuffer"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// The other half of the same rule: the abort must still stop the
|
||||
/// *chain*. `:w <bad> . q` is improvise-8tk's data loss spelled on one
|
||||
/// `:` line, and moving the mode restoration to the front must not have
|
||||
/// bought the fix above at the price of that one.
|
||||
#[test]
|
||||
fn a_failed_colon_write_still_refuses_to_quit_after_it() {
|
||||
let mut app = App::new(Workbook::new("T"), None);
|
||||
app.view_state.buffers.insert(
|
||||
"command".to_string(),
|
||||
"w /nonexistent-dir-improvise-lae/m.improv . q".to_string(),
|
||||
);
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char(':'), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
// `:` clears the buffer on entry (gotcha 11), so type it instead.
|
||||
for c in "w /nonexistent-dir-improvise-lae/m.improv . q".chars() {
|
||||
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
}
|
||||
app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
|
||||
.unwrap();
|
||||
assert_ne!(
|
||||
app.view_state.mode,
|
||||
AppMode::Quit,
|
||||
"a failed write must still abort the rest of the `:` line"
|
||||
);
|
||||
assert!(
|
||||
app.view_state.status_msg.starts_with("Save error:"),
|
||||
"got {:?}",
|
||||
app.view_state.status_msg
|
||||
);
|
||||
}
|
||||
|
||||
/// `ZZ` is save-and-quit. With no file path the save fails and aborts the
|
||||
/// chain (improvise-8tk), which also proves the second Z reached `wq`.
|
||||
#[test]
|
||||
@@ -1579,6 +2158,56 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// Gotcha 21 claims a failed `Save` aborts across `Binding::Sequence`
|
||||
/// steps as well as within one command's effect list. Nothing tested it:
|
||||
/// no shipped `Sequence` has a first step that can abort, and `ZZ` is a
|
||||
/// *prefix* resolving to the single `wq` command, so
|
||||
/// `prefix_z_then_z_reaches_save_and_quit` exercises the within-a-command
|
||||
/// case only. The claim was structurally true and unguarded.
|
||||
///
|
||||
/// It is true because `Keymap::dispatch` flattens a `Sequence` into ONE
|
||||
/// effect vec and `App::apply_effects` runs that as one batch, breaking
|
||||
/// on `abort_effects`. It is *load-bearing*, because it is the reason a
|
||||
/// `[w, force-quit]` Sequence would be safe — the argument the
|
||||
/// improvise-git grooming note unblocked on.
|
||||
///
|
||||
/// And it is exactly what improvise-0hb would remove. Command-level
|
||||
/// transactionality means applying effects *between* steps, and
|
||||
/// `apply_effects` resets `abort_effects` at entry and again on abort, so
|
||||
/// step 2 would be dispatched into a fresh batch with the flag cleared —
|
||||
/// the `Quit` after a failed save would land, silently reopening the
|
||||
/// improvise-8tk P1 data loss. This is the guard that would catch it.
|
||||
#[test]
|
||||
fn a_sequence_whose_first_step_aborts_does_not_run_the_second() {
|
||||
use improvise_protocol::keymap::Keymap;
|
||||
|
||||
// No file path, so `w` fails and sets `abort_effects`.
|
||||
let mut app = App::new(Workbook::new("T"), None);
|
||||
let key = KeyCode::Char('x');
|
||||
let mods = KeyModifiers::NONE;
|
||||
let mut km = Keymap::new();
|
||||
km.bind_seq(key, mods, vec![("save", vec![]), ("force-quit", vec![])]);
|
||||
|
||||
let effects = {
|
||||
let ctx = app.cmd_context(key, mods);
|
||||
app.keymap_set.dispatch_transient(&km, &ctx, key, mods)
|
||||
}
|
||||
.expect("both steps resolve in the registry");
|
||||
app.apply_effects(effects);
|
||||
|
||||
assert!(
|
||||
app.view_state.status_msg.contains("No file path"),
|
||||
"setup: the save must actually have failed, got {:?}",
|
||||
app.view_state.status_msg
|
||||
);
|
||||
assert_ne!(
|
||||
app.view_state.mode,
|
||||
AppMode::Quit,
|
||||
"the abort must carry into the next Sequence step, not just the \
|
||||
next effect of the same command"
|
||||
);
|
||||
}
|
||||
|
||||
/// A prefix is consumed whether or not the next key matches, so a stray
|
||||
/// key after `g` does nothing rather than being treated as a fresh
|
||||
/// normal-mode binding.
|
||||
|
||||
@@ -95,8 +95,20 @@ macro_rules! simple_effect {
|
||||
pub struct AddCategory(pub String);
|
||||
impl Effect for AddCategory {
|
||||
fn apply(&self, app: &mut App) {
|
||||
// A refusal aborts the batch, for the same reason a failed save does
|
||||
// (gotcha 21): what follows assumes the category exists. From the
|
||||
// `+cat` column (improvise-2zq) the rest of the batch would advance
|
||||
// the cursor and re-open an editor on a column that was never
|
||||
// created — and re-opening the minibuffer is also what would *hide*
|
||||
// this message, since `draw_bottom_bar` renders the minibuffer
|
||||
// instead of the status line.
|
||||
//
|
||||
// `MAX_CATEGORIES` is a reachable refusal on that path rather than a
|
||||
// theoretical one, and the limit is checked in exactly one place —
|
||||
// `Model::add_category` — so nothing here restates it.
|
||||
if let Err(e) = app.model_state.workbook.add_category(&self.0) {
|
||||
app.view_state.status_msg = e.to_string();
|
||||
app.abort_effects = true;
|
||||
}
|
||||
}
|
||||
fn kind(&self) -> EffectKind {
|
||||
@@ -658,8 +670,9 @@ impl Effect for ApplyAndClearDrill {
|
||||
apply_coordinate_rename(app, row, category, new_value);
|
||||
}
|
||||
// Never staged — `commit_cell_value` applies a new measure
|
||||
// immediately, since it is a schema change and not a cell.
|
||||
RecordCol::NewMeasure => {}
|
||||
// or category immediately, since a schema change is not a
|
||||
// cell edit and has nothing to roll back with the drill.
|
||||
RecordCol::NewMeasure | RecordCol::NewCategory => {}
|
||||
}
|
||||
}
|
||||
app.model_state.dirty = true;
|
||||
@@ -769,7 +782,7 @@ fn save_to_current_path(
|
||||
state: &crate::ui::app::ModelState,
|
||||
) -> Result<String, String> {
|
||||
state.file_path.as_ref().map_or_else(
|
||||
|| Err("No file path — use :w <path>".to_string()),
|
||||
|| Err("No file path — Ctrl-W to name one, or :w <path>".to_string()),
|
||||
|path| {
|
||||
crate::persistence::save_to(storage, &state.workbook, path)
|
||||
.map(|()| format!("Saved to {}", path.display()))
|
||||
@@ -788,8 +801,12 @@ impl Effect for SaveAs {
|
||||
app.model_state.dirty = false;
|
||||
app.view_state.status_msg = format!("Saved to {}", self.0.display());
|
||||
}
|
||||
// The same abort `Save` owes (improvise-8tk). `SaveAs` is the
|
||||
// other arm of that rule and was left out of the original fix,
|
||||
// so `:w /bad/path . q` quit on a failed write.
|
||||
Err(e) => {
|
||||
app.view_state.status_msg = format!("Save error: {e}");
|
||||
app.abort_effects = true;
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1961,6 +1978,74 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// BUG (improvise-lae, found reviewing it): improvise-8tk fixed **one
|
||||
/// arm** of a rule with two. `Save` aborts the batch when the write
|
||||
/// fails; `SaveAs` reported the error to the status bar and let the
|
||||
/// batch continue, which is the same silent discard 8tk closed — the
|
||||
/// shape `doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)`
|
||||
/// names, and the three tests above are evidence about `Save` only.
|
||||
///
|
||||
/// It is reachable today: the `:` line composes with ` . `, so
|
||||
/// `:w /bad/path . q` builds exactly this batch, and
|
||||
/// `ExecuteCommand` concatenates the two commands' effects into one.
|
||||
/// improvise-lae's new save prompt also commits to `SaveAs`, so the
|
||||
/// arm was about to carry more traffic.
|
||||
///
|
||||
/// Note what the abort does and does not buy the prompt: the typed path
|
||||
/// survives for the user to correct, but the message does not reach the
|
||||
/// screen, because `draw_bottom_bar` draws the minibuffer instead of the
|
||||
/// status line while a prompt is open. That is pre-existing and reported
|
||||
/// separately; the data-loss half is what this fixes.
|
||||
#[test]
|
||||
fn failed_save_as_aborts_the_chain() {
|
||||
let mut app = test_app();
|
||||
app.model_state.dirty = true;
|
||||
app.apply_effects(vec![
|
||||
Box::new(SaveAs(PathBuf::from(
|
||||
"/nonexistent-dir-improvise-lae/model.improv",
|
||||
))),
|
||||
Box::new(ChangeMode(AppMode::Quit)),
|
||||
]);
|
||||
assert_ne!(
|
||||
app.view_state.mode,
|
||||
AppMode::Quit,
|
||||
"a failed save-as must not let the trailing Quit apply"
|
||||
);
|
||||
assert!(
|
||||
app.model_state.dirty,
|
||||
"a failed save-as must leave the model dirty"
|
||||
);
|
||||
assert!(
|
||||
app.view_state.status_msg.starts_with("Save error:"),
|
||||
"expected the save error to stay on screen, got: {}",
|
||||
app.view_state.status_msg
|
||||
);
|
||||
}
|
||||
|
||||
/// The positive arm: a `SaveAs` that succeeds must not abort, or every
|
||||
/// `:w <path>` would swallow whatever followed it. Without this,
|
||||
/// "always abort" passes the test above.
|
||||
#[test]
|
||||
fn successful_save_as_lets_the_chain_continue() {
|
||||
let dir = std::env::temp_dir().join("improvise-lae-save-as-ok");
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let path = dir.join("model.improv");
|
||||
let mut app = test_app();
|
||||
app.model_state.dirty = true;
|
||||
app.apply_effects(vec![
|
||||
Box::new(SaveAs(path.clone())),
|
||||
Box::new(ChangeMode(AppMode::Quit)),
|
||||
]);
|
||||
assert_eq!(app.view_state.mode, AppMode::Quit);
|
||||
assert!(!app.model_state.dirty);
|
||||
assert_eq!(
|
||||
app.model_state.file_path.as_deref(),
|
||||
Some(path.as_path()),
|
||||
"a successful save-as adopts the path"
|
||||
);
|
||||
let _ = std::fs::remove_file(&path);
|
||||
}
|
||||
|
||||
/// A successful save must not abort — `wq` still quits.
|
||||
#[test]
|
||||
fn successful_save_lets_the_chain_continue() {
|
||||
|
||||
@@ -315,10 +315,12 @@ fn cell_id(app: &App, row: usize, col: usize) -> Option<CellId> {
|
||||
RecordCol::Measure(measure) => Some(measure.clone()),
|
||||
// Not cells to write into. Editing a category column renames a
|
||||
// coordinate across every measure cell in the row (gotcha 1) and
|
||||
// the `+` column creates a measure; both are different intents
|
||||
// from `SetCell`, so neither gets an id it could be composed
|
||||
// with.
|
||||
RecordCol::Category(_) | RecordCol::NewMeasure => return None,
|
||||
// the `+`/`+cat` columns create a measure or a category; all are
|
||||
// different intents from `SetCell`, so none gets an id it could
|
||||
// be composed with.
|
||||
RecordCol::Category(_) | RecordCol::NewMeasure | RecordCol::NewCategory => {
|
||||
return None;
|
||||
}
|
||||
};
|
||||
return Some(CellId::Record(RecordCell {
|
||||
record: RecordHash::of(record),
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
use std::rc::Rc;
|
||||
|
||||
use crate::model::Model;
|
||||
use crate::model::category::{CategoryKind, DIM_CATEGORY, INDEX_CATEGORY, MEASURE_CATEGORY};
|
||||
use crate::model::category::{DIM_CATEGORY, INDEX_CATEGORY, MEASURE_CATEGORY};
|
||||
use crate::model::cell::{CellKey, CellValue};
|
||||
use crate::view::{Axis, View};
|
||||
|
||||
@@ -10,6 +10,14 @@ use crate::view::{Axis, View};
|
||||
/// measure column (improvise-c5v).
|
||||
pub const NEW_MEASURE_COL: &str = "+";
|
||||
|
||||
/// The label of the column that creates a new **category** (improvise-2zq).
|
||||
///
|
||||
/// It sits at the end of the category block rather than trailing the whole
|
||||
/// grid, so each `+` is adjacent to what it adds to: `+cat` after the
|
||||
/// coordinates, `+` after the measures. Two trailing columns would have made
|
||||
/// the user read a label to tell them apart; position says it instead.
|
||||
pub const NEW_CATEGORY_COL: &str = "+cat";
|
||||
|
||||
/// Extract (record_index, column_index) from a synthetic records-mode
|
||||
/// CellKey. Returns None for normal pivot-mode keys.
|
||||
///
|
||||
@@ -84,6 +92,9 @@ pub enum RecordCol {
|
||||
Measure(String),
|
||||
/// The trailing column that creates a new measure.
|
||||
NewMeasure,
|
||||
/// The column that creates a new category (improvise-2zq). Sits after
|
||||
/// the coordinates, where the column it creates will appear.
|
||||
NewCategory,
|
||||
}
|
||||
|
||||
/// One entry on a grid axis: either a visual group header or a data-item tuple.
|
||||
@@ -241,11 +252,10 @@ impl GridLayout {
|
||||
})
|
||||
.collect();
|
||||
|
||||
// Detect records mode: _Index on Row and _Dim on Col
|
||||
let is_records_mode = row_cats.iter().any(|c| CategoryKind::of_name(c).is_index())
|
||||
&& col_cats.iter().any(|c| CategoryKind::of_name(c).is_dim());
|
||||
|
||||
if is_records_mode {
|
||||
// Records mode is a property of the view's axes, and `View` owns the
|
||||
// one definition of it (improvise-2zq): `on_category_added` must ask
|
||||
// the same question, and two derivations would drift.
|
||||
if view.is_records_layout() {
|
||||
Self::build_records_mode(model, view, page_coords, none_cats)
|
||||
} else {
|
||||
let row_items = cross_product(model, view, &row_cats);
|
||||
@@ -321,6 +331,7 @@ impl GridLayout {
|
||||
})
|
||||
.map(|c| RecordCol::Category(c.to_string()))
|
||||
.collect();
|
||||
cols.push(RecordCol::NewCategory);
|
||||
cols.extend(
|
||||
model
|
||||
.category(MEASURE_CATEGORY)
|
||||
@@ -337,6 +348,7 @@ impl GridLayout {
|
||||
match col {
|
||||
RecordCol::Category(name) | RecordCol::Measure(name) => name.clone(),
|
||||
RecordCol::NewMeasure => NEW_MEASURE_COL.to_string(),
|
||||
RecordCol::NewCategory => NEW_CATEGORY_COL.to_string(),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -368,7 +380,7 @@ impl GridLayout {
|
||||
),
|
||||
// Always blank: it is where a new measure name is typed, never
|
||||
// a place a value is stored.
|
||||
RecordCol::NewMeasure => Some(String::new()),
|
||||
RecordCol::NewMeasure | RecordCol::NewCategory => Some(String::new()),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -606,8 +618,8 @@ fn cross_product(model: &Model, view: &View, cats: &[String]) -> Vec<AxisEntry>
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::{
|
||||
AxisEntry, GridLayout, NEW_MEASURE_COL, RecordCol, data_items, prune_axis_items,
|
||||
synthetic_record_info,
|
||||
AxisEntry, GridLayout, NEW_CATEGORY_COL, NEW_MEASURE_COL, RecordCol, data_items,
|
||||
prune_axis_items, synthetic_record_info,
|
||||
};
|
||||
use crate::model::cell::{CellKey, CellValue};
|
||||
use crate::view::Axis;
|
||||
@@ -836,22 +848,33 @@ mod tests {
|
||||
.collect();
|
||||
assert_eq!(
|
||||
cols,
|
||||
vec!["Region", "Revenue", "Cost", NEW_MEASURE_COL],
|
||||
"categories, then one column per measure, then the new-measure column"
|
||||
vec![
|
||||
"Region",
|
||||
NEW_CATEGORY_COL,
|
||||
"Revenue",
|
||||
"Cost",
|
||||
NEW_MEASURE_COL
|
||||
],
|
||||
"categories, the new-category column, one column per measure, \
|
||||
then the new-measure column — each `+` adjacent to what it adds \
|
||||
to (improvise-2zq)"
|
||||
);
|
||||
assert_eq!(
|
||||
layout.record_col(0),
|
||||
Some(&RecordCol::Category("Region".to_string()))
|
||||
);
|
||||
assert_eq!(layout.record_col(1), Some(&RecordCol::NewCategory));
|
||||
assert_eq!(
|
||||
layout.record_col(1),
|
||||
layout.record_col(2),
|
||||
Some(&RecordCol::Measure("Revenue".to_string()))
|
||||
);
|
||||
assert_eq!(layout.record_col(3), Some(&RecordCol::NewMeasure));
|
||||
assert_eq!(layout.record_col(4), Some(&RecordCol::NewMeasure));
|
||||
|
||||
// Both measures of the one row are readable side by side.
|
||||
assert_eq!(layout.records_display(0, 1).as_deref(), Some("100"));
|
||||
assert_eq!(layout.records_display(0, 2).as_deref(), Some("50"));
|
||||
// Both measures of the one row are readable side by side, and the
|
||||
// `+cat` column between them and the coordinate is always blank.
|
||||
assert_eq!(layout.records_display(0, 1).as_deref(), Some(""));
|
||||
assert_eq!(layout.records_display(0, 2).as_deref(), Some("100"));
|
||||
assert_eq!(layout.records_display(0, 3).as_deref(), Some("50"));
|
||||
}
|
||||
|
||||
/// On initial entry into records mode, rows should be in a deterministic
|
||||
|
||||
@@ -4,6 +4,7 @@ pub mod types;
|
||||
|
||||
pub use axis::{Axis, AxisColor};
|
||||
pub use layout::{
|
||||
AxisEntry, GridLayout, NEW_MEASURE_COL, RecordCol, RecordRow, synthetic_record_info,
|
||||
AxisEntry, GridLayout, NEW_CATEGORY_COL, NEW_MEASURE_COL, RecordCol, RecordRow,
|
||||
synthetic_record_info,
|
||||
};
|
||||
pub use types::{DRILL_VIEW, RECORDS_VIEW, VIRTUAL_VIEW_NAMES, View};
|
||||
|
||||
@@ -3,6 +3,7 @@ use serde::{Deserialize, Serialize};
|
||||
use std::collections::{HashMap, HashSet};
|
||||
|
||||
use super::axis::Axis;
|
||||
use crate::model::category::CategoryKind;
|
||||
|
||||
fn default_prune() -> bool {
|
||||
true
|
||||
@@ -56,52 +57,70 @@ impl View {
|
||||
}
|
||||
}
|
||||
|
||||
/// True when this view's axes spell **records mode**: `_Index` on Row and
|
||||
/// `_Dim` on Column.
|
||||
///
|
||||
/// The one definition — `GridLayout` asks this rather than re-deriving it,
|
||||
/// because two answers to "is this a records view" is the disease this
|
||||
/// codebase keeps curing (improvise-edp). It is a property of the *view's
|
||||
/// axes* alone, which is why it lives here and not on the layout.
|
||||
pub fn is_records_layout(&self) -> bool {
|
||||
self.categories_on(Axis::Row)
|
||||
.iter()
|
||||
.any(|c| CategoryKind::of_name(c).is_index())
|
||||
&& self
|
||||
.categories_on(Axis::Column)
|
||||
.iter()
|
||||
.any(|c| CategoryKind::of_name(c).is_dim())
|
||||
}
|
||||
|
||||
/// Seat a newly added category on an axis.
|
||||
///
|
||||
/// Virtual (underscore) categories default to `Axis::None`. A regular
|
||||
/// category takes the first slot no *regular* category holds — Row, then
|
||||
/// Column, then Page.
|
||||
///
|
||||
/// # A virtual on an axis is a configuration, not a vacancy (improvise-2zq)
|
||||
///
|
||||
/// This used to **bump** a virtual off Row or Column to make room, on the
|
||||
/// assumption that a virtual sitting there was a placeholder. It never is,
|
||||
/// and the assumption cost two live bugs:
|
||||
///
|
||||
/// - `_Index` on Row together with `_Dim` on Column *is* records mode, and
|
||||
/// the only production writers of those axes are the records and drill
|
||||
/// setups. Bumping `_Index` therefore **ejected the user from the grid
|
||||
/// they were standing in**: naming a category with `N` in records mode
|
||||
/// silently returned them to a pivot.
|
||||
/// - `_Measure` on Column is an ordinary user configuration (it is what
|
||||
/// `web/demo.improv` ships). Bumping it made the measures vanish from
|
||||
/// the grid the moment a second regular category was added.
|
||||
///
|
||||
/// So a virtual is never moved. A records view seats the new category on
|
||||
/// `Axis::None`, which is where records mode keeps **every** regular
|
||||
/// category and where `GridLayout::records_columns` — which filters on
|
||||
/// `kind.is_regular()` and never looks at an axis — picks it up as a
|
||||
/// column immediately. Elsewhere the new category simply joins whatever
|
||||
/// virtual is already there, which nests a header rather than destroying
|
||||
/// one.
|
||||
///
|
||||
/// The Row → Column → Page progression for regular categories is
|
||||
/// unchanged, and `a_pivot_view_still_seats_categories_row_then_column_then_page`
|
||||
/// is the negative arm that says so — without it, "always seat on None"
|
||||
/// would pass every assertion about records mode.
|
||||
pub fn on_category_added(&mut self, cat_name: &str) {
|
||||
if !self.category_axes.contains_key(cat_name) {
|
||||
// Virtual/underscore categories default to Axis::None.
|
||||
// Regular categories auto-assign: first → Row, second → Column, rest → Page.
|
||||
// If a virtual currently holds Row or Column and a regular category needs
|
||||
// the slot, bump the virtual to None.
|
||||
let axis = if cat_name.starts_with('_') {
|
||||
let axis = if cat_name.starts_with('_') || self.is_records_layout() {
|
||||
Axis::None
|
||||
} else {
|
||||
let regular_rows: Vec<String> = self
|
||||
.categories_on(Axis::Row)
|
||||
.into_iter()
|
||||
.filter(|c| !c.starts_with('_'))
|
||||
.map(String::from)
|
||||
.collect();
|
||||
let regular_cols: Vec<String> = self
|
||||
.categories_on(Axis::Column)
|
||||
.into_iter()
|
||||
.filter(|c| !c.starts_with('_'))
|
||||
.map(String::from)
|
||||
.collect();
|
||||
if regular_rows.is_empty() {
|
||||
// Bump any virtual on Row to None
|
||||
let bump: Vec<String> = self
|
||||
.categories_on(Axis::Row)
|
||||
let holds_a_regular = |axis: Axis| {
|
||||
self.categories_on(axis)
|
||||
.into_iter()
|
||||
.filter(|c| c.starts_with('_'))
|
||||
.map(String::from)
|
||||
.collect();
|
||||
for c in bump {
|
||||
self.category_axes.insert(c, Axis::None);
|
||||
}
|
||||
Axis::Row
|
||||
} else if regular_cols.is_empty() {
|
||||
let bump: Vec<String> = self
|
||||
.categories_on(Axis::Column)
|
||||
.into_iter()
|
||||
.filter(|c| c.starts_with('_'))
|
||||
.map(String::from)
|
||||
.collect();
|
||||
for c in bump {
|
||||
self.category_axes.insert(c, Axis::None);
|
||||
}
|
||||
Axis::Column
|
||||
} else {
|
||||
Axis::Page
|
||||
.any(|c| !c.starts_with('_'))
|
||||
};
|
||||
match (holds_a_regular(Axis::Row), holds_a_regular(Axis::Column)) {
|
||||
(false, _) => Axis::Row,
|
||||
(true, false) => Axis::Column,
|
||||
(true, true) => Axis::Page,
|
||||
}
|
||||
};
|
||||
self.category_axes.insert(cat_name.to_string(), axis);
|
||||
@@ -278,6 +297,71 @@ mod tests {
|
||||
v
|
||||
}
|
||||
|
||||
/// improvise-2zq, the **negative arm**. The fix makes a records view seat
|
||||
/// a new category on `None`; without this, "always seat on None" would
|
||||
/// pass every assertion about records mode while quietly deleting the
|
||||
/// auto-assignment `on_category_added` exists for.
|
||||
///
|
||||
/// Probing confirmed this arm is the load-bearing one: seating every
|
||||
/// category on `None` fails 12 tests, while removing the virtual bump
|
||||
/// fails none at all — which is how the bump came to be wrong in two
|
||||
/// directions with nothing noticing.
|
||||
#[test]
|
||||
fn a_pivot_view_still_seats_categories_row_then_column_then_page() {
|
||||
let v = view_with_cats(&["A", "B", "C", "D"]);
|
||||
assert_eq!(v.axis_of("A"), Axis::Row, "first regular → Row");
|
||||
assert_eq!(v.axis_of("B"), Axis::Column, "second regular → Column");
|
||||
assert_eq!(v.axis_of("C"), Axis::Page, "the rest → Page");
|
||||
assert_eq!(v.axis_of("D"), Axis::Page);
|
||||
}
|
||||
|
||||
/// A records view seats a new regular category on `None`, where records
|
||||
/// mode keeps all of them — and, crucially, leaves `_Index`/`_Dim` alone,
|
||||
/// so the view is still a records view afterwards.
|
||||
#[test]
|
||||
fn a_records_view_seats_a_new_category_on_none_and_stays_a_records_view() {
|
||||
let mut v = view_with_cats(&["_Index", "_Dim", "Region"]);
|
||||
v.set_axis("_Index", Axis::Row);
|
||||
v.set_axis("_Dim", Axis::Column);
|
||||
v.set_axis("Region", Axis::None);
|
||||
assert!(v.is_records_layout(), "setup");
|
||||
|
||||
v.on_category_added("Product");
|
||||
assert_eq!(v.axis_of("Product"), Axis::None);
|
||||
assert_eq!(v.axis_of("_Index"), Axis::Row, "_Index must not be bumped");
|
||||
assert_eq!(v.axis_of("_Dim"), Axis::Column, "_Dim must not be bumped");
|
||||
assert!(v.is_records_layout(), "still a records view");
|
||||
}
|
||||
|
||||
/// The second thing the bump broke, and the reason the fix is about
|
||||
/// virtuals in general rather than about records mode: `_Measure` on
|
||||
/// Column is an ordinary configuration — it is what `web/demo.improv`
|
||||
/// ships — and adding a second regular category used to kick it off the
|
||||
/// axis, making every measure vanish from the grid.
|
||||
///
|
||||
/// This view is deliberately **not** a records layout, so it exercises
|
||||
/// the pivot path rather than the early return above.
|
||||
#[test]
|
||||
fn a_measure_on_column_is_not_bumped_off_it() {
|
||||
let mut v = view_with_cats(&["_Measure", "Region"]);
|
||||
v.set_axis("_Measure", Axis::Column);
|
||||
assert_eq!(v.axis_of("Region"), Axis::Row, "setup");
|
||||
assert!(!v.is_records_layout(), "setup: a pivot view, not records");
|
||||
|
||||
v.on_category_added("Product");
|
||||
assert_eq!(
|
||||
v.axis_of("_Measure"),
|
||||
Axis::Column,
|
||||
"the user's measure axis must survive a new category"
|
||||
);
|
||||
assert_eq!(
|
||||
v.axis_of("Product"),
|
||||
Axis::Column,
|
||||
"the new category joins Column, nesting a header rather than \
|
||||
destroying one — Row already holds a regular category"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn first_category_assigned_to_row() {
|
||||
let v = view_with_cats(&["Region"]);
|
||||
|
||||
@@ -89,6 +89,7 @@ pub enum ModeKey {
|
||||
CategoryAdd,
|
||||
ItemAdd,
|
||||
ExportPrompt,
|
||||
SavePrompt,
|
||||
CommandMode,
|
||||
SearchMode,
|
||||
ImportWizard,
|
||||
@@ -111,6 +112,7 @@ impl ModeKey {
|
||||
AppMode::CategoryAdd { .. } => Some(ModeKey::CategoryAdd),
|
||||
AppMode::ItemAdd { .. } => Some(ModeKey::ItemAdd),
|
||||
AppMode::ExportPrompt { .. } => Some(ModeKey::ExportPrompt),
|
||||
AppMode::SavePrompt { .. } => Some(ModeKey::SavePrompt),
|
||||
AppMode::CommandMode { .. } => Some(ModeKey::CommandMode),
|
||||
AppMode::ImportWizard => Some(ModeKey::ImportWizard),
|
||||
AppMode::RecordsNormal => Some(ModeKey::RecordsNormal),
|
||||
@@ -570,7 +572,6 @@ impl Keymaps {
|
||||
);
|
||||
normal.bind(KeyCode::Char('/'), none, "search");
|
||||
normal.bind(KeyCode::Char('s'), ctrl, "save");
|
||||
normal.bind(KeyCode::F(1), none, "enter-mode");
|
||||
normal.bind_args(KeyCode::F(1), none, "enter-mode", vec!["help".into()]);
|
||||
normal.bind_args(KeyCode::Char('?'), none, "enter-mode", vec!["help".into()]);
|
||||
|
||||
@@ -619,6 +620,15 @@ impl Keymaps {
|
||||
"enter-mode",
|
||||
vec!["export-prompt".into()],
|
||||
);
|
||||
// Ctrl-S saves to the known path; Ctrl-W asks for one. The pair a
|
||||
// user with no file path needs, and what the "No file path" status
|
||||
// message points at (improvise-lae).
|
||||
normal.bind_args(
|
||||
KeyCode::Char('w'),
|
||||
ctrl,
|
||||
"enter-mode",
|
||||
vec!["save-prompt".into()],
|
||||
);
|
||||
|
||||
// Search / category add
|
||||
normal.bind_args(
|
||||
@@ -908,6 +918,15 @@ impl Keymaps {
|
||||
),
|
||||
);
|
||||
|
||||
set.insert(
|
||||
ModeKey::SavePrompt,
|
||||
Arc::new(
|
||||
TextEntry::new(&AppMode::save_prompt(), "normal")
|
||||
.commit(KeyCode::Enter, "commit-save", vec![])
|
||||
.finish(),
|
||||
),
|
||||
);
|
||||
|
||||
set.insert(
|
||||
ModeKey::CommandMode,
|
||||
Arc::new(
|
||||
|
||||
@@ -175,6 +175,11 @@ pub enum AppMode {
|
||||
ExportPrompt {
|
||||
minibuf: MinibufferConfig,
|
||||
},
|
||||
/// Ask for a path to write the *model* to. The sibling of `ExportPrompt`,
|
||||
/// which writes CSV; this one commits to `SaveAs` (improvise-lae).
|
||||
SavePrompt {
|
||||
minibuf: MinibufferConfig,
|
||||
},
|
||||
/// Vim-style `:` command line
|
||||
CommandMode {
|
||||
minibuf: MinibufferConfig,
|
||||
@@ -223,6 +228,7 @@ impl AppMode {
|
||||
"records-editing" => Some(Self::records_editing()),
|
||||
"formula-edit" => Some(Self::formula_edit()),
|
||||
"export-prompt" => Some(Self::export_prompt()),
|
||||
"save-prompt" => Some(Self::save_prompt()),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
@@ -241,6 +247,7 @@ impl AppMode {
|
||||
Self::TileSelect => "TILES",
|
||||
Self::ImportWizard => "IMPORT",
|
||||
Self::ExportPrompt { .. } => "EXPORT",
|
||||
Self::SavePrompt { .. } => "SAVE AS",
|
||||
Self::CommandMode { .. } => "COMMAND",
|
||||
Self::Help => "HELP",
|
||||
Self::Quit => "QUIT",
|
||||
@@ -278,6 +285,7 @@ impl AppMode {
|
||||
Self::ImportWizard => "Space:toggle c:cycle Enter:next Esc:cancel",
|
||||
Self::Help => "h/l:pages q/Esc:close",
|
||||
Self::ExportPrompt { .. } => "Enter:export Esc:cancel — type a file path",
|
||||
Self::SavePrompt { .. } => "Enter:save Esc:cancel — type a file path",
|
||||
Self::Quit => "",
|
||||
}
|
||||
}
|
||||
@@ -297,6 +305,7 @@ impl AppMode {
|
||||
| Self::TileSelect
|
||||
| Self::ImportWizard
|
||||
| Self::ExportPrompt { .. }
|
||||
| Self::SavePrompt { .. }
|
||||
| Self::CommandMode { .. }
|
||||
| Self::Help
|
||||
| Self::Quit
|
||||
@@ -314,7 +323,8 @@ impl AppMode {
|
||||
| Self::CommandMode { minibuf, .. }
|
||||
| Self::CategoryAdd { minibuf, .. }
|
||||
| Self::ItemAdd { minibuf, .. }
|
||||
| Self::ExportPrompt { minibuf, .. } => Some(minibuf),
|
||||
| Self::ExportPrompt { minibuf, .. }
|
||||
| Self::SavePrompt { minibuf, .. } => Some(minibuf),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
@@ -400,6 +410,16 @@ impl AppMode {
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
pub fn save_prompt() -> Self {
|
||||
Self::SavePrompt {
|
||||
minibuf: MinibufferConfig {
|
||||
buffer_key: "save".into(),
|
||||
prompt: "save as: ".into(),
|
||||
color: MinibufColor::Yellow,
|
||||
},
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// UI session-state slice: mode, cursors, panels, buffers, navigation stacks,
|
||||
|
||||
@@ -316,7 +316,10 @@ fn step(state: &ClientState, event: KeyEvent, name: &str, args: &[String]) -> Op
|
||||
// ── mode entry ───────────────────────────────────────────────
|
||||
"enter-mode" => {
|
||||
let mode = AppMode::from_name(args.first()?)?;
|
||||
Some(entered(mode))
|
||||
match strands_the_user(&mode) {
|
||||
true => None,
|
||||
false => Some(entered(mode)),
|
||||
}
|
||||
}
|
||||
|
||||
// ── panels ───────────────────────────────────────────────────
|
||||
@@ -427,6 +430,68 @@ fn write_buffer(name: &str, value: String) -> Command {
|
||||
}
|
||||
}
|
||||
|
||||
/// A prompt this client can *enter* but cannot *commit*.
|
||||
///
|
||||
/// `enter-mode` is trivially resolvable here, and that is the trap: the user
|
||||
/// reaches the prompt, types a path, presses Enter — and the commit resolves
|
||||
/// to nothing, because `Host::handle` answers a `Key` frame with an empty
|
||||
/// vec (`doctrine(an_upstream_key_is_a_black_hole)`). A dead room is worse
|
||||
/// than a locked door: the door costs one inert key, the room swallows work
|
||||
/// the user believes was done.
|
||||
///
|
||||
/// Both prompts are host-only **by construction**, not by omission.
|
||||
/// `effect::Save`, `SaveAs` and `ExportCsv` are all `EffectKind::ViewOnly`,
|
||||
/// so the derivation that builds `ModelCommand` from the model effects
|
||||
/// cannot name them — no client on any transport can ask its host to write a
|
||||
/// file (gotcha 45). Closing this is not a resolver to add; it is a wire
|
||||
/// operation that does not exist. In a browser it is also close to
|
||||
/// meaningless: the worker persists on every `touches_document`, so the
|
||||
/// document *is* the OPFS entry and there is no Ctrl-S semantics to offer.
|
||||
///
|
||||
/// Named as a property rather than matched at the call site
|
||||
/// (`doctrine(two_questions_one_enum)`): "can this client finish what
|
||||
/// entering the mode starts" is not the same question as "is this a
|
||||
/// text-entry mode", and a third such prompt belongs here.
|
||||
///
|
||||
/// Found by Copilot on PR #27 against the newly added `SavePrompt`.
|
||||
/// `ExportPrompt` had the identical trap already and is fixed with it —
|
||||
/// fixing one arm of a rule with two is the shape
|
||||
/// `doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)`
|
||||
/// names.
|
||||
///
|
||||
/// # `CommandMode` is dead by **omission**, and listed anyway
|
||||
///
|
||||
/// Copilot found it on the next round, and it was a false negative in this
|
||||
/// predicate's *first* version — which mattered more than its severity,
|
||||
/// because `a_prompt_this_client_cannot_commit_is_never_entered` used `:`
|
||||
/// as the negative arm that stops "refuse every mode" from passing. An arm
|
||||
/// asserting something false does not merely fail to guard; it **certifies
|
||||
/// the behaviour it was added to rule out.**
|
||||
///
|
||||
/// `:` resolves locally into `CommandMode` and typing appends locally, but
|
||||
/// Enter is `[execute-command, clear-buffer]` and [`step`] has no
|
||||
/// `execute-command` arm, so `resolve` returns `Upstream` and the frame is
|
||||
/// dropped. Every ex-command behind `:` is inert for the same reason —
|
||||
/// there is one door and no client arm for it — so refusing the mode
|
||||
/// removes **no working surface**, only a room that swallows typing.
|
||||
/// `the_command_line_cannot_be_committed_by_this_client` is the evidence,
|
||||
/// not the assertion.
|
||||
///
|
||||
/// The distinction from the two prompts is worth keeping visible, because
|
||||
/// it says who removes this line. They are dead **by construction**: no
|
||||
/// `ModelCommand` can ever name `Save`/`SaveAs`/`ExportCsv`, so no resolver
|
||||
/// would help. This is dead **by omission**: `execute-command` is
|
||||
/// implementable client-side and improvise-e24v is scheduled to do it.
|
||||
/// Listing it here is therefore also the named gap gotcha 47 asks for —
|
||||
/// closing e24v has to come to this line and delete it, which a comment
|
||||
/// could not have forced.
|
||||
fn strands_the_user(mode: &AppMode) -> bool {
|
||||
matches!(
|
||||
mode,
|
||||
AppMode::SavePrompt { .. } | AppMode::ExportPrompt { .. } | AppMode::CommandMode { .. }
|
||||
)
|
||||
}
|
||||
|
||||
/// `EnterMode::execute`: clear the incoming mode's minibuffer, then change.
|
||||
fn entered(mode: AppMode) -> Vec<Command> {
|
||||
let mut commands = Vec::new();
|
||||
@@ -726,8 +791,16 @@ mod tests {
|
||||
KeyEvent::new(code, KeyModifiers::NONE)
|
||||
}
|
||||
|
||||
/// Every text-entry mode, built by its own constructor so a new one shows
|
||||
/// up here as a missing line rather than as untested behaviour.
|
||||
/// Every text-entry mode, built by its own constructor.
|
||||
///
|
||||
/// The comment here used to claim a new mode "shows up as a missing line
|
||||
/// rather than as untested behaviour". It does not: this is a hand-written
|
||||
/// list and nothing checks it against `AppMode`, so a new text-entry mode
|
||||
/// is simply absent and silently uncovered — which is what happened to
|
||||
/// `save_prompt` (improvise-lae) until it was added by hand. There is a
|
||||
/// second hand-written copy in `improvise-command`'s
|
||||
/// `keymap::tests::text_entry_modes_with_exits`. Deriving both from one
|
||||
/// place is filed, not done here.
|
||||
fn text_entry_modes() -> Vec<AppMode> {
|
||||
vec![
|
||||
AppMode::editing(),
|
||||
@@ -737,6 +810,7 @@ mod tests {
|
||||
AppMode::category_add(),
|
||||
AppMode::item_add("Region".into()),
|
||||
AppMode::export_prompt(),
|
||||
AppMode::save_prompt(),
|
||||
]
|
||||
}
|
||||
|
||||
@@ -1004,6 +1078,17 @@ mod tests {
|
||||
KeyEvent::new(KeyCode::Char('q'), KeyModifiers::CONTROL),
|
||||
"force-quit",
|
||||
),
|
||||
// The two doors this client refuses rather than opens —
|
||||
// `enter-mode` itself is resolvable, the commit behind it is
|
||||
// not. See `strands_the_user`.
|
||||
(
|
||||
KeyEvent::new(KeyCode::Char('w'), KeyModifiers::CONTROL),
|
||||
"enter-mode save-prompt (commit-save is the host's)",
|
||||
),
|
||||
(
|
||||
KeyEvent::new(KeyCode::Char('e'), KeyModifiers::CONTROL),
|
||||
"enter-mode export-prompt (commit-export is the host's)",
|
||||
),
|
||||
] {
|
||||
assert_eq!(
|
||||
resolve(&state, key),
|
||||
@@ -1015,6 +1100,102 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// The refusal above is only half the property. `Upstream` says the key
|
||||
/// produced no `Command`; it does not say the client stayed put, and the
|
||||
/// bug being guarded is precisely *entering a room with no exit*.
|
||||
///
|
||||
/// Found by Copilot on PR #27. Without `strands_the_user`, `Ctrl-W`
|
||||
/// resolved `enter-mode save-prompt` locally — `ChangeMode(SavePrompt)`,
|
||||
/// a perfectly good `Command` — and the user then typed a path into a
|
||||
/// prompt whose Enter goes nowhere. `Ctrl-E` did the same into
|
||||
/// `ExportPrompt` and had done since before this branch.
|
||||
///
|
||||
/// The negative arm matters as much: refusing *every* `enter-mode` would
|
||||
/// pass the assertions above, so a mode the client can finish is checked
|
||||
/// here too.
|
||||
#[test]
|
||||
fn a_prompt_this_client_cannot_commit_is_never_entered() {
|
||||
let state = in_mode(AppMode::Normal);
|
||||
for (key, mode) in [
|
||||
(
|
||||
KeyEvent::new(KeyCode::Char('w'), KeyModifiers::CONTROL),
|
||||
"save-prompt",
|
||||
),
|
||||
(
|
||||
KeyEvent::new(KeyCode::Char('e'), KeyModifiers::CONTROL),
|
||||
"export-prompt",
|
||||
),
|
||||
] {
|
||||
assert_eq!(
|
||||
resolve(&state, key),
|
||||
KeyOutcome::Upstream(key),
|
||||
"{mode} must not be entered by a client that cannot commit it"
|
||||
);
|
||||
}
|
||||
|
||||
// The negative arm has **two halves**, and both are needed.
|
||||
//
|
||||
// Half one: a mode that is not refused is still entered. This is the
|
||||
// half that goes through `strands_the_user`, so it is the only thing
|
||||
// that can fail when the predicate degenerates to "refuse
|
||||
// everything". `?` is honest here where `:` was not: Help has no
|
||||
// commit at all, so nothing a user types into it can be swallowed,
|
||||
// and Esc closes it locally. (Its page keys *are* upstream, which
|
||||
// makes page navigation inert — but inert navigation loses no work,
|
||||
// which is the line this predicate draws.)
|
||||
assert_eq!(
|
||||
resolve(&state, ev(KeyCode::Char('?'))),
|
||||
KeyOutcome::Local(vec![Command::ChangeMode(AppMode::Help)]),
|
||||
"a mode this client can finish must still be entered, or the \
|
||||
guard above is just \"refuse every mode\""
|
||||
);
|
||||
|
||||
// Half two: the property the predicate is *about* is finishing, not
|
||||
// entering — so something has to press Enter and observe. The first
|
||||
// version of this test asserted only that `:` entered its mode, and
|
||||
// `:` is itself a dead room (see
|
||||
// `the_command_line_cannot_be_committed_by_this_client`), so the arm
|
||||
// was certifying the very behaviour it existed to rule out.
|
||||
let mut client = client_on(record_cell());
|
||||
client.state_mut().view.mode = AppMode::editing();
|
||||
client
|
||||
.state_mut()
|
||||
.view
|
||||
.buffers
|
||||
.insert("edit".into(), "42".into());
|
||||
let KeyOutcome::Local(commands) = resolve(client.state(), ev(KeyCode::Enter)) else {
|
||||
panic!("Enter in Editing must resolve here — that is the arm");
|
||||
};
|
||||
assert!(
|
||||
commands.iter().any(|c| matches!(c, Command::Model { .. })),
|
||||
"and it must actually write something, not merely change mode: \
|
||||
{commands:?}"
|
||||
);
|
||||
}
|
||||
|
||||
/// The evidence behind listing `CommandMode` in `strands_the_user`,
|
||||
/// rather than the assertion that it is listed.
|
||||
///
|
||||
/// `:` opens locally and typing appends locally, but Enter is
|
||||
/// `[execute-command, clear-buffer]` and `step` has no `execute-command`
|
||||
/// arm — so the whole sequence fails to resolve, `Host::handle` drops
|
||||
/// the frame, and every ex-command behind `:` is inert. That is what
|
||||
/// makes refusing the mode cost no working surface.
|
||||
///
|
||||
/// When improvise-e24v gives this client `execute-command`, this test is
|
||||
/// the one that should start failing, and the `CommandMode` arm of
|
||||
/// `strands_the_user` is what it points at.
|
||||
#[test]
|
||||
fn the_command_line_cannot_be_committed_by_this_client() {
|
||||
let mut state = in_mode(AppMode::command_mode());
|
||||
state.view.buffers.insert("command".into(), "q".into());
|
||||
assert_eq!(
|
||||
resolve(&state, ev(KeyCode::Enter)),
|
||||
KeyOutcome::Upstream(ev(KeyCode::Enter)),
|
||||
"`:q` — and every other ex-command — resolves to nothing here"
|
||||
);
|
||||
}
|
||||
|
||||
/// The four keys an independent browser smoke test found inert, plus the
|
||||
/// two the editing path needs. Each asserts the *shape* of what is now
|
||||
/// composed, because "resolves locally" alone would pass on a wrong
|
||||
|
||||
@@ -39,8 +39,10 @@ fn imported_model_shows_one_row_per_record_with_measure_columns() {
|
||||
.collect();
|
||||
assert_eq!(
|
||||
cols,
|
||||
vec!["product", "region", "cost", "revenue", "+"],
|
||||
"categories, then one column per measure, then the new-measure column"
|
||||
vec!["product", "region", "+cat", "cost", "revenue", "+"],
|
||||
"categories, the new-category column, one column per measure, then \
|
||||
the new-measure column — each `+` sits next to what it adds to \
|
||||
(improvise-2zq)"
|
||||
);
|
||||
assert_eq!(layout.row_count(), 2, "two source records, not four cells");
|
||||
|
||||
@@ -57,8 +59,8 @@ fn imported_model_shows_one_row_per_record_with_measure_columns() {
|
||||
assert_eq!(
|
||||
rows,
|
||||
vec![
|
||||
vec!["Pants", "West", "80", "200", ""],
|
||||
vec!["Shirts", "East", "40", "100", ""],
|
||||
vec!["Pants", "West", "", "80", "200", ""],
|
||||
vec!["Shirts", "East", "", "40", "100", ""],
|
||||
],
|
||||
"got {rows:?}"
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user