Ensure querying keystrokes or actions is safe

Antonio Scandurra created

This is achieved by moving `available_actions` into `AsyncAppContext` (where
we know no view/window is on the stack) and `keystrokes_for_action` into `LayoutContext`
where we'll fetch the previous frame's ancestors and notify the current view if those
change after we perform a layout.

Change summary

crates/command_palette/src/command_palette.rs    |  94 +++++-----
crates/gpui/src/app.rs                           | 160 +++++++++++++----
crates/gpui/src/app/test_app_context.rs          |  28 ++-
crates/gpui/src/app/window.rs                    |  45 ----
crates/gpui/src/elements/keystroke_label.rs      |   2 
crates/gpui/src/keymap_matcher/binding.rs        |  10 +
crates/gpui/src/keymap_matcher/keymap_context.rs |   2 
crates/settings/src/settings_file.rs             |  54 +++---
crates/workspace/src/dock.rs                     |   2 
9 files changed, 233 insertions(+), 164 deletions(-)

Detailed changes

crates/command_palette/src/command_palette.rs 🔗

@@ -2,7 +2,7 @@ use collections::CommandPaletteFilter;
 use fuzzy::{StringMatch, StringMatchCandidate};
 use gpui::{
     actions, elements::*, keymap_matcher::Keystroke, Action, AppContext, Element, MouseState,
-    ViewContext, WindowContext,
+    ViewContext,
 };
 use picker::{Picker, PickerDelegate, PickerEvent};
 use settings::Settings;
@@ -41,47 +41,17 @@ struct Command {
     keystrokes: Vec<Keystroke>,
 }
 
