Fire focus handlers on draw to avoid timing with newly created item

Julia and Antonio Scandurra created

Co-Authored-By: Antonio Scandurra <antonio@zed.dev>

Change summary

crates/gpui2/src/app.rs          |  68 +--------------
crates/gpui2/src/elements/div.rs |  13 --
crates/gpui2/src/interactive.rs  |   8 -
crates/gpui2/src/key_dispatch.rs |  20 ++++
crates/gpui2/src/window.rs       | 137 +++++++++++++--------------------
5 files changed, 84 insertions(+), 162 deletions(-)

Detailed changes

crates/gpui2/src/app.rs 🔗

@@ -17,11 +17,10 @@ use time::UtcOffset;
 use crate::{
     current_platform, image_cache::ImageCache, init_app_menus, Action, ActionRegistry, Any,
     AnyView, AnyWindowHandle, AppMetadata, AssetSource, BackgroundExecutor, ClipboardItem, Context,
-    DispatchPhase, DisplayId, Entity, EventEmitter, FocusEvent, FocusHandle, FocusId,
-    ForegroundExecutor, KeyBinding, Keymap, Keystroke, LayoutId, Menu, PathPromptOptions, Pixels,
-    Platform, PlatformDisplay, Point, Render, SharedString, SubscriberSet, Subscription,
-    SvgRenderer, Task, TextStyle, TextStyleRefinement, TextSystem, View, ViewContext, Window,
-    WindowContext, WindowHandle, WindowId,
+    DispatchPhase, DisplayId, Entity, EventEmitter, ForegroundExecutor, KeyBinding, Keymap,
+    Keystroke, LayoutId, Menu, PathPromptOptions, Pixels, Platform, PlatformDisplay, Point, Render,
+    SharedString, SubscriberSet, Subscription, SvgRenderer, Task, TextStyle, TextStyleRefinement,
+    TextSystem, View, ViewContext, Window, WindowContext, WindowHandle, WindowId,
 };
 use anyhow::{anyhow, Result};
 use collections::{FxHashMap, FxHashSet, VecDeque};
@@ -577,23 +576,21 @@ impl AppContext {
                     Effect::Notify { emitter } => {
                         self.apply_notify_effect(emitter);
                     }
+
                     Effect::Emit {
                         emitter,
                         event_type,
                         event,
                     } => self.apply_emit_effect(emitter, event_type, event),
-                    Effect::FocusChanged {
-                        window_handle,
-                        focused,
-                    } => {
-                        self.apply_focus_changed_effect(window_handle, focused);
-                    }
+
                     Effect::Refresh => {
                         self.apply_refresh_effect();
                     }
+
                     Effect::NotifyGlobalObservers { global_type } => {
                         self.apply_notify_global_observers_effect(global_type);
                     }
+
                     Effect::Defer { callback } => {
                         self.apply_defer_effect(callback);
                     }
@@ -693,51 +690,6 @@ impl AppContext {
             });
     }
 
