Variant C: Add on_blur_by_user and migrate blur-cancelling controls onto it - #3
Closed
butvinm wants to merge 1 commit into
Closed
Variant C: Add on_blur_by_user and migrate blur-cancelling controls onto it#3butvinm wants to merge 1 commit into
butvinm wants to merge 1 commit into
Conversation
Window deactivation blanks the window's focus path, so every blur listener observes something indistinguishable from the user moving focus away. Controls that cancel, dismiss or commit in-progress input therefore destroy user input when the window loses OS focus, which on Wayland happens on a mere keyboard layout switch. This has been patched one control at a time with opt-in window.is_window_active() guards (zed-industries#41320, zed-industries#46866, zed-industries#47044, zed-industries#39286). Add Context::on_blur_by_user and Context::on_focus_out_by_user, which fire only while the window is still active, and migrate the project panel rename editor, the collab panel channel rename editor, Go to Line, the picker head, the terminal tab rename editor and the modal layer onto them. GPUI's event semantics are unchanged; on_blur, on_focus_out and WindowFocusEvent behave exactly as before, and the new constructors are thin wrappers that point at each other from their doc comments. Drop ErasedEditorEvent::Blurred, which had no consumer left once the picker subscribed to the erased editor's focus handle instead. test_rename_survives_window_deactivation could not fail before: test windows start inactive, so deactivate_window() was a no-op and no blur was ever emitted. Activate the window and assert the preconditions.
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 A and variant B PRs.
Base is
fix-layout-switch-cancels-rename(merged upstream as zed-industries#61852), so the diff shows only this design.Design: leave GPUI alone, make the correct pattern the easy one
Two subscription constructors on
Context<T>, placed immediately beside the two methods that carry the trap (crates/gpui/src/app/context.rs):Each is a three-line wrapper gating the existing constructor on
window.is_window_active(). GPUI's event semantics are entirely untouched, so this is the lowest-risk option to land.Migrated: project panel, collab panel, Go to Line, picker (both head kinds), terminal tab rename, and
modal_layer.ErasedEditorEvent::Blurredwas deleted once it had no consumer left - the one place the convention became an actual constraint.Diffstat
13 files changed, 368 insertions, 65 deletions.
Verification
cargo check -p zed --all-targetspassing; the test step failed, but only because this branch's own test repair was missingworkspace.add_panel(...), so its precondition assertion fired.focus-variant-c-cmp): https://github.com/butvinm/zed/actions/runs/30462742452 - passing,test_rename_survives_window_deactivation ... ok. The implementation is fine; the earlier red was a test-harness artifact.Findings worth more than the code
modal_layeris not the good prior art it appears to be. It has no activation awareness and only looks correct becausedismiss_on_focus_lostis usually false.file_finder'son_before_dismissdoes set it, so that was a real latent bug, fixed here.fix-vacuous-deactivation-testPR.Honest weaknesses
cx.on_blur,cx.on_focus_out, andEditorEvent::Blurredall still exist and still misbehave; 12 otherBlurredsubscribers are untouched. Nothing stops the next contributor from hand-rolling the buggy version. It does not close the bug class for future UI and cannot without removing or renaming the dangerous API outright.EditorEvent::Blurredsubscribers (which run in the effect flush) to focus listeners (synchronous insideWindow::draw). For the picker,cancel/DismissEventnow runs beforeModalLayer's focus-out listener rather than after. Wants hands-on exercise of the command palette, file finder, and file-finder submenu.track_focusdivs producing the expected focus paths) is the kind of thing that is easy to get wrong on paper.Full API rationale, migration-by-migration notes on how cleanly each call site fit, and all judgment calls are in
VARIANT-NOTES.mdat the repo root of this branch.Release Notes: