From 16da73baacc3fd5eeceaa2e02962aa51da30b0bc 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 5a996a162e3382b07074cf92935d5b9143ed200c..0713bc129f3daef62d4aca72625cc2ea41e9845b 100644 --- a/crates/settings_ui/src/keybindings.rs +++ b/crates/settings_ui/src/keybindings.rs @@ -2148,11 +2148,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") }) @@ -2260,29 +2260,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, @@ -2292,25 +2274,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(); @@ -2907,7 +2907,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(); })