Fix `WindowContext::available_actions` missing some actions (#3752)

Antonio Scandurra created 2 years ago

This pull request also allows turning an `ArenaBox` into an `ArenaRef`
when non-mutable access is required, which makes `ArenaRef: Clone`. This
fixes a bug that prevented the command palette from reading all the
available actions while the `command_palette::Toggle` action was being
dispatched.

Release Notes:

- N/A

Change summary

crates/gpui2/src/arena.rs        | 42 ++++++++++++--
crates/gpui2/src/element.rs      |  4 
crates/gpui2/src/key_dispatch.rs |  7 --
crates/gpui2/src/window.rs       | 92 ++++++++++++---------------------
4 files changed, 70 insertions(+), 75 deletions(-)

Detailed changes

crates/gpui2/src/arena.rs

@@ -52,7 +52,7 @@ impl Arena {
     }
 
     #[inline(always)]
-    pub fn alloc<T>(&mut self, f: impl FnOnce() -> T) -> ArenaRef<T> {
+    pub fn alloc<T>(&mut self, f: impl FnOnce() -> T) -> ArenaBox<T> {
         #[inline(always)]
         unsafe fn inner_writer<T, F>(ptr: *mut T, f: F)
         where
@@ -70,7 +70,7 @@ impl Arena {
             let next_offset = self.offset.add(layout.size());
             assert!(next_offset <= self.end);
 
-            let result = ArenaRef {
+            let result = ArenaBox {
                 ptr: self.offset.cast(),
                 valid: self.valid.clone(),
             };
@@ -93,15 +93,15 @@ impl Drop for Arena {
     }
 }
 
-pub struct ArenaRef<T: ?Sized> {
+pub struct ArenaBox<T: ?Sized> {
     ptr: *mut T,
     valid: Rc<Cell<bool>>,
 }
 
-impl<T: ?Sized> ArenaRef<T> {
+impl<T: ?Sized> ArenaBox<T> {
     #[inline(always)]
-    pub fn map<U: ?Sized>(mut self, f: impl FnOnce(&mut T) -> &mut U) -> ArenaRef<U> {
-        ArenaRef {
+    pub fn map<U: ?Sized>(mut self, f: impl FnOnce(&mut T) -> &mut U) -> ArenaBox<U> {
+        ArenaBox {
             ptr: f(&mut self),
             valid: self.valid,
         }
@@ -115,7 +115,7 @@ impl<T: ?Sized> ArenaRef<T> {
     }
 }
 
-impl<T: ?Sized> Deref for ArenaRef<T> {
+impl<T: ?Sized> Deref for ArenaBox<T> {
     type Target = T;
 
     #[inline(always)]
@@ -125,7 +125,7 @@ impl<T: ?Sized> Deref for ArenaRef<T> {
     }
 }
 
-impl<T: ?Sized> DerefMut for ArenaRef<T> {
+impl<T: ?Sized> DerefMut for ArenaBox<T> {
     #[inline(always)]
     fn deref_mut(&mut self) -> &mut Self::Target {
         self.validate();
@@ -133,6 +133,32 @@ impl<T: ?Sized> DerefMut for ArenaRef<T> {
     }
 }
 
+pub struct ArenaRef<T: ?Sized>(ArenaBox<T>);
+
+impl<T: ?Sized> From<ArenaBox<T>> for ArenaRef<T> {
+    fn from(value: ArenaBox<T>) -> Self {
+        ArenaRef(value)
+    }
+}
+
+impl<T: ?Sized> Clone for ArenaRef<T> {
+    fn clone(&self) -> Self {
+        Self(ArenaBox {
+            ptr: self.0.ptr,
+            valid: self.0.valid.clone(),
+        })
+    }
+}
+
+impl<T: ?Sized> Deref for ArenaRef<T> {
+    type Target = T;
+
+    #[inline(always)]
+    fn deref(&self) -> &Self::Target {
+        self.0.deref()
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use std::{cell::Cell, rc::Rc};

crates/gpui2/src/element.rs

@@ -1,5 +1,5 @@
 use crate::{
-    ArenaRef, AvailableSpace, BorrowWindow, Bounds, ElementId, LayoutId, Pixels, Point, Size,
+    ArenaBox, AvailableSpace, BorrowWindow, Bounds, ElementId, LayoutId, Pixels, Point, Size,
     ViewContext, WindowContext, ELEMENT_ARENA,
 };
 use derive_more::{Deref, DerefMut};
@@ -405,7 +405,7 @@ where
     }
 }
 
-pub struct AnyElement(ArenaRef<dyn ElementObject>);
+pub struct AnyElement(ArenaBox<dyn ElementObject>);
 
 impl AnyElement {
     pub fn new<E>(element: E) -> Self

crates/gpui2/src/key_dispatch.rs

@@ -35,6 +35,7 @@ pub(crate) struct DispatchNode {
 
 type KeyListener = ArenaRef<dyn Fn(&dyn Any, DispatchPhase, &mut WindowContext)>;
 
+#[derive(Clone)]
 pub(crate) struct DispatchActionListener {
     pub(crate) action_type: TypeId,
     pub(crate) listener: ArenaRef<dyn Fn(&dyn Any, DispatchPhase, &mut WindowContext)>,
@@ -103,8 +104,6 @@ impl DispatchTree {
                     .keystroke_matchers
                     .remove_entry(self.context_stack.as_slice())
                 {
-                    dbg!("preserve matcher", matcher.has_pending_keystrokes());
-
                     self.keystroke_matchers.insert(context_stack, matcher);
                 }
             }
@@ -268,10 +267,6 @@ impl DispatchTree {
         &self.nodes[node_id.0]
     }
 
-    pub fn node_mut(&mut self, node_id: DispatchNodeId) -> &mut DispatchNode {
-        &mut self.nodes[node_id.0]
-    }
-
     fn active_node(&mut self) -> &mut DispatchNode {
         let active_node_id = self.active_node_id();
         &mut self.nodes[active_node_id.0]

crates/gpui2/src/window.rs

@@ -1,17 +1,15 @@
 use crate::{
-    arena::{Arena, ArenaRef},
-    key_dispatch::DispatchActionListener,
-    px, size, transparent_black, Action, AnyDrag, AnyView, AppContext, AsyncWindowContext,
-    AvailableSpace, Bounds, BoxShadow, Context, Corners, CursorStyle, DevicePixels, DispatchNodeId,
-    DispatchTree, DisplayId, Edges, Effect, Entity, EntityId, EventEmitter, FileDropEvent, Flatten,
-    FontId, GlobalElementId, GlyphId, Hsla, ImageData, InputEvent, IsZero, KeyBinding, KeyContext,
-    KeyDownEvent, KeystrokeEvent, LayoutId, Model, ModelContext, Modifiers, MonochromeSprite,
-    MouseButton, MouseMoveEvent, MouseUpEvent, Path, Pixels, PlatformAtlas, PlatformDisplay,
-    PlatformInputHandler, PlatformWindow, Point, PolychromeSprite, PromptLevel, Quad, Render,
-    RenderGlyphParams, RenderImageParams, RenderSvgParams, ScaledPixels, Scene, SceneBuilder,
-    Shadow, SharedString, Size, Style, SubscriberSet, Subscription, Surface, TaffyLayoutEngine,
-    Task, Underline, UnderlineStyle, View, VisualContext, WeakView, WindowBounds, WindowOptions,
-    SUBPIXEL_VARIANTS,
+    px, size, transparent_black, Action, AnyDrag, AnyView, AppContext, Arena, ArenaBox, ArenaRef,
+    AsyncWindowContext, AvailableSpace, Bounds, BoxShadow, Context, Corners, CursorStyle,
+    DevicePixels, DispatchActionListener, DispatchNodeId, DispatchTree, DisplayId, Edges, Effect,
+    Entity, EntityId, EventEmitter, FileDropEvent, Flatten, FontId, GlobalElementId, GlyphId, Hsla,
+    ImageData, InputEvent, IsZero, KeyBinding, KeyContext, KeyDownEvent, KeystrokeEvent, LayoutId,
+    Model, ModelContext, Modifiers, MonochromeSprite, MouseButton, MouseMoveEvent, MouseUpEvent,
+    Path, Pixels, PlatformAtlas, PlatformDisplay, PlatformInputHandler, PlatformWindow, Point,
+    PolychromeSprite, PromptLevel, Quad, Render, RenderGlyphParams, RenderImageParams,
+    RenderSvgParams, ScaledPixels, Scene, SceneBuilder, Shadow, SharedString, Size, Style,
+    SubscriberSet, Subscription, Surface, TaffyLayoutEngine, Task, Underline, UnderlineStyle, View,
+    VisualContext, WeakView, WindowBounds, WindowOptions, SUBPIXEL_VARIANTS,
 };
 use anyhow::{anyhow, Context as _, Result};
 use collections::FxHashMap;
@@ -96,7 +94,7 @@ impl DispatchPhase {
 }
 
 type AnyObserver = Box<dyn FnMut(&mut WindowContext) -> bool + 'static>;
-type AnyMouseListener = ArenaRef<dyn FnMut(&dyn Any, DispatchPhase, &mut WindowContext) + 'static>;
+type AnyMouseListener = ArenaBox<dyn FnMut(&dyn Any, DispatchPhase, &mut WindowContext) + 'static>;
 type AnyWindowFocusListener = Box<dyn FnMut(&FocusEvent, &mut WindowContext) -> bool + 'static>;
 
 struct FocusEvent {
@@ -906,7 +904,10 @@ impl<'a> WindowContext<'a> {
                 }
             })
             .map(|handler| handler as _);
-        self.window.next_frame.dispatch_tree.on_key_event(listener);
+        self.window
+            .next_frame
+            .dispatch_tree
+            .on_key_event(ArenaRef::from(listener));
     }
 
     /// Register an action listener on the window for the next frame. The type of action
@@ -928,7 +929,7 @@ impl<'a> WindowContext<'a> {
         self.window
             .next_frame
             .dispatch_tree
-            .on_action(action_type, listener);
+            .on_action(action_type, ArenaRef::from(listener));
     }
 
     pub fn is_action_available(&self, action: &dyn Action) -> bool {
@@ -1339,7 +1340,7 @@ impl<'a> WindowContext<'a> {
                         cx.window
                             .next_frame
                             .dispatch_tree
-                            .on_action(*action_type, listener)
+                            .on_action(*action_type, ArenaRef::from(listener))
                     }
                 }
 
@@ -1583,43 +1584,30 @@ impl<'a> WindowContext<'a> {
         self.propagate_event = true;
 
         for node_id in &dispatch_path {
-            let node = self.window.rendered_frame.dispatch_tree.node_mut(*node_id);
+            let node = self.window.rendered_frame.dispatch_tree.node(*node_id);
+
             if let Some(context) = node.context.clone() {
                 context_stack.push(context);
             }
 
-            let key_listeners = mem::take(&mut node.key_listeners);
-            for key_listener in &key_listeners {
+            for key_listener in node.key_listeners.clone() {
                 key_listener(event, DispatchPhase::Capture, self);
                 if !self.propagate_event {
-                    break;
+                    return;
                 }
             }
-            let node = self.window.rendered_frame.dispatch_tree.node_mut(*node_id);
-            node.key_listeners = key_listeners;
-
-            if !self.propagate_event {
-                return;
-            }
         }
 
         // Bubble phase
         for node_id in dispatch_path.iter().rev() {
             // Handle low level key events
-            let node = self.window.rendered_frame.dispatch_tree.node_mut(*node_id);
-            let key_listeners = mem::take(&mut node.key_listeners);
-            for key_listener in &key_listeners {
+            let node = self.window.rendered_frame.dispatch_tree.node(*node_id);
+            for key_listener in node.key_listeners.clone() {
                 key_listener(event, DispatchPhase::Bubble, self);
                 if !self.propagate_event {
-                    break;
+                    return;
                 }
             }
-            let node = self.window.rendered_frame.dispatch_tree.node_mut(*node_id);
-            node.key_listeners = key_listeners;
-
-            if !self.propagate_event {
-                return;
-            }
 
             // Match keystrokes
             let node = self.window.rendered_frame.dispatch_tree.node(*node_id);
@@ -1667,52 +1655,38 @@ impl<'a> WindowContext<'a> {
 
         // Capture phase
         for node_id in &dispatch_path {
-            let node = self.window.rendered_frame.dispatch_tree.node_mut(*node_id);
-            let action_listeners = mem::take(&mut node.action_listeners);
+            let node = self.window.rendered_frame.dispatch_tree.node(*node_id);
             for DispatchActionListener {
                 action_type,
                 listener,
-            } in &action_listeners
+            } in node.action_listeners.clone()
             {
                 let any_action = action.as_any();
-                if *action_type == any_action.type_id() {
+                if action_type == any_action.type_id() {
                     listener(any_action, DispatchPhase::Capture, self);
                     if !self.propagate_event {
-                        break;
+                        return;
                     }
                 }
             }
-            let node = self.window.rendered_frame.dispatch_tree.node_mut(*node_id);
-            node.action_listeners = action_listeners;
-
-            if !self.propagate_event {
-                return;
-            }
         }
         // Bubble phase
         for node_id in dispatch_path.iter().rev() {
-            let node = self.window.rendered_frame.dispatch_tree.node_mut(*node_id);
-            let action_listeners = mem::take(&mut node.action_listeners);
+            let node = self.window.rendered_frame.dispatch_tree.node(*node_id);
             for DispatchActionListener {
                 action_type,
                 listener,
-            } in &action_listeners
+            } in node.action_listeners.clone()
             {
                 let any_action = action.as_any();
-                if *action_type == any_action.type_id() {
+                if action_type == any_action.type_id() {
                     self.propagate_event = false; // Actions stop propagation by default during the bubble phase
                     listener(any_action, DispatchPhase::Bubble, self);
                     if !self.propagate_event {
-                        break;
+                        return;
                     }
                 }
             }
-
-            let node = self.window.rendered_frame.dispatch_tree.node_mut(*node_id);
-            node.action_listeners = action_listeners;
-            if !self.propagate_event {
-                return;
-            }
         }
     }