From d7f0241d7b3745f32ca3d5e0c10a2ff70bf031a2 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Fri, 30 May 2025 01:53:02 -0600 Subject: [PATCH] editor: Defer the effects of `change_selections` to end of `transact` (#31731) In quite a few places the selection is changed multiple times in a transaction. For example, `backspace` might do it 3 times: * `select_autoclose_pair` * selection of the ranges to delete * `insert` of empty string also updates selection Before this change, each of these selection changes appended to selection history and did a bunch of work that's only relevant to selections the user actually sees. So for each backspace, `editor::UndoSelection` would need to be invoked 3-4 times before the cursor actually moves. It still needs to be run twice after this change, but that is a separate issue. Signature help even had a `backspace_pressed: bool` as an incomplete workaround, to avoid it flickering due to the selection switching between being a range and being cursor-like. The original motivation for this change is work I'm doing on not re-querying completions when the language server provides a response that has `is_incomplete: false`. Whether the menu is still visible is determined by the cursor position, and this was complicated by it seeing `backspace` temporarily moving the head of the selection 1 character to the left. This change also removes some redundant uses of `push_to_selection_history`. Not super stoked with the name `DeferredSelectionEffectsState`. Naming is hard. Release Notes: - N/A --- crates/editor/src/editor.rs | 139 ++++++++++++++++++------ crates/editor/src/jsx_tag_auto_close.rs | 2 +- crates/editor/src/signature_help.rs | 15 +-- 3 files changed, 108 insertions(+), 48 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index ccf18ee195151aa1e91970f4d33fb6999599d0ea..82769927340200f941d217cd3c9e33c328b38a3a 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -936,6 +936,8 @@ pub struct Editor { select_next_state: Option, select_prev_state: Option, selection_history: SelectionHistory, + defer_selection_effects: bool, + deferred_selection_effects_state: Option, autoclose_regions: Vec, snippet_stack: InvalidationStack, select_syntax_node_history: SelectSyntaxNodeHistory, @@ -1195,6 +1197,14 @@ impl Default for SelectionHistoryMode { } } +struct DeferredSelectionEffectsState { + changed: bool, + show_completions: bool, + autoscroll: Option, + old_cursor_position: Anchor, + history_entry: SelectionHistoryEntry, +} + #[derive(Default)] struct SelectionHistory { #[allow(clippy::type_complexity)] @@ -1791,6 +1801,8 @@ impl Editor { select_next_state: None, select_prev_state: None, selection_history: SelectionHistory::default(), + defer_selection_effects: false, + deferred_selection_effects_state: None, autoclose_regions: Vec::new(), snippet_stack: InvalidationStack::default(), select_syntax_node_history: SelectSyntaxNodeHistory::default(), @@ -2954,6 +2966,9 @@ impl Editor { Subscription::join(other_subscription, this_subscription) } + /// Changes selections using the provided mutation function. Changes to `self.selections` occur + /// immediately, but when run within `transact` or `with_selection_effects_deferred` other + /// effects of selection change occur at the end of the transaction. pub fn change_selections( &mut self, autoscroll: Option, @@ -2961,39 +2976,105 @@ impl Editor { cx: &mut Context, change: impl FnOnce(&mut MutableSelectionsCollection<'_>) -> R, ) -> R { - self.change_selections_inner(autoscroll, true, window, cx, change) + self.change_selections_inner(true, autoscroll, window, cx, change) } - fn change_selections_inner( + pub(crate) fn change_selections_without_showing_completions( &mut self, autoscroll: Option, - request_completions: bool, window: &mut Window, cx: &mut Context, change: impl FnOnce(&mut MutableSelectionsCollection<'_>) -> R, ) -> R { - let old_cursor_position = self.selections.newest_anchor().head(); - self.push_to_selection_history(); + self.change_selections_inner(false, autoscroll, window, cx, change) + } + fn change_selections_inner( + &mut self, + show_completions: bool, + autoscroll: Option, + window: &mut Window, + cx: &mut Context, + change: impl FnOnce(&mut MutableSelectionsCollection<'_>) -> R, + ) -> R { + if let Some(state) = &mut self.deferred_selection_effects_state { + state.autoscroll = autoscroll.or(state.autoscroll); + state.show_completions = show_completions; + let (changed, result) = self.selections.change_with(cx, change); + state.changed |= changed; + return result; + } + let mut state = DeferredSelectionEffectsState { + changed: false, + show_completions, + autoscroll, + old_cursor_position: self.selections.newest_anchor().head(), + history_entry: SelectionHistoryEntry { + selections: self.selections.disjoint_anchors(), + select_next_state: self.select_next_state.clone(), + select_prev_state: self.select_prev_state.clone(), + add_selections_state: self.add_selections_state.clone(), + }, + }; let (changed, result) = self.selections.change_with(cx, change); + state.changed = state.changed || changed; + if self.defer_selection_effects { + self.deferred_selection_effects_state = Some(state); + } else { + self.apply_selection_effects(state, window, cx); + } + result + } + + /// Defers the effects of selection change, so that the effects of multiple calls to + /// `change_selections` are applied at the end. This way these intermediate states aren't added + /// to selection history and the state of popovers based on selection position aren't + /// erroneously updated. + pub fn with_selection_effects_deferred( + &mut self, + window: &mut Window, + cx: &mut Context, + update: impl FnOnce(&mut Self, &mut Window, &mut Context) -> R, + ) -> R { + let already_deferred = self.defer_selection_effects; + self.defer_selection_effects = true; + let result = update(self, window, cx); + if !already_deferred { + self.defer_selection_effects = false; + if let Some(state) = self.deferred_selection_effects_state.take() { + self.apply_selection_effects(state, window, cx); + } + } + result + } - if changed { - if let Some(autoscroll) = autoscroll { + fn apply_selection_effects( + &mut self, + state: DeferredSelectionEffectsState, + window: &mut Window, + cx: &mut Context, + ) { + if state.changed { + self.selection_history.push(state.history_entry); + + if let Some(autoscroll) = state.autoscroll { self.request_autoscroll(autoscroll, cx); } - self.selections_did_change(true, &old_cursor_position, request_completions, window, cx); - if self.should_open_signature_help_automatically( + let old_cursor_position = &state.old_cursor_position; + + self.selections_did_change( + true, &old_cursor_position, - self.signature_help_state.backspace_pressed(), + state.show_completions, + window, cx, - ) { + ); + + if self.should_open_signature_help_automatically(&old_cursor_position, cx) { self.show_signature_help(&ShowSignatureHelp, window, cx); } - self.signature_help_state.set_backspace_pressed(false); } - - result } pub fn edit(&mut self, edits: I, cx: &mut Context) @@ -3877,9 +3958,12 @@ impl Editor { } let had_active_inline_completion = this.has_active_inline_completion(); - this.change_selections_inner(Some(Autoscroll::fit()), false, window, cx, |s| { - s.select(new_selections) - }); + this.change_selections_without_showing_completions( + Some(Autoscroll::fit()), + window, + cx, + |s| s.select(new_selections), + ); if !bracket_inserted { if let Some(on_type_format_task) = @@ -9033,7 +9117,6 @@ impl Editor { } } - this.signature_help_state.set_backspace_pressed(true); this.change_selections(Some(Autoscroll::fit()), window, cx, |s| { s.select(selections) }); @@ -12755,7 +12838,6 @@ impl Editor { ) -> Result<()> { self.hide_mouse_cursor(&HideMouseCursorOrigin::MovementAction); - self.push_to_selection_history(); let display_map = self.display_map.update(cx, |map, cx| map.snapshot(cx)); self.select_next_match_internal(&display_map, false, None, window, cx)?; @@ -12808,7 +12890,6 @@ impl Editor { cx: &mut Context, ) -> Result<()> { self.hide_mouse_cursor(&HideMouseCursorOrigin::MovementAction); - self.push_to_selection_history(); let display_map = self.display_map.update(cx, |map, cx| map.snapshot(cx)); self.select_next_match_internal( &display_map, @@ -12827,7 +12908,6 @@ impl Editor { cx: &mut Context, ) -> Result<()> { self.hide_mouse_cursor(&HideMouseCursorOrigin::MovementAction); - self.push_to_selection_history(); let display_map = self.display_map.update(cx, |map, cx| map.snapshot(cx)); let buffer = &display_map.buffer_snapshot; let mut selections = self.selections.all::(cx); @@ -15697,24 +15777,17 @@ impl Editor { self.selections_did_change(false, &old_cursor_position, true, window, cx); } - fn push_to_selection_history(&mut self) { - self.selection_history.push(SelectionHistoryEntry { - selections: self.selections.disjoint_anchors(), - select_next_state: self.select_next_state.clone(), - select_prev_state: self.select_prev_state.clone(), - add_selections_state: self.add_selections_state.clone(), - }); - } - pub fn transact( &mut self, window: &mut Window, cx: &mut Context, update: impl FnOnce(&mut Self, &mut Window, &mut Context), ) -> Option { - self.start_transaction_at(Instant::now(), window, cx); - update(self, window, cx); - self.end_transaction_at(Instant::now(), cx) + self.with_selection_effects_deferred(window, cx, |this, window, cx| { + this.start_transaction_at(Instant::now(), window, cx); + update(this, window, cx); + this.end_transaction_at(Instant::now(), cx) + }) } pub fn start_transaction_at( diff --git a/crates/editor/src/jsx_tag_auto_close.rs b/crates/editor/src/jsx_tag_auto_close.rs index 50e2ae5127dfd47f1997cab24700f3f1bf72776d..df50ab9b2f2b01ae8df9a036d139dce0155bdba1 100644 --- a/crates/editor/src/jsx_tag_auto_close.rs +++ b/crates/editor/src/jsx_tag_auto_close.rs @@ -600,7 +600,7 @@ pub(crate) fn handle_from( }) .collect::>(); this.update_in(cx, |this, window, cx| { - this.change_selections_inner(None, false, window, cx, |s| { + this.change_selections_without_showing_completions(None, window, cx, |s| { s.select(base_selections); }); }) diff --git a/crates/editor/src/signature_help.rs b/crates/editor/src/signature_help.rs index 8a2c33fb65c51fa79ed2350dccc4d2e10cb92cc6..9d69b10193cbac2f3b779704f5098ccd7cbdb527 100644 --- a/crates/editor/src/signature_help.rs +++ b/crates/editor/src/signature_help.rs @@ -74,8 +74,6 @@ impl Editor { pub(super) fn should_open_signature_help_automatically( &mut self, old_cursor_position: &Anchor, - backspace_pressed: bool, - cx: &mut Context, ) -> bool { if !(self.signature_help_state.is_shown() || self.auto_signature_help_enabled(cx)) { @@ -84,9 +82,7 @@ impl Editor { let newest_selection = self.selections.newest::(cx); let head = newest_selection.head(); - // There are two cases where the head and tail of a selection are different: selecting multiple ranges and using backspace. - // If we don’t exclude the backspace case, signature_help will blink every time backspace is pressed, so we need to prevent this. - if !newest_selection.is_empty() && !backspace_pressed && head != newest_selection.tail() { + if !newest_selection.is_empty() && head != newest_selection.tail() { self.signature_help_state .hide(SignatureHelpHiddenBy::Selection); return false; @@ -232,7 +228,6 @@ pub struct SignatureHelpState { task: Option>, popover: Option, hidden_by: Option, - backspace_pressed: bool, } impl SignatureHelpState { @@ -254,14 +249,6 @@ impl SignatureHelpState { self.popover.as_mut() } - pub fn backspace_pressed(&self) -> bool { - self.backspace_pressed - } - - pub fn set_backspace_pressed(&mut self, backspace_pressed: bool) { - self.backspace_pressed = backspace_pressed; - } - pub fn set_popover(&mut self, popover: SignatureHelpPopover) { self.popover = Some(popover); self.hidden_by = None;