Variant A: Stop synthesizing focus events from window activation - #1
Closed
butvinm wants to merge 1 commit into
Closed
Variant A: Stop synthesizing focus events from window activation#1butvinm wants to merge 1 commit into
butvinm wants to merge 1 commit into
Conversation
Window activation and element focus were conflated: on deactivation draw() blanked the focus paths of WindowFocusEvent, so every focused element observed something indistinguishable from "the user moved focus away". Controls that cancel, dismiss, or commit on blur therefore destroyed user input whenever the window lost OS focus, which on Wayland happens merely by switching keyboard layout. Each occurrence has been patched one control at a time with an opt-in window.is_window_active() guard (zed-industries#41320, zed-industries#46866, zed-industries#47044, zed-industries#39286). Make the two signals independent instead. Focus events now fire only when the focused element inside the window changes; deactivation and reactivation emit nothing, which matches window.focus never being cleared on deactivation. Consumers that genuinely wanted the deactivation blur move to the existing observe_window_activation API: - Editor: hover popover, completion menu and active edit prediction are hidden from the subscription that already handles cursor blink. - ContextMenu: dismissal on window blur, previously a side effect of the synthesized blur, is now requested explicitly. - TerminalView: xterm focus reporting and the hollow cursor keep tracking window activation. The is_window_active() guards in the picker, project panel, collab panel and Go to Line are now redundant and are removed. Frame::window_active has no remaining readers and is deleted.
Owner
Author
|
Superseded by #5 (variant D). Closing to keep the field clear; the diff stays viewable here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review-only PR in the fork. Do not merge upstream from here. One of three competing designs for the same problem, built so they can be compared side by side. See also the variant B and variant C PRs.
Base is
fix-layout-switch-cancels-rename(merged upstream as zed-industries#61852), so the diff shows only this design.Design: make window activation stop producing focus events
Window activation and element focus become two genuinely independent signals. The core change is about 8 net-removed lines in
crates/gpui/src/window.rs: the focus paths are no longer blanked when the window's active state changes, and theprevious_window_active != current_window_activedisjunct is deleted entirely. With the paths unblanked, an activation-only event is provably a no-op for all six focus-listener registrations, soFrame::window_activeloses its last reader and is removed.Consequence: every dismiss/cancel handler becomes correct with no code at all, including handlers written in the future. Anything that genuinely wants the deactivation signal subscribes via
cx.observe_window_activation.Three consumers wanted the old behavior and were migrated: the editor's transient UI (hover popover, completion menu, edit prediction), the context menu (preserving zed-industries#46866's deliberate dismiss-on-window-blur), and the terminal's xterm focus reporting plus hollow cursor. Four now-redundant
is_window_active()guards were deleted, including the ones added by zed-industries#61852.Diffstat
11 files changed, 380 insertions, 111 deletions.
Verification
cargo check -p zed --all-targetspassingfocus-variant-a-cmp): https://github.com/butvinm/zed/actions/runs/30462742284 - passing,test_rename_survives_window_deactivation ... okNothing was compiled locally (this machine cannot build Zed); all verification is fork CI.
Honest weaknesses
Editor::handle_focus,Pane::focus_in, and Dock zoom bookkeeping no longer run on window reactivation. One concrete loss:buffer.finalize_last_transactionno longer creates an undo boundary at an app switch.ade8749537while fixing macOS emoji-palette typing. The claim that the emoji bug came from Preserve and restore focus across window activation cycles zed-industries/zed#47044's focus save/restore rather than ui: Dismiss context menus when window loses focus zed-industries/zed#46866 could not be verified without a Mac.test_window_activation_does_not_produce_focus_events(crates/gpui/src/key_dispatch.rs) compiled but did not run - the fork workflow only executesproject_panelandgo_to_linetests.Full per-file rationale, all judgment calls, and the platform-layer comparison are in
VARIANT-NOTES.mdat the repo root of this branch.Release Notes: