From c4203868ea101f3232901aaa554e22dfa0461816 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 14 Apr 2022 07:53:15 -0600 Subject: [PATCH 1/5] Revert "Focus Project Search query editor always when deployed" --- crates/search/src/project_search.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/search/src/project_search.rs b/crates/search/src/project_search.rs index 7ed4e802b3..7d11fcbea5 100644 --- a/crates/search/src/project_search.rs +++ b/crates/search/src/project_search.rs @@ -170,7 +170,11 @@ impl View for ProjectSearchView { .insert(self.model.read(cx).project.downgrade(), handle) }); - self.focus_query_editor(cx); + if self.model.read(cx).match_ranges.is_empty() { + cx.focus(&self.query_editor); + } else { + self.focus_results_editor(cx); + } } } @@ -394,7 +398,6 @@ impl ProjectSearchView { if let Some(existing) = existing { workspace.activate_item(&existing, cx); - existing.update(cx, |existing, cx| existing.focus_query_editor(cx)); } else { let model = cx.add_model(|cx| ProjectSearch::new(workspace.project().clone(), cx)); workspace.add_item( From 27057fdb1b25af3efd4b5e29d6b168f10a76a82b Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 14 Apr 2022 08:52:24 -0600 Subject: [PATCH 2/5] Only process a single focus effect per batch This allows us to focus the query editor of the project search when deploying it. Previously, a complex interplay between focus events was preventing this from working in an intuitive way. What happened previously: - We'd activate the project search, which enqueued a focus effect for the project search view - We'd focus the query editor, which enqueued an effect - We'd process the focus effect for the search view, which would enqueue an effect to transfer focus to the results editor - We'd process the effect to focus the query editor - We'd process the effect to focus the results editor Now... - We activate the project search pane item, enqueuing a focus effect for the project search itself - We focus the query editor and *remove* the previous pending focus change effect - We process the focus effect --- crates/gpui/src/app.rs | 61 +++++++++++++++++------------ crates/search/src/project_search.rs | 3 ++ 2 files changed, 40 insertions(+), 24 deletions(-) diff --git a/crates/gpui/src/app.rs b/crates/gpui/src/app.rs index eb97b85c67..b70602f2ca 100644 --- a/crates/gpui/src/app.rs +++ b/crates/gpui/src/app.rs @@ -745,6 +745,7 @@ pub struct MutableAppContext { HashMap>, Box)>, foreground: Rc, pending_effects: VecDeque, + pending_focus_index: Option, pending_notifications: HashSet, pending_global_notifications: HashSet, pending_flushes: usize, @@ -795,6 +796,7 @@ impl MutableAppContext { presenters_and_platform_windows: HashMap::new(), foreground, pending_effects: VecDeque::new(), + pending_focus_index: None, pending_notifications: HashSet::new(), pending_global_notifications: HashSet::new(), pending_flushes: 0, @@ -1670,7 +1672,7 @@ impl MutableAppContext { }); if let Some(view_id) = change_focus_to { - self.focus(window_id, Some(view_id)); + self.handle_focus_effect(window_id, Some(view_id)); } self.pending_effects @@ -1693,6 +1695,9 @@ impl MutableAppContext { let mut refreshing = false; loop { if let Some(effect) = self.pending_effects.pop_front() { + if let Some(pending_focus_index) = self.pending_focus_index.as_mut() { + *pending_focus_index = pending_focus_index.saturating_sub(1); + } match effect { Effect::Subscription { entity_id, @@ -1716,13 +1721,13 @@ impl MutableAppContext { callback, } => self.handle_observation_effect(entity_id, subscription_id, callback), Effect::ModelNotification { model_id } => { - self.notify_model_observers(model_id) + self.handle_model_notification_effect(model_id) } Effect::ViewNotification { window_id, view_id } => { - self.notify_view_observers(window_id, view_id) + self.handle_view_notification_effect(window_id, view_id) } Effect::GlobalNotification { type_id } => { - self.notify_global_observers(type_id) + self.handle_global_notification_effect(type_id) } Effect::Deferred { callback, @@ -1735,13 +1740,13 @@ impl MutableAppContext { } } Effect::ModelRelease { model_id, model } => { - self.notify_release_observers(model_id, model.as_any()) + self.handle_entity_release_effect(model_id, model.as_any()) } Effect::ViewRelease { view_id, view } => { - self.notify_release_observers(view_id, view.as_any()) + self.handle_entity_release_effect(view_id, view.as_any()) } Effect::Focus { window_id, view_id } => { - self.focus(window_id, view_id); + self.handle_focus_effect(window_id, view_id); } Effect::ResizeWindow { window_id } => { if let Some(window) = self.cx.windows.get_mut(&window_id) { @@ -1973,7 +1978,7 @@ impl MutableAppContext { } } - fn notify_model_observers(&mut self, observed_id: usize) { + fn handle_model_notification_effect(&mut self, observed_id: usize) { let callbacks = self.observations.lock().remove(&observed_id); if let Some(callbacks) = callbacks { if self.cx.models.contains_key(&observed_id) { @@ -2002,7 +2007,11 @@ impl MutableAppContext { } } - fn notify_view_observers(&mut self, observed_window_id: usize, observed_view_id: usize) { + fn handle_view_notification_effect( + &mut self, + observed_window_id: usize, + observed_view_id: usize, + ) { if let Some(window) = self.cx.windows.get_mut(&observed_window_id) { window .invalidation @@ -2043,7 +2052,7 @@ impl MutableAppContext { } } - fn notify_global_observers(&mut self, observed_type_id: TypeId) { + fn handle_global_notification_effect(&mut self, observed_type_id: TypeId) { let callbacks = self.global_observations.lock().remove(&observed_type_id); if let Some(callbacks) = callbacks { if let Some(global) = self.cx.globals.remove(&observed_type_id) { @@ -2071,7 +2080,7 @@ impl MutableAppContext { } } - fn notify_release_observers(&mut self, entity_id: usize, entity: &dyn Any) { + fn handle_entity_release_effect(&mut self, entity_id: usize, entity: &dyn Any) { let callbacks = self.release_observations.lock().remove(&entity_id); if let Some(callbacks) = callbacks { for (_, mut callback) in callbacks { @@ -2080,7 +2089,9 @@ impl MutableAppContext { } } - fn focus(&mut self, window_id: usize, focused_id: Option) { + fn handle_focus_effect(&mut self, window_id: usize, focused_id: Option) { + self.pending_focus_index.take(); + if self .cx .windows @@ -2091,6 +2102,8 @@ impl MutableAppContext { return; } + log::info!("process focus effect {:?}", focused_id); + self.update(|this| { let blurred_id = this.cx.windows.get_mut(&window_id).and_then(|window| { let blurred_id = window.focused_view_id; @@ -2114,6 +2127,15 @@ impl MutableAppContext { }) } + fn focus(&mut self, window_id: usize, view_id: Option) { + if let Some(pending_focus_index) = self.pending_focus_index { + self.pending_effects.remove(pending_focus_index); + } + self.pending_focus_index = Some(self.pending_effects.len()); + self.pending_effects + .push_back(Effect::Focus { window_id, view_id }); + } + pub fn spawn(&self, f: F) -> Task where F: FnOnce(AsyncAppContext) -> Fut, @@ -2948,24 +2970,15 @@ impl<'a, T: View> ViewContext<'a, T> { S: Into, { let handle = handle.into(); - self.app.pending_effects.push_back(Effect::Focus { - window_id: handle.window_id, - view_id: Some(handle.view_id), - }); + self.app.focus(handle.window_id, Some(handle.view_id)); } pub fn focus_self(&mut self) { - self.app.pending_effects.push_back(Effect::Focus { - window_id: self.window_id, - view_id: Some(self.view_id), - }); + self.app.focus(self.window_id, Some(self.view_id)); } pub fn blur(&mut self) { - self.app.pending_effects.push_back(Effect::Focus { - window_id: self.window_id, - view_id: None, - }); + self.app.focus(self.window_id, None); } pub fn add_model(&mut self, build_model: F) -> ModelHandle diff --git a/crates/search/src/project_search.rs b/crates/search/src/project_search.rs index 7d11fcbea5..2d1d6c780f 100644 --- a/crates/search/src/project_search.rs +++ b/crates/search/src/project_search.rs @@ -398,6 +398,9 @@ impl ProjectSearchView { if let Some(existing) = existing { workspace.activate_item(&existing, cx); + existing.update(cx, |existing, cx| { + existing.focus_query_editor(cx); + }); } else { let model = cx.add_model(|cx| ProjectSearch::new(workspace.project().clone(), cx)); workspace.add_item( From 5a8297a02ffe5e8eca8040af8178eafe7dbeed50 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Thu, 14 Apr 2022 18:22:32 +0200 Subject: [PATCH 3/5] Introduce `ViewContext::observe_focus` --- crates/gpui/src/app.rs | 196 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 189 insertions(+), 7 deletions(-) diff --git a/crates/gpui/src/app.rs b/crates/gpui/src/app.rs index b70602f2ca..d553b9ba26 100644 --- a/crates/gpui/src/app.rs +++ b/crates/gpui/src/app.rs @@ -716,6 +716,7 @@ type GlobalActionCallback = dyn FnMut(&dyn Action, &mut MutableAppContext); type SubscriptionCallback = Box bool>; type GlobalSubscriptionCallback = Box; type ObservationCallback = Box bool>; +type FocusObservationCallback = Box bool>; type GlobalObservationCallback = Box; type ReleaseObservationCallback = Box; type DeserializeActionCallback = fn(json: &str) -> anyhow::Result>; @@ -738,6 +739,8 @@ pub struct MutableAppContext { global_subscriptions: Arc>>>>, observations: Arc>>>>, + focus_observations: + Arc>>>>, global_observations: Arc>>>>, release_observations: Arc>>>, @@ -791,6 +794,7 @@ impl MutableAppContext { subscriptions: Default::default(), global_subscriptions: Default::default(), observations: Default::default(), + focus_observations: Default::default(), release_observations: Default::default(), global_observations: Default::default(), presenters_and_platform_windows: HashMap::new(), @@ -1185,6 +1189,32 @@ impl MutableAppContext { } } + fn observe_focus(&mut self, handle: &ViewHandle, mut callback: F) -> Subscription + where + F: 'static + FnMut(ViewHandle, &mut MutableAppContext) -> bool, + V: View, + { + let subscription_id = post_inc(&mut self.next_subscription_id); + let observed = handle.downgrade(); + let view_id = handle.id(); + self.pending_effects.push_back(Effect::FocusObservation { + view_id, + subscription_id, + callback: Box::new(move |cx| { + if let Some(observed) = observed.upgrade(cx) { + callback(observed, cx) + } else { + false + } + }), + }); + Subscription::FocusObservation { + id: subscription_id, + view_id, + observations: Some(Arc::downgrade(&self.focus_observations)), + } + } + pub fn observe_global(&mut self, mut observe: F) -> Subscription where G: Any, @@ -1748,6 +1778,13 @@ impl MutableAppContext { Effect::Focus { window_id, view_id } => { self.handle_focus_effect(window_id, view_id); } + Effect::FocusObservation { + view_id, + subscription_id, + callback, + } => { + self.handle_focus_observation_effect(view_id, subscription_id, callback) + } Effect::ResizeWindow { window_id } => { if let Some(window) = self.cx.windows.get_mut(&window_id) { window @@ -1978,6 +2015,30 @@ impl MutableAppContext { } } + fn handle_focus_observation_effect( + &mut self, + view_id: usize, + subscription_id: usize, + callback: FocusObservationCallback, + ) { + match self + .focus_observations + .lock() + .entry(view_id) + .or_default() + .entry(subscription_id) + { + btree_map::Entry::Vacant(entry) => { + entry.insert(Some(callback)); + } + // Observation was dropped before effect was processed + btree_map::Entry::Occupied(entry) => { + debug_assert!(entry.get().is_none()); + entry.remove(); + } + } + } + fn handle_model_notification_effect(&mut self, observed_id: usize) { let callbacks = self.observations.lock().remove(&observed_id); if let Some(callbacks) = callbacks { @@ -2102,8 +2163,6 @@ impl MutableAppContext { return; } - log::info!("process focus effect {:?}", focused_id); - self.update(|this| { let blurred_id = this.cx.windows.get_mut(&window_id).and_then(|window| { let blurred_id = window.focused_view_id; @@ -2122,6 +2181,31 @@ impl MutableAppContext { if let Some(mut focused_view) = this.cx.views.remove(&(window_id, focused_id)) { focused_view.on_focus(this, window_id, focused_id); this.cx.views.insert((window_id, focused_id), focused_view); + + let callbacks = this.focus_observations.lock().remove(&focused_id); + if let Some(callbacks) = callbacks { + for (id, callback) in callbacks { + if let Some(mut callback) = callback { + let alive = callback(this); + if alive { + match this + .focus_observations + .lock() + .entry(focused_id) + .or_default() + .entry(id) + { + btree_map::Entry::Vacant(entry) => { + entry.insert(Some(callback)); + } + btree_map::Entry::Occupied(entry) => { + entry.remove(); + } + } + } + } + } + } } } }) @@ -2487,6 +2571,11 @@ pub enum Effect { window_id: usize, view_id: Option, }, + FocusObservation { + view_id: usize, + subscription_id: usize, + callback: FocusObservationCallback, + }, ResizeWindow { window_id: usize, }, @@ -2558,6 +2647,15 @@ impl Debug for Effect { .field("window_id", window_id) .field("view_id", view_id) .finish(), + Effect::FocusObservation { + view_id, + subscription_id, + .. + } => f + .debug_struct("Effect::FocusObservation") + .field("view_id", view_id) + .field("subscription_id", subscription_id) + .finish(), Effect::ResizeWindow { window_id } => f .debug_struct("Effect::RefreshWindow") .field("window_id", window_id) @@ -3045,6 +3143,24 @@ impl<'a, T: View> ViewContext<'a, T> { }) } + pub fn observe_focus(&mut self, handle: &ViewHandle, mut callback: F) -> Subscription + where + F: 'static + FnMut(&mut T, ViewHandle, &mut ViewContext), + V: View, + { + let observer = self.weak_handle(); + self.app.observe_focus(handle, move |observed, cx| { + if let Some(observer) = observer.upgrade(cx) { + observer.update(cx, |observer, cx| { + callback(observer, observed, cx); + }); + true + } else { + false + } + }) + } + pub fn observe_release(&mut self, handle: &H, mut callback: F) -> Subscription where E: Entity, @@ -4309,6 +4425,12 @@ pub enum Subscription { Weak>>>>, >, }, + FocusObservation { + id: usize, + view_id: usize, + observations: + Option>>>>>, + }, ReleaseObservation { id: usize, entity_id: usize, @@ -4335,6 +4457,9 @@ impl Subscription { Subscription::ReleaseObservation { observations, .. } => { observations.take(); } + Subscription::FocusObservation { observations, .. } => { + observations.take(); + } } } } @@ -4427,6 +4552,22 @@ impl Drop for Subscription { } } } + Subscription::FocusObservation { + id, + view_id, + observations, + } => { + if let Some(observations) = observations.as_ref().and_then(Weak::upgrade) { + match observations.lock().entry(*view_id).or_default().entry(*id) { + btree_map::Entry::Vacant(entry) => { + entry.insert(None); + } + btree_map::Entry::Occupied(entry) => { + entry.remove(); + } + } + } + } } } } @@ -5715,23 +5856,56 @@ mod tests { } } - let events: Arc>> = Default::default(); + let view_events: Arc>> = Default::default(); let (window_id, view_1) = cx.add_window(Default::default(), |_| View { - events: events.clone(), + events: view_events.clone(), name: "view 1".to_string(), }); let view_2 = cx.add_view(window_id, |_| View { - events: events.clone(), + events: view_events.clone(), name: "view 2".to_string(), }); - view_1.update(cx, |_, cx| cx.focus(&view_2)); + let observed_events: Arc>> = Default::default(); + view_1.update(cx, |_, cx| { + cx.observe_focus(&view_2, { + let observed_events = observed_events.clone(); + move |this, view, cx| { + observed_events.lock().push(format!( + "{} observed {} focus", + this.name, + view.read(cx).name + )) + } + }) + .detach(); + }); + view_2.update(cx, |_, cx| { + cx.observe_focus(&view_1, { + let observed_events = observed_events.clone(); + move |this, view, cx| { + observed_events.lock().push(format!( + "{} observed {}'s focus", + this.name, + view.read(cx).name + )) + } + }) + .detach(); + }); + + view_1.update(cx, |_, cx| { + // Ensure only the latest focus is honored. + cx.focus(&view_2); + cx.focus(&view_1); + cx.focus(&view_2); + }); view_1.update(cx, |_, cx| cx.focus(&view_1)); view_1.update(cx, |_, cx| cx.focus(&view_2)); view_1.update(cx, |_, _| drop(view_2)); assert_eq!( - *events.lock(), + *view_events.lock(), [ "view 1 focused".to_string(), "view 1 blurred".to_string(), @@ -5743,6 +5917,14 @@ mod tests { "view 1 focused".to_string(), ], ); + assert_eq!( + *observed_events.lock(), + [ + "view 1 observed view 2's focus".to_string(), + "view 2 observed view 1's focus".to_string(), + "view 1 observed view 2's focus".to_string(), + ] + ); } #[crate::test(self)] From ce3a31d8bde0f86d964cc90f00c06c68d06e539e Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Thu, 14 Apr 2022 18:36:42 +0200 Subject: [PATCH 4/5] Persist project search focus state ...so that we can re-focus the previously-active editor when switching back to the project search tab. --- crates/search/src/project_search.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/crates/search/src/project_search.rs b/crates/search/src/project_search.rs index 2d1d6c780f..c49ca34052 100644 --- a/crates/search/src/project_search.rs +++ b/crates/search/src/project_search.rs @@ -55,6 +55,7 @@ pub struct ProjectSearchView { regex: bool, query_contains_error: bool, active_match_index: Option, + results_editor_was_focused: bool, } pub struct ProjectSearchBar { @@ -170,10 +171,10 @@ impl View for ProjectSearchView { .insert(self.model.read(cx).project.downgrade(), handle) }); - if self.model.read(cx).match_ranges.is_empty() { - cx.focus(&self.query_editor); - } else { + if self.results_editor_was_focused && !self.model.read(cx).match_ranges.is_empty() { self.focus_results_editor(cx); + } else { + cx.focus(&self.query_editor); } } } @@ -344,6 +345,10 @@ impl ProjectSearchView { cx.emit(ViewEvent::EditorEvent(event.clone())) }) .detach(); + cx.observe_focus(&query_editor, |this, _, _| { + this.results_editor_was_focused = false; + }) + .detach(); let results_editor = cx.add_view(|cx| { let mut editor = Editor::for_multibuffer(excerpts, Some(project), cx); @@ -352,6 +357,10 @@ impl ProjectSearchView { }); cx.observe(&results_editor, |_, _, cx| cx.emit(ViewEvent::UpdateTab)) .detach(); + cx.observe_focus(&results_editor, |this, _, _| { + this.results_editor_was_focused = true; + }) + .detach(); cx.subscribe(&results_editor, |this, _, event, cx| { if matches!(event, editor::Event::SelectionsChanged { .. }) { this.update_match_index(cx); @@ -370,6 +379,7 @@ impl ProjectSearchView { regex, query_contains_error: false, active_match_index: None, + results_editor_was_focused: false, }; this.model_changed(false, cx); this From 77d3cc359ef8e420db008a40ba2f2a3e7604992f Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Thu, 14 Apr 2022 18:50:45 +0200 Subject: [PATCH 5/5] Fix tests --- crates/gpui/src/app.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/gpui/src/app.rs b/crates/gpui/src/app.rs index d553b9ba26..acc84588b0 100644 --- a/crates/gpui/src/app.rs +++ b/crates/gpui/src/app.rs @@ -5872,7 +5872,7 @@ mod tests { let observed_events = observed_events.clone(); move |this, view, cx| { observed_events.lock().push(format!( - "{} observed {} focus", + "{} observed {}'s focus", this.name, view.read(cx).name ))