-    fn apply_focus_changed_effect(
-        &mut self,
-        window_handle: AnyWindowHandle,
-        focused: Option<FocusId>,
-    ) {
-        window_handle
-            .update(self, |_, cx| {
-                // The window might change focus multiple times in an effect cycle.
-                // We only honor effects for the most recently focused handle.
-                if cx.window.focus == focused {
-                    // if someone calls focus multiple times in one frame with the same handle
-                    // the first apply_focus_changed_effect will have taken the last blur already
-                    // and run the rest of this, so we can return.
-                    let Some(last_blur) = cx.window.last_blur.take() else {
-                        return;
-                    };
-
-                    let focused = focused
-                        .map(|id| FocusHandle::for_id(id, &cx.window.focus_handles).unwrap());
-
-                    let blurred =
-                        last_blur.and_then(|id| FocusHandle::for_id(id, &cx.window.focus_handles));
-
-                    let focus_changed = focused.is_some() || blurred.is_some();
-                    let event = FocusEvent { focused, blurred };
-
-                    let mut listeners = mem::take(&mut cx.window.rendered_frame.focus_listeners);
-                    if focus_changed {
-                        for listener in &mut listeners {
-                            listener(&event, cx);
-                        }
-                    }
-                    cx.window.rendered_frame.focus_listeners = listeners;
-
-                    if focus_changed {
-                        cx.window
-                            .focus_listeners
-                            .clone()
-                            .retain(&(), |listener| listener(&event, cx));
-                    }
-                }
-            })
-            .ok();
-    }
-
     fn apply_refresh_effect(&mut self) {
         for window in self.windows.values_mut() {
             if let Some(window) = window.as_mut() {
@@ -1246,10 +1198,6 @@ pub(crate) enum Effect {
         event_type: TypeId,
         event: Box<dyn Any>,
     },
-    FocusChanged {
-        window_handle: AnyWindowHandle,
-        focused: Option<FocusId>,
-    },
     Refresh,
     NotifyGlobalObservers {
         global_type: TypeId,

crates/gpui2/src/elements/div.rs 🔗

@@ -1,10 +1,9 @@
 use crate::{
     point, px, Action, AnyDrag, AnyElement, AnyTooltip, AnyView, AppContext, BorrowAppContext,
-    BorrowWindow, Bounds, ClickEvent, DispatchPhase, Element, ElementId, FocusEvent, FocusHandle,
-    IntoElement, KeyContext, KeyDownEvent, KeyUpEvent, LayoutId, MouseButton, MouseDownEvent,
-    MouseMoveEvent, MouseUpEvent, ParentElement, Pixels, Point, Render, ScrollWheelEvent,
-    SharedString, Size, StackingOrder, Style, StyleRefinement, Styled, Task, View, Visibility,
-    WindowContext,
+    BorrowWindow, Bounds, ClickEvent, DispatchPhase, Element, ElementId, FocusHandle, IntoElement,
+    KeyContext, KeyDownEvent, KeyUpEvent, LayoutId, MouseButton, MouseDownEvent, MouseMoveEvent,
+    MouseUpEvent, ParentElement, Pixels, Point, Render, ScrollWheelEvent, SharedString, Size,
+    StackingOrder, Style, StyleRefinement, Styled, Task, View, Visibility, WindowContext,
 };
 use collections::HashMap;
 use refineable::Refineable;
@@ -462,10 +461,6 @@ pub trait FocusableElement: InteractiveElement {
     }
 }
 
-pub type FocusListeners = Vec<FocusListener>;
-
-pub type FocusListener = Box<dyn Fn(&FocusHandle, &FocusEvent, &mut WindowContext) + 'static>;
-
 pub type MouseDownListener =
     Box<dyn Fn(&MouseDownEvent, &InteractiveBounds, DispatchPhase, &mut WindowContext) + 'static>;
 pub type MouseUpListener =

crates/gpui2/src/interactive.rs 🔗

@@ -1,6 +1,5 @@
 use crate::{
-    div, point, Div, Element, FocusHandle, IntoElement, Keystroke, Modifiers, Pixels, Point,
-    Render, ViewContext,
+    div, point, Div, Element, IntoElement, Keystroke, Modifiers, Pixels, Point, Render, ViewContext,
 };
 use smallvec::SmallVec;
 use std::{any::Any, fmt::Debug, marker::PhantomData, ops::Deref, path::PathBuf};
@@ -290,11 +289,6 @@ impl InputEvent {
     }
 }
 
-pub struct FocusEvent {
-    pub blurred: Option<FocusHandle>,
-    pub focused: Option<FocusHandle>,
-}
-
 #[cfg(test)]
 mod test {
     use crate::{

crates/gpui2/src/key_dispatch.rs 🔗

@@ -29,6 +29,7 @@ pub(crate) struct DispatchNode {
     pub key_listeners: Vec<KeyListener>,
     pub action_listeners: Vec<DispatchActionListener>,
     pub context: Option<KeyContext>,
+    focus_id: Option<FocusId>,
     parent: Option<DispatchNodeId>,
 }
 
@@ -127,8 +128,9 @@ impl DispatchTree {
     }
 
     pub fn make_focusable(&mut self, focus_id: FocusId) {
-        self.focusable_node_ids
-            .insert(focus_id, self.active_node_id());
+        let node_id = self.active_node_id();
+        self.active_node().focus_id = Some(focus_id);
+        self.focusable_node_ids.insert(focus_id, node_id);
     }
 
     pub fn focus_contains(&self, parent: FocusId, child: FocusId) -> bool {
@@ -247,6 +249,20 @@ impl DispatchTree {
         dispatch_path
     }
 
+    pub fn focus_path(&self, focus_id: FocusId) -> SmallVec<[FocusId; 8]> {
+        let mut focus_path: SmallVec<[FocusId; 8]> = SmallVec::new();
+        let mut current_node_id = self.focusable_node_ids.get(&focus_id).copied();
+        while let Some(node_id) = current_node_id {
+            let node = self.node(node_id);
+            if let Some(focus_id) = node.focus_id {
+                focus_path.push(focus_id);
+            }
+            current_node_id = node.parent;
+        }
+        focus_path.reverse(); // Reverse the path so it goes from the root to the focused node.
+        focus_path
+    }
+
     pub fn node(&self, node_id: DispatchNodeId) -> &DispatchNode {
         &self.nodes[node_id.0]
     }

crates/gpui2/src/window.rs 🔗

@@ -2,10 +2,10 @@ use crate::{
     key_dispatch::DispatchActionListener, px, size, Action, AnyDrag, AnyView, AppContext,
     AsyncWindowContext, AvailableSpace, Bounds, BoxShadow, Context, Corners, CursorStyle,
     DevicePixels, DispatchNodeId, DispatchTree, DisplayId, Edges, Effect, Entity, EntityId,
-    EventEmitter, FileDropEvent, Flatten, FocusEvent, 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,
+    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,
@@ -74,9 +74,13 @@ impl DispatchPhase {
 
 type AnyObserver = Box<dyn FnMut(&mut WindowContext) -> bool + 'static>;
 type AnyMouseListener = Box<dyn FnMut(&dyn Any, DispatchPhase, &mut WindowContext) + 'static>;
-type AnyFocusListener = Box<dyn Fn(&FocusEvent, &mut WindowContext) + 'static>;
 type AnyWindowFocusListener = Box<dyn FnMut(&FocusEvent, &mut WindowContext) -> bool + 'static>;
 
+struct FocusEvent {
+    previous_focus_path: SmallVec<[FocusId; 8]>,
+    current_focus_path: SmallVec<[FocusId; 8]>,
+}
+
 slotmap::new_key_type! { pub struct FocusId; }
 
 impl FocusId {
@@ -227,8 +231,8 @@ pub struct Window {
     pub(crate) rendered_frame: Frame,
     pub(crate) next_frame: Frame,
     pub(crate) focus_handles: Arc<RwLock<SlotMap<FocusId, AtomicUsize>>>,
-    pub(crate) focus_listeners: SubscriberSet<(), AnyWindowFocusListener>,
-    pub(crate) blur_listeners: SubscriberSet<(), AnyObserver>,
+    focus_listeners: SubscriberSet<(), AnyWindowFocusListener>,
+    blur_listeners: SubscriberSet<(), AnyObserver>,
     default_prevented: bool,
     mouse_position: Point<Pixels>,
     requested_cursor_style: Option<CursorStyle>,
@@ -238,7 +242,6 @@ pub struct Window {
     active: bool,
     pub(crate) dirty: bool,
     activation_observers: SubscriberSet<(), AnyObserver>,
-    pub(crate) last_blur: Option<Option<FocusId>>,
     pub(crate) focus: Option<FocusId>,
 }
 
@@ -250,10 +253,10 @@ pub(crate) struct ElementStateBox {
 
 // #[derive(Default)]
 pub(crate) struct Frame {
+    focus: Option<FocusId>,
     pub(crate) element_states: HashMap<GlobalElementId, ElementStateBox>,
     mouse_listeners: HashMap<TypeId, Vec<(StackingOrder, AnyMouseListener)>>,
     pub(crate) dispatch_tree: DispatchTree,
-    pub(crate) focus_listeners: Vec<AnyFocusListener>,
     pub(crate) scene_builder: SceneBuilder,
     pub(crate) depth_map: Vec<(StackingOrder, Bounds<Pixels>)>,
     pub(crate) z_index_stack: StackingOrder,
@@ -264,10 +267,10 @@ pub(crate) struct Frame {
 impl Frame {
     fn new(dispatch_tree: DispatchTree) -> Self {
         Frame {
+            focus: None,
             element_states: HashMap::default(),
             mouse_listeners: HashMap::default(),
             dispatch_tree,
-            focus_listeners: Vec::new(),
             scene_builder: SceneBuilder::default(),
             z_index_stack: StackingOrder::default(),
             depth_map: Default::default(),
@@ -279,10 +282,15 @@ impl Frame {
     fn clear(&mut self) {
         self.element_states.clear();
         self.mouse_listeners.values_mut().for_each(Vec::clear);
-        self.focus_listeners.clear();
         self.dispatch_tree.clear();
         self.depth_map.clear();
     }
+
+    fn focus_path(&self) -> SmallVec<[FocusId; 8]> {
+        self.focus
+            .map(|focus_id| self.dispatch_tree.focus_path(focus_id))
+            .unwrap_or_default()
+    }
 }
 
 impl Window {
@@ -372,7 +380,6 @@ impl Window {
             active: false,
             dirty: false,
             activation_observers: SubscriberSet::new(),
-            last_blur: None,
             focus: None,
         }
     }
@@ -449,35 +456,17 @@ impl<'a> WindowContext<'a> {
             return;
         }
 
-        let focus_id = handle.id;
-
-        if self.window.last_blur.is_none() {
-            self.window.last_blur = Some(self.window.focus);
-        }
-
-        self.window.focus = Some(focus_id);
+        self.window.focus = Some(handle.id);
         self.window
             .rendered_frame
             .dispatch_tree
             .clear_pending_keystrokes();
-        self.app.push_effect(Effect::FocusChanged {
-            window_handle: self.window.handle,
-            focused: Some(focus_id),
-        });
         self.notify();
     }
 
     /// Remove focus from all elements within this context's window.
     pub fn blur(&mut self) {
-        if self.window.last_blur.is_none() {
-            self.window.last_blur = Some(self.window.focus);
-        }
-
         self.window.focus = None;
-        self.app.push_effect(Effect::FocusChanged {
-            window_handle: self.window.handle,
-            focused: None,
-        });
         self.notify();
     }
 
@@ -1236,16 +1225,6 @@ impl<'a> WindowContext<'a> {
 
     /// Draw pixels to the display for this window based on the contents of its scene.
     pub(crate) fn draw(&mut self) -> Scene {
-        let window_was_focused = self
-            .window
-            .focus
-            .and_then(|focus_id| {
-                self.window
-                    .rendered_frame
-                    .dispatch_tree
-                    .focusable_node_id(focus_id)
-            })
-            .is_some();
         self.text_system().start_frame();
         self.window.platform_window.clear_input_handler();
         self.window.layout_engine.as_mut().unwrap().clear();
@@ -1284,23 +1263,6 @@ impl<'a> WindowContext<'a> {
             });
         }
 
-        let window_is_focused = self
-            .window
-            .focus
-            .and_then(|focus_id| {
-                self.window
-                    .next_frame
-                    .dispatch_tree
-                    .focusable_node_id(focus_id)
-            })
-            .is_some();
-        if window_was_focused && !window_is_focused {
-            self.window
-                .blur_listeners
-                .clone()
-                .retain(&(), |listener| listener(self));
-        }
-
         self.window
             .next_frame
             .dispatch_tree
@@ -1308,11 +1270,34 @@ impl<'a> WindowContext<'a> {
                 &mut self.window.rendered_frame.dispatch_tree,
                 self.window.focus,
             );
+        self.window.next_frame.focus = self.window.focus;
         self.window.root_view = Some(root_view);
 
+        let previous_focus_path = self.window.rendered_frame.focus_path();
+
         let window = &mut self.window;
         mem::swap(&mut window.rendered_frame, &mut 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 scene = self.window.rendered_frame.scene_builder.build();
 
         // Set the cursor only if we're the active window.
@@ -1699,22 +1684,6 @@ impl<'a> WindowContext<'a> {
         result
     }
 
-    /// Register a focus listener for the next frame only. It will be cleared
-    /// on the next frame render. You should use this method only from within elements,
-    /// and we may want to enforce that better via a different context type.
-    // todo!() Move this to `FrameContext` to emphasize its individuality?
-    pub fn on_focus_changed(
-        &mut self,
-        listener: impl Fn(&FocusEvent, &mut WindowContext) + 'static,
-    ) {
-        self.window
-            .next_frame
-            .focus_listeners
-            .push(Box::new(move |event, cx| {
-                listener(event, cx);
-            }));
-    }
-
     /// Set an input handler, such as [ElementInputHandler], which interfaces with the
     /// platform to receive textual input with proper integration with concerns such
     /// as IME interactions.
@@ -2389,7 +2358,9 @@ impl<'a, V: 'static> ViewContext<'a, V> {
             (),
             Box::new(move |event, cx| {
                 view.update(cx, |view, cx| {
-                    if event.focused.as_ref().map(|focused| focused.id) == Some(focus_id) {
+                    if event.previous_focus_path.last() != Some(&focus_id)
+                        && event.current_focus_path.last() == Some(&focus_id)
+                    {
                         listener(view, cx)
                     }
                 })
@@ -2414,10 +2385,8 @@ impl<'a, V: 'static> ViewContext<'a, V> {
             (),
             Box::new(move |event, cx| {
                 view.update(cx, |view, cx| {
-                    if event
-                        .focused
-                        .as_ref()
-                        .map_or(false, |focused| focus_id.contains(focused.id, cx))
+                    if !event.previous_focus_path.contains(&focus_id)
+                        && event.current_focus_path.contains(&focus_id)
                     {
                         listener(view, cx)
                     }
@@ -2443,7 +2412,9 @@ impl<'a, V: 'static> ViewContext<'a, V> {
             (),
             Box::new(move |event, cx| {
                 view.update(cx, |view, cx| {
-                    if event.blurred.as_ref().map(|blurred| blurred.id) == Some(focus_id) {
+                    if event.previous_focus_path.last() == Some(&focus_id)
+                        && event.current_focus_path.last() != Some(&focus_id)
+                    {
                         listener(view, cx)
                     }
                 })
@@ -2484,10 +2455,8 @@ impl<'a, V: 'static> ViewContext<'a, V> {
             (),
             Box::new(move |event, cx| {
                 view.update(cx, |view, cx| {
-                    if event
-                        .blurred
-                        .as_ref()
-                        .map_or(false, |blurred| focus_id.contains(blurred.id, cx))
+                    if event.previous_focus_path.contains(&focus_id)
+                        && !event.current_focus_path.contains(&focus_id)
                     {
                         listener(view, cx)
                     }