diff --git a/crates/ember-app/src/main.rs b/crates/ember-app/src/main.rs index 141b568..dd888f3 100644 --- a/crates/ember-app/src/main.rs +++ b/crates/ember-app/src/main.rs @@ -1268,15 +1268,47 @@ impl ApplicationHandler for App { ); if button == MouseButton::Left { let was_dragging = shared.drag.is_some() || win.tab_drag.is_some(); - let ended = win.left_release(shared, id); - // A pane drop only STAGES its move (`WindowState:: - // pending_move`) — run it through the same canonical - // `apply_move` path `ctl drag` (`run_ctl_drag`) and every - // other surface-mobility gesture uses, right here: `win` - // isn't touched again in this arm, so its borrow of - // `self.windows` ends at the `take()` below, freeing - // `self.windows` for `apply_move` to reborrow whole. - let pending = win.pending_move.take(); + // Routing fix: a release always resolves against the + // drag's SOURCE window, never blindly whichever window + // the OS delivered the event to. On Wayland winit has no + // implicit pointer grab (unlike macOS/X11's — see + // `update_cross_window_drag`'s doc), so a cross-window + // carry's release can land on the TARGET window + // instead of the source that tore the surface off; + // trusting `id` there ran `resolve_drag_drop` as a + // method on the wrong `WindowState` and skipped the + // real source's carried-exclusion cleanup, leaving it + // hidden forever ("alive but invisible"). `win`'s + // borrow of `self.windows` ends at its last use in + // EITHER arm below, so both can freely reborrow + // `self.windows` afterward — the same NLL shape + // `apply_move`'s other callers already rely on. + let drag_source = shared.drag.as_ref().map(|d| d.source_window); + let (ended, pending) = match route_release(drag_source, id) { + ReleaseRouting::Misrouted { + resolve_for, + hover_from, + } => { + let (x, y) = win.cursor; + resolve_misrouted_release( + &mut self.windows, + shared, + resolve_for, + hover_from, + x, + y, + ) + } + ReleaseRouting::OnSource | ReleaseRouting::NotDragging => { + let ended = win.left_release(shared, id); + // A pane drop only STAGES its move (`WindowState:: + // pending_move`) — run it through the same canonical + // `apply_move` path `ctl drag` (`run_ctl_drag`) and + // every other surface-mobility gesture uses. + let pending = win.pending_move.take(); + (ended, pending) + } + }; if was_dragging { // Every window's incoming-drop/preview visual is // stale the instant the drag resolves. The ctl-drag @@ -1315,17 +1347,22 @@ impl ApplicationHandler for App { Err(e) => eprintln!("[ember] drag drop rejected: {e}"), } } - // Fix 2: a window whose carried-surface exclusion is - // still hidden (`hidden_for_carry`) gets its OS re-show - // deferred here — AFTER `apply_move` — so a - // sole-tab window that's about to be destroyed by this - // very move (its only tab moved out → `WindowClosed`) - // never flashes back on screen first. A window - // `apply_move` closed is already gone from - // `self.windows`, so it's silently skipped below — - // exactly the point. + // Fix 2 + invariant backstop: every window's carried + // exclusion gets cleared/its OS re-show applied here — + // AFTER `apply_move` — so a sole-tab window that's about + // to be destroyed by this very move (its only tab moved + // out → `WindowClosed`) never flashes back on screen + // first. A window `apply_move` closed is already gone + // from `self.windows`, so it's silently skipped below — + // exactly the point. Sweeping EVERY window (not just the + // drag's recorded source) rather than only re-showing + // deferred `hidden_for_carry` ones is deliberate: it's + // what makes "alive but invisible" structurally + // impossible even when resolution ran against the wrong + // `WindowState` (the exact routing bug fixed above) — + // see `ensure_carry_exclusion_cleared`'s doc. for w in self.windows.values_mut() { - w.finish_carry_reshow(); + w.ensure_carry_exclusion_cleared(shared); } } } @@ -3391,21 +3428,31 @@ fn cancel_drag_everywhere(windows: &mut HashMap, shared: // already handles that path); a missing source here is a no-op. if let Some(w) = windows.get_mut(&drag.source_window) { // Carry-time source vanish: "pops back exactly where - // it was" — re-show the OS window / restore the filtered - // layout BEFORE the pour-out below repaints over it, so the - // very first frame of the pour-out is already showing the - // real, restored content, not a stale exclusion. - w.clear_carried_exclusion(shared); - // Fix 2: a cancel never closes the source - // window (unlike a completed move, where the whole point of - // deferring the re-show is that the window might be about to - // die) — so unlike the drop paths, re-show it immediately - // rather than leaving it deferred. - w.finish_carry_reshow(); + // it was" — clear/re-show BEFORE the pour-out below repaints + // over it, so the very first frame of the pour-out is already + // showing the real, restored content, not a stale exclusion. + // Fix 2: a cancel never closes the source window (unlike a + // completed move, where the whole point of deferring the + // re-show is that the window might be about to die) — so + // unlike the drop paths, `ensure_carry_exclusion_cleared`'s + // re-show lands immediately rather than staying deferred. + w.ensure_carry_exclusion_cleared(shared); let rect = w.viewport(); let grab = w.cursor; w.start_pour_out(shared, rect, grab); } + // Invariant backstop: sweep every OTHER window too. Only the + // recorded source should ever have a live carried exclusion for + // this drag, but "alive but invisible" must be structurally + // impossible even for event-routing/compositor behavior this code + // hasn't modeled — see the identical sweep after a real drop + // (`App::window_event`'s `MouseInput` release arm) and the `ctl + // drag` tail (`finish_ctl_drag_tail`). + for (wid, w) in windows.iter_mut() { + if *wid != drag.source_window { + w.ensure_carry_exclusion_cleared(shared); + } + } clear_all_drag_visuals(windows); for w in windows.values_mut() { w.renderer.window().request_redraw(); @@ -3413,6 +3460,109 @@ fn cancel_drag_everywhere(windows: &mut HashMap, shared: } } +/// The release-owner decision a `MouseInput::Released` needs once a drag +/// might be live: which `WindowState` the drop must resolve against, and +/// which window's coordinate space `drag.hover` should be recomputed +/// against, if either differs from a plain same-window release. Extracted +/// as its own pure function (no `WindowState`/`HashMap` involved) so the +/// exact routing decision — the fix for the Wayland "release misdelivered +/// to the target window instead of the source" bug — has a seam to unit +/// test without spinning up real windows. +#[derive(Debug, PartialEq, Eq)] +enum ReleaseRouting { + /// No live drag: this release isn't drag-related at all. + NotDragging, + /// The release landed on the drag's own source window — resolve + /// directly against it (`win.left_release`), trusting the hover + /// `update_drag_hover`/`update_cross_window_drag` already kept live. + OnSource, + /// The release landed on a DIFFERENT window than the drag's recorded + /// source (no implicit pointer grab on Wayland — see + /// `update_cross_window_drag`'s doc for why this can happen at all): + /// resolve against `resolve_for` (always the source — the only place + /// `pending_move`/the carried exclusion actually live), but recompute + /// `drag.hover` fresh against `hover_from` (the window that actually + /// received the event) first, via `resolve_misrouted_release`. + Misrouted { + resolve_for: WindowId, + hover_from: WindowId, + }, +} + +/// Decide `ReleaseRouting` for a `MouseInput::Released` delivered to +/// `event_window`, given the live drag's source (`None` if no drag is +/// live). Pure and total: every `(drag_source, event_window)` pair maps to +/// exactly one `ReleaseRouting`. +fn route_release(drag_source: Option, event_window: WindowId) -> ReleaseRouting { + match drag_source { + None => ReleaseRouting::NotDragging, + Some(source) if source == event_window => ReleaseRouting::OnSource, + Some(source) => ReleaseRouting::Misrouted { + resolve_for: source, + hover_from: event_window, + }, + } +} + +/// Resolve a `MouseInput::Released` the OS delivered to `event_window` — +/// NOT `drag.source_window` — while a drag is live: the release-routing +/// half of the Wayland cross-window carry fix. Wayland's `wl_pointer` has +/// no implicit grab (unlike macOS/X11 — see `update_cross_window_drag`'s +/// doc, which documents the "release always lands on the source" premise +/// this function stops relying on): once the pointer is over a different +/// top-level window, the compositor is free to hand that window the +/// release instead of the source. Trusting the event's window as the +/// drag's owner ran `resolve_drag_drop` as a method on the WRONG +/// `WindowState` — the real source's `pending_move` was never set and its +/// carried-exclusion cleanup never ran, leaving it hidden forever ("alive +/// but invisible"). +/// +/// The fix has two parts: resolve the drop against the SOURCE window's own +/// `WindowState` (the only place `pending_move`/the carried exclusion +/// actually live — `resolve_drag_release` requires this), but recompute +/// `drag.hover` FRESH against `event_window` first, via the same +/// `hover_at` a live cross-window drag tick already uses to preview a +/// target (`update_cross_window_drag`, right above). `drag.hover` can't be +/// trusted as-is here: it only ever gets updated by the SOURCE's own +/// `CursorMoved`/`update_cross_window_drag` calls, which never fire for +/// `event_window` (its own `CursorMoved`s early-return out of +/// `update_drag_hover` once source/window mismatch), so it's frozen at +/// whatever it was before the compositor handed the pointer to +/// `event_window` — exactly the "silent no-op" the investigation traced. +/// `(x, y)` are `event_window`'s own local logical px (its `WindowState:: +/// cursor`, kept live by its own real `CursorMoved`s regardless of drag +/// ownership). +fn resolve_misrouted_release( + windows: &mut HashMap, + shared: &mut Shared, + source: WindowId, + event_window: WindowId, + x: f64, + y: f64, +) -> (DragEnded, Option<(SurfaceRef, SurfaceDest)>) { + let Some(mut drag) = shared.drag.take() else { + return (DragEnded::None, None); + }; + drag.hover = windows + .get(&event_window) + .and_then(|w| w.hover_at(event_window, x, y, false)); + let Some(src_win) = windows.get_mut(&source) else { + // The source window vanished mid-drag through some other path + // (`clear_drag_on_window_close` clears `shared.drag` itself when + // its own source closes, so this shouldn't happen) — stay honest + // rather than panic if it ever does. + shared.wisp_end_drag(); + return (DragEnded::Cancel, None); + }; + // `window_id` here is the SOURCE's own id, not `event_window`'s — see + // `resolve_drag_release`'s doc for why that's the parameter's actual + // meaning (it names the window `self`/`src_win` IS, not whichever + // window the OS event was addressed to). + let ended = src_win.resolve_drag_release(shared, source, drag); + let pending = src_win.pending_move.take(); + (ended, pending) +} + /// The session ids `src` currently names, resolved against the SOURCE /// window's tree as it stands right now — must be called BEFORE `apply_move` /// runs (that's what actually relocates them). `pour_out_after_move` uses @@ -4500,12 +4650,15 @@ fn finish_ctl_drag_tail( } } } - // Fix 2: see the identical comment at the real-mouse - // release site (`App::window_event`'s `MouseInput` release arm) — same - // reasoning, same "after apply_move, regardless of whether pending was - // Some" placement. + // Fix 2 + invariant backstop: see the identical comment/sweep at the + // real-mouse release site (`App::window_event`'s `MouseInput` release + // arm) — same reasoning, same "after apply_move, regardless of whether + // pending was Some" placement. `ctl drag` can't itself misroute a + // release (`window` here is always the fixed press window, never a + // different one an OS event could misdeliver to), but this sweep stays + // in lockstep with the real-mouse path rather than a weaker copy. for w in windows.values_mut() { - w.finish_carry_reshow(); + w.ensure_carry_exclusion_cleared(shared); } let mut reply = serde_json::json!({ "ok": true, @@ -5262,14 +5415,15 @@ fn encode_key( #[cfg(test)] mod tests { use super::{ - BELL_FLASH_SECS, DeferredMoveOp, DeferredWindowAction, PaneMeta, SessionId, TabId, - bell_flash_intensity, bracket_paste, clamp_to_visible_monitor, clear_captured_commands, - encode_key, match_tab_title, match_tab_title_across, needs_deferred_replay, - next_prev_index, pane_snap_for, pretype_bytes, queue_close_this, queue_close_window, - resolve_index, resolve_restore_cwd, shell_escape_path, tab_display_title, url_is_openable, - window_owning_tab, + BELL_FLASH_SECS, DeferredMoveOp, DeferredWindowAction, PaneMeta, ReleaseRouting, SessionId, + TabId, bell_flash_intensity, bracket_paste, clamp_to_visible_monitor, + clear_captured_commands, encode_key, match_tab_title, match_tab_title_across, + needs_deferred_replay, next_prev_index, pane_snap_for, pretype_bytes, queue_close_this, + queue_close_window, resolve_index, resolve_restore_cwd, route_release, shell_escape_path, + tab_display_title, url_is_openable, window_owning_tab, }; use winit::keyboard::{Key, ModifiersState, NamedKey, SmolStr}; + use winit::window::WindowId; fn enc(key: Key, mods: ModifiersState) -> Option> { encode_key(&key, mods, false, false) @@ -6011,4 +6165,62 @@ mod tests { let by_window: Vec<(u32, Vec)> = vec![(10, vec![TabId(1)]), (20, vec![TabId(2)])]; assert_eq!(window_owning_tab(&by_window, TabId(99)), None); } + + // --- route_release (Wayland cross-window drop-routing fix) --------- + // + // `MouseInput::Released` used to trust whichever window the OS + // delivered the event to as a live drag's owner. On Wayland (no + // implicit pointer grab) the release can land on the drag's TARGET + // window instead of its recorded `source_window`; resolving the drop + // against the wrong `WindowState` skipped the real source's + // carried-exclusion cleanup and left it hidden forever ("alive but + // invisible" — the field bug this fix addresses). `route_release` is + // the pure decision these three cases boil down to. + + #[test] + fn route_release_with_no_live_drag_is_not_dragging() { + let a = WindowId::from(1u64); + assert_eq!(route_release(None, a), ReleaseRouting::NotDragging); + } + + #[test] + fn route_release_on_the_drags_own_source_resolves_there() { + let a = WindowId::from(1u64); + assert_eq!(route_release(Some(a), a), ReleaseRouting::OnSource); + } + + #[test] + fn route_release_on_a_different_window_still_resolves_against_the_source() { + // The misdelivery case: the OS handed the release to `b`, but the + // drag's source is `a` -- the drop must still resolve against `a` + // (`resolve_for`), with the hover recomputed fresh against `b` + // (`hover_from`), not silently dropped or resolved against the + // wrong window. + let a = WindowId::from(1u64); + let b = WindowId::from(2u64); + assert_eq!( + route_release(Some(a), b), + ReleaseRouting::Misrouted { + resolve_for: a, + hover_from: b, + } + ); + } + + #[test] + fn route_release_is_symmetric_in_which_window_is_named_source() { + // Swapping which window is "the source" and which received the + // event swaps resolve_for/hover_from accordingly -- routing isn't + // hardcoded to a particular WindowId, only to the relationship + // between the two. + let a = WindowId::from(1u64); + let b = WindowId::from(2u64); + assert_eq!( + route_release(Some(b), a), + ReleaseRouting::Misrouted { + resolve_for: b, + hover_from: a, + } + ); + } } diff --git a/crates/ember-app/src/window_state.rs b/crates/ember-app/src/window_state.rs index 1303fc5..e92cffd 100644 --- a/crates/ember-app/src/window_state.rs +++ b/crates/ember-app/src/window_state.rs @@ -402,6 +402,20 @@ enum CarriedExclusion { WholeWindow, } +/// Whether a window's carried-exclusion bookkeeping is orphaned and needs +/// [`WindowState::clear_carried_exclusion`] — armed (about to apply) or +/// already applied (its OS window possibly hidden right now). Pulled out of +/// [`WindowState::ensure_carry_exclusion_cleared`] as its own pure +/// predicate, the invariant backstop's actual "is there anything to clear +/// here" check, so it has a testable seam that doesn't need a real +/// `WindowState`/renderer to spin up. +fn carry_exclusion_needs_clearing( + exclusion_applied: bool, + carried_exclusion: Option, +) -> bool { + exclusion_applied || carried_exclusion.is_some() +} + /// Everything tied to a single window and its surface. pub(crate) struct WindowState { pub(crate) renderer: Renderer, @@ -2165,11 +2179,13 @@ impl WindowState { } let mut ended = DragEnded::None; if let Some(drag) = shared.drag.take() { - ended = self.resolve_drag_drop(shared, window_id, drag); - // Task 5: end the wisp's fade-out here too — this is the ONLY - // release path shared by both a real mouse-up and `ctl drag`'s - // synthesized one (`run_ctl_drag` calls this same method). - shared.wisp_end_drag(); + // Task 5: `resolve_drag_release` ends the wisp's fade-out too — + // this is the ONLY release path shared by a real mouse-up, `ctl + // drag`'s synthesized one (`run_ctl_drag` calls this same + // method), AND a release misdelivered to a different window + // (`resolve_misrouted_release`, `main.rs`, reuses the same + // wrapper against the SOURCE window's own `WindowState`). + ended = self.resolve_drag_release(shared, window_id, drag); } else if let Some(d) = self.tab_drag.take() { self.renderer.set_tab_drag(None); ended = if d.active { @@ -2735,6 +2751,28 @@ impl WindowState { } } + /// Invariant backstop: no drag-end path may leave this window's carried + /// exclusion armed, or its OS window hidden for a carry that's already + /// over. Safe to call on EVERY window after ANY drag resolution (a + /// drop, a cancel, or a release misdelivered to a different window) — + /// `carried_exclusion`/`exclusion_applied` are only ever armed on a + /// window while it IS a live drag's own source (`begin_carried_ + /// exclusion`, set once per tear-off), and this always runs after + /// `shared.drag` has already been taken, so there is never a + /// genuinely live drag left to protect; a window with nothing armed + /// no-ops through both calls below. This is what makes "alive but + /// invisible" — a window `apply_carried_exclusion` hid whose matching + /// `clear_carried_exclusion` never ran, because resolution happened to + /// run against a DIFFERENT window's `WindowState` — structurally + /// impossible, independent of whatever specific routing bug caused it + /// (the release-routing fix this backstop ships alongside included). + pub(crate) fn ensure_carry_exclusion_cleared(&mut self, shared: &Shared) { + if carry_exclusion_needs_clearing(self.exclusion_applied, self.carried_exclusion) { + self.clear_carried_exclusion(shared); + } + self.finish_carry_reshow(); + } + /// Record what strip chip (if any) a live drag is hovering on THIS /// window right now — the spring-loaded tab select's input (finding #2). /// Restarts the dwell clock when the chip changes; keeps it running @@ -3674,6 +3712,31 @@ impl WindowState { } } + /// Resolve a drag THIS window is the source of (`self` must already BE + /// `drag.source_window`'s `WindowState` — `resolve_drag_drop`'s doc + /// explains why that's load-bearing), pairing it with the matching wisp + /// teardown. `window_id` must be this window's own id: for an ordinary + /// same-window release that's simply the id the OS delivered the event + /// to (`left_release`'s caller); for a release the OS misdelivered to a + /// DIFFERENT window, the caller must still pass THIS (source) window's + /// own id, never the event's — see `resolve_drag_drop`'s "Strip hover on + /// the SOURCE's own window" check, which compares `drag.hover`'s window + /// against exactly this parameter. One shared wrapper (rather than two + /// copies of `resolve_drag_drop` + `wisp_end_drag`) so same-window and + /// misrouted releases can't drift apart: `left_release` uses this for + /// the normal case, `main.rs`'s `resolve_misrouted_release` reuses it + /// against the source's own `WindowState` for the misdelivered one. + pub(crate) fn resolve_drag_release( + &mut self, + shared: &mut Shared, + window_id: WindowId, + drag: DragState, + ) -> DragEnded { + let ended = self.resolve_drag_drop(shared, window_id, drag); + shared.wisp_end_drag(); + ended + } + /// Begin inline rename of tab `i` (double-click); seeds the buffer with its title. pub(crate) fn start_rename(&mut self, shared: &Shared, i: usize) { if i >= self.tree.tabs.len() { @@ -4874,7 +4937,10 @@ impl WindowState { #[cfg(test)] mod tests { - use super::{TEAR_OFF_THRESHOLD, strip_band_exit}; + use super::{ + CarriedExclusion, TEAR_OFF_THRESHOLD, TabId, carry_exclusion_needs_clearing, + strip_band_exit, + }; #[test] fn strip_band_exit_stays_false_inside_the_band() { @@ -5379,4 +5445,52 @@ mod tests { // panic, so `adjust_setting` just skips mutation for it. assert!(find_setting_row_by_label("Delete saved sessions (4)…").is_none()); } + + // --- carry_exclusion_needs_clearing (invariant backstop) ----------- + // + // `ensure_carry_exclusion_cleared` must treat a window's carried + // exclusion as orphaned -- and force it clear -- whenever EITHER half + // of its bookkeeping is still live: armed but not yet applied, or + // applied (meaning the OS window may actually be hidden right now). + // This is the exact predicate that makes "alive but invisible" + // structurally impossible even for a drag-end path this code hasn't + // modeled, independent of the release-routing fix it ships alongside. + + #[test] + fn carry_exclusion_needs_clearing_is_false_with_nothing_armed() { + assert!(!carry_exclusion_needs_clearing(false, None)); + } + + #[test] + fn carry_exclusion_needs_clearing_is_true_once_applied() { + // The OS window may be hidden right now (`WholeWindow`, applied) -- + // this must never be treated as "nothing to do". + assert!(carry_exclusion_needs_clearing( + true, + Some(CarriedExclusion::WholeWindow) + )); + } + + #[test] + fn carry_exclusion_needs_clearing_is_true_when_armed_but_not_yet_applied() { + // Torn off, but the suck-in animation hasn't finished yet + // (`apply_carried_exclusion` gates on `!self.morph_live()`) -- + // `exclusion_applied` is still false, but `carried_exclusion` is + // Some: this is still orphaned state if the drag ends here. + assert!(carry_exclusion_needs_clearing( + false, + Some(CarriedExclusion::Tab(TabId(7))) + )); + } + + #[test] + fn carry_exclusion_needs_clearing_covers_every_carried_exclusion_variant() { + for ex in [ + CarriedExclusion::Pane(super::PaneId(1)), + CarriedExclusion::Tab(TabId(1)), + CarriedExclusion::WholeWindow, + ] { + assert!(carry_exclusion_needs_clearing(true, Some(ex))); + } + } }