diff --git a/src/app.rs b/src/app.rs index a3c45a3..a062b9e 100644 --- a/src/app.rs +++ b/src/app.rs @@ -639,16 +639,22 @@ impl App { } /// Tab key handler — insert / cycle forward through the - /// candidates at the cursor (ADR-0022 stage 8). Three - /// shapes: - /// - memo present: cycle to next candidate, replace - /// inserted_range, advance memo.selection_idx; - /// - memo absent + multi-candidate: insert first - /// candidate (with trailing space) and create memo; - /// - memo absent + single candidate: insert it and - /// create a one-element memo so Esc/Backspace can - /// still undo; - /// - no candidates: no-op. + /// candidates at the cursor (ADR-0022 stage 8). Behaviour + /// is split between single and multi-candidate cases per + /// the user's stage-8 feedback round 2: + /// + /// - **Single candidate**: insert with trailing space, + /// no memo. The space is the natural commit; subsequent + /// Tab fresh-computes at the new cursor. Esc/Backspace + /// are normal. + /// - **Multi candidate**: insert WITHOUT trailing space, + /// create memo for cycling. Tab again cycles; any other + /// key clears the memo. Pressing space (the natural + /// "I'm done picking" gesture) clears the memo and adds + /// the space, completing the chosen candidate. + /// - **Memo present**: cycle to next candidate, replacing + /// the inserted text in place (still no trailing space). + /// - **No candidates**: no-op. fn completion_tab_forward(&mut self) -> Vec { self.cancel_history_navigation(); if let Some(memo) = self.last_completion.take() { @@ -656,17 +662,14 @@ impl App { self.last_completion = Some(self.replace_inserted(memo, next)); return Vec::new(); } - if let Some(memo) = self.start_completion(0) { - self.last_completion = Some(memo); - } + self.start_or_complete_at(0); Vec::new() } - /// Shift-Tab key handler — symmetric to - /// `completion_tab_forward` but cycles backward, and - /// starts from the last candidate (alphabetically) if no - /// memo exists. Per the user's #2: wrap-around at both - /// ends. + /// Shift-Tab key handler — symmetric to forward; on a + /// fresh multi-candidate position starts from the last + /// candidate (per the user's #2 wrap-from-both-ends). + /// Single candidate behaves identically to Tab. fn completion_tab_backward(&mut self) -> Vec { self.cancel_history_navigation(); if let Some(memo) = self.last_completion.take() { @@ -674,16 +677,17 @@ impl App { self.last_completion = Some(self.replace_inserted(memo, prev)); return Vec::new(); } - if let Some(memo) = self.start_completion_last() { - self.last_completion = Some(memo); - } + self.start_or_complete_last(); Vec::new() } /// Esc / Backspace handler while a completion memo is - /// alive — restore the original text in - /// `inserted_range` and place the cursor where the user - /// was when they hit Tab. The memo is cleared. + /// alive — restore the original text in `inserted_range` + /// and place the cursor where the user was when they hit + /// Tab. The memo is cleared. Only fires on multi-candidate + /// completions (single-candidate paths don't create a + /// memo); the user accepts that single-candidate Tab + /// requires regular backspace to undo. fn undo_last_completion(&mut self) { let Some(memo) = self.last_completion.take() else { return; @@ -693,60 +697,87 @@ impl App { self.input_cursor = start + memo.original_text.len(); } - /// Compute the candidates at the cursor and insert the - /// candidate at index `start_idx` (with a trailing space). - /// Returns the freshly-built memo. `None` if there are no - /// candidates to insert. - fn start_completion(&mut self, start_idx: usize) -> Option { + fn start_or_complete_at(&mut self, multi_start_idx: usize) { let cursor = self.input_cursor.min(self.input.len()); - let comp = crate::completion::candidates_at_cursor( + let Some(comp) = crate::completion::candidates_at_cursor( &self.input, cursor, &self.schema_cache, - )?; - let idx = start_idx % comp.candidates.len(); - let inserted = format!("{} ", comp.candidates[idx].text); + ) else { + return; + }; + if comp.candidates.len() == 1 { + self.commit_unique(&comp); + } else { + let idx = multi_start_idx % comp.candidates.len(); + self.last_completion = Some(self.commit_multi(comp, idx)); + } + } + + fn start_or_complete_last(&mut self) { + let cursor = self.input_cursor.min(self.input.len()); + let Some(comp) = crate::completion::candidates_at_cursor( + &self.input, + cursor, + &self.schema_cache, + ) else { + return; + }; + if comp.candidates.len() == 1 { + self.commit_unique(&comp); + } else { + let idx = comp.candidates.len() - 1; + self.last_completion = Some(self.commit_multi(comp, idx)); + } + } + + /// Single-candidate commit: insert " " (with trailing + /// space) and DO NOT create a memo. The user can keep + /// typing or press Tab again to fresh-complete at the new + /// cursor. + fn commit_unique(&mut self, comp: &crate::completion::Completion) { + let inserted = format!("{} ", comp.candidates[0].text); + self.input + .replace_range(comp.replaced_range.0..comp.replaced_range.1, &inserted); + self.input_cursor = comp.replaced_range.0 + inserted.len(); + } + + /// Multi-candidate commit: insert just the candidate text + /// (no trailing space) and return the memo carrying the + /// full candidate list for cycling. The user presses space + /// (or any other non-Tab key) to commit the choice — that + /// clears the memo and inserts whatever they typed + /// normally, naturally producing " " as the + /// completed text. + fn commit_multi( + &mut self, + comp: crate::completion::Completion, + idx: usize, + ) -> crate::completion::LastCompletion { + let inserted = comp.candidates[idx].text.clone(); let original_text = self.input[comp.replaced_range.0..comp.replaced_range.1].to_string(); self.input .replace_range(comp.replaced_range.0..comp.replaced_range.1, &inserted); let new_end = comp.replaced_range.0 + inserted.len(); self.input_cursor = new_end; - Some(crate::completion::LastCompletion { + crate::completion::LastCompletion { inserted_range: (comp.replaced_range.0, new_end), original_text, candidates: comp.candidates, selection_idx: idx, - }) + } } - /// Same as `start_completion` but starts from the last - /// candidate alphabetically (for Shift-Tab from a fresh - /// state). - fn start_completion_last(&mut self) -> Option { - // We need to know how many candidates there will be - // before we can pick "last". Compute the completion - // first, then call start_completion with that index. - let cursor = self.input_cursor.min(self.input.len()); - let comp = crate::completion::candidates_at_cursor( - &self.input, - cursor, - &self.schema_cache, - )?; - let last = comp.candidates.len() - 1; - // Fall through to the same path so the inserted text - // and memo construction stay in one place. - self.start_completion(last) - } - - /// Replace the inserted text with `candidates[idx]` and + /// Replace the inserted text with `candidates[idx]` (no + /// trailing space — same multi-candidate convention) and /// return an updated memo. Used by Tab/Shift-Tab cycling. fn replace_inserted( &mut self, memo: crate::completion::LastCompletion, idx: usize, ) -> crate::completion::LastCompletion { - let new_inserted = format!("{} ", memo.candidates[idx].text); + let new_inserted = memo.candidates[idx].text.clone(); let (start, end) = memo.inserted_range; self.input.replace_range(start..end, &new_inserted); let new_end = start + new_inserted.len(); @@ -1776,25 +1807,30 @@ mod tests { // ---- ADR-0022 stage 8: Tab completion + Esc/Backspace undo ---- #[test] - fn tab_inserts_unique_keyword_with_trailing_space() { + fn tab_with_unique_candidate_inserts_with_space_and_no_memo() { + // Single-candidate path: insert " ", no memo. + // Stage-8 follow-up #2 (testing-round-2): no memo + // for unique completions so subsequent Tab fresh- + // computes at the new cursor. let mut app = App::new(); type_str(&mut app, "cre"); let actions = app.update(key(KeyCode::Tab)); assert!(actions.is_empty()); assert_eq!(app.input, "create "); assert_eq!(app.input_cursor, 7); - // Memo is alive even for a single-candidate completion - // so Esc/Backspace can still undo. - assert!(app.last_completion.is_some()); + assert!(app.last_completion.is_none()); } #[test] fn tab_at_word_boundary_inserts_next_expected_keyword() { + // `create ` → expects only `table`. Single candidate; + // insert "table " with space, no memo. let mut app = App::new(); type_str(&mut app, "create "); let actions = app.update(key(KeyCode::Tab)); assert!(actions.is_empty()); assert_eq!(app.input, "create table "); + assert!(app.last_completion.is_none()); } #[test] @@ -1810,70 +1846,89 @@ mod tests { } #[test] - fn tab_cycles_forward_through_multi_candidate_set() { - // After `show ` two candidates: data, table. + fn tab_with_multi_candidates_inserts_without_space_and_creates_memo() { + // Multi-candidate path: insert WITHOUT trailing space + // so the user can press space to commit. Memo carries + // the candidate list for cycling. let mut app = App::new(); type_str(&mut app, "show "); app.update(key(KeyCode::Tab)); - assert_eq!(app.input, "show data "); + assert_eq!(app.input, "show data"); + assert!(app.last_completion.is_some()); + } + + #[test] + fn tab_cycles_forward_through_multi_candidate_set() { + let mut app = App::new(); + type_str(&mut app, "show "); app.update(key(KeyCode::Tab)); - assert_eq!(app.input, "show table "); - // Wrap-around per the user's #2. + assert_eq!(app.input, "show data"); app.update(key(KeyCode::Tab)); - assert_eq!(app.input, "show data "); + assert_eq!(app.input, "show table"); + // Wrap-around. + app.update(key(KeyCode::Tab)); + assert_eq!(app.input, "show data"); } #[test] fn shift_tab_cycles_backward_starting_from_last() { - // Fresh Shift-Tab on multi-candidate position starts - // at the last candidate alphabetically. let mut app = App::new(); type_str(&mut app, "show "); app.update(key(KeyCode::BackTab)); - assert_eq!(app.input, "show table "); + assert_eq!(app.input, "show table"); app.update(key(KeyCode::BackTab)); - assert_eq!(app.input, "show data "); - // Wrap. + assert_eq!(app.input, "show data"); app.update(key(KeyCode::BackTab)); - assert_eq!(app.input, "show table "); + assert_eq!(app.input, "show table"); } #[test] - fn esc_after_tab_restores_original_in_one_keystroke() { + fn space_after_multi_candidate_tab_commits_the_choice() { + // The natural commit gesture for multi-candidate Tab: + // press space. Memo clears (any non-Tab key clears), + // space is inserted normally → "show data " ready + // for the next position. let mut app = App::new(); - type_str(&mut app, "cre"); - app.update(key(KeyCode::Tab)); - assert_eq!(app.input, "create "); - app.update(key(KeyCode::Esc)); - assert_eq!(app.input, "cre"); - assert_eq!(app.input_cursor, 3); + type_str(&mut app, "show "); + app.update(key(KeyCode::Tab)); // → "show data" (no space) + type_str(&mut app, " "); + assert_eq!(app.input, "show data "); assert!(app.last_completion.is_none()); } #[test] - fn backspace_after_tab_restores_original_in_one_keystroke() { - // Symmetry: one keystroke to insert, one to remove. + fn esc_after_multi_tab_restores_original_in_one_keystroke() { + // Multi-candidate Tab leaves a memo; Esc undoes the + // whole insertion regardless of cycle depth. let mut app = App::new(); - type_str(&mut app, "cre"); + type_str(&mut app, "show "); + app.update(key(KeyCode::Tab)); // → "show data" + app.update(key(KeyCode::Esc)); + assert_eq!(app.input, "show "); + assert!(app.last_completion.is_none()); + } + + #[test] + fn backspace_after_multi_tab_restores_original_in_one_keystroke() { + let mut app = App::new(); + type_str(&mut app, "show "); app.update(key(KeyCode::Tab)); - assert_eq!(app.input, "create "); app.update(key(KeyCode::Backspace)); - assert_eq!(app.input, "cre"); - assert_eq!(app.input_cursor, 3); + assert_eq!(app.input, "show "); assert!(app.last_completion.is_none()); } #[test] fn esc_after_multiple_tabs_restores_original_state_not_previous_cycle() { // Tab Tab Tab cycled through three candidates; Esc - // restores the pre-completion state (the original - // partial prefix), not the previous cycle. + // restores the pre-completion state (no insertion at + // all), not the previous cycle. let mut app = App::new(); type_str(&mut app, "drop "); app.update(key(KeyCode::Tab)); // column app.update(key(KeyCode::Tab)); // relationship app.update(key(KeyCode::Tab)); // table - assert_eq!(app.input, "drop table "); + assert_eq!(app.input, "drop table"); app.update(key(KeyCode::Esc)); assert_eq!(app.input, "drop "); } @@ -1881,31 +1936,46 @@ mod tests { #[test] fn typing_a_letter_clears_the_completion_memo() { let mut app = App::new(); - type_str(&mut app, "cre"); + type_str(&mut app, "show "); app.update(key(KeyCode::Tab)); assert!(app.last_completion.is_some()); // Typing any non-Tab key clears the memo. The // inserted text stays in the buffer. type_str(&mut app, "x"); - assert_eq!(app.input, "create x"); + assert_eq!(app.input, "show datax"); assert!(app.last_completion.is_none()); // Backspace now does its normal job — delete one char. app.update(key(KeyCode::Backspace)); - assert_eq!(app.input, "create "); + assert_eq!(app.input, "show data"); } #[test] fn cursor_movement_clears_the_completion_memo() { let mut app = App::new(); - type_str(&mut app, "cre"); + type_str(&mut app, "show "); app.update(key(KeyCode::Tab)); - // Per the user's #3 — cursor movement clears the - // memo. After this, Esc / Backspace behave normally - // and don't trigger the whole-span undo. + // Cursor movement clears the memo. After this, + // Esc / Backspace behave normally — no whole-span + // undo. app.update(key(KeyCode::Home)); assert!(app.last_completion.is_none()); } + #[test] + fn unique_tab_then_another_unique_tab_chains_naturally() { + // Stage-8 follow-up #2 (testing-round-2): the + // single-candidate-no-memo design lets the user chain + // Tabs through unique completions without getting + // stuck. From "a", Tab → "add ", Tab → "add column ". + let mut app = App::new(); + type_str(&mut app, "a"); + app.update(key(KeyCode::Tab)); + assert_eq!(app.input, "add "); + app.update(key(KeyCode::Tab)); + assert_eq!(app.input, "add column "); + assert!(app.last_completion.is_none()); + } + fn sample_description(name: &str) -> TableDescription { TableDescription { name: name.to_string(), diff --git a/src/completion.rs b/src/completion.rs index bdb1de5..20f1524 100644 --- a/src/completion.rs +++ b/src/completion.rs @@ -126,15 +126,18 @@ pub fn candidates_at_cursor( let matches_prefix = |s: &str| s.to_lowercase().starts_with(&lowered_prefix); // Source 1: keyword candidates from the parser's - // expected-set. + // expected-set. Preserve `expected`'s order — it reflects + // chumsky's source-order traversal of `or_not` / `choice` + // chains, which matches the canonical command shape (e.g. + // `to` before `table` for `add column [to] [table] …`). let mut keywords: Vec = expected .iter() .filter_map(|item| strip_backticks(item)) .filter_map(|name| Keyword::from_word(name).map(|_| name.to_string())) .filter(|name| matches_prefix(name)) .collect(); - keywords.sort(); - keywords.dedup(); + let mut seen_kw = std::collections::HashSet::new(); + keywords.retain(|k| seen_kw.insert(k.clone())); // Source 2: schema identifiers — accumulated across every // matching known-set slot. `NewName` slots return `&[]`. @@ -444,24 +447,25 @@ mod tests { } #[test] - fn keywords_come_before_identifiers_in_candidate_order() { - // "add column " has both keyword candidates (`to`, - // `table`) and schema-identifier candidates. Per the - // user feedback after stage 8: keywords first - // (alphabetical within), identifiers second - // (alphabetical within). + fn keywords_come_before_identifiers_in_grammar_order() { + // "add column " has both keyword candidates and + // schema-identifier candidates. Per the user's stage-8 + // feedback round 2: keywords first in *grammar order* + // (so `to` before `table` because the canonical shape + // is `add column [to] [table] :…`), identifiers + // after, alphabetised. The grammar order falls out of + // chumsky's source-order expected-set traversal — we + // preserve that order through `describe_expected`. let cache = SchemaCache { tables: vec!["Customers".to_string(), "Orders".to_string()], ..SchemaCache::default() }; let kinds = cand_kinds_with("add column ", 11, &cache); - // Expect: ["table", "to"] keywords, then ["Customers", - // "Orders"] identifiers. assert_eq!( kinds, vec![ - ("table".to_string(), CandidateKind::Keyword), ("to".to_string(), CandidateKind::Keyword), + ("table".to_string(), CandidateKind::Keyword), ("Customers".to_string(), CandidateKind::Identifier), ("Orders".to_string(), CandidateKind::Identifier), ], diff --git a/src/dsl/parser.rs b/src/dsl/parser.rs index 33dee52..384fb0b 100644 --- a/src/dsl/parser.rs +++ b/src/dsl/parser.rs @@ -765,8 +765,14 @@ fn describe_expected(expected: &[RichPattern<'_, Token>]) -> Vec { }) .map(describe_pattern) .collect(); - items.sort(); - items.dedup(); + // Dedup preserving first occurrence (which reflects + // chumsky's traversal order — typically source order for + // `or_not` / `choice` chains). Empirically this gives a + // grammar-natural ordering: `to` before `table` in + // `add column [to] [table] …`, which alphabetical + // (table, to) would invert. + let mut seen = std::collections::HashSet::new(); + items.retain(|s| seen.insert(s.clone())); items }