From 1bf33b4b61acfda1cf4ee0c6f67e4059bc6e839a Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Tue, 9 Jan 2024 20:31:13 -0700 Subject: [PATCH 1/2] Ensure focus_in and focus_out fire on window activation 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 --- crates/gpui/src/window.rs | 63 ++++++++++++++++++++----------- crates/vim/src/editor_events.rs | 40 +++++++++++++++++++- crates/workspace/src/workspace.rs | 2 +- 3 files changed, 80 insertions(+), 25 deletions(-) diff --git a/crates/gpui/src/window.rs b/crates/gpui/src/window.rs index ec4713639e930cd759a8af5cb4435ea588c5ec30..bd0e72054982d4d3760d0d06b32b17fe58c0c38f 100644 --- a/crates/gpui/src/window.rs +++ b/crates/gpui/src/window.rs @@ -269,7 +269,7 @@ pub struct Window { frame_arena: Arena, pub(crate) focus_handles: Arc>>, focus_listeners: SubscriberSet<(), AnyWindowFocusListener>, - blur_listeners: SubscriberSet<(), AnyObserver>, + focus_lost_listeners: SubscriberSet<(), AnyObserver>, default_prevented: bool, mouse_position: Point, modifiers: Modifiers, @@ -296,6 +296,7 @@ pub(crate) struct ElementStateBox { pub(crate) struct Frame { focus: Option, + window_active: bool, pub(crate) element_states: FxHashMap, mouse_listeners: FxHashMap>, 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 } @@ -2645,14 +2660,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) + '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()), ); diff --git a/crates/vim/src/editor_events.rs b/crates/vim/src/editor_events.rs index 3c2f373f9455209a36ef9dac88c3144f7f1a4bdd..e3c759aaebdcc7de27d593036a7046e4784269b1 100644 --- a/crates/vim/src/editor_events.rs +++ b/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 @@ -101,4 +101,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(), + ) + }); + } } diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index 09e0a1378d440e0bbf2eb2a7dc21ec88557fa320..ad74f44f17bb9775b74cde8ecbc32fc377f8503d 100644 --- a/crates/workspace/src/workspace.rs +++ b/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); }) From 72c022f4135e26359fb215feb1ab17b552bbd140 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Tue, 9 Jan 2024 21:37:25 -0700 Subject: [PATCH 2/2] Ensure focus-sensitive tests have active windows --- 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/vim/src/editor_events.rs | 2 ++ 4 files changed, 11 insertions(+), 1 deletion(-) diff --git a/crates/collab/src/tests/following_tests.rs b/crates/collab/src/tests/following_tests.rs index 0486e294619fd4fd34604c6cc4113fb03f1057ad..d50b34859d13c19d9cac7b7796ba3b840b2da3a9 100644 --- a/crates/collab/src/tests/following_tests.rs +++ b/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 diff --git a/crates/collab/src/tests/test_server.rs b/crates/collab/src/tests/test_server.rs index 034a85961f8e98966a2764c4196682fabf4b33cf..4f3634c7b1f9c9575a177810d9e5eb89ed12c891 100644 --- a/crates/collab/src/tests/test_server.rs +++ b/crates/collab/src/tests/test_server.rs @@ -617,7 +617,10 @@ impl TestClient { project: &Model, cx: &'a mut TestAppContext, ) -> (View, &'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) + }) } } diff --git a/crates/gpui/src/app/test_context.rs b/crates/gpui/src/app/test_context.rs index de31339b8d79bb3b4a80d4e8c9bc95834ffef92c..7ff751ef1ce7232e7714eac6bf2ddc87892a9e39 100644 --- a/crates/gpui/src/app/test_context.rs +++ b/crates/gpui/src/app/test_context.rs @@ -174,6 +174,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)) } diff --git a/crates/vim/src/editor_events.rs b/crates/vim/src/editor_events.rs index e3c759aaebdcc7de27d593036a7046e4784269b1..e3ed076698d101a12f92c16048748532ea596e1c 100644 --- a/crates/vim/src/editor_events.rs +++ b/crates/vim/src/editor_events.rs @@ -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);