Fix keystroke observer leak in vim crate (#17913)

Release Notes:

- Fixed a performance problem that happened when using vim mode after
opening and closing many editors

Co-authored-by: Antonio <antonio@zed.dev>
Co-authored-by: Nathan <nathan@zed.dev>

crates/gpui/src/app.rs

@@ -204,7 +204,8 @@ impl App {
 
 type Handler = Box<dyn FnMut(&mut AppContext) -> bool + 'static>;
 type Listener = Box<dyn FnMut(&dyn Any, &mut AppContext) -> bool + 'static>;
-type KeystrokeObserver = Box<dyn FnMut(&KeystrokeEvent, &mut WindowContext) + 'static>;
+pub(crate) type KeystrokeObserver =
+    Box<dyn FnMut(&KeystrokeEvent, &mut WindowContext) -> bool + 'static>;
 type QuitHandler = Box<dyn FnOnce(&mut AppContext) -> LocalBoxFuture<'static, ()> + 'static>;
 type ReleaseListener = Box<dyn FnOnce(&mut dyn Any, &mut AppContext) + 'static>;
 type NewViewListener = Box<dyn FnMut(AnyView, &mut WindowContext) + 'static>;
@@ -1050,7 +1051,7 @@ impl AppContext {
     /// and that this API will not be invoked if the event's propagation is stopped.
     pub fn observe_keystrokes(
         &mut self,
-        f: impl FnMut(&KeystrokeEvent, &mut WindowContext) + 'static,
+        mut f: impl FnMut(&KeystrokeEvent, &mut WindowContext) + 'static,
     ) -> Subscription {
         fn inner(
             keystroke_observers: &mut SubscriberSet<(), KeystrokeObserver>,
@@ -1060,7 +1061,14 @@ impl AppContext {
             activate();
             subscription
         }
-        inner(&mut self.keystroke_observers, Box::new(f))
+
+        inner(
+            &mut self.keystroke_observers,
+            Box::new(move |event, cx| {
+                f(event, cx);
+                true
+            }),
+        )
     }
 
     /// Register key bindings.

crates/gpui/src/window.rs

@@ -4,16 +4,17 @@ use crate::{
     Context, Corners, CursorStyle, Decorations, DevicePixels, DispatchActionListener,
     DispatchNodeId, DispatchTree, DisplayId, Edges, Effect, Entity, EntityId, EventEmitter,
     FileDropEvent, Flatten, FontId, GPUSpecs, Global, GlobalElementId, GlyphId, Hsla, InputHandler,
-    IsZero, KeyBinding, KeyContext, KeyDownEvent, KeyEvent, Keystroke, KeystrokeEvent, LayoutId,
+    IsZero, KeyBinding, KeyContext, KeyDownEvent, KeyEvent, Keystroke, KeystrokeEvent,
-    LineLayoutIndex, Model, ModelContext, Modifiers, ModifiersChangedEvent, MonochromeSprite,
+    KeystrokeObserver, LayoutId, LineLayoutIndex, Model, ModelContext, Modifiers,
-    MouseButton, MouseEvent, MouseMoveEvent, MouseUpEvent, Path, Pixels, PlatformAtlas,
+    ModifiersChangedEvent, MonochromeSprite, MouseButton, MouseEvent, MouseMoveEvent, MouseUpEvent,
-    PlatformDisplay, PlatformInput, PlatformInputHandler, PlatformWindow, Point, PolychromeSprite,
+    Path, Pixels, PlatformAtlas, PlatformDisplay, PlatformInput, PlatformInputHandler,
-    PromptLevel, Quad, Render, RenderGlyphParams, RenderImage, RenderImageParams, RenderSvgParams,
+    PlatformWindow, Point, PolychromeSprite, PromptLevel, Quad, Render, RenderGlyphParams,
-    Replay, ResizeEdge, ScaledPixels, Scene, Shadow, SharedString, Size, StrikethroughStyle, Style,
+    RenderImage, RenderImageParams, RenderSvgParams, Replay, ResizeEdge, ScaledPixels, Scene,
-    SubscriberSet, Subscription, TaffyLayoutEngine, Task, TextStyle, TextStyleRefinement,
+    Shadow, SharedString, Size, StrikethroughStyle, Style, SubscriberSet, Subscription,
-    TransformationMatrix, Underline, UnderlineStyle, View, VisualContext, WeakView,
+    TaffyLayoutEngine, Task, TextStyle, TextStyleRefinement, TransformationMatrix, Underline,
-    WindowAppearance, WindowBackgroundAppearance, WindowBounds, WindowControls, WindowDecorations,
+    UnderlineStyle, View, VisualContext, WeakView, WindowAppearance, WindowBackgroundAppearance,
-    WindowOptions, WindowParams, WindowTextSystem, SUBPIXEL_VARIANTS,
+    WindowBounds, WindowControls, WindowDecorations, WindowOptions, WindowParams, WindowTextSystem,
+    SUBPIXEL_VARIANTS,
 };
 use anyhow::{anyhow, Context as _, Result};
 use collections::{FxHashMap, FxHashSet};
@@ -1043,8 +1044,7 @@ impl<'a> WindowContext<'a> {
                         action: action.as_ref().map(|action| action.boxed_clone()),
                     },
                     self,
-                );
+                )
-                true
             });
     }
 
