From 78a5cf0257c58e029a2986939d2d4c408c470b7b Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Mon, 10 Feb 2025 13:01:42 -0700 Subject: [PATCH] Fix display of bindings for `editor::AcceptInlineCompletion` + add validation + use modifiers from keymap (#24442) Release Notes: - N/A --- Cargo.lock | 2 + Cargo.toml | 1 + crates/editor/Cargo.toml | 1 + crates/editor/src/editor.rs | 36 +++++- crates/editor/src/element.rs | 198 ++++++++++++----------------- crates/gpui/Cargo.toml | 2 +- crates/settings/Cargo.toml | 1 + crates/settings/src/keymap_file.rs | 58 +++++++-- crates/settings/src/settings.rs | 4 +- 9 files changed, 166 insertions(+), 137 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4c9bf2c525..c114e0ee52 100644 --- 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", diff --git a/Cargo.toml b/Cargo.toml index b46372c8ce..e0481d632a 100644 --- 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" } diff --git a/crates/editor/Cargo.toml b/crates/editor/Cargo.toml index d78dff8d2a..282c10eb60 100644 --- 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 diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 4105091395..bdf1fd9056 100644 --- 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, 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, ) { 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( diff --git a/crates/editor/src/element.rs b/crates/editor/src/element.rs index fe829e2ad6..784c1d5178 100644 --- 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.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; - - // 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, icon: Option, previewing: bool, - focus_handle: FocusHandle, + editor_focus_handle: FocusHandle, window: &Window, cx: &App, ) -> Option { - 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); + +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::() + } + + 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, diff --git a/crates/gpui/Cargo.toml b/crates/gpui/Cargo.toml index 05a5b28e76..5c692860c9 100644 --- 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" diff --git a/crates/settings/Cargo.toml b/crates/settings/Cargo.toml index bfac8236e5..a851b431f7 100644 --- 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 diff --git a/crates/settings/src/keymap_file.rs b/crates/settings/src/keymap_file.rs index fb967bde3f..f4de45270a 100644 --- 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); + +inventory::collect!(KeyBindingValidatorRegistration); + +pub(crate) static KEY_BINDING_VALIDATORS: LazyLock>> = + LazyLock::new(|| { + let mut validators = BTreeMap::new(); + for validator_registration in inventory::iter:: { + 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) } } diff --git a/crates/settings/src/settings.rs b/crates/settings/src/settings.rs index aa3176c69a..cfec7f2d16 100644 --- 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,