diff --git a/VARIANT-NOTES.md b/VARIANT-NOTES.md new file mode 100644 index 00000000000000..0a8a1c88d8efe3 --- /dev/null +++ b/VARIANT-NOTES.md @@ -0,0 +1,139 @@ +# Variant C: centralized helper + +Branch: `focus-variant-c-helper`, based on `fix-layout-switch-cancels-rename`. + +## The abstraction + +Two subscription constructors on `gpui::Context`, in `crates/gpui/src/app/context.rs`, placed directly next to the two methods that carry the trap: + +```rust +pub fn on_blur_by_user( + &mut self, + handle: &FocusHandle, + window: &mut Window, + listener: impl FnMut(&mut T, &mut Window, &mut Context) + 'static, +) -> Subscription + +pub fn on_focus_out_by_user( + &mut self, + handle: &FocusHandle, + window: &mut Window, + listener: impl FnMut(&mut T, FocusOutEvent, &mut Window, &mut Context) + 'static, +) -> Subscription +``` + +Each is a three-line wrapper around its unguarded twin: + +```rust +self.on_blur(handle, window, move |view, window, cx| { + if window.is_window_active() { + listener(view, window, cx) + } +}) +``` + +GPUI's event semantics are untouched: no change to `WindowFocusEvent` synthesis in `Window::draw`, no change to `on_blur` / `on_focus_out` / `focus_lost_listeners`, no change to what any existing subscriber observes. The only additions are two constructors and doc text. + +### Why this shape + +Shape (a) from the brief - a subscription constructor - over (b) an "edit session" struct or (c) an extension trait. + +- (b) an edit-session struct owning focus handle + subscription + commit/cancel callbacks would have to model five genuinely different lifecycles: the project panel commits on blur but only when `processing_filename.is_none()`, the collab panel discards but only when there is no pending name, `GoToLine` emits `DismissEvent`, the picker cancels only when `is_modal`, and the terminal defers a frame and re-checks that the editor it captured is still the current one. The only thing all five share is _when_ to run, not _what_ to run. A struct would have ended up as a callback bag with one extra indirection. +- (c) an extension trait buys nothing over inherent methods, since `Context` is a gpui type and gpui is where the methods belong. + +### Why gpui and not `ui` or `workspace` + +The trap is `Context::on_blur`, which lives in gpui. Any helper that lives further out (in `ui` or `workspace`) is invisible at the moment a contributor types `cx.on_bl` and takes the completion - and it would be unreachable from `crates/picker` without a new dependency edge, and unreachable from gpui-only code entirely. Putting the correct constructor immediately below the incorrect one in the same `impl` block is the only placement where autocomplete and rustdoc do the teaching. gpui cannot depend on `workspace`, but nothing here needs to. + +### Why `is_window_active()` and not "the new focus path is non-empty" + +The purer-looking predicate - fire only when focus actually landed on something else in the window - would also suppress the callback when focus is cleared entirely while the window is still active (`Window::blur`, or the focused element being removed from the tree). Today, in an active window, that path _does_ commit a project-panel rename. Using `is_window_active()` makes every migrated call site byte-identical to the guarded code on the base branch, which matters a lot when the change cannot be compiled locally. See "Weaknesses" for what this costs. + +## Files touched + +| File | Why | +| ------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `crates/gpui/src/app/context.rs` | Adds `on_blur_by_user` and `on_focus_out_by_user`; adds warnings to the `on_blur` and `on_focus_out` doc comments pointing at them; adds two unit tests for the helper. | +| `crates/gpui/src/window.rs` | Doc-only: `Window::on_focus_out` gets the same warning and cross-reference. | +| `crates/project_panel/src/project_panel.rs` | Rename/create filename editor: `EditorEvent::Blurred` arm moved out of the `subscribe_in` match onto `on_blur_by_user`. | +| `crates/project_panel/src/project_panel_tests.rs` | Repairs `test_rename_survives_window_deactivation`, which was vacuous. See "Findings". | +| `crates/collab_ui/src/collab_panel.rs` | Channel rename editor: the whole `subscribe_in` (which only handled `Blurred`) becomes one `on_blur_by_user`. | +| `crates/go_to_line/src/go_to_line.rs` | `Blurred` arm split out of `on_line_editor_event` into `on_line_editor_blur`, registered with `on_blur_by_user`. | +| `crates/picker/src/head.rs` | `Head::editor` gains a `blur_handler` parameter routed through `on_blur_by_user`; `Head::empty` switches from `on_blur` to `on_blur_by_user`. | +| `crates/picker/src/picker.rs` | `Blurred` arm removed from `on_input_editor_event`, replaced by `on_input_editor_blur`; both blur handlers lose their `is_window_active()` guard. | +| `crates/terminal_view/src/terminal_view.rs` | Tab rename editor moved from `subscribe_in` to `on_blur_by_user`; its defer-and-recheck logic is kept verbatim. | +| `crates/ui_input/src/ui_input.rs` | Removes `ErasedEditorEvent::Blurred`, which became dead once the picker migrated, and documents why blur is deliberately not re-emitted here. | +| `crates/editor/src/editor.rs` | Drops the `EditorEvent::Blurred -> ErasedEditorEvent::Blurred` mapping line (falls through the existing `_ => return`). | +| `crates/workspace/src/modal_layer.rs` | `show_modal`'s `cx.on_focus_out` becomes `cx.on_focus_out_by_user`. | + +13 files including this one. + +## How each call site fit + +**`crates/picker/src/head.rs` `Head::empty` - perfect fit.** One identifier changed, `on_blur` to `on_blur_by_user`, and the `window.is_window_active()` line inside `Picker::on_empty_head_blur` deleted. This is the only site that was already subscribing to a focus handle, and it is the shape the helper was designed for. + +**`crates/workspace/src/modal_layer.rs` - perfect fit,** but only after I added `on_focus_out_by_user`. One identifier changed. This site is the reason the second constructor exists (see "Judgment calls"). + +**`crates/collab_ui/src/collab_panel.rs` - clean fit.** The `subscribe_in` existed solely to filter for `Blurred`, so the migration deletes a level of nesting and two early-return guards. + +**`crates/go_to_line/src/go_to_line.rs` - clean fit,** at the cost of splitting one handler method into two and adding an entry to `_subscriptions`. The remaining `on_line_editor_event` handles only `BufferEdited`, so its `match` collapsed to an `if let` (also avoids `clippy::single_match`). + +**`crates/project_panel/src/project_panel.rs` - acceptable fit, slightly awkward.** The editor subscription is a three-arm match (`BufferEdited`, `SelectionsChanged`, `Blurred`); pulling one arm out means the panel now has two subscriptions on the same editor for what a reader might think of as one concern, and the surviving closure's `window` parameter had to be renamed `_window`. The result is arguably clearer - the commit-on-blur logic is no longer buried in a match arm - but it is not a strict reduction in code. + +**`crates/terminal_view/src/terminal_view.rs` - partial fit, and diagnostic.** This site was already correct by accident, because after deferring a frame it re-checks `!rename_editor.focus_handle(cx).is_focused(window)`, and `Window::focus` is _not_ cleared on deactivation, so that check is genuinely window-activation-independent. Migrating removed the `if let EditorEvent::Blurred` wrapper but kept the defer, the `still_current` check, and the `is_focused` re-check verbatim, because those solve a _different_ problem: a double-click transiently blurs and refocuses within the window. So the helper subsumes exactly one of this site's three guards. That is honest evidence that "did the user really leave?" is not a single question, and a one-predicate helper cannot answer all of it. + +**`crates/picker/src/picker.rs` - the awkward one, and the riskiest.** The picker did not subscribe to a focus handle at all; it subscribed to `ErasedEditorEvent`, an erased re-emission of `EditorEvent::Blurred` that exists so `crates/picker` need not depend on `crates/editor`. The helper is invisible from behind that indirection. Making it fit required threading a second callback through `Head::editor` and deleting the `Blurred` variant from `ErasedEditorEvent` (the picker was its only consumer). That is the single place in this change where the convention became an actual constraint - a future `ui_input` consumer now _cannot_ hand-roll a blur subscription through the erased editor, because there is no blur event to subscribe to. It is also the change with the largest unverifiable blast radius, since every picker in the app goes through it. + +## Judgment calls + +1. **Two constructors, not one.** YAGNI argued for shipping only `on_blur_by_user`. I added `on_focus_out_by_user` because `modal_layer` is a concrete existing call site with a concrete latent bug, not a speculative future one: `ModalLayer::hide_modal` sets `dismiss_on_focus_lost = true` whenever a modal's `on_before_dismiss` returns `Dismiss(false)`, and `crates/file_finder/src/file_finder.rs:67` does exactly that while a submenu popover is focused. With that flag set, the old `on_focus_out` would hide the file finder when the user switched apps. Without the second constructor, `modal_layer` could not be migrated at all, and "does the abstraction fit the site the brief called the good prior art" would have gone unanswered. + +2. **`on_blur` semantics, not `on_focus_out` semantics, for the five editor sites.** `EditorEvent::Blurred` is emitted from `cx.on_blur(&editor.focus_handle)`, i.e. "this exact handle stopped being the innermost focused". `on_focus_out` fires for a subtree and would _not_ fire when, say, an editor's completion menu takes focus. Using `on_blur_by_user` preserves current behavior exactly. + +3. **Deleted `ErasedEditorEvent::Blurred` rather than leaving a dead variant.** After the picker migration nothing produced or consumed it usefully. Leaving it would have meant a permanently dead enum variant plus an `ErasedEditorEvent::Blurred => {}` arm in the picker. Cost: two extra files in the diff (`ui_input`, `editor`). + +4. **Left `crates/ui/src/components/context_menu.rs` alone,** even though it has four raw `cx.on_blur` subscriptions. See "Findings" - dismiss-on-deactivation is _wanted_ there, and it was tried and reverted for unrelated reasons. Converting it would be a behavior change nobody asked for. + +5. **Kept `is_modal` / `edit_state` / `pending_name` guards where they were.** The helper answers "did focus really leave?"; it deliberately does not try to own "should we then commit or cancel?". + +6. **Repaired the existing regression test instead of leaving it green-but-meaningless.** The brief said to keep it passing; leaving a test that cannot fail would have misrepresented this variant's evidence. + +## Findings (out of scope, reported not fixed) + +**The regression test on the base branch is vacuous.** `TestWindow::is_active()` in `crates/gpui/src/platform/test/window.rs:205` is hardcoded `false`, and `Window::new` seeds `active` from it (`crates/gpui/src/window.rs:1175`). Nothing activates a test window unless the test calls `window.activate_window()` explicitly, as `crates/zed/src/zed.rs:5282` does. So in `test_rename_survives_window_deactivation`, `VisualTestContext::deactivate_window` (`crates/gpui/src/app/test_context.rs:878`) hits its `if Some(self.window) == self.test_platform.active_window()` guard, does nothing, no blur is ever emitted, and the assertion passes with or without the fix. The test now activates the window first and asserts both that the window is active and that the filename editor holds focus before deactivating, so it can actually fail. **Any variant of this fix that relies on that test as evidence should re-check this.** + +**Window deactivation is sometimes a legitimate reason to dismiss.** PR #46866 deliberately made context menus close on deactivation via `observe_window_activation`, and #47044 tried to preserve and restore focus across activation cycles; both were reverted together in `ade8749537` ("Fix typing emoji") because they broke the macOS system emoji palette. Two lessons: dismiss-on-deactivation is a real requirement for some controls, so the right design keeps "focus moved" and "window went away" separately expressible rather than merging them; and globally changing focus behavior across activation has already blown up once in this codebase, which is a point in favor of this variant's conservatism and against any variant that changes `WindowFocusEvent` synthesis. + +**`crates/workspace/src/modal_layer.rs` is not the good prior art the brief assumes.** Its `on_focus_out` handler is not window-activation-aware at all. It looks correct only because `dismiss_on_focus_lost` is `false` in the common case, so the handler is usually a no-op. The actual prior art for the correct pattern is the `window.is_window_active()` guard added to `crates/picker/src/picker.rs` in #41320. + +**Dead commented-out code** at `crates/picker/src/head.rs:37-43` (an old `cx.subscribe_in` call). Left untouched to keep the diff focused. + +## Weaknesses - read this part + +**This is a convention, not a constraint, and mostly it stays one.** `cx.on_blur` and `cx.on_focus_out` still exist, still compile, and still do the wrong thing for any control that cancels on blur. `EditorEvent::Blurred` still exists and still fires on window deactivation, and there are twelve other subscribers to it that this change does not touch (`crates/vim/src/vim.rs:1093`, `crates/search/src/buffer_search.rs:1399`, `crates/agent_ui/src/inline_prompt_editor.rs:469`, `crates/diagnostics/src/diagnostics.rs:249`, `crates/rules_library/src/rules_library.rs:1028`, and others). Most of those are harmless - they toggle a boolean or hide a popover - but nothing in this change _tells_ the next contributor that. A new panel with a rename field, written by copying the collab panel from before this commit, is exactly as broken as it was. What was actually bought: + +- Three doc warnings that appear in autocomplete and rustdoc at the moment of choosing. Real, but easy to skim past. +- One genuine constraint, at `ErasedEditorEvent`: a `ui_input` consumer can no longer subscribe to blur, because the event is gone. This closes the bug class for that one channel only. +- Six call sites that now demonstrate the correct pattern rather than the guard-after-the-fact pattern. + +If the goal is "no future contributor can reintroduce this bug", this variant does not achieve it and cannot, short of removing or renaming `on_blur` - which would be a large, mechanical, and separately reviewable change (`cx.on_blur` has ~10 call sites outside gpui, but the equivalent surgery on `EditorEvent::Blurred` touches a dozen crates). The honest claim is narrower: it makes the correct pattern _cheaper to write than the incorrect one_ at the sites that matter, and leaves a trail of six examples plus three doc warnings for the next person. That is a real reduction in expected future bugs, not an elimination. + +**The predicate is coarser than its name.** `on_blur_by_user` fires for _any_ focus move inside an active window, including one made programmatically by a background task; and it suppresses genuine user-initiated blur that happens while the window is inactive (a click on an inactive window can move focus before activation lands, depending on platform). Naming it `by_user` states intent, not mechanism. `on_blur_while_window_active` would be honest and unusable as guidance. I chose intent and documented the mechanism. + +**Merging is not the same as fixing.** Because the helper is `is_window_active()` in a wrapper, it inherits every property of the hand-written guards, including that GPUI still blanks the focus path on deactivation. Anything that observes focus paths directly - `contains_focused`, `within_focused`, custom `new_focus_listener` users - still sees the blanked path and is unaffected by this change. This variant does not fix the underlying conflation; it makes it survivable at the sites that care. + +**The five editor migrations change callback ordering, and I could not verify it.** Previously the handlers ran as `EditorEvent::Blurred` subscribers, which fire during GPUI's effect flush, _after_ all focus listeners for that frame. They now run as focus listeners, synchronously inside `focus_listeners.retain(...)` in `Window::draw`, immediately after the editor's own `handle_blur`. Registration order means they still run after the editor's internal cleanup, but they now run _before_ other focus listeners registered later - most notably `ModalLayer`'s. For the picker that means `Picker::cancel` (and the `DismissEvent` it emits) now runs earlier relative to modal-layer bookkeeping and focus restoration. I reasoned through the modal-open path and expect no difference in outcome, since both paths converge on the same effect flush, but this is the single change I would most want exercised by hand in a real build: open the command palette, the file finder, and a file-finder submenu, and confirm dismissal and focus restoration still behave. + +**Nothing here was compiled.** Per the task constraints I ran no `cargo` command. Specific things a compiler would catch that I checked only by reading: the two-phase borrow in `cx.on_blur_by_user(&editor.focus_handle(cx), ...)` (precedent: the pre-existing `cx.on_blur(&head.focus_handle(cx), ...)` in `crates/picker/src/head.rs`); `Focusable` being in scope in each migrated file (verified by grep, each file already called `.focus_handle(cx)`); the new `#[cfg(test)] mod tests` in `crates/gpui/src/app/context.rs` importing `TestAppContext` and `add_window_view` (modelled on the existing test in `crates/gpui/src/elements/uniform_list.rs:709`); and clippy's `single_match` on the reduced matches. + +**The two new gpui tests are the least-verified code in the change.** They assert that `on_blur` fires and `on_blur_by_user` does not when `deactivate_window()` is called, and that both fire on an in-window focus move. The logic follows from reading `Window::draw`, but the test scaffolding (does a window with two `track_focus` divs produce the focus paths I expect after `run_until_parked`?) is exactly the kind of thing that is obvious once run and easy to get subtly wrong on paper. If CI fails, look here first. + +## Suggested `.rules` addition (not applied, per the repo's rules-hygiene policy) + +Proposed for `crates/gpui/.rules`: + +> Do not use `cx.on_blur` / `cx.on_focus_out` / `EditorEvent::Blurred` to cancel, dismiss, or commit in-progress user input. Window deactivation blanks the window's focus path, so those fire when the user merely switches apps - or, on Wayland, switches keyboard layout - and the control destroys what the user typed. Use `cx.on_blur_by_user` / `cx.on_focus_out_by_user`. If the control genuinely should also react to the window going away, say so separately with `cx.observe_window_activation`. + +Proposed for `crates/gpui/.rules` (second, independent rule): + +> Test windows start inactive: `TestWindow::is_active()` returns `false` and nothing activates a window implicitly. A test that calls `cx.deactivate_window()` without first calling `window.activate_window()` and running until parked is a no-op and proves nothing. diff --git a/crates/collab_ui/src/collab_panel.rs b/crates/collab_ui/src/collab_panel.rs index 5a1679e4167225..4b52a0ff886d1d 100644 --- a/crates/collab_ui/src/collab_panel.rs +++ b/crates/collab_ui/src/collab_panel.rs @@ -350,23 +350,18 @@ impl CollabPanel { let channel_name_editor = cx.new(|cx| Editor::single_line(window, cx)); - cx.subscribe_in( - &channel_name_editor, + cx.on_blur_by_user( + &channel_name_editor.focus_handle(cx), window, - |this: &mut Self, _, event, window, cx| { - if let editor::EditorEvent::Blurred = event { - if !window.is_window_active() { - return; - } - if let Some(state) = &this.channel_editing_state - && state.pending_name().is_some() - { - return; - } - this.take_editing_state(window, cx); - this.update_entries(false, cx); - cx.notify(); + |this: &mut Self, window, cx| { + if let Some(state) = &this.channel_editing_state + && state.pending_name().is_some() + { + return; } + this.take_editing_state(window, cx); + this.update_entries(false, cx); + cx.notify(); }, ) .detach(); diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index a6ad7219b59195..648709a88936a4 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -28743,7 +28743,6 @@ impl ui_input::ErasedEditor for ErasedEditorImpl { window.subscribe(&self.0, cx, move |_, event: &EditorEvent, window, cx| { let event = match event { EditorEvent::BufferEdited => ui_input::ErasedEditorEvent::BufferEdited, - EditorEvent::Blurred => ui_input::ErasedEditorEvent::Blurred, _ => return, }; (callback)(event, window, cx); diff --git a/crates/go_to_line/src/go_to_line.rs b/crates/go_to_line/src/go_to_line.rs index c1e244935715ba..d777c7fe89d35d 100644 --- a/crates/go_to_line/src/go_to_line.rs +++ b/crates/go_to_line/src/go_to_line.rs @@ -134,6 +134,8 @@ impl GoToLine { editor }); let line_editor_change = cx.subscribe_in(&line_editor, window, Self::on_line_editor_event); + let line_editor_blur = + cx.on_blur_by_user(&line_editor.focus_handle(cx), window, Self::on_line_editor_blur); let current_text = format!( "Current Line: {} of {} (column {})", @@ -148,7 +150,11 @@ impl GoToLine { current_text: current_text.into(), prev_scroll_position: Some(scroll_position), current_line: line, - _subscriptions: vec![line_editor_change, cx.on_release_in(window, Self::release)], + _subscriptions: vec![ + line_editor_change, + line_editor_blur, + cx.on_release_in(window, Self::release), + ], } } @@ -167,21 +173,19 @@ impl GoToLine { &mut self, _: &Entity, event: &editor::EditorEvent, - window: &mut Window, + _window: &mut Window, cx: &mut Context, ) { - match event { - editor::EditorEvent::Blurred => { - if window.is_window_active() { - self.prev_scroll_position.take(); - cx.emit(DismissEvent) - } - } - editor::EditorEvent::BufferEdited => self.highlight_current_line(cx), - _ => {} + if let editor::EditorEvent::BufferEdited = event { + self.highlight_current_line(cx) } } + fn on_line_editor_blur(&mut self, _window: &mut Window, cx: &mut Context) { + self.prev_scroll_position.take(); + cx.emit(DismissEvent) + } + fn highlight_current_line(&mut self, cx: &mut Context) { self.active_editor.update(cx, |editor, cx| { editor.clear_row_highlights::(); diff --git a/crates/gpui/src/app/context.rs b/crates/gpui/src/app/context.rs index c2c74a0d57c8f0..26f32992c9436b 100644 --- a/crates/gpui/src/app/context.rs +++ b/crates/gpui/src/app/context.rs @@ -595,6 +595,11 @@ impl<'a, T: 'static> Context<'a, T> { } /// Register a listener to be called when the given focus handle loses focus. + /// + /// This also fires when the window is deactivated by the operating system, because window + /// deactivation blanks the window's focus path. Controls that cancel, dismiss or commit + /// in-progress user input on blur want [`Context::on_blur_by_user`] instead. + /// /// Returns a subscription and persists until the subscription is dropped. pub fn on_blur( &mut self, @@ -619,6 +624,33 @@ impl<'a, T: 'static> Context<'a, T> { subscription } + /// Register a listener to be called when the given focus handle loses focus while the + /// window is still active, i.e. because focus moved somewhere else inside the window. + /// + /// Prefer this over [`Context::on_blur`] for any control that cancels, dismisses or commits + /// in-progress user input, such as a rename field or a modal. The operating system + /// deactivating the window (switching apps, or on Wayland merely switching keyboard layout) + /// blanks the window's focus path and is otherwise indistinguishable from the user moving + /// focus away, which would destroy what the user typed. + /// + /// A control that additionally wants to react to the window going away should say so + /// explicitly with [`Context::observe_window_activation`], so that the two facts stay + /// separate. + /// + /// Returns a subscription and persists until the subscription is dropped. + pub fn on_blur_by_user( + &mut self, + handle: &FocusHandle, + window: &mut Window, + mut listener: impl FnMut(&mut T, &mut Window, &mut Context) + 'static, + ) -> Subscription { + self.on_blur(handle, window, move |view, window, cx| { + if window.is_window_active() { + listener(view, window, cx) + } + }) + } + /// 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. @@ -641,6 +673,10 @@ impl<'a, T: 'static> Context<'a, T> { } /// Register a listener to be called when the given focus handle or one of its descendants loses focus. + /// + /// Like [`Context::on_blur`], this fires on window deactivation as well; see + /// [`Context::on_blur_by_user`]. + /// /// Returns a subscription and persists until the subscription is dropped. pub fn on_focus_out( &mut self, @@ -671,6 +707,26 @@ impl<'a, T: 'static> Context<'a, T> { subscription } + /// Register a listener to be called when the given focus handle or one of its descendants + /// loses focus while the window is still active. + /// + /// This is the subtree-wide counterpart of [`Context::on_blur_by_user`]; see that method for + /// why blur alone is not enough to decide whether the user is done with a control. + /// + /// Returns a subscription and persists until the subscription is dropped. + pub fn on_focus_out_by_user( + &mut self, + handle: &FocusHandle, + window: &mut Window, + mut listener: impl FnMut(&mut T, FocusOutEvent, &mut Window, &mut Context) + 'static, + ) -> Subscription { + self.on_focus_out(handle, window, move |view, event, window, cx| { + if window.is_window_active() { + listener(view, event, window, cx) + } + }) + } + /// Schedule a future to be run asynchronously. /// The given callback is invoked with a [`WeakEntity`] to avoid leaking the entity for a long-running process. /// It's also given an [`AsyncWindowContext`], which can be used to access the state of the entity across await points. @@ -875,3 +931,85 @@ impl BorrowMut for Context<'_, T> { self.app } } + +#[cfg(test)] +mod tests { + use crate::{Context, FocusHandle, Subscription, TestAppContext, Window, div, prelude::*}; + use std::{cell::Cell, rc::Rc}; + + struct BlurCounters { + first: FocusHandle, + second: FocusHandle, + _subscriptions: Vec, + } + + impl Render for BlurCounters { + fn render(&mut self, _window: &mut Window, _cx: &mut Context) -> impl IntoElement { + div() + .child(div().track_focus(&self.first)) + .child(div().track_focus(&self.second)) + } + } + + fn build_blur_counters( + blurs: Rc>, + blurs_by_user: Rc>, + window: &mut Window, + cx: &mut Context, + ) -> BlurCounters { + let first = cx.focus_handle(); + let second = cx.focus_handle(); + window.focus(&first, cx); + let subscriptions = vec![ + cx.on_blur(&first, window, move |_, _, _| blurs.set(blurs.get() + 1)), + cx.on_blur_by_user(&first, window, move |_, _, _| { + blurs_by_user.set(blurs_by_user.get() + 1) + }), + ]; + BlurCounters { + first, + second, + _subscriptions: subscriptions, + } + } + + #[gpui::test] + fn test_on_blur_by_user_fires_when_focus_moves_within_window(cx: &mut TestAppContext) { + let blurs = Rc::new(Cell::new(0)); + let blurs_by_user = Rc::new(Cell::new(0)); + let (view, cx) = cx.add_window_view({ + let blurs = blurs.clone(); + let blurs_by_user = blurs_by_user.clone(); + move |window, cx| build_blur_counters(blurs, blurs_by_user, window, cx) + }); + cx.update(|window, _| window.activate_window()); + cx.run_until_parked(); + + view.update_in(cx, |view, window, cx| { + let second = view.second.clone(); + window.focus(&second, cx); + }); + cx.run_until_parked(); + + assert_eq!(blurs.get(), 1); + assert_eq!(blurs_by_user.get(), 1); + } + + #[gpui::test] + fn test_on_blur_by_user_ignores_window_deactivation(cx: &mut TestAppContext) { + let blurs = Rc::new(Cell::new(0)); + let blurs_by_user = Rc::new(Cell::new(0)); + let (_view, cx) = cx.add_window_view({ + let blurs = blurs.clone(); + let blurs_by_user = blurs_by_user.clone(); + move |window, cx| build_blur_counters(blurs, blurs_by_user, window, cx) + }); + cx.update(|window, _| window.activate_window()); + cx.run_until_parked(); + + cx.deactivate_window(); + + assert_eq!(blurs.get(), 1); + assert_eq!(blurs_by_user.get(), 0); + } +} diff --git a/crates/gpui/src/window.rs b/crates/gpui/src/window.rs index 088dabb3c0cefa..e24137b6c48431 100644 --- a/crates/gpui/src/window.rs +++ b/crates/gpui/src/window.rs @@ -4010,6 +4010,11 @@ impl Window { } /// Register a listener to be called when the given focus handle or one of its descendants loses focus. + /// + /// This also fires when the window is deactivated by the operating system; controls that + /// cancel, dismiss or commit in-progress user input want [`crate::Context::on_blur_by_user`] + /// or [`crate::Context::on_focus_out_by_user`] instead. + /// /// Returns a subscription and persists until the subscription is dropped. pub fn on_focus_out( &mut self, diff --git a/crates/picker/src/head.rs b/crates/picker/src/head.rs index 18cff64fb6432f..27f87008dce065 100644 --- a/crates/picker/src/head.rs +++ b/crates/picker/src/head.rs @@ -17,12 +17,15 @@ impl Head { pub fn editor( placeholder_text: Arc, mut edit_handler: impl FnMut(&mut V, &ErasedEditorEvent, &mut Window, &mut Context) + 'static, + blur_handler: impl FnMut(&mut V, &mut Window, &mut Context) + 'static, window: &mut Window, cx: &mut Context, ) -> Self { let editor = (ui_input::ERASED_EDITOR_FACTORY.get().unwrap())(window, cx); editor.set_placeholder_text(placeholder_text.as_ref(), window, cx); + cx.on_blur_by_user(&editor.focus_handle(cx), window, blur_handler) + .detach(); let this = cx.weak_entity(); editor .subscribe( @@ -47,7 +50,7 @@ impl Head { cx: &mut Context, ) -> Self { let head = cx.new(EmptyHead::new); - cx.on_blur(&head.focus_handle(cx), window, blur_handler) + cx.on_blur_by_user(&head.focus_handle(cx), window, blur_handler) .detach(); Self::Empty(head) } diff --git a/crates/picker/src/picker.rs b/crates/picker/src/picker.rs index 2eb8d71bd4aa14..f6a05108232993 100644 --- a/crates/picker/src/picker.rs +++ b/crates/picker/src/picker.rs @@ -279,6 +279,7 @@ impl Picker { let head = Head::editor( delegate.placeholder_text(window, cx), Self::on_input_editor_event, + Self::on_input_editor_blur, window, cx, ); @@ -314,6 +315,7 @@ impl Picker { let head = Head::editor( delegate.placeholder_text(window, cx), Self::on_input_editor_event, + Self::on_input_editor_blur, window, cx, ); @@ -649,11 +651,12 @@ impl Picker { let query = editor.text(cx); self.update_matches(query, window, cx); } - ErasedEditorEvent::Blurred => { - if self.is_modal && window.is_window_active() { - self.cancel(&menu::Cancel, window, cx); - } - } + } + } + + fn on_input_editor_blur(&mut self, window: &mut Window, cx: &mut Context) { + if self.is_modal { + self.cancel(&menu::Cancel, window, cx); } } @@ -661,9 +664,7 @@ impl Picker { let Head::Empty(_) = &self.head else { panic!("unexpected call"); }; - if window.is_window_active() { - self.cancel(&menu::Cancel, window, cx); - } + self.cancel(&menu::Cancel, window, cx); } pub fn refresh_placeholder(&mut self, window: &mut Window, cx: &mut Context) { diff --git a/crates/project_panel/src/project_panel.rs b/crates/project_panel/src/project_panel.rs index f189248c12f17a..c6e6ab1379aea3 100644 --- a/crates/project_panel/src/project_panel.rs +++ b/crates/project_panel/src/project_panel.rs @@ -789,7 +789,7 @@ impl ProjectPanel { cx.subscribe_in( &filename_editor, window, - |project_panel, _, editor_event, window, cx| match editor_event { + |project_panel, _, editor_event, _window, cx| match editor_event { EditorEvent::BufferEdited => { project_panel.populate_validation_error(cx); project_panel.autoscroll(cx); @@ -797,29 +797,34 @@ impl ProjectPanel { EditorEvent::SelectionsChanged { .. } => { project_panel.autoscroll(cx); } - EditorEvent::Blurred => { - if window.is_window_active() - && project_panel - .state - .edit_state - .as_ref() - .is_some_and(|state| state.processing_filename.is_none()) - { - match project_panel.confirm_edit(false, window, cx) { - Some(task) => { - task.detach_and_notify_err( - project_panel.workspace.clone(), - window, - cx, - ); - } - None => { - project_panel.discard_edit_state(window, cx); - } + _ => {} + }, + ) + .detach(); + + cx.on_blur_by_user( + &filename_editor.focus_handle(cx), + window, + |project_panel, window, cx| { + if project_panel + .state + .edit_state + .as_ref() + .is_some_and(|state| state.processing_filename.is_none()) + { + match project_panel.confirm_edit(false, window, cx) { + Some(task) => { + task.detach_and_notify_err( + project_panel.workspace.clone(), + window, + cx, + ); + } + None => { + project_panel.discard_edit_state(window, cx); } } } - _ => {} }, ) .detach(); diff --git a/crates/project_panel/src/project_panel_tests.rs b/crates/project_panel/src/project_panel_tests.rs index 51482ccb839cf0..d472165d105d0f 100644 --- a/crates/project_panel/src/project_panel_tests.rs +++ b/crates/project_panel/src/project_panel_tests.rs @@ -4673,14 +4673,27 @@ async fn test_rename_survives_window_deactivation(cx: &mut gpui::TestAppContext) .unwrap(); let cx = &mut VisualTestContext::from_window(window.into(), cx); let panel = workspace.update_in(cx, ProjectPanel::new); + cx.update(|window, _| window.activate_window()); cx.run_until_parked(); + assert!( + cx.update(|window, _| window.is_window_active()), + "The window must be active, otherwise deactivating it below is a no-op and this test proves nothing" + ); select_path(&panel, "root/file1.txt", cx); panel.update_in(cx, |panel, window, cx| panel.rename(&Rename, window, cx)); + cx.run_until_parked(); assert!( panel.read_with(cx, |panel, _| panel.state.edit_state.is_some()), "Rename should have started" ); + assert!( + panel.update_in(cx, |panel, window, cx| panel + .filename_editor + .focus_handle(cx) + .is_focused(window)), + "The filename editor must be focused, otherwise deactivating the window below blurs nothing" + ); cx.deactivate_window(); diff --git a/crates/terminal_view/src/terminal_view.rs b/crates/terminal_view/src/terminal_view.rs index 0b2bfa44870282..4ba07dd4bc951d 100644 --- a/crates/terminal_view/src/terminal_view.rs +++ b/crates/terminal_view/src/terminal_view.rs @@ -453,10 +453,10 @@ impl TerminalView { .unwrap_or_else(|| self.terminal.read(cx).title(true)); let rename_editor = cx.new(|cx| Editor::single_line(window, cx)); - let rename_editor_subscription = cx.subscribe_in(&rename_editor, window, { - let rename_editor = rename_editor.clone(); - move |_this, _, event, window, cx| { - if let editor::EditorEvent::Blurred = event { + let rename_editor_subscription = + cx.on_blur_by_user(&rename_editor.focus_handle(cx), window, { + let rename_editor = rename_editor.clone(); + move |_this, window, cx| { // Defer to let focus settle (avoids canceling during double-click). let rename_editor = rename_editor.clone(); cx.defer_in(window, move |this, window, cx| { @@ -469,8 +469,7 @@ impl TerminalView { } }); } - } - }); + }); self.rename_editor = Some(rename_editor.clone()); self.rename_editor_subscription = Some(rename_editor_subscription); diff --git a/crates/ui_input/src/ui_input.rs b/crates/ui_input/src/ui_input.rs index ab3addc35c1900..d9887fbea2f053 100644 --- a/crates/ui_input/src/ui_input.rs +++ b/crates/ui_input/src/ui_input.rs @@ -33,10 +33,12 @@ pub trait ErasedEditor: 'static { fn as_any(&self) -> &dyn Any; } +/// Blur is deliberately absent here: observing blur correctly requires distinguishing a +/// genuine focus move from the window being deactivated, so consumers should subscribe to +/// [`ErasedEditor::focus_handle`] with `Context::on_blur_by_user` instead. #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum ErasedEditorEvent { BufferEdited, - Blurred, } pub static ERASED_EDITOR_FACTORY: OnceLock Arc> = OnceLock::new(); diff --git a/crates/workspace/src/modal_layer.rs b/crates/workspace/src/modal_layer.rs index cb6f21206fc5e1..b7b4c68230adeb 100644 --- a/crates/workspace/src/modal_layer.rs +++ b/crates/workspace/src/modal_layer.rs @@ -138,7 +138,7 @@ impl ModalLayer { this.hide_modal(window, cx); }, ), - cx.on_focus_out(&focus_handle, window, |this, _event, window, cx| { + cx.on_focus_out_by_user(&focus_handle, window, |this, _event, window, cx| { if this.dismiss_on_focus_lost { this.hide_modal(window, cx); }