Skip to content

Commit 71bcb6f

Browse files
fix(engine): decide menu completions once the completer answers (#1144)
* fix(engine): decide menu completions once the completer answers Since #1093 a completer may answer with `Pending` and compute in the background. Activating a menu inspects its values immediately, so the quick completion and the partial completion both read an empty menu and neither fires, leaving a menu open where a lone suggestion used to be accepted outright (#1142). Both decisions are now replayed when the results land, guarded by a snapshot of the line taken as the menu opened, since a completion applied after the user has typed on is worse than one that never fires. * fix(engine): stop a stale suggestion from swallowing the completion Since #1093 a completer may answer with `Stale`: real suggestions, but computed against a line the user has moved off. A lone one is indistinguishable from a lone fresh match where the menu decides whether to accept, so Tab closed the menu having changed nothing. It showed up only on the first completion of a pattern, while the cache still held a neighbouring entry to answer with. * fix(engine): keep an opening menu off screen until the completer answers A grace period was tried first and does not work: it only suppresses the frame when the completer beats the deadline, and just past it the frame is shorter still, which reads worse rather than better. Waiting on the answer holds at any latency instead. The cost is that a slow completer now shows nothing until it answers. That is what Tab did before #1093 made completers asynchronous, so it is a restoration rather than a regression, but it does mean nushell#18799 is felt as a dead terminal rather than a busy one. * refactor(completion): let CompletionOrigin answer whether the line moved on "Is this stamp still the live line" was spelled three ways, one of which built a throwaway `CompletionOrigin` just to compare against it, cloning the buffer to do so. Add a field to the stamp and one of the three keeps comparing the old notion of sameness. Takes `&str` and `usize` rather than `&Editor`, so `completion` does not gain a dependency on `core_editor` for a two-field test. * refactor(engine): decide menu completions in one place Opening a menu and replaying a deferred answer have to reach the same verdict, since the replay exists precisely to stand in for the decision the completer was too slow to allow. They were two branches 250 lines apart, in different shapes, held together by a comment asking the reader to check they still matched. `values_updated` is what differs: the activation path may still need to fetch, the replay has just refreshed. Both other differences were only apparent. The early `Enter` return made the activation path's partial step unreachable for a lone value, which is what `!accept_lone_value` now says outright, and the replay's provisional check was already guaranteed by its caller. * fix(engine): give an opening menu its indicator on the frame it appears The paint loop asked whether the menu was visible before `update_working_details`, which is where a first answer lands and ends the opening phase, while the painter picked the menu to draw after it. A menu whose answer arrived during that update therefore had its rows drawn under the ordinary prompt indicator. The two questions cannot simply be reordered: the indicator sets the prompt width, the width positions the cursor, and the update consumes that position. Asking twice keeps the indicator and the drawn rows agreeing, at the cost of the ide menu being positioned against the pre-menu width on the one frame a menu opens. Visibility becomes `Menu::is_visible`, since it is now read from two places and both must mean the same thing. * fix(menu): forget the previous line's answer when a menu re-opens `on_activate` cleared the suggestions but not the flags describing how final they were, so a re-opened menu started out claiming whatever the last line's answer had been. The engine reads that to decide whether a completion is still owed: with quick and partial completions both off nothing queries the completer on activation, so a leftover `true` armed a replay for an answer that was never requested. * test(engine): fold the two deferred completer fixtures into one `DeferredCompleter` and `StaleThenFreshCompleter` differed only in the answer they gave first, and `force_terminal_size_for_test` was a second test-only door into the painter that `handle_resize` already opens, once the size is set before the anchor it invalidates. * fix(engine): don't let MenuNext close the menu over a lone stale value The activation route gained this guard when a stale suggestion stopped swallowing the completion; `MenuNext` reaches the same lone-value accept with the menu already open and kept firing the `Enter`, whose `Deactivate` left the deferred completion nothing to land in. * fix(engine): re-verify the prompt anchor when a completion settles The dispatch sites mark the anchor stale at the keystroke, but the request runs on after that repaint re-verified: host code that scrolls (nushell's fzf-style completer) can do so between the dispatch and the settle, thus the settle repaint was the one completer path still trusting the cached row. The settle-time `update_values` can also dispatch a fresh request for a line that moved on, which the keystroke sites never see. * refactor(menu): fold the completion flags into one phase value `awaiting_results`, `provisional_results` and `opening` were three facts about one thing, duplicated across both menus, with their invariants living in the update order: pending implies provisional, and only a final answer may end the opening phase, which the `&=` latch encoded rather than stated. `CompletionPhase` holds the same information as one value, the three trait answers derive from it, and a re-activation is a single reset, which is the bug class the previous fix here was.
1 parent 9c43cfb commit 71bcb6f

6 files changed

Lines changed: 741 additions & 58 deletions

File tree

src/completion/base.rs

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,14 @@ impl CompletionOrigin {
4949
insertion_point,
5050
}
5151
}
52+
53+
/// Whether `buffer` and `insertion_point` are still exactly what this was
54+
/// stamped for. Anything else means the line moved on and results computed
55+
/// against this origin no longer describe it.
56+
pub fn matches(&self, buffer: &str, insertion_point: usize) -> bool {
57+
// Cursor first: it rules out most drift without comparing the line.
58+
self.insertion_point == insertion_point && self.buffer == buffer
59+
}
5260
}
5361

5462
/// Longest common prefix extension computed by the completer.
@@ -152,6 +160,12 @@ impl CompletionResult {
152160
pub fn is_pending(&self) -> bool {
153161
matches!(self, CompletionResult::Pending)
154162
}
163+
164+
/// Whether a later result may still supersede this one: either nothing has
165+
/// arrived yet, or what arrived was computed against a different line.
166+
pub fn is_provisional(&self) -> bool {
167+
!matches!(self, CompletionResult::Fresh { .. })
168+
}
155169
}
156170

157171
/// Vitality of a completer's background work, grabbed by the engine once per

0 commit comments

Comments
 (0)