@@ -4250,6 +4250,36 @@ impl<'a, V: 'static> ViewContext<'a, V> {
         subscription
     }
 
+    /// Register a callback to be invoked when a keystroke is received by the application
+    /// in any window. Note that this fires after all other action and event mechanisms have resolved
+    /// and that this API will not be invoked if the event's propagation is stopped.
+    pub fn observe_keystrokes(
+        &mut self,
+        mut f: impl FnMut(&mut V, &KeystrokeEvent, &mut ViewContext<V>) + 'static,
+    ) -> Subscription {
+        fn inner(
+            keystroke_observers: &mut SubscriberSet<(), KeystrokeObserver>,
+            handler: KeystrokeObserver,
+        ) -> Subscription {
+            let (subscription, activate) = keystroke_observers.insert((), handler);
+            activate();
+            subscription
+        }
+
+        let view = self.view.downgrade();
+        inner(
+            &mut self.keystroke_observers,
+            Box::new(move |event, cx| {
+                if let Some(view) = view.upgrade() {
+                    view.update(cx, |view, cx| f(view, event, cx));
+                    true
+                } else {
+                    false
+                }
+            }),
+        )
+    }
+
     /// Register a callback to be invoked when the window's pending input changes.
     pub fn observe_pending_input(
         &mut self,

crates/vim/src/vim.rs

@@ -24,7 +24,7 @@ use editor::{
 };
 use gpui::{
     actions, impl_actions, Action, AppContext, Entity, EventEmitter, KeyContext, KeystrokeEvent,
-    Render, View, ViewContext, WeakView,
+    Render, Subscription, View, ViewContext, WeakView,
 };
 use insert::NormalBefore;
 use language::{CursorShape, Point, Selection, SelectionGoal, TransactionId};
@@ -166,6 +166,8 @@ pub(crate) struct Vim {
     pub search: SearchState,
 
     editor: WeakView<Editor>,
+
+    _subscriptions: Vec<Subscription>,
 }
 
 // Hack: Vim intercepts events dispatched to a window and updates the view in response.
@@ -189,36 +191,32 @@ impl Vim {
     pub fn new(cx: &mut ViewContext<Editor>) -> View<Self> {
         let editor = cx.view().clone();
 
-        cx.new_view(|cx: &mut ViewContext<Vim>| {
+        cx.new_view(|cx| Vim {
-            cx.subscribe(&editor, |vim, _, event, cx| {
+            mode: Mode::Normal,
-                vim.handle_editor_event(event, cx)
+            last_mode: Mode::Normal,
-            })
+            pre_count: None,
-            .detach();
+            post_count: None,
+            operator_stack: Vec::new(),
+            replacements: Vec::new(),
 
-            let listener = cx.listener(Vim::observe_keystrokes);
+            marks: HashMap::default(),
-            cx.observe_keystrokes(listener).detach();
+            stored_visual_mode: None,
+            change_list: Vec::new(),
+            change_list_position: None,
+            current_tx: None,
+            current_anchor: None,
+            undo_modes: HashMap::default(),
 
-            Vim {
+            selected_register: None,
-                mode: Mode::Normal,
+            search: SearchState::default(),
-                last_mode: Mode::Normal,
-                pre_count: None,
-                post_count: None,
-                operator_stack: Vec::new(),
-                replacements: Vec::new(),
 
-                marks: HashMap::default(),
+            editor: editor.downgrade(),
-                stored_visual_mode: None,
+            _subscriptions: vec![
-                change_list: Vec::new(),
+                cx.observe_keystrokes(Self::observe_keystrokes),
-                change_list_position: None,
+                cx.subscribe(&editor, |this, _, event, cx| {
-                current_tx: None,
+                    this.handle_editor_event(event, cx)
-                current_anchor: None,
+                }),
-                undo_modes: HashMap::default(),
+            ],
-
-                selected_register: None,
-                search: SearchState::default(),
-
-                editor: editor.downgrade(),
-            }
         })
     }