Compare commits

...
Sign in to create a new pull request.

5 commits

Author SHA1 Message Date
Anthony
1624ae79ac Rename action_input to action_arguments in keybinding contexts 2025-07-15 13:34:27 -04:00
Anthony
907225f002 Merge remote-tracking branch 'origin/main' into keymap-ui-prevent-hard-reloading 2025-07-15 13:02:05 -04:00
Anthony
1a836521a8 Fallback to previous scrollbar position when prev binding is not found 2025-07-15 12:58:28 -04:00
Anthony
750582d2a2 Maintain keymap editor position when deleting or modifing a binding
When a key binding is deleted we keep the exact same scroll bar
position. When a keybinding is modified we select that keybinding in
it's new position and scroll to it.

I also changed save/modified keybinding to use fs.write istead of
fs.atomic_write. Atomic write was creating two FS events that some
scrollbar bugs when refreshing the keymap editor.
2025-07-15 12:53:31 -04:00
Anthony
0c14b615c1 Start work on preserving keymap editor scroll position through refreshes 2025-07-15 00:57:53 -04:00
3 changed files with 206 additions and 87 deletions

View file

@ -623,7 +623,7 @@ impl KeymapFile {
target_keybind_source,
} if target_keybind_source != KeybindSource::User => {
target.action_name = gpui::NoAction.name();
target.input.take();
target.action_arguments.take();
operation = KeybindUpdateOperation::Add(target);
}
_ => {}
@ -848,17 +848,17 @@ pub struct KeybindUpdateTarget<'a> {
pub keystrokes: &'a [Keystroke],
pub action_name: &'a str,
pub use_key_equivalents: bool,
pub input: Option<&'a str>,
pub action_arguments: Option<&'a str>,
}
impl<'a> KeybindUpdateTarget<'a> {
fn action_value(&self) -> Result<Value> {
let action_name: Value = self.action_name.into();
let value = match self.input {
Some(input) => {
let input = serde_json::from_str::<Value>(input)
.context("Failed to parse action input as JSON")?;
serde_json::json!([action_name, input])
let value = match self.action_arguments {
Some(args) => {
let args = serde_json::from_str::<Value>(args)
.context("Failed to parse action arguments as JSON")?;
serde_json::json!([action_name, args])
}
None => action_name,
};
@ -986,7 +986,7 @@ mod tests {
action_name: "zed::SomeAction",
context: None,
use_key_equivalents: false,
input: None,
action_arguments: None,
}),
r#"[
{
@ -1012,7 +1012,7 @@ mod tests {
action_name: "zed::SomeOtherAction",
context: None,
use_key_equivalents: false,
input: None,
action_arguments: None,
}),
r#"[
{
@ -1043,7 +1043,7 @@ mod tests {
action_name: "zed::SomeOtherAction",
context: None,
use_key_equivalents: false,
input: Some(r#"{"foo": "bar"}"#),
action_arguments: Some(r#"{"foo": "bar"}"#),
}),
r#"[
{
@ -1079,7 +1079,7 @@ mod tests {
action_name: "zed::SomeOtherAction",
context: Some("Zed > Editor && some_condition = true"),
use_key_equivalents: true,
input: Some(r#"{"foo": "bar"}"#),
action_arguments: Some(r#"{"foo": "bar"}"#),
}),
r#"[
{
@ -1118,14 +1118,14 @@ mod tests {
action_name: "zed::SomeAction",
context: None,
use_key_equivalents: false,
input: None,
action_arguments: None,
},
source: KeybindUpdateTarget {
keystrokes: &parse_keystrokes("ctrl-b"),
action_name: "zed::SomeOtherAction",
context: None,
use_key_equivalents: false,
input: Some(r#"{"foo": "bar"}"#),
action_arguments: Some(r#"{"foo": "bar"}"#),
},
target_keybind_source: KeybindSource::Base,
},
@ -1164,14 +1164,14 @@ mod tests {
action_name: "zed::SomeAction",
context: None,
use_key_equivalents: false,
input: None,
action_arguments: None,
},
source: KeybindUpdateTarget {
keystrokes: &parse_keystrokes("ctrl-b"),
action_name: "zed::SomeOtherAction",
context: None,
use_key_equivalents: false,
input: Some(r#"{"foo": "bar"}"#),
action_arguments: Some(r#"{"foo": "bar"}"#),
},
target_keybind_source: KeybindSource::User,
},
@ -1205,14 +1205,14 @@ mod tests {
action_name: "zed::SomeNonexistentAction",
context: None,
use_key_equivalents: false,
input: None,
action_arguments: None,
},
source: KeybindUpdateTarget {
keystrokes: &parse_keystrokes("ctrl-b"),
action_name: "zed::SomeOtherAction",
context: None,
use_key_equivalents: false,
input: None,
action_arguments: None,
},
target_keybind_source: KeybindSource::User,
},
@ -1248,14 +1248,14 @@ mod tests {
action_name: "zed::SomeAction",
context: None,
use_key_equivalents: false,
input: None,
action_arguments: None,
},
source: KeybindUpdateTarget {
keystrokes: &parse_keystrokes("ctrl-b"),
action_name: "zed::SomeOtherAction",
context: None,
use_key_equivalents: false,
input: Some(r#"{"foo": "bar"}"#),
action_arguments: Some(r#"{"foo": "bar"}"#),
},
target_keybind_source: KeybindSource::User,
},
@ -1293,14 +1293,14 @@ mod tests {
action_name: "foo::bar",
context: Some("SomeContext"),
use_key_equivalents: false,
input: None,
action_arguments: None,
},
source: KeybindUpdateTarget {
keystrokes: &parse_keystrokes("c"),
action_name: "foo::baz",
context: Some("SomeOtherContext"),
use_key_equivalents: false,
input: None,
action_arguments: None,
},
target_keybind_source: KeybindSource::User,
},
@ -1337,14 +1337,14 @@ mod tests {
action_name: "foo::bar",
context: Some("SomeContext"),
use_key_equivalents: false,
input: None,
action_arguments: None,
},
source: KeybindUpdateTarget {
keystrokes: &parse_keystrokes("c"),
action_name: "foo::baz",
context: Some("SomeOtherContext"),
use_key_equivalents: false,
input: None,
action_arguments: None,
},
target_keybind_source: KeybindSource::User,
},
@ -1376,7 +1376,7 @@ mod tests {
keystrokes: &parse_keystrokes("a"),
action_name: "foo::bar",
use_key_equivalents: false,
input: None,
action_arguments: None,
},
target_keybind_source: KeybindSource::User,
},
@ -1408,7 +1408,7 @@ mod tests {
keystrokes: &parse_keystrokes("a"),
action_name: "foo::bar",
use_key_equivalents: false,
input: Some("true"),
action_arguments: Some("true"),
},
target_keybind_source: KeybindSource::User,
},
@ -1451,7 +1451,7 @@ mod tests {
keystrokes: &parse_keystrokes("a"),
action_name: "foo::bar",
use_key_equivalents: false,
input: Some("true"),
action_arguments: Some("true"),
},
target_keybind_source: KeybindSource::User,
},

View file

@ -10,9 +10,9 @@ use feature_flags::FeatureFlagViewExt;
use fs::Fs;
use fuzzy::{StringMatch, StringMatchCandidate};
use gpui::{
Action, Animation, AnimationExt, AppContext as _, AsyncApp, ClickEvent, Context, DismissEvent,
Entity, EventEmitter, FocusHandle, Focusable, FontWeight, Global, IsZero, KeyContext,
Keystroke, Modifiers, ModifiersChangedEvent, MouseButton, Point, ScrollStrategy,
Action, Animation, AnimationExt, AppContext as _, AsyncApp, Axis, ClickEvent, Context,
DismissEvent, Entity, EventEmitter, FocusHandle, Focusable, FontWeight, Global, IsZero,
KeyContext, Keystroke, Modifiers, ModifiersChangedEvent, MouseButton, Point, ScrollStrategy,
ScrollWheelEvent, StyledText, Subscription, WeakEntity, actions, anchored, deferred, div,
};
use language::{Language, LanguageConfig, ToOffset as _};
@ -282,6 +282,25 @@ struct KeymapEditor {
keystroke_editor: Entity<KeystrokeInput>,
selected_index: Option<usize>,
context_menu: Option<(Entity<ContextMenu>, Point<Pixels>, Subscription)>,
previous_edit: Option<PreviousEdit>,
}
enum PreviousEdit {
/// When deleting, we want to maintain the same scroll position
ScrollBarOffset(Point<Pixels>),
/// When editing or creating, because the new keybinding could be in a different position in the sort order
/// we store metadata about the new binding (either the modified version or newly created one)
/// and upon reload, we search for this binding in the list of keybindings, and if we find the one that matches
/// this metadata, we set the selected index to it and scroll to it,
/// and if we don't find it, we scroll to 0 and don't set a selected index
Keybinding {
action_mapping: ActionMapping,
action_name: SharedString,
/// The scrollbar position to fallback to if we don't find the keybinding during a refresh
/// this can happen if there's a filter applied to the search and the keybinding modification
/// filters the binding from the search results
fallback: Point<Pixels>,
},
}
impl EventEmitter<()> for KeymapEditor {}
@ -294,8 +313,7 @@ impl Focusable for KeymapEditor {
impl KeymapEditor {
fn new(workspace: WeakEntity<Workspace>, window: &mut Window, cx: &mut Context<Self>) -> Self {
let _keymap_subscription =
cx.observe_global::<KeymapEventChannel>(Self::update_keybindings);
let _keymap_subscription = cx.observe_global::<KeymapEventChannel>(Self::on_keymap_changed);
let table_interaction_state = TableInteractionState::new(window, cx);
let keystroke_editor = cx.new(|cx| {
@ -315,7 +333,7 @@ impl KeymapEditor {
return;
}
this.update_matches(cx);
this.on_query_changed(cx);
})
.detach();
@ -324,7 +342,7 @@ impl KeymapEditor {
return;
}
this.update_matches(cx);
this.on_query_changed(cx);
})
.detach();
@ -343,9 +361,10 @@ impl KeymapEditor {
keystroke_editor,
selected_index: None,
context_menu: None,
previous_edit: None,
};
this.update_keybindings(cx);
this.on_keymap_changed(cx);
this
}
@ -367,17 +386,20 @@ impl KeymapEditor {
}
}
fn update_matches(&self, cx: &mut Context<Self>) {
fn on_query_changed(&self, cx: &mut Context<Self>) {
let action_query = self.current_action_query(cx);
let keystroke_query = self.current_keystroke_query(cx);
cx.spawn(async move |this, cx| {
Self::process_query(this, action_query, keystroke_query, cx).await
Self::update_matches(this.clone(), action_query, keystroke_query, cx).await?;
this.update(cx, |this, cx| {
this.scroll_to_item(0, ScrollStrategy::Top, cx)
})
})
.detach();
}
async fn process_query(
async fn update_matches(
this: WeakEntity<Self>,
action_query: String,
keystroke_query: Vec<Keystroke>,
@ -445,7 +467,6 @@ impl KeymapEditor {
});
}
this.selected_index.take();
this.scroll_to_item(0, ScrollStrategy::Top, cx);
this.matches = matches;
cx.notify();
})
@ -499,9 +520,9 @@ impl KeymapEditor {
let action_name = key_binding.action().name();
unmapped_action_names.remove(&action_name);
let action_input = key_binding
let action_arguments = key_binding
.action_input()
.map(|input| SyntaxHighlightedText::new(input, json_language.clone()));
.map(|arguments| SyntaxHighlightedText::new(arguments, json_language.clone()));
let action_docs = action_documentation.get(action_name).copied();
let index = processed_bindings.len();
@ -510,7 +531,7 @@ impl KeymapEditor {
keystroke_text: keystroke_text.into(),
ui_key_binding,
action_name: action_name.into(),
action_input,
action_arguments,
action_docs,
action_schema: action_schema.get(action_name).cloned(),
context: Some(context),
@ -527,7 +548,7 @@ impl KeymapEditor {
keystroke_text: empty.clone(),
ui_key_binding: None,
action_name: action_name.into(),
action_input: None,
action_arguments: None,
action_docs: action_documentation.get(action_name).copied(),
action_schema: action_schema.get(action_name).cloned(),
context: None,
@ -539,7 +560,7 @@ impl KeymapEditor {
(processed_bindings, string_match_candidates)
}
fn update_keybindings(&mut self, cx: &mut Context<KeymapEditor>) {
fn on_keymap_changed(&mut self, cx: &mut Context<KeymapEditor>) {
let workspace = self.workspace.clone();
cx.spawn(async move |this, cx| {
let json_language = load_json_language(workspace.clone(), cx).await;
@ -574,7 +595,47 @@ impl KeymapEditor {
)
})?;
// calls cx.notify
Self::process_query(this, action_query, keystroke_query, cx).await
Self::update_matches(this.clone(), action_query, keystroke_query, cx).await?;
this.update(cx, |this, cx| {
if let Some(previous_edit) = this.previous_edit.take() {
match previous_edit {
// should remove scroll from process_query
PreviousEdit::ScrollBarOffset(offset) => {
this.table_interaction_state.update(cx, |table, _| {
table.set_scrollbar_offset(Axis::Vertical, offset)
})
// set selected index and scroll
}
PreviousEdit::Keybinding {
action_mapping,
action_name,
fallback,
} => {
let scroll_position =
this.matches.iter().enumerate().find_map(|(index, item)| {
let binding = &this.keybindings[item.candidate_id];
if binding.get_action_mapping() == action_mapping
&& binding.action_name == action_name
{
Some(index)
} else {
None
}
});
if let Some(scroll_position) = scroll_position {
this.scroll_to_item(scroll_position, ScrollStrategy::Top, cx);
this.selected_index = Some(scroll_position);
} else {
this.table_interaction_state.update(cx, |table, _| {
table.set_scrollbar_offset(Axis::Vertical, fallback)
});
}
cx.notify();
}
}
}
})
})
.detach_and_log_err(cx);
}
@ -806,6 +867,7 @@ impl KeymapEditor {
let Some(to_remove) = self.selected_binding().cloned() else {
return;
};
let Ok(fs) = self
.workspace
.read_with(cx, |workspace, _| workspace.app_state().fs.clone())
@ -813,6 +875,11 @@ impl KeymapEditor {
return;
};
let tab_size = cx.global::<settings::SettingsStore>().json_tab_size();
self.previous_edit = Some(PreviousEdit::ScrollBarOffset(
self.table_interaction_state
.read(cx)
.get_scrollbar_offset(Axis::Vertical),
));
cx.spawn(async move |_, _| remove_keybinding(to_remove, &fs, tab_size).await)
.detach_and_notify_err(window, cx);
}
@ -861,7 +928,7 @@ impl KeymapEditor {
fn set_filter_state(&mut self, filter_state: FilterState, cx: &mut Context<Self>) {
if self.filter_state != filter_state {
self.filter_state = filter_state;
self.update_matches(cx);
self.on_query_changed(cx);
}
}
@ -872,7 +939,7 @@ impl KeymapEditor {
cx: &mut Context<Self>,
) {
self.search_mode = self.search_mode.invert();
self.update_matches(cx);
self.on_query_changed(cx);
// Update the keystroke editor to turn the `search` bool on
self.keystroke_editor.update(cx, |keystroke_editor, cx| {
@ -894,7 +961,7 @@ struct ProcessedKeybinding {
keystroke_text: SharedString,
ui_key_binding: Option<ui::KeyBinding>,
action_name: SharedString,
action_input: Option<SyntaxHighlightedText>,
action_arguments: Option<SyntaxHighlightedText>,
action_docs: Option<&'static str>,
action_schema: Option<schemars::Schema>,
context: Option<KeybindContextString>,
@ -1177,8 +1244,8 @@ impl Render for KeymapEditor {
binding.keystroke_text.clone().into_any_element(),
IntoElement::into_any_element,
);
let action_input = match binding.action_input.clone() {
Some(input) => input.into_any_element(),
let action_arguments = match binding.action_arguments.clone() {
Some(arguments) => arguments.into_any_element(),
None => {
if binding.action_schema.is_some() {
muted_styled_text(NO_ACTION_ARGUMENTS_TEXT, cx)
@ -1212,7 +1279,14 @@ impl Render for KeymapEditor {
.map(|(_source, name)| name)
.unwrap_or_default()
.into_any_element();
Some([icon, action, action_input, keystrokes, context, source])
Some([
icon,
action,
action_arguments,
keystrokes,
context,
source,
])
})
.collect()
}),
@ -1379,7 +1453,7 @@ struct KeybindingEditorModal {
editing_keybind_idx: usize,
keybind_editor: Entity<KeystrokeInput>,
context_editor: Entity<SingleLineInput>,
input_editor: Option<Entity<Editor>>,
action_arguments_editor: Option<Entity<Editor>>,
fs: Arc<dyn Fs>,
error: Option<InputError>,
keymap_editor: Entity<KeymapEditor>,
@ -1444,16 +1518,16 @@ impl KeybindingEditorModal {
input
});
let input_editor = editing_keybind.action_schema.clone().map(|_schema| {
let action_arguments_editor = editing_keybind.action_schema.clone().map(|_schema| {
cx.new(|cx| {
let mut editor = Editor::auto_height_unbounded(1, window, cx);
let workspace = workspace.clone();
if let Some(input) = editing_keybind.action_input.clone() {
editor.set_text(input.text, window, cx);
if let Some(arguments) = editing_keybind.action_arguments.clone() {
editor.set_text(arguments.text, window, cx);
} else {
// TODO: default value from schema?
editor.set_placeholder_text("Action Input", cx);
editor.set_placeholder_text("Action Arguments", cx);
}
cx.spawn(async |editor, cx| {
let json_language = load_json_language(workspace, cx).await;
@ -1465,7 +1539,7 @@ impl KeybindingEditorModal {
});
}
})
.context("Failed to load JSON language for editing keybinding action input")
.context("Failed to load JSON language for editing keybinding action arguments input")
})
.detach_and_log_err(cx);
editor
@ -1479,7 +1553,7 @@ impl KeybindingEditorModal {
fs,
keybind_editor,
context_editor,
input_editor,
action_arguments_editor,
error: None,
keymap_editor,
workspace,
@ -1500,22 +1574,22 @@ impl KeybindingEditorModal {
}
}
fn validate_action_input(&self, cx: &App) -> anyhow::Result<Option<String>> {
let input = self
.input_editor
fn validate_action_arguments(&self, cx: &App) -> anyhow::Result<Option<String>> {
let action_arguments = self
.action_arguments_editor
.as_ref()
.map(|editor| editor.read(cx).text(cx));
let value = input
let value = action_arguments
.as_ref()
.map(|input| {
serde_json::from_str(input).context("Failed to parse action input as JSON")
.map(|args| {
serde_json::from_str(args).context("Failed to parse action arguments as JSON")
})
.transpose()?;
cx.build_action(&self.editing_keybind.action_name, value)
.context("Failed to validate action input")?;
Ok(input)
.context("Failed to validate action arguments")?;
Ok(action_arguments)
}
fn save(&mut self, cx: &mut Context<Self>) {
@ -1545,7 +1619,7 @@ impl KeybindingEditorModal {
return;
}
let new_input = match self.validate_action_input(cx) {
let new_action_args = match self.validate_action_arguments(cx) {
Err(input_err) => {
self.set_error(InputError::error(input_err.to_string()), cx);
return;
@ -1623,12 +1697,14 @@ impl KeybindingEditorModal {
.log_err();
cx.spawn(async move |this, cx| {
let action_name = existing_keybind.action_name.clone();
if let Err(err) = save_keybinding_update(
create,
existing_keybind,
&new_keystrokes,
new_context.as_deref(),
new_input.as_deref(),
new_action_args.as_deref(),
&fs,
tab_size,
)
@ -1639,7 +1715,22 @@ impl KeybindingEditorModal {
})
.log_err();
} else {
this.update(cx, |_this, cx| {
this.update(cx, |this, cx| {
let action_mapping = (
ui::text_for_keystrokes(new_keystrokes.as_slice(), cx).into(),
new_context.map(SharedString::from),
);
this.keymap_editor.update(cx, |keymap, cx| {
keymap.previous_edit = Some(PreviousEdit::Keybinding {
action_mapping,
action_name,
fallback: keymap
.table_interaction_state
.read(cx)
.get_scrollbar_offset(Axis::Vertical),
})
});
cx.emit(DismissEvent);
})
.ok();
@ -1683,7 +1774,7 @@ impl Render for KeybindingEditorModal {
.gap_1()
.child(self.keybind_editor.clone()),
)
.when_some(self.input_editor.clone(), |this, editor| {
.when_some(self.action_arguments_editor.clone(), |this, editor| {
this.child(
v_flex()
.mt_1p5()
@ -1865,7 +1956,7 @@ async fn save_keybinding_update(
existing: ProcessedKeybinding,
new_keystrokes: &[Keystroke],
new_context: Option<&str>,
new_input: Option<&str>,
new_args: Option<&str>,
fs: &Arc<dyn Fs>,
tab_size: usize,
) -> anyhow::Result<()> {
@ -1879,10 +1970,10 @@ async fn save_keybinding_update(
.context
.as_ref()
.and_then(KeybindContextString::local_str);
let existing_input = existing
.action_input
let existing_args = existing
.action_arguments
.as_ref()
.map(|input| input.text.as_ref());
.map(|args| args.text.as_ref());
settings::KeybindUpdateOperation::Replace {
target: settings::KeybindUpdateTarget {
@ -1890,7 +1981,7 @@ async fn save_keybinding_update(
keystrokes: existing_keystrokes,
action_name: &existing.action_name,
use_key_equivalents: false,
input: existing_input,
action_arguments: existing_args,
},
target_keybind_source: existing
.source
@ -1902,7 +1993,7 @@ async fn save_keybinding_update(
keystrokes: new_keystrokes,
action_name: &existing.action_name,
use_key_equivalents: false,
input: new_input,
action_arguments: new_args,
},
}
} else {
@ -1911,15 +2002,18 @@ async fn save_keybinding_update(
keystrokes: new_keystrokes,
action_name: &existing.action_name,
use_key_equivalents: false,
input: new_input,
action_arguments: new_args,
})
};
let updated_keymap_contents =
settings::KeymapFile::update_keybinding(operation, keymap_contents, tab_size)
.context("Failed to update keybinding")?;
fs.atomic_write(paths::keymap_file().clone(), updated_keymap_contents)
.await
.context("Failed to write keymap file")?;
fs.write(
paths::keymap_file().as_path(),
updated_keymap_contents.as_bytes(),
)
.await
.context("Failed to write keymap file")?;
Ok(())
}
@ -1944,10 +2038,10 @@ async fn remove_keybinding(
keystrokes,
action_name: &existing.action_name,
use_key_equivalents: false,
input: existing
.action_input
action_arguments: existing
.action_arguments
.as_ref()
.map(|input| input.text.as_ref()),
.map(|arguments| arguments.text.as_ref()),
},
target_keybind_source: existing
.source
@ -1959,9 +2053,12 @@ async fn remove_keybinding(
let updated_keymap_contents =
settings::KeymapFile::update_keybinding(operation, keymap_contents, tab_size)
.context("Failed to update keybinding")?;
fs.atomic_write(paths::keymap_file().clone(), updated_keymap_contents)
.await
.context("Failed to write keymap file")?;
fs.write(
paths::keymap_file().as_path(),
updated_keymap_contents.as_bytes(),
)
.await
.context("Failed to write keymap file")?;
Ok(())
}

View file

@ -3,8 +3,8 @@ use std::{ops::Range, rc::Rc, time::Duration};
use editor::{EditorSettings, ShowScrollbar, scroll::ScrollbarAutoHide};
use gpui::{
AppContext, Axis, Context, Entity, FocusHandle, Length, ListHorizontalSizingBehavior,
ListSizingBehavior, MouseButton, Task, UniformListScrollHandle, WeakEntity, transparent_black,
uniform_list,
ListSizingBehavior, MouseButton, Point, Task, UniformListScrollHandle, WeakEntity,
transparent_black, uniform_list,
};
use settings::Settings as _;
use ui::{
@ -90,6 +90,28 @@ impl TableInteractionState {
})
}
pub fn get_scrollbar_offset(&self, axis: Axis) -> Point<Pixels> {
match axis {
Axis::Vertical => self.vertical_scrollbar.state.scroll_handle().offset(),
Axis::Horizontal => self.horizontal_scrollbar.state.scroll_handle().offset(),
}
}
pub fn set_scrollbar_offset(&self, axis: Axis, offset: Point<Pixels>) {
match axis {
Axis::Vertical => self
.vertical_scrollbar
.state
.scroll_handle()
.set_offset(offset),
Axis::Horizontal => self
.horizontal_scrollbar
.state
.scroll_handle()
.set_offset(offset),
}
}
fn update_scrollbar_visibility(&mut self, cx: &mut Context<Self>) {
let show_setting = EditorSettings::get_global(cx).scrollbar.show;