-fn toggle_command_palette(_: &mut Workspace, _: &Toggle, cx: &mut ViewContext<Workspace>) {
-    let workspace = cx.handle();
-    let focused_view_id = cx.focused_view_id().unwrap_or_else(|| workspace.id());
-
-    cx.window_context().defer(move |cx| {
-        // Build the delegate before the workspace is put on the stack so we can find it when
-        // computing the actions. We should really not allow available_actions to be called
-        // if it's not reliable however.
-        let delegate = CommandPaletteDelegate::new(focused_view_id, cx);
-        workspace.update(cx, |workspace, cx| {
-            workspace.toggle_modal(cx, |_, cx| cx.add_view(|cx| Picker::new(delegate, cx)));
-        })
+fn toggle_command_palette(workspace: &mut Workspace, _: &Toggle, cx: &mut ViewContext<Workspace>) {
+    let focused_view_id = cx.focused_view_id().unwrap_or_else(|| cx.view_id());
+    workspace.toggle_modal(cx, |_, cx| {
+        cx.add_view(|cx| Picker::new(CommandPaletteDelegate::new(focused_view_id), cx))
     });
 }
 
 impl CommandPaletteDelegate {
-    pub fn new(focused_view_id: usize, cx: &mut WindowContext) -> Self {
-        let actions = cx
-            .available_actions(focused_view_id)
-            .filter_map(|(name, action, bindings)| {
-                if cx.has_global::<CommandPaletteFilter>() {
-                    let filter = cx.global::<CommandPaletteFilter>();
-                    if filter.filtered_namespaces.contains(action.namespace()) {
-                        return None;
-                    }
-                }
-
-                Some(Command {
-                    name: humanize_action_name(name),
-                    action,
-                    keystrokes: bindings
-                        .iter()
-                        .map(|binding| binding.keystrokes())
-                        .last()
-                        .map_or(Vec::new(), |keystrokes| keystrokes.to_vec()),
-                })
-            })
-            .collect();
-
+    pub fn new(focused_view_id: usize) -> Self {
         Self {
-            actions,
+            actions: Default::default(),
             matches: vec![],
             selected_ix: 0,
             focused_view_id,
@@ -111,17 +81,46 @@ impl PickerDelegate for CommandPaletteDelegate {
         query: String,
         cx: &mut ViewContext<Picker<Self>>,
     ) -> gpui::Task<()> {
-        let candidates = self
-            .actions
-            .iter()
-            .enumerate()
-            .map(|(ix, command)| StringMatchCandidate {
-                id: ix,
-                string: command.name.to_string(),
-                char_bag: command.name.chars().collect(),
-            })
-            .collect::<Vec<_>>();
+        let window_id = cx.window_id();
+        let view_id = self.focused_view_id;
         cx.spawn(move |picker, mut cx| async move {
+            let actions = cx
+                .available_actions(window_id, view_id)
+                .into_iter()
+                .filter_map(|(name, action, bindings)| {
+                    let filtered = cx.read(|cx| {
+                        if cx.has_global::<CommandPaletteFilter>() {
+                            let filter = cx.global::<CommandPaletteFilter>();
+                            filter.filtered_namespaces.contains(action.namespace())
+                        } else {
+                            false
+                        }
+                    });
+
+                    if filtered {
+                        None
+                    } else {
+                        Some(Command {
+                            name: humanize_action_name(name),
+                            action,
+                            keystrokes: bindings
+                                .iter()
+                                .map(|binding| binding.keystrokes())
+                                .last()
+                                .map_or(Vec::new(), |keystrokes| keystrokes.to_vec()),
+                        })
+                    }
+                })
+                .collect::<Vec<_>>();
+            let candidates = actions
+                .iter()
+                .enumerate()
+                .map(|(ix, command)| StringMatchCandidate {
+                    id: ix,
+                    string: command.name.to_string(),
+                    char_bag: command.name.chars().collect(),
+                })
+                .collect::<Vec<_>>();
             let matches = if query.is_empty() {
                 candidates
                     .into_iter()
@@ -147,6 +146,7 @@ impl PickerDelegate for CommandPaletteDelegate {
             picker
                 .update(&mut cx, |picker, _| {
                     let delegate = picker.delegate_mut();
+                    delegate.actions = actions;
                     delegate.matches = matches;
                     if delegate.matches.is_empty() {
                         delegate.selected_ix = 0;

crates/gpui/src/app.rs 🔗

@@ -341,6 +341,15 @@ impl AsyncAppContext {
             .ok_or_else(|| anyhow!("window not found"))
     }
 
+    pub fn available_actions(
+        &self,
+        window_id: usize,
+        view_id: usize,
+    ) -> Vec<(&'static str, Box<dyn Action>, SmallVec<[Binding; 1]>)> {
+        self.read_window(window_id, |cx| cx.available_actions(view_id))
+            .unwrap_or_default()
+    }
+
     pub fn has_window(&self, window_id: usize) -> bool {
         self.read(|cx| cx.windows.contains_key(&window_id))
     }
@@ -3221,6 +3230,64 @@ impl<'a, 'b, 'c, V: View> LayoutContext<'a, 'b, 'c, V> {
     pub fn view_context(&mut self) -> &mut ViewContext<'a, 'b, V> {
         self.view_context
     }
+
+    /// Return keystrokes that would dispatch the given action on the given view.
+    pub(crate) fn keystrokes_for_action(
+        &mut self,
+        view: &V,
+        view_id: usize,
+        action: &dyn Action,
+    ) -> Option<SmallVec<[Keystroke; 2]>> {
+        self.notify_if_view_ancestors_change(view_id);
+
+        let window_id = self.window_id;
+        let mut contexts = Vec::new();
+        let mut handler_depth = None;
+        for (i, view_id) in self.ancestors(view_id).enumerate() {
+            let view = if view_id == self.view_id {
+                Some(view as _)
+            } else {
+                self.views
+                    .get(&(window_id, view_id))
+                    .map(|view| view.as_ref())
+            };
+
+            if let Some(view) = view {
+                if let Some(actions) = self.actions.get(&view.as_any().type_id()) {
+                    if actions.contains_key(&action.as_any().type_id()) {
+                        handler_depth = Some(i);
+                    }
+                }
+                contexts.push(view.keymap_context(self));
+            }
+        }
+
+        if self.global_actions.contains_key(&action.as_any().type_id()) {
+            handler_depth = Some(contexts.len())
+        }
+
+        self.keystroke_matcher
+            .bindings_for_action_type(action.as_any().type_id())
+            .find_map(|b| {
+                handler_depth
+                    .map(|highest_handler| {
+                        if (0..=highest_handler).any(|depth| b.match_context(&contexts[depth..])) {
+                            Some(b.keystrokes().into())
+                        } else {
+                            None
+                        }
+                    })
+                    .flatten()
+            })
+    }
+
+    fn notify_if_view_ancestors_change(&mut self, view_id: usize) {
+        let self_view_id = self.view_id;
+        self.views_to_notify_if_ancestors_change
+            .entry(view_id)
+            .or_default()
+            .push(self_view_id);
+    }
 }
 
 impl<'a, 'b, 'c, V: View> Deref for LayoutContext<'a, 'b, 'c, V> {
@@ -5740,7 +5807,7 @@ mod tests {
     }
 
     #[crate::test(self)]
-    fn test_keystrokes_for_action(cx: &mut AppContext) {
+    fn test_keystrokes_for_action(cx: &mut TestAppContext) {
         actions!(test, [Action1, Action2, GlobalAction]);
 
         struct View1 {
@@ -5772,35 +5839,47 @@ mod tests {
             }
         }
 
-        let (window_id, view_1) = cx.add_window(Default::default(), |cx| {
+        let (window_id, view_1) = cx.add_window(|cx| {
             let view_2 = cx.add_view(|cx| {
                 cx.focus_self();
                 View2 {}
             });
             View1 { child: view_2 }
         });
-        let view_2 = view_1.read(cx).child.clone();
-
-        cx.add_action(|_: &mut View1, _: &Action1, _cx| {});
-        cx.add_action(|_: &mut View2, _: &Action2, _cx| {});
-        cx.add_global_action(|_: &GlobalAction, _| {});
+        let view_2 = view_1.read_with(cx, |view, _| view.child.clone());
 
-        cx.add_bindings(vec![
-            Binding::new("a", Action1, Some("View1")),
-            Binding::new("b", Action2, Some("View1 > View2")),
-            Binding::new("c", GlobalAction, Some("View3")), // View 3 does not exist
-        ]);
+        cx.update(|cx| {
+            cx.add_action(|_: &mut View1, _: &Action1, _cx| {});
+            cx.add_action(|_: &mut View2, _: &Action2, _cx| {});
+            cx.add_global_action(|_: &GlobalAction, _| {});
+            cx.add_bindings(vec![
+                Binding::new("a", Action1, Some("View1")),
+                Binding::new("b", Action2, Some("View1 > View2")),
+                Binding::new("c", GlobalAction, Some("View3")), // View 3 does not exist
+            ]);
+        });
 
-        cx.update_window(window_id, |cx| {
+        let view_1_id = view_1.id();
+        view_1.update(cx, |view_1, cx| {
             // Sanity check
+            let mut new_parents = Default::default();
+            let mut notify_views_if_parents_change = Default::default();
+            let mut layout_cx = LayoutContext::new(
+                cx,
+                &mut new_parents,
+                &mut notify_views_if_parents_change,
+                false,
+            );
             assert_eq!(
-                cx.keystrokes_for_action(view_1.id(), &Action1)
+                layout_cx
+                    .keystrokes_for_action(view_1, view_1_id, &Action1)
                     .unwrap()
                     .as_slice(),
                 &[Keystroke::parse("a").unwrap()]
             );
             assert_eq!(
-                cx.keystrokes_for_action(view_2.id(), &Action2)
+                layout_cx
+                    .keystrokes_for_action(view_1, view_2.id(), &Action2)
                     .unwrap()
                     .as_slice(),
                 &[Keystroke::parse("b").unwrap()]
@@ -5809,44 +5888,53 @@ mod tests {
             // The 'a' keystroke propagates up the view tree from view_2
             // to view_1. The action, Action1, is handled by view_1.
             assert_eq!(
-                cx.keystrokes_for_action(view_2.id(), &Action1)
+                layout_cx
+                    .keystrokes_for_action(view_1, view_2.id(), &Action1)
                     .unwrap()
                     .as_slice(),
                 &[Keystroke::parse("a").unwrap()]
             );
 
             // Actions that are handled below the current view don't have bindings
-            assert_eq!(cx.keystrokes_for_action(view_1.id(), &Action2), None);
-
-            // Actions that are handled in other branches of the tree should not have a binding
-            assert_eq!(cx.keystrokes_for_action(view_2.id(), &GlobalAction), None);
-
-            // Check that global actions do not have a binding, even if a binding does exist in another view
             assert_eq!(
-                &available_actions(view_1.id(), cx),
-                &[
-                    ("test::Action1", vec![Keystroke::parse("a").unwrap()]),
-                    ("test::GlobalAction", vec![])
-                ],
+                layout_cx.keystrokes_for_action(view_1, view_1_id, &Action2),
+                None
             );
 
-            // Check that view 1 actions and bindings are available even when called from view 2
+            // Actions that are handled in other branches of the tree should not have a binding
             assert_eq!(
-                &available_actions(view_2.id(), cx),
-                &[
-                    ("test::Action1", vec![Keystroke::parse("a").unwrap()]),
-                    ("test::Action2", vec![Keystroke::parse("b").unwrap()]),
-                    ("test::GlobalAction", vec![]),
-                ],
+                layout_cx.keystrokes_for_action(view_1, view_2.id(), &GlobalAction),
+                None
             );
         });
 
+        // Check that global actions do not have a binding, even if a binding does exist in another view
+        assert_eq!(
+            &available_actions(window_id, view_1.id(), cx),
+            &[
+                ("test::Action1", vec![Keystroke::parse("a").unwrap()]),
+                ("test::GlobalAction", vec![])
+            ],
+        );
+
+        // Check that view 1 actions and bindings are available even when called from view 2
+        assert_eq!(
+            &available_actions(window_id, view_2.id(), cx),
+            &[
+                ("test::Action1", vec![Keystroke::parse("a").unwrap()]),
+                ("test::Action2", vec![Keystroke::parse("b").unwrap()]),
+                ("test::GlobalAction", vec![]),
+            ],
+        );
+
         // Produces a list of actions and key bindings
         fn available_actions(
+            window_id: usize,
             view_id: usize,
-            cx: &WindowContext,
+            cx: &TestAppContext,
         ) -> Vec<(&'static str, Vec<Keystroke>)> {
-            cx.available_actions(view_id)
+            cx.available_actions(window_id, view_id)
+                .into_iter()
                 .map(|(action_name, _, bindings)| {
                     (
                         action_name,

crates/gpui/src/app/test_app_context.rs 🔗

@@ -1,7 +1,7 @@
 use crate::{
     executor,
     geometry::vector::Vector2F,
-    keymap_matcher::Keystroke,
+    keymap_matcher::{Binding, Keystroke},
     platform,
     platform::{Event, InputHandler, KeyDownEvent, Platform},
     Action, AppContext, BorrowAppContext, BorrowWindowContext, Entity, FontCache, Handle,
@@ -12,6 +12,7 @@ use collections::BTreeMap;
 use futures::Future;
 use itertools::Itertools;
 use parking_lot::{Mutex, RwLock};
+use smallvec::SmallVec;
 use smol::stream::StreamExt;
 use std::{
     any::Any,
@@ -71,17 +72,24 @@ impl TestAppContext {
         cx
     }
 
-    pub fn dispatch_action<A: Action>(&self, window_id: usize, action: A) {
-        self.cx
-            .borrow_mut()
-            .update_window(window_id, |window| {
-                window.dispatch_action(window.focused_view_id(), &action);
-            })
-            .expect("window not found");
+    pub fn dispatch_action<A: Action>(&mut self, window_id: usize, action: A) {
+        self.update_window(window_id, |window| {
+            window.dispatch_action(window.focused_view_id(), &action);
+        })
+        .expect("window not found");
+    }
+
+    pub fn available_actions(
+        &self,
+        window_id: usize,
+        view_id: usize,
+    ) -> Vec<(&'static str, Box<dyn Action>, SmallVec<[Binding; 1]>)> {
+        self.read_window(window_id, |cx| cx.available_actions(view_id))
+            .unwrap_or_default()
     }
 
-    pub fn dispatch_global_action<A: Action>(&self, action: A) {
-        self.cx.borrow_mut().dispatch_global_action_any(&action);
+    pub fn dispatch_global_action<A: Action>(&mut self, action: A) {
+        self.update(|cx| cx.dispatch_global_action_any(&action));
     }
 
     pub fn dispatch_keystroke(&mut self, window_id: usize, keystroke: Keystroke, is_held: bool) {

crates/gpui/src/app/window.rs 🔗

@@ -356,49 +356,10 @@ impl<'a> WindowContext<'a> {
         )
     }
 
-    /// Return keystrokes that would dispatch the given action on the given view.
-    pub(crate) fn keystrokes_for_action(
-        &mut self,
-        view_id: usize,
-        action: &dyn Action,
-    ) -> Option<SmallVec<[Keystroke; 2]>> {
-        let window_id = self.window_id;
-        let mut contexts = Vec::new();
-        let mut handler_depth = None;
-        for (i, view_id) in self.ancestors(view_id).enumerate() {
-            if let Some(view) = self.views.get(&(window_id, view_id)) {
-                if let Some(actions) = self.actions.get(&view.as_any().type_id()) {
-                    if actions.contains_key(&action.as_any().type_id()) {
-                        handler_depth = Some(i);
-                    }
-                }
-                contexts.push(view.keymap_context(self));
-            }
-        }
-
-        if self.global_actions.contains_key(&action.as_any().type_id()) {
-            handler_depth = Some(contexts.len())
-        }
-
-        self.keystroke_matcher
-            .bindings_for_action_type(action.as_any().type_id())
-            .find_map(|b| {
-                handler_depth
-                    .map(|highest_handler| {
-                        if (0..=highest_handler).any(|depth| b.match_context(&contexts[depth..])) {
-                            Some(b.keystrokes().into())
-                        } else {
-                            None
-                        }
-                    })
-                    .flatten()
-            })
-    }
-
-    pub fn available_actions(
+    pub(crate) fn available_actions(
         &self,
         view_id: usize,
-    ) -> impl Iterator<Item = (&'static str, Box<dyn Action>, SmallVec<[&Binding; 1]>)> {
+    ) -> Vec<(&'static str, Box<dyn Action>, SmallVec<[Binding; 1]>)> {
         let window_id = self.window_id;
         let mut contexts = Vec::new();
         let mut handler_depths_by_action_type = HashMap::<TypeId, usize>::default();
@@ -441,12 +402,14 @@ impl<'a> WindowContext<'a> {
                             .filter(|b| {
                                 (0..=action_depth).any(|depth| b.match_context(&contexts[depth..]))
                             })
+                            .cloned()
                             .collect(),
                     ))
                 } else {
                     None
                 }
             })
+            .collect()
     }
 
     pub(crate) fn dispatch_keystroke(&mut self, keystroke: &Keystroke) -> bool {

crates/gpui/src/elements/keystroke_label.rs 🔗

@@ -42,7 +42,7 @@ impl<V: View> Element<V> for KeystrokeLabel {
         cx: &mut LayoutContext<V>,
     ) -> (Vector2F, AnyElement<V>) {
         let mut element = if let Some(keystrokes) =
-            cx.keystrokes_for_action(self.view_id, self.action.as_ref())
+            cx.keystrokes_for_action(view, self.view_id, self.action.as_ref())
         {
             Flex::row()
                 .with_children(keystrokes.iter().map(|keystroke| {

crates/gpui/src/keymap_matcher/binding.rs 🔗

@@ -11,6 +11,16 @@ pub struct Binding {
     context_predicate: Option<KeymapContextPredicate>,
 }
 
+impl Clone for Binding {
+    fn clone(&self) -> Self {
+        Self {
+            action: self.action.boxed_clone(),
+            keystrokes: self.keystrokes.clone(),
+            context_predicate: self.context_predicate.clone(),
+        }
+    }
+}
+
 impl Binding {
     pub fn new<A: Action>(keystrokes: &str, action: A, context: Option<&str>) -> Self {
         Self::load(keystrokes, Box::new(action), context).unwrap()

crates/settings/src/settings_file.rs 🔗

@@ -80,7 +80,7 @@ mod tests {
         watch_files, watched_json::watch_settings_file, EditorSettings, Settings, SoftWrap,
     };
     use fs::FakeFs;
-    use gpui::{actions, elements::*, Action, Entity, View, ViewContext, WindowContext};
+    use gpui::{actions, elements::*, Action, Entity, TestAppContext, View, ViewContext};
     use theme::ThemeRegistry;
 
     struct TestView;
@@ -167,13 +167,12 @@ mod tests {
         let (window_id, _view) = cx.add_window(|_| TestView);
 
         // Test loading the keymap base at all
-        cx.read_window(window_id, |cx| {
-            assert_key_bindings_for(
-                cx,
-                vec![("backspace", &A), ("k", &ActivatePreviousPane)],
-                line!(),
-            );
-        });
+        assert_key_bindings_for(
+            window_id,
+            cx,
+            vec![("backspace", &A), ("k", &ActivatePreviousPane)],
+            line!(),
+        );
 
         // Test modifying the users keymap, while retaining the base keymap
         fs.save(
@@ -195,13 +194,12 @@ mod tests {
 
         cx.foreground().run_until_parked();
 
-        cx.read_window(window_id, |cx| {
-            assert_key_bindings_for(
-                cx,
-                vec![("backspace", &B), ("k", &ActivatePreviousPane)],
-                line!(),
-            );
-        });
+        assert_key_bindings_for(
+            window_id,
+            cx,
+            vec![("backspace", &B), ("k", &ActivatePreviousPane)],
+            line!(),
+        );
 
         // Test modifying the base, while retaining the users keymap
         fs.save(
@@ -219,31 +217,33 @@ mod tests {
 
         cx.foreground().run_until_parked();
 
-        cx.read_window(window_id, |cx| {
-            assert_key_bindings_for(
-                cx,
-                vec![("backspace", &B), ("[", &ActivatePrevItem)],
-                line!(),
-            );
-        });
+        assert_key_bindings_for(
+            window_id,
+            cx,
+            vec![("backspace", &B), ("[", &ActivatePrevItem)],
+            line!(),
+        );
     }
 
     fn assert_key_bindings_for<'a>(
-        cx: &WindowContext,
+        window_id: usize,
+        cx: &TestAppContext,
         actions: Vec<(&'static str, &'a dyn Action)>,
         line: u32,
     ) {
         for (key, action) in actions {
             // assert that...
             assert!(
-                cx.available_actions(0).any(|(_, bound_action, b)| {
-                    // action names match...
-                    bound_action.name() == action.name()
+                cx.available_actions(window_id, 0)
+                    .into_iter()
+                    .any(|(_, bound_action, b)| {
+                        // action names match...
+                        bound_action.name() == action.name()
                     && bound_action.namespace() == action.namespace()
                     // and key strokes contain the given key
                     && b.iter()
                         .any(|binding| binding.keystrokes().iter().any(|k| k.key == key))
-                }),
+                    }),
                 "On {} Failed to find {} with key binding {}",
                 line,
                 action.name(),

crates/workspace/src/dock.rs 🔗

@@ -726,7 +726,7 @@ mod tests {
             self.update_workspace(|workspace, cx| Dock::move_dock(workspace, anchor, true, cx));
         }
 
-        pub fn hide_dock(&self) {
+        pub fn hide_dock(&mut self) {
             self.cx.dispatch_action(self.window_id, HideDock);
         }