fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge remote-tracking branch 'gh/main' into wave/c-host-and-save

Browse Source
This commit is contained in:
Edward Langley
2026-09-19 23:43:18 -07:00
parent c9a4961d12 391599476b
commit 6b7a044a01
31 changed files with 2507 additions and 1491 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/improvise-command/src/command/cmd/commit.rs
+167 -2
View File
@@ -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, &reg);
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, &reg);
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) ────────────────────────
@@ -292,6 +341,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),
}
}
@@ -309,6 +359,29 @@ fn add_measure(value: &str, effects: &mut Vec<Box<dyn Effect>>) {
}));
}
/// 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,
@@ -320,8 +393,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),
}
@@ -331,6 +406,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 {
@@ -374,14 +462,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 {
@@ -542,6 +659,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;