From 843336970ad65fcb12c73f45f8d23823ed1167d5 Mon Sep 17 00:00:00 2001 From: Finn Evers Date: Mon, 18 Aug 2025 13:01:32 +0200 Subject: [PATCH] keymap_ui: Ensure keybind with empty arguments can be saved (#36393) Follow up to #36278 to ensure this bug is actually fixed. Also fixes this on two layers and adds a test for the lower layer, as we cannot properly test it in the UI. Furthermore, this improves the error message to show some more context and ensures the status toast is actually only shown when the keybind was successfully updated: Before, we would show the success toast whilst also showing an error in the editor. Lastly, this also fixes some issues with the status toast (and animations) where no status toast or no animation would show in certain scenarios. Release Notes: - N/A --- crates/settings/src/keymap_file.rs | 24 +++++++- crates/settings_ui/src/keybindings.rs | 84 +++++++++++++-------------- crates/ui/src/styles/animation.rs | 27 +++++---- crates/workspace/src/toast_layer.rs | 32 +++++----- 4 files changed, 93 insertions(+), 74 deletions(-) diff --git a/crates/settings/src/keymap_file.rs b/crates/settings/src/keymap_file.rs index 7802671fecdcafe26a22057b8484ddfcbe7556fd..fb036622907487fd0e42b3e58d28d93acf77b340 100644 --- a/crates/settings/src/keymap_file.rs +++ b/crates/settings/src/keymap_file.rs @@ -928,14 +928,14 @@ impl<'a> KeybindUpdateTarget<'a> { } let action_name: Value = self.action_name.into(); let value = match self.action_arguments { - Some(args) => { + Some(args) if !args.is_empty() => { let args = serde_json::from_str::(args) .context("Failed to parse action arguments as JSON")?; serde_json::json!([action_name, args]) } - None => action_name, + _ => action_name, }; - return Ok(value); + Ok(value) } fn keystrokes_unparsed(&self) -> String { @@ -1084,6 +1084,24 @@ mod tests { .unindent(), ); + check_keymap_update( + "[]", + KeybindUpdateOperation::add(KeybindUpdateTarget { + keystrokes: &parse_keystrokes("ctrl-a"), + action_name: "zed::SomeAction", + context: None, + action_arguments: Some(""), + }), + r#"[ + { + "bindings": { + "ctrl-a": "zed::SomeAction" + } + } + ]"# + .unindent(), + ); + check_keymap_update( r#"[ { diff --git a/crates/settings_ui/src/keybindings.rs b/crates/settings_ui/src/keybindings.rs index b4e871c617461ce7d760c4d9374b6ad3dacb2f23..5181d86a789680483b642087e7e0df1ff8a5f562 100644 --- a/crates/settings_ui/src/keybindings.rs +++ b/crates/settings_ui/src/keybindings.rs @@ -2177,11 +2177,11 @@ impl KeybindingEditorModal { let action_arguments = self .action_arguments_editor .as_ref() - .map(|editor| editor.read(cx).editor.read(cx).text(cx)); + .map(|arguments_editor| arguments_editor.read(cx).editor.read(cx).text(cx)) + .filter(|args| !args.is_empty()); let value = action_arguments .as_ref() - .filter(|args| !args.is_empty()) .map(|args| { serde_json::from_str(args).context("Failed to parse action arguments as JSON") }) @@ -2289,29 +2289,11 @@ impl KeybindingEditorModal { let create = self.creating; - let status_toast = StatusToast::new( - format!( - "Saved edits to the {} action.", - &self.editing_keybind.action().humanized_name - ), - cx, - move |this, _cx| { - this.icon(ToastIcon::new(IconName::Check).color(Color::Success)) - .dismiss_button(true) - // .action("Undo", f) todo: wire the undo functionality - }, - ); - - self.workspace - .update(cx, |workspace, cx| { - workspace.toggle_status_toast(status_toast, cx); - }) - .log_err(); - cx.spawn(async move |this, cx| { let action_name = existing_keybind.action().name; + let humanized_action_name = existing_keybind.action().humanized_name.clone(); - if let Err(err) = save_keybinding_update( + match save_keybinding_update( create, existing_keybind, &action_mapping, @@ -2321,25 +2303,43 @@ impl KeybindingEditorModal { ) .await { - this.update(cx, |this, cx| { - this.set_error(InputError::error(err), cx); - }) - .log_err(); - } else { - this.update(cx, |this, cx| { - 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(); + Ok(_) => { + this.update(cx, |this, cx| { + 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), + }); + let status_toast = StatusToast::new( + format!("Saved edits to the {} action.", humanized_action_name), + cx, + move |this, _cx| { + this.icon(ToastIcon::new(IconName::Check).color(Color::Success)) + .dismiss_button(true) + // .action("Undo", f) todo: wire the undo functionality + }, + ); + + this.workspace + .update(cx, |workspace, cx| { + workspace.toggle_status_toast(status_toast, cx); + }) + .log_err(); + }); + cx.emit(DismissEvent); + }) + .ok(); + } + Err(err) => { + this.update(cx, |this, cx| { + this.set_error(InputError::error(err), cx); + }) + .log_err(); + } } }) .detach(); @@ -3011,7 +3011,7 @@ async fn save_keybinding_update( let updated_keymap_contents = settings::KeymapFile::update_keybinding(operation, keymap_contents, tab_size) - .context("Failed to update keybinding")?; + .map_err(|err| anyhow::anyhow!("Could not save updated keybinding: {}", err))?; fs.write( paths::keymap_file().as_path(), updated_keymap_contents.as_bytes(), diff --git a/crates/ui/src/styles/animation.rs b/crates/ui/src/styles/animation.rs index ee5352d45403183555fe8d6c72806a5b90f88ca8..acea834548675b3a896a03da731a7eb4fae777e4 100644 --- a/crates/ui/src/styles/animation.rs +++ b/crates/ui/src/styles/animation.rs @@ -31,7 +31,7 @@ pub enum AnimationDirection { FromTop, } -pub trait DefaultAnimations: Styled + Sized { +pub trait DefaultAnimations: Styled + Sized + Element { fn animate_in( self, animation_type: AnimationDirection, @@ -44,8 +44,13 @@ pub trait DefaultAnimations: Styled + Sized { AnimationDirection::FromTop => "animate_from_top", }; + let animation_id = self.id().map_or_else( + || ElementId::from(animation_name), + |id| (id, animation_name).into(), + ); + self.with_animation( - animation_name, + animation_id, gpui::Animation::new(AnimationDuration::Fast.into()).with_easing(ease_out_quint()), move |mut this, delta| { let start_opacity = 0.4; @@ -91,7 +96,7 @@ pub trait DefaultAnimations: Styled + Sized { } } -impl DefaultAnimations for E {} +impl DefaultAnimations for E {} // Don't use this directly, it only exists to show animation previews #[derive(RegisterComponent)] @@ -132,7 +137,7 @@ impl Component for Animation { .left(px(offset)) .rounded_md() .bg(gpui::red()) - .animate_in(AnimationDirection::FromBottom, false), + .animate_in_from_bottom(false), ) .into_any_element(), ), @@ -151,7 +156,7 @@ impl Component for Animation { .left(px(offset)) .rounded_md() .bg(gpui::blue()) - .animate_in(AnimationDirection::FromTop, false), + .animate_in_from_top(false), ) .into_any_element(), ), @@ -170,7 +175,7 @@ impl Component for Animation { .top(px(offset)) .rounded_md() .bg(gpui::green()) - .animate_in(AnimationDirection::FromLeft, false), + .animate_in_from_left(false), ) .into_any_element(), ), @@ -189,7 +194,7 @@ impl Component for Animation { .top(px(offset)) .rounded_md() .bg(gpui::yellow()) - .animate_in(AnimationDirection::FromRight, false), + .animate_in_from_right(false), ) .into_any_element(), ), @@ -214,7 +219,7 @@ impl Component for Animation { .left(px(offset)) .rounded_md() .bg(gpui::red()) - .animate_in(AnimationDirection::FromBottom, true), + .animate_in_from_bottom(true), ) .into_any_element(), ), @@ -233,7 +238,7 @@ impl Component for Animation { .left(px(offset)) .rounded_md() .bg(gpui::blue()) - .animate_in(AnimationDirection::FromTop, true), + .animate_in_from_top(true), ) .into_any_element(), ), @@ -252,7 +257,7 @@ impl Component for Animation { .top(px(offset)) .rounded_md() .bg(gpui::green()) - .animate_in(AnimationDirection::FromLeft, true), + .animate_in_from_left(true), ) .into_any_element(), ), @@ -271,7 +276,7 @@ impl Component for Animation { .top(px(offset)) .rounded_md() .bg(gpui::yellow()) - .animate_in(AnimationDirection::FromRight, true), + .animate_in_from_right(true), ) .into_any_element(), ), diff --git a/crates/workspace/src/toast_layer.rs b/crates/workspace/src/toast_layer.rs index 28be3e7e47a7d617725ce4a67936bd481baf53db..515794554831dc62bdf8babf717ce1f372f37763 100644 --- a/crates/workspace/src/toast_layer.rs +++ b/crates/workspace/src/toast_layer.rs @@ -3,7 +3,7 @@ use std::{ time::{Duration, Instant}, }; -use gpui::{AnyView, DismissEvent, Entity, FocusHandle, ManagedView, Subscription, Task}; +use gpui::{AnyView, DismissEvent, Entity, EntityId, FocusHandle, ManagedView, Subscription, Task}; use ui::{animation::DefaultAnimations, prelude::*}; use zed_actions::toast; @@ -76,6 +76,7 @@ impl ToastViewHandle for Entity { } pub struct ActiveToast { + id: EntityId, toast: Box, action: Option, _subscriptions: [Subscription; 1], @@ -113,9 +114,9 @@ impl ToastLayer { V: ToastView, { if let Some(active_toast) = &self.active_toast { - let is_close = active_toast.toast.view().downcast::().is_ok(); - let did_close = self.hide_toast(cx); - if is_close || !did_close { + let show_new = active_toast.id != new_toast.entity_id(); + self.hide_toast(cx); + if !show_new { return; } } @@ -130,11 +131,12 @@ impl ToastLayer { let focus_handle = cx.focus_handle(); self.active_toast = Some(ActiveToast { - toast: Box::new(new_toast.clone()), - action, _subscriptions: [cx.subscribe(&new_toast, |this, _, _: &DismissEvent, cx| { this.hide_toast(cx); })], + id: new_toast.entity_id(), + toast: Box::new(new_toast), + action, focus_handle, }); @@ -143,11 +145,9 @@ impl ToastLayer { cx.notify(); } - pub fn hide_toast(&mut self, cx: &mut Context) -> bool { + pub fn hide_toast(&mut self, cx: &mut Context) { self.active_toast.take(); cx.notify(); - - true } pub fn active_toast(&self) -> Option> @@ -218,11 +218,10 @@ impl Render for ToastLayer { let Some(active_toast) = &self.active_toast else { return div(); }; - let handle = cx.weak_entity(); div().absolute().size_full().bottom_0().left_0().child( v_flex() - .id("toast-layer-container") + .id(("toast-layer-container", active_toast.id)) .absolute() .w_full() .bottom(px(0.)) @@ -234,17 +233,14 @@ impl Render for ToastLayer { h_flex() .id("active-toast-container") .occlude() - .on_hover(move |hover_start, _window, cx| { - let Some(this) = handle.upgrade() else { - return; - }; + .on_hover(cx.listener(|this, hover_start, _window, cx| { if *hover_start { - this.update(cx, |this, _| this.pause_dismiss_timer()); + this.pause_dismiss_timer(); } else { - this.update(cx, |this, cx| this.restart_dismiss_timer(cx)); + this.restart_dismiss_timer(cx); } cx.stop_propagation(); - }) + })) .on_click(|_, _, cx| { cx.stop_propagation(); })