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

@@ -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::<Value>(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#"[
                 {

crates/settings_ui/src/keybindings.rs

@@ -2150,11 +2150,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")
             })
@@ -2262,29 +2262,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,
@@ -2294,25 +2276,43 @@ impl KeybindingEditorModal {
             )
             .await
             {
-                this.update(cx, |this, cx| {
+                Ok(_) => {
-                    this.set_error(InputError::error(err), cx);
+                    this.update(cx, |this, cx| {
-                })
+                        this.keymap_editor.update(cx, |keymap, cx| {
-                .log_err();
+                            keymap.previous_edit = Some(PreviousEdit::Keybinding {
-            } else {
+                                action_mapping,
-                this.update(cx, |this, cx| {
+                                action_name,
-                    this.keymap_editor.update(cx, |keymap, cx| {
+                                fallback: keymap
-                        keymap.previous_edit = Some(PreviousEdit::Keybinding {
+                                    .table_interaction_state
-                            action_mapping,
+                                    .read(cx)
-                            action_name,
+                                    .get_scrollbar_offset(Axis::Vertical),
-                            fallback: keymap
+                            });
-                                .table_interaction_state
+                            let status_toast = StatusToast::new(
-                                .read(cx)
+                                format!("Saved edits to the {} action.", humanized_action_name),
-                                .get_scrollbar_offset(Axis::Vertical),
+                                cx,
-                        })
+                                move |this, _cx| {
-                    });
+                                    this.icon(ToastIcon::new(IconName::Check).color(Color::Success))
-                    cx.emit(DismissEvent);
+                                        .dismiss_button(true)
-                })
+                                    // .action("Undo", f) todo: wire the undo functionality
-                .ok();
+                                },
+                            );
+
+                            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();
@@ -2984,7 +2984,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(),

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<E: Styled> DefaultAnimations for E {}
+impl<E: Styled + Element> 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(),
                             ),

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<V: ToastView> ToastViewHandle for Entity<V> {
 }
 
 pub struct ActiveToast {
+    id: EntityId,
     toast: Box<dyn ToastViewHandle>,
     action: Option<ToastAction>,
     _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::<V>().is_ok();
+            let show_new = active_toast.id != new_toast.entity_id();
-            let did_close = self.hide_toast(cx);
+            self.hide_toast(cx);
-            if is_close || !did_close {
+            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<Self>) -> bool {
+    pub fn hide_toast(&mut self, cx: &mut Context<Self>) {
         self.active_toast.take();
         cx.notify();
-
-        true
     }
 
     pub fn active_toast<V>(&self) -> Option<Entity<V>>
@@ -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| {
+                        .on_hover(cx.listener(|this, hover_start, _window, cx| {
-                            let Some(this) = handle.upgrade() else {
-                                return;
-                            };
                             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();
                         })