Fix display of bindings for editor::AcceptInlineCompletion + add validation + use modifiers from keymap (#24442)

Release Notes: - N/A
2025-02-10 13:01:42 -07:00 · 2025-02-10 13:01:42 -07:00 · 78a5cf0257
commit 78a5cf0257
parent 43afa68dab
9 changed files with 166 additions and 137 deletions
--- a/Cargo.lock
+++ b/Cargo.lock
@ -4060,6 +4060,7 @@ dependencies = [
 "http_client",
 "indoc",
 "inline_completion",
+ "inventory",
 "itertools 0.14.0",
 "language",
 "linkify",
@ -12032,6 +12033,7 @@ dependencies = [
 "futures 0.3.31",
 "gpui",
 "indoc",
+ "inventory",
 "log",
 "migrator",
 "paths",
--- a/Cargo.toml
+++ b/Cargo.toml
@ -423,6 +423,7 @@ ignore = "0.4.22"
 image = "0.25.1"
 indexmap = { version = "2.7.0", features = ["serde"] }
 indoc = "2"
+inventory = "0.3.19"
 itertools = "0.14.0"
 jsonwebtoken = "9.3"
 jupyter-protocol = { version = "0.6.0" }
--- a/crates/editor/Cargo.toml
+++ b/crates/editor/Cargo.toml
@ -49,6 +49,7 @@ gpui.workspace = true
 http_client.workspace = true
 indoc.workspace = true
 inline_completion.workspace = true
+inventory.workspace = true
 itertools.workspace = true
 language.workspace = true
 linkify.workspace = true
--- a/crates/editor/src/editor.rs
+++ b/crates/editor/src/editor.rs
@ -62,10 +62,10 @@ pub use editor_settings::{
    CurrentLineHighlight, EditorSettings, ScrollBeyondLastLine, SearchSettings, ShowScrollbar,
 };
 pub use editor_settings_controls::*;
+use element::{AcceptEditPredictionBinding, LineWithInvisibles, PositionMap};
 pub use element::{
    CursorLayout, EditorElement, HighlightedRange, HighlightedRangeLine, PointForPosition,
 };
-use element::{LineWithInvisibles, PositionMap};
 use futures::{future, FutureExt};
 use fuzzy::StringMatchCandidate;

@ -190,6 +190,9 @@ pub const CODE_ACTIONS_DEBOUNCE_TIMEOUT: Duration = Duration::from_millis(250);
 pub(crate) const FORMAT_TIMEOUT: Duration = Duration::from_secs(2);
 pub(crate) const SCROLL_CENTER_TOP_BOTTOM_DEBOUNCE_TIMEOUT: Duration = Duration::from_secs(1);

+pub(crate) const EDIT_PREDICTION_REQUIRES_MODIFIER_KEY_CONTEXT: &str =
+    "edit_prediction_requires_modifier";
+
 pub fn render_parsed_markdown(
    element_id: impl Into<ElementId>,
    parsed: &language::ParsedMarkdown,
@ -1528,7 +1531,7 @@ impl Editor {
            key_context.add("edit_prediction");

            if showing_completions || self.edit_prediction_requires_modifier(cx) {
-                key_context.add("edit_prediction_requires_modifier");
+                key_context.add(EDIT_PREDICTION_REQUIRES_MODIFIER_KEY_CONTEXT);
            }
        }

@ -5092,19 +5095,38 @@ impl Editor {
        has_completion && self.edit_prediction_requires_modifier(cx)
    }

-    fn update_inline_completion_preview(
+    fn handle_modifiers_changed(
        &mut self,
-        modifiers: &Modifiers,
+        modifiers: Modifiers,
+        position_map: &PositionMap,
        window: &mut Window,
        cx: &mut Context<Self>,
    ) {
        if !self.show_edit_predictions_in_menu(cx) {
+            let accept_binding =
+                AcceptEditPredictionBinding::resolve(self.focus_handle(cx), window);
+            if let Some(accept_keystroke) = accept_binding.keystroke() {
+                let was_previewing_inline_completion = self.previewing_inline_completion;
+                self.previewing_inline_completion = modifiers == accept_keystroke.modifiers
+                    && accept_keystroke.modifiers.modified();
+                if self.previewing_inline_completion != was_previewing_inline_completion {
+                    self.update_visible_inline_completion(window, cx);
+                }
+            }
+        }
+
+        let mouse_position = window.mouse_position();
+        if !position_map.text_hitbox.is_hovered(window) {
            return;
        }

-        self.previewing_inline_completion = modifiers.alt;
-        self.update_visible_inline_completion(window, cx);
-        cx.notify();
+        self.update_hovered_link(
+            position_map.point_for_position(mouse_position),
+            &position_map.snapshot,
+            modifiers,
+            window,
+            cx,
+        )
    }

    fn update_visible_inline_completion(
--- a/crates/editor/src/element.rs
+++ b/crates/editor/src/element.rs
@ -15,13 +15,14 @@ use crate::{
    items::BufferSearchHighlights,
    mouse_context_menu::{self, MenuPosition, MouseContextMenu},
    scroll::{axis_pair, scroll_amount::ScrollAmount, AxisPair},
-    BlockId, ChunkReplacement, CursorShape, CustomBlockId, DisplayPoint, DisplayRow,
-    DocumentHighlightRead, DocumentHighlightWrite, EditDisplayMode, Editor, EditorMode,
+    AcceptEditPrediction, BlockId, ChunkReplacement, CursorShape, CustomBlockId, DisplayPoint,
+    DisplayRow, DocumentHighlightRead, DocumentHighlightWrite, EditDisplayMode, Editor, EditorMode,
    EditorSettings, EditorSnapshot, EditorStyle, ExpandExcerpts, FocusedBlock, GoToHunk,
    GoToPrevHunk, GutterDimensions, HalfPageDown, HalfPageUp, HandleInput, HoveredCursor,
    InlineCompletion, JumpData, LineDown, LineUp, OpenExcerpts, PageDown, PageUp, Point,
    RevertSelectedHunks, RowExt, RowRangeExt, SelectPhase, Selection, SoftWrap,
-    StickyHeaderExcerpt, ToPoint, ToggleFold, CURSORS_VISIBLE_FOR, FILE_HEADER_HEIGHT,
+    StickyHeaderExcerpt, ToPoint, ToggleFold, CURSORS_VISIBLE_FOR,
+    EDIT_PREDICTION_REQUIRES_MODIFIER_KEY_CONTEXT, FILE_HEADER_HEIGHT,
    GIT_BLAME_MAX_AUTHOR_CHARS_DISPLAYED, MAX_LINE_LEN, MULTI_BUFFER_EXCERPT_HEADER_HEIGHT,
 };
 use client::ParticipantIndex;
@ -34,11 +35,11 @@ use gpui::{
    relative, size, svg, transparent_black, Action, AnyElement, App, AvailableSpace, Axis, Bounds,
    ClickEvent, ClipboardItem, ContentMask, Context, Corner, Corners, CursorStyle, DispatchPhase,
    Edges, Element, ElementInputHandler, Entity, FocusHandle, Focusable as _, FontId,
-    GlobalElementId, Hitbox, Hsla, InteractiveElement, IntoElement, Keystroke, Length,
-    ModifiersChangedEvent, MouseButton, MouseDownEvent, MouseMoveEvent, MouseUpEvent, PaintQuad,
-    ParentElement, Pixels, ScrollDelta, ScrollWheelEvent, ShapedLine, SharedString, Size,
-    StatefulInteractiveElement, Style, Styled, Subscription, TextRun, TextStyleRefinement,
-    WeakEntity, Window,
+    GlobalElementId, Hitbox, Hsla, InteractiveElement, IntoElement, KeyBindingContextPredicate,
+    Keystroke, Length, ModifiersChangedEvent, MouseButton, MouseDownEvent, MouseMoveEvent,
+    MouseUpEvent, PaintQuad, ParentElement, Pixels, ScrollDelta, ScrollWheelEvent, ShapedLine,
+    SharedString, Size, StatefulInteractiveElement, Style, Styled, Subscription, TextRun,
+    TextStyleRefinement, WeakEntity, Window,
 };
 use itertools::Itertools;
 use language::{
@ -54,7 +55,7 @@ use multi_buffer::{
    RowInfo, ToOffset,
 };
 use project::project_settings::{GitGutterSetting, ProjectSettings};
-use settings::Settings;
+use settings::{KeyBindingValidator, KeyBindingValidatorRegistration, Settings};
 use smallvec::{smallvec, SmallVec};
 use std::{
    any::TypeId,
@ -74,7 +75,7 @@ use ui::{
    POPOVER_Y_PADDING,
 };
 use unicode_segmentation::UnicodeSegmentation;
-use util::{RangeExt, ResultExt};
+use util::{markdown::MarkdownString, RangeExt, ResultExt};
 use workspace::{item::Item, notifications::NotifyTaskExt, Workspace};

 const INLINE_BLAME_PADDING_EM_WIDTHS: f32 = 7.;
@ -511,35 +512,12 @@ impl EditorElement {
                    if editor.hover_state.focused(window, cx) {
                        return;
                    }
-                    Self::modifiers_changed(editor, event, &position_map, window, cx)
+                    editor.handle_modifiers_changed(event.modifiers, &position_map, window, cx);
                })
            }
        });
    }

-    fn modifiers_changed(
-        editor: &mut Editor,
-        event: &ModifiersChangedEvent,
-        position_map: &PositionMap,
-        window: &mut Window,
-        cx: &mut Context<Editor>,
-    ) {
-        editor.update_inline_completion_preview(&event.modifiers, window, cx);
-
-        let mouse_position = window.mouse_position();
-        if !position_map.text_hitbox.is_hovered(window) {
-            return;
-        }
-
-        editor.update_hovered_link(
-            position_map.point_for_position(mouse_position),
-            &position_map.snapshot,
-            event.modifiers,
-            window,
-            cx,
-        )
-    }
-
    fn mouse_left_down(
        editor: &mut Editor,
        event: &MouseDownEvent,
@ -3190,49 +3168,8 @@ impl EditorElement {
                );

                let edit_prediction = if edit_prediction_popover_visible {
-                    let accept_keystroke: Option<Keystroke>;
-
-                    // TODO: load modifier from keymap.
-                    // `bindings_for_action_in` returns `None` in Linux, and is intermittent on macOS
-                    #[cfg(target_os = "macos")]
-                    {
-                        // let bindings = window.bindings_for_action_in(
-                        //     &crate::AcceptEditPrediction,
-                        //     &self.editor.focus_handle(cx),
-                        // );
-
-                        // let last_binding = bindings.last();
-
-                        // accept_keystroke = if let Some(binding) = last_binding {
-                        //     match &binding.keystrokes() {
-                        //         // TODO: no need to clone once this logic works on linux.
-                        //         [keystroke] => Some(keystroke.clone()),
-                        //         _ => None,
-                        //     }
-                        // } else {
-                        //     None
-                        // };
-                        accept_keystroke = Some(Keystroke {
-                            modifiers: gpui::Modifiers {
-                                alt: true,
-                                ..Default::default()
-                            },
-                            key: "tab".to_string(),
-                            key_char: None,
-                        });
-                    }
-
-                    #[cfg(not(target_os = "macos"))]
-                    {
-                        accept_keystroke = Some(Keystroke {
-                            modifiers: gpui::Modifiers {
-                                alt: true,
-                                ..Default::default()
-                            },
-                            key: "enter".to_string(),
-                            key_char: None,
-                        });
-                    }
+                    let accept_binding =
+                        AcceptEditPredictionBinding::resolve(self.editor.focus_handle(cx), window);

                    self.editor.update(cx, move |editor, cx| {
                        let mut element = editor.render_edit_prediction_cursor_popover(
@ -3240,7 +3177,7 @@ impl EditorElement {
                            max_width,
                            cursor_point,
                            style,
-                            accept_keystroke.as_ref()?,
+                            accept_binding.keystroke()?,
                            window,
                            cx,
                        )?;
@ -5739,48 +5676,12 @@ fn inline_completion_accept_indicator(
    label: impl Into<SharedString>,
    icon: Option<IconName>,
    previewing: bool,
-    focus_handle: FocusHandle,
+    editor_focus_handle: FocusHandle,
    window: &Window,
    cx: &App,
 ) -> Option<AnyElement> {
-    let use_hardcoded_linux_bindings;
-
-    #[cfg(target_os = "macos")]
-    {
-        use_hardcoded_linux_bindings = false;
-    }
-
-    #[cfg(not(target_os = "macos"))]
-    {
-        use_hardcoded_linux_bindings = true;
-    }
-
-    let accept_keystroke = if use_hardcoded_linux_bindings {
-        if previewing {
-            Keystroke {
-                modifiers: Default::default(),
-                key: "enter".to_string(),
-                key_char: None,
-            }
-        } else {
-            Keystroke {
-                modifiers: Default::default(),
-                key: "tab".to_string(),
-                key_char: None,
-            }
-        }
-    } else {
-        let bindings = window.bindings_for_action_in(&crate::AcceptEditPrediction, &focus_handle);
-        if let Some(keystroke) = bindings
-            .last()
-            .and_then(|binding| binding.keystrokes().first())
-        {
-            // TODO: clone unnecessary once `use_hardcoded_linux_bindings` is removed.
-            keystroke.clone()
-        } else {
-            return None;
-        }
-    };
+    let accept_binding = AcceptEditPredictionBinding::resolve(editor_focus_handle, window);
+    let accept_keystroke = accept_binding.keystroke()?;

    let accept_key = h_flex()
        .px_0p5()
@ -5828,6 +5729,69 @@ fn inline_completion_accept_indicator(
    )
 }

+pub struct AcceptEditPredictionBinding(Option<gpui::KeyBinding>);
+
+impl AcceptEditPredictionBinding {
+    pub fn resolve(editor_focus_handle: FocusHandle, window: &Window) -> Self {
+        AcceptEditPredictionBinding(
+            window
+                .bindings_for_action_in(&AcceptEditPrediction, &editor_focus_handle)
+                .into_iter()
+                .next(),
+        )
+    }
+
+    pub fn keystroke(&self) -> Option<&Keystroke> {
+        if let Some(binding) = self.0.as_ref() {
+            match &binding.keystrokes() {
+                [keystroke] => Some(keystroke),
+                _ => None,
+            }
+        } else {
+            None
+        }
+    }
+}
+
+struct AcceptEditPredictionsBindingValidator;
+
+inventory::submit! { KeyBindingValidatorRegistration(|| Box::new(AcceptEditPredictionsBindingValidator)) }
+
+impl KeyBindingValidator for AcceptEditPredictionsBindingValidator {
+    fn action_type_id(&self) -> TypeId {
+        TypeId::of::<AcceptEditPrediction>()
+    }
+
+    fn validate(&self, binding: &gpui::KeyBinding) -> Result<(), MarkdownString> {
+        use KeyBindingContextPredicate::*;
+
+        if binding.keystrokes().len() == 1 && binding.keystrokes()[0].modifiers.modified() {
+            return Ok(());
+        }
+        let required_predicate =
+            Not(Identifier(EDIT_PREDICTION_REQUIRES_MODIFIER_KEY_CONTEXT.into()).into());
+        match binding.predicate() {
+            Some(predicate) if required_predicate.is_superset(&predicate) => {
+                return Ok(());
+            }
+            _ => {}
+        }
+        Err(MarkdownString(format!(
+            "{} can only be bound to a single keystroke with modifiers, so \
+            that holding down these modifiers can be used to preview \
+            completions inline when the completions menu is open.\n\n\
+            This restriction does not apply when the context requires {}, \
+            since these bindings will not be used when the completions menu \
+            is open.",
+            MarkdownString::inline_code(AcceptEditPrediction.name()),
+            MarkdownString::inline_code(&format!(
+                "!{}",
+                EDIT_PREDICTION_REQUIRES_MODIFIER_KEY_CONTEXT
+            )),
+        )))
+    }
+}
+
 #[allow(clippy::too_many_arguments)]
 fn prepaint_gutter_button(
    button: IconButton,
--- a/crates/gpui/Cargo.toml
+++ b/crates/gpui/Cargo.toml
@ -79,7 +79,7 @@ futures.workspace = true
 gpui_macros.workspace = true
 http_client = { optional = true, workspace = true }
 image = "0.25.1"
-inventory = "0.3.19"
+inventory.workspace = true
 itertools.workspace = true
 log.workspace = true
 num_cpus = "1.13"
--- a/crates/settings/Cargo.toml
+++ b/crates/settings/Cargo.toml
@ -22,6 +22,7 @@ ec4rs.workspace = true
 fs.workspace = true
 futures.workspace = true
 gpui.workspace = true
+inventory.workspace = true
 log.workspace = true
 paths.workspace = true
 release_channel.workspace = true
--- a/crates/settings/src/keymap_file.rs
+++ b/crates/settings/src/keymap_file.rs
@ -1,5 +1,5 @@
 use anyhow::{anyhow, Context as _, Result};
-use collections::{HashMap, IndexMap};
+use collections::{BTreeMap, HashMap, IndexMap};
 use fs::Fs;
 use gpui::{
    Action, ActionBuildError, App, InvalidKeystrokeError, KeyBinding, KeyBindingContextPredicate,
@ -13,12 +13,30 @@ use schemars::{
 };
 use serde::{Deserialize, Serialize};
 use serde_json::Value;
-use std::rc::Rc;
-use std::{fmt::Write, sync::Arc};
+use std::{any::TypeId, fmt::Write, rc::Rc, sync::Arc, sync::LazyLock};
 use util::{asset_str, markdown::MarkdownString};

 use crate::{settings_store::parse_json_with_comments, SettingsAssets};

+pub trait KeyBindingValidator: Send + Sync {
+    fn action_type_id(&self) -> TypeId;
+    fn validate(&self, binding: &KeyBinding) -> Result<(), MarkdownString>;
+}
+
+pub struct KeyBindingValidatorRegistration(pub fn() -> Box<dyn KeyBindingValidator>);
+
+inventory::collect!(KeyBindingValidatorRegistration);
+
+pub(crate) static KEY_BINDING_VALIDATORS: LazyLock<BTreeMap<TypeId, Box<dyn KeyBindingValidator>>> =
+    LazyLock::new(|| {
+        let mut validators = BTreeMap::new();
+        for validator_registration in inventory::iter::<KeyBindingValidatorRegistration> {
+            let validator = validator_registration.0();
+            validators.insert(validator.action_type_id(), validator);
+        }
+        validators
+    });
+
 // Note that the doc comments on these are shown by json-language-server when editing the keymap, so
 // they should be considered user-facing documentation. Documentation is not handled well with
 // schemars-0.8 - when there are newlines, it is rendered as plaintext (see
@ -255,9 +273,16 @@ impl KeymapFile {
                            key_bindings.push(key_binding);
                        }
                        Err(err) => {
+                            let mut lines = err.lines();
+                            let mut indented_err = lines.next().unwrap().to_string();
+                            for line in lines {
+                                indented_err.push_str("  ");
+                                indented_err.push_str(line);
+                                indented_err.push_str("\n");
+                            }
                            write!(
                                section_errors,
-                                "\n\n - In binding {}, {err}",
+                                "\n\n- In binding {}, {indented_err}",
                                inline_code_string(keystrokes),
                            )
                            .unwrap();
@ -367,13 +392,24 @@ impl KeymapFile {
            },
        };

-        match KeyBinding::load(keystrokes, action, context, key_equivalents) {
-            Ok(binding) => Ok(binding),
-            Err(InvalidKeystrokeError { keystroke }) => Err(format!(
-                "invalid keystroke {}. {}",
-                inline_code_string(&keystroke),
-                KEYSTROKE_PARSE_EXPECTED_MESSAGE
-            )),
+        let key_binding = match KeyBinding::load(keystrokes, action, context, key_equivalents) {
+            Ok(key_binding) => key_binding,
+            Err(InvalidKeystrokeError { keystroke }) => {
+                return Err(format!(
+                    "invalid keystroke {}. {}",
+                    inline_code_string(&keystroke),
+                    KEYSTROKE_PARSE_EXPECTED_MESSAGE
+                ))
+            }
+        };
+
+        if let Some(validator) = KEY_BINDING_VALIDATORS.get(&key_binding.action().type_id()) {
+            match validator.validate(&key_binding) {
+                Ok(()) => Ok(key_binding),
+                Err(error) => Err(error.0),
+            }
+        } else {
+            Ok(key_binding)
        }
    }

--- a/crates/settings/src/settings.rs
+++ b/crates/settings/src/settings.rs
@ -13,7 +13,9 @@ use util::asset_str;
 pub use editable_setting_control::*;
 pub use json_schema::*;
 pub use key_equivalents::*;
-pub use keymap_file::{KeymapFile, KeymapFileLoadResult};
+pub use keymap_file::{
+    KeyBindingValidator, KeyBindingValidatorRegistration, KeymapFile, KeymapFileLoadResult,
+};
 pub use settings_file::*;
 pub use settings_store::{
    parse_json_with_comments, InvalidSettingsError, LocalSettingsKind, Settings, SettingsLocation,