Ensure focus_in and focus_out fire on window activation (#3993)

Conrad Irwin created

Also:
- Rename cx.on_blur to cx.on_focus_lost
- Fix a bug where notify calls in focus handlers were ignored
- Fix a bug where vim would get stuck in the wrong mode when switching
  windows

Release Notes:

- (preview only) vim: fix switching between multiple windows

Change summary

crates/collab/src/tests/following_tests.rs |  4 +
crates/collab/src/tests/test_server.rs     |  5 +
crates/gpui/src/app/test_context.rs        |  1 
crates/gpui/src/window.rs                  | 63 +++++++++++++++--------
crates/vim/src/editor_events.rs            | 42 +++++++++++++++
crates/workspace/src/workspace.rs          |  2 
6 files changed, 91 insertions(+), 26 deletions(-)

Detailed changes

crates/collab/src/tests/following_tests.rs 🔗

@@ -76,6 +76,10 @@ async fn test_basic_following(
     let (workspace_a, cx_a) = client_a.build_workspace(&project_a, cx_a);
     let (workspace_b, cx_b) = client_b.build_workspace(&project_b, cx_b);
 
+    cx_b.update(|cx| {
+        assert!(cx.is_window_active());
+    });
+
     // Client A opens some editors.
     let pane_a = workspace_a.update(cx_a, |workspace, _| workspace.active_pane().clone());
     let editor_a1 = workspace_a

crates/collab/src/tests/test_server.rs 🔗

@@ -664,7 +664,10 @@ impl TestClient {
         project: &Model<Project>,
         cx: &'a mut TestAppContext,
     ) -> (View<Workspace>, &'a mut VisualTestContext) {
-        cx.add_window_view(|cx| Workspace::new(0, project.clone(), self.app_state.clone(), cx))
+        cx.add_window_view(|cx| {
+            cx.activate_window();
+            Workspace::new(0, project.clone(), self.app_state.clone(), cx)
+        })
     }
 
     pub fn active_workspace<'a>(

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

@@ -186,6 +186,7 @@ impl TestAppContext {
         drop(cx);
         let view = window.root_view(self).unwrap();
         let cx = Box::new(VisualTestContext::from_window(*window.deref(), self));
+        cx.run_until_parked();
         // it might be nice to try and cleanup these at the end of each test.
         (view, Box::leak(cx))
     }

crates/gpui/src/window.rs 🔗

@@ -269,7 +269,7 @@ pub struct Window {
     frame_arena: Arena,
     pub(crate) focus_handles: Arc<RwLock<SlotMap<FocusId, AtomicUsize>>>,
     focus_listeners: SubscriberSet<(), AnyWindowFocusListener>,
-    blur_listeners: SubscriberSet<(), AnyObserver>,
+    focus_lost_listeners: SubscriberSet<(), AnyObserver>,
     default_prevented: bool,
     mouse_position: Point<Pixels>,
     modifiers: Modifiers,
@@ -296,6 +296,7 @@ pub(crate) struct ElementStateBox {
 
 pub(crate) struct Frame {
     focus: Option<FocusId>,
+    window_active: bool,
     pub(crate) element_states: FxHashMap<GlobalElementId, ElementStateBox>,
     mouse_listeners: FxHashMap<TypeId, Vec<(StackingOrder, AnyMouseListener)>>,
     pub(crate) dispatch_tree: DispatchTree,
@@ -311,6 +312,7 @@ impl Frame {
     fn new(dispatch_tree: DispatchTree) -> Self {
         Frame {
             focus: None,
+            window_active: false,
             element_states: FxHashMap::default(),
             mouse_listeners: FxHashMap::default(),
             dispatch_tree,
@@ -417,7 +419,7 @@ impl Window {
             frame_arena: Arena::new(1024 * 1024),
             focus_handles: Arc::new(RwLock::new(SlotMap::with_key())),
             focus_listeners: SubscriberSet::new(),
-            blur_listeners: SubscriberSet::new(),
+            focus_lost_listeners: SubscriberSet::new(),
             default_prevented: true,
             mouse_position,
             modifiers,
@@ -1406,29 +1408,14 @@ impl<'a> WindowContext<'a> {
                 self.window.focus,
             );
         self.window.next_frame.focus = self.window.focus;
+        self.window.next_frame.window_active = self.window.active;
         self.window.root_view = Some(root_view);
 
         let previous_focus_path = self.window.rendered_frame.focus_path();
+        let previous_window_active = self.window.rendered_frame.window_active;
         mem::swap(&mut self.window.rendered_frame, &mut self.window.next_frame);
         let current_focus_path = self.window.rendered_frame.focus_path();
-
-        if previous_focus_path != current_focus_path {
-            if !previous_focus_path.is_empty() && current_focus_path.is_empty() {
-                self.window
-                    .blur_listeners
-                    .clone()
-                    .retain(&(), |listener| listener(self));
-            }
-
-            let event = FocusEvent {
-                previous_focus_path,
-                current_focus_path,
-            };
-            self.window
-                .focus_listeners
-                .clone()
-                .retain(&(), |listener| listener(&event, self));
-        }
+        let current_window_active = self.window.rendered_frame.window_active;
 
         let scene = self.window.rendered_frame.scene_builder.build();
 
@@ -1445,6 +1432,34 @@ impl<'a> WindowContext<'a> {
         self.window.drawing = false;
         ELEMENT_ARENA.with_borrow_mut(|element_arena| element_arena.clear());
 
+        if previous_focus_path != current_focus_path
+            || previous_window_active != current_window_active
+        {
+            if !previous_focus_path.is_empty() && current_focus_path.is_empty() {
+                self.window
+                    .focus_lost_listeners
+                    .clone()
+                    .retain(&(), |listener| listener(self));
+            }
+
+            let event = FocusEvent {
+                previous_focus_path: if previous_window_active {
+                    previous_focus_path
+                } else {
+                    Default::default()
+                },
+                current_focus_path: if current_window_active {
+                    current_focus_path
+                } else {
+                    Default::default()
+                },
+            };
+            self.window
+                .focus_listeners
+                .clone()
+                .retain(&(), |listener| listener(&event, self));
+        }
+
         scene
     }
 
@@ -2643,14 +2658,16 @@ impl<'a, V: 'static> ViewContext<'a, V> {
         subscription
     }
 
-    /// Register a listener to be called when the window loses focus.
+    /// Register a listener to be called when nothing in the window has focus.
+    /// This typically happens when the node that was focused is removed from the tree,
+    /// and this callback lets you chose a default place to restore the users focus.
     /// Returns a subscription and persists until the subscription is dropped.
-    pub fn on_blur_window(
+    pub fn on_focus_lost(
         &mut self,
         mut listener: impl FnMut(&mut V, &mut ViewContext<V>) + 'static,
     ) -> Subscription {
         let view = self.view.downgrade();
-        let (subscription, activate) = self.window.blur_listeners.insert(
+        let (subscription, activate) = self.window.focus_lost_listeners.insert(
             (),
             Box::new(move |cx| view.update(cx, |view, cx| listener(view, cx)).is_ok()),
         );

crates/vim/src/editor_events.rs 🔗

@@ -69,7 +69,7 @@ fn released(entity_id: EntityId, cx: &mut AppContext) {
 mod test {
     use crate::{test::VimTestContext, Vim};
     use editor::Editor;
-    use gpui::{Context, Entity};
+    use gpui::{Context, Entity, VisualTestContext};
     use language::Buffer;
 
     // regression test for blur called with a different active editor
@@ -82,11 +82,13 @@ mod test {
         let editor2 = cx
             .update(|cx| {
                 window2.update(cx, |_, cx| {
+                    cx.activate_window();
                     cx.focus_self();
                     cx.view().clone()
                 })
             })
             .unwrap();
+        cx.run_until_parked();
 
         cx.update(|cx| {
             let vim = Vim::read(cx);
@@ -101,4 +103,42 @@ mod test {
             editor1.handle_blur(cx);
         });
     }
+
+    // regression test for focus_in/focus_out being called on window activation
+    #[gpui::test]
+    async fn test_focus_across_windows(cx: &mut gpui::TestAppContext) {
+        let mut cx = VimTestContext::new(cx, true).await;
+
+        let mut cx1 = VisualTestContext::from_window(cx.window, &cx);
+        let editor1 = cx.editor.clone();
+        dbg!(editor1.entity_id());
+
+        let buffer = cx.new_model(|_| Buffer::new(0, 0, "a = 1\nb = 2\n"));
+        let (editor2, cx2) = cx.add_window_view(|cx| Editor::for_buffer(buffer, None, cx));
+
+        editor2.update(cx2, |_, cx| {
+            cx.focus_self();
+            cx.activate_window();
+        });
+        cx.run_until_parked();
+
+        cx1.update(|cx| {
+            assert_eq!(
+                Vim::read(cx).active_editor.as_ref().unwrap().entity_id(),
+                editor2.entity_id(),
+            )
+        });
+
+        cx1.update(|cx| {
+            cx.activate_window();
+        });
+        cx.run_until_parked();
+
+        cx.update(|cx| {
+            assert_eq!(
+                Vim::read(cx).active_editor.as_ref().unwrap().entity_id(),
+                editor1.entity_id(),
+            )
+        });
+    }
 }

crates/workspace/src/workspace.rs 🔗

@@ -537,7 +537,7 @@ impl Workspace {
         })
         .detach();
 
-        cx.on_blur_window(|this, cx| {
+        cx.on_focus_lost(|this, cx| {
             let focus_handle = this.focus_handle(cx);
             cx.focus(&focus_handle);
         })