From db1ecef949953fa6cea9f7a32a5fa30e7e1a87a4 Mon Sep 17 00:00:00 2001 From: Luke Amdor Date: Wed, 19 Aug 2026 16:10:10 -0500 Subject: [PATCH 1/2] Add diagnostic tests for revealing a window moved into a four-column workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A window moved to an adjacent workspace can land selected in a column outside the destination viewport: parked offscreen, given no frame write, yet holding keyboard focus. These tests record the viewport state observed in that reproduction and the reveal behavior around it. The fixture is the destination geometry from the reproduction: four 1150pt columns with a 20pt gap at x = 0, 1170, 2340, 3510, seen through a 2560pt viewport, with the moved window landing in the last column. In that state the rebase step of ensureSelectionVisible leaves viewOffsetPixels at -3530 with viewStart at -20, so column 3 starts roughly 970pt beyond the right viewport edge. Five tests assert the reveal contract under disabled motion: the rebase is view-neutral, the moved column is parked, a parked destination column is revealed, a scroll is scheduled, and scroll lock does not exempt a fully parked target. Three further tests separate the two motion configurations, because they behave differently in a way that matters for the surrounding code. With motion disabled, animateToOffset takes its static fallback and both current and target land on the snap. With motion enabled — the value MotionPolicy.snapshot() returns, so the one the runtime move path uses — it installs a spring: the target moves while the current offset stays put for the display-link driver to interpolate. The plan-build step in NiriLayoutHandler tests abs(viewOffsetPixels.current() - offsetBefore) > 1 to decide whether to emit a .startNiriScroll directive, so it observes the former but not the latter. These are diagnostic rather than a regression guard for a landed fix. The runtime defect is timing-dependent and does not reproduce under trace capture. --- .provenance.json | 1 + ...dowRevealAfterWorkspaceTransferTests.swift | 234 ++++++++++++++++++ 2 files changed, 235 insertions(+) create mode 100644 Tests/NehirTests/MovedWindowRevealAfterWorkspaceTransferTests.swift diff --git a/.provenance.json b/.provenance.json index 0ace784c..830703f8 100644 --- a/.provenance.json +++ b/.provenance.json @@ -93,6 +93,7 @@ "Tests/NehirTests/FocusPreviousCrossWorkspaceTests.swift", "Tests/NehirTests/InteractiveMoveDemoModelTests.swift", "Tests/NehirTests/MonitorIdentityMatchingTests.swift", + "Tests/NehirTests/MovedWindowRevealAfterWorkspaceTransferTests.swift", "Tests/NehirTests/NiriMonitorTests.swift", "Tests/NehirTests/ParkedViewportRelayoutTests.swift", "Tests/NehirTests/PureLayoutAgreementTests.swift", diff --git a/Tests/NehirTests/MovedWindowRevealAfterWorkspaceTransferTests.swift b/Tests/NehirTests/MovedWindowRevealAfterWorkspaceTransferTests.swift new file mode 100644 index 00000000..d58e16c8 --- /dev/null +++ b/Tests/NehirTests/MovedWindowRevealAfterWorkspaceTransferTests.swift @@ -0,0 +1,234 @@ +// SPDX-FileCopyrightText: 2026 Aleksei Gurianov and Nehir contributors +// SPDX-FileComment: Provenance=nehir-original; See=NOTICE.md +// +// SPDX-License-Identifier: GPL-2.0-only + +import AppKit +@testable import Nehir +import Testing + +/// Moving a window to another workspace must leave its column inside the +/// destination viewport, not parked outside it. +/// +/// `ensureSelectionVisible` does two things in sequence: it rebases +/// `viewOffsetPixels` so that retargeting `activeColumnIndex` does not shift what +/// is on screen, and it then calls `scrollToReveal` to bring the newly selected +/// column into view. The rebase is deliberately view-neutral, so the reveal is +/// the only step that can actually move the viewport. +/// +/// The state under test is the one observed in a real reproduction: a window moved +/// into a four-column destination workspace ends up selected in the last column +/// while the viewport still rests at the far left, so the focused window is parked +/// offscreen and never receives a frame write. +@Suite struct MovedWindowRevealAfterWorkspaceTransferTests { + /// The rebase preserves the on-screen view exactly, which is what makes the + /// follow-up reveal load-bearing. Retargeting from column 2 to column 3 turns + /// `viewOffsetPixels` -2360 into -3530 while `viewStart` stays at -20. + @Test func rebasingActiveColumnPreservesViewStart() { + let fixture = makeFixture() + + let beforeViewStart = fixture.state.viewPosPixels(columns: fixture.columns, gap: fixture.gap) + #expect(beforeViewStart == -20) + + var state = fixture.state + let oldActivePos = state.containerPosition( + at: state.activeColumnIndex, + containers: fixture.columns, + gap: fixture.gap, + sizeKeyPath: \.cachedWidth + ) + let newActivePos = state.containerPosition( + at: movedColumn, + containers: fixture.columns, + gap: fixture.gap, + sizeKeyPath: \.cachedWidth + ) + state.viewOffsetPixels.offset(delta: Double(oldActivePos - newActivePos)) + state.activeColumnIndex = movedColumn + + #expect(state.viewOffsetPixels.current() == -3530) + #expect(state.viewPosPixels(columns: fixture.columns, gap: fixture.gap) == -20) + } + + /// With the viewport resting at -20 and the moved window's column starting at + /// 3510, the column is entirely outside the viewport. + @Test func movedColumnIsParkedBeforeReveal() { + let fixture = rebasedFixture() + + #expect(fixture.visibility(of: movedColumn) == .parked(.maximum)) + } + + /// The behaviour the real reproduction contradicts: a parked destination column + /// must be revealed, leaving the moved window inside the viewport. + @Test func revealsMovedColumnParkedOutsideDestinationViewport() { + var fixture = rebasedFixture() + #expect(fixture.visibility(of: movedColumn) == .parked(.maximum)) + + let revealed = fixture.reveal(movedColumn, trigger: .automatic) + + #expect(revealed) + #expect(fixture.visibility(of: movedColumn) != .parked(.maximum)) + } + + /// A scroll must actually be scheduled. In the reproduction the committed state + /// had `currentViewStart == targetViewStart == -20`, meaning nothing was queued. + @Test func schedulesScrollTowardMovedColumn() { + var fixture = rebasedFixture() + let originalTarget = fixture.state.viewOffsetPixels.target() + + _ = fixture.reveal(movedColumn, trigger: .automatic) + + #expect(fixture.state.viewOffsetPixels.target() != originalTarget) + } + + /// Scroll lock does not exempt this: its own contract is that a fully parked + /// target is still revealed, because focus otherwise lands somewhere invisible. + @Test func revealsMovedColumnEvenWhenViewportIsScrollLocked() { + var fixture = rebasedFixture() + fixture.state.isScrollLocked = true + #expect(fixture.visibility(of: movedColumn) == .parked(.maximum)) + + let revealed = fixture.reveal(movedColumn, trigger: .automatic) + + #expect(revealed) + #expect(fixture.visibility(of: movedColumn) != .parked(.maximum)) + } + + /// With motion disabled the reveal is instantaneous: `animateToOffset` takes its + /// static fallback, so both the current and target offsets land on the snap and + /// nothing is left animating. The five tests above exercise this path. + @Test func revealWithMotionDisabledLandsStaticallyOnTheSnap() { + var fixture = rebasedFixture() + + _ = fixture.reveal(movedColumn, trigger: .automatic, motion: .disabled) + + #expect(!fixture.state.viewOffsetPixels.isAnimating) + #expect(fixture.state.viewOffsetPixels.current() == fixture.state.viewOffsetPixels.target()) + } + + /// `MotionPolicy.snapshot()` returns `.enabled`, so this is the configuration the + /// runtime move path actually uses. Here `animateToOffset` installs a spring: + /// the target moves to the snap while the current offset stays put for the + /// display-link driver to interpolate. + @Test func revealWithMotionEnabledSchedulesAnimationWithoutMovingCurrentOffset() { + var fixture = rebasedFixture() + let currentBefore = fixture.state.viewOffsetPixels.current() + let targetBefore = fixture.state.viewOffsetPixels.target() + + _ = fixture.reveal(movedColumn, trigger: .automatic, motion: .enabled) + + #expect(fixture.state.viewOffsetPixels.isAnimating) + #expect(fixture.state.viewOffsetPixels.target() != targetBefore) + #expect(abs(fixture.state.viewOffsetPixels.current() - currentBefore) <= 1) + } + + /// The plan-build step in `NiriLayoutHandler` decides whether to emit a + /// `.startNiriScroll` directive by testing + /// `abs(viewOffsetPixels.current() - offsetBefore) > 1`. Under the production + /// motion value the reveal moves only the target, so that comparison does not + /// observe the scheduled scroll — while `isAnimating` and the target delta do. + /// + /// This records the asymmetry between the three signals rather than asserting a + /// desired directive outcome. + @Test func currentOffsetDeltaDoesNotObserveAnAnimatedScroll() { + var fixture = rebasedFixture() + let offsetBefore = fixture.state.viewOffsetPixels.current() + + _ = fixture.reveal(movedColumn, trigger: .automatic, motion: .enabled) + + let currentDelta = abs(fixture.state.viewOffsetPixels.current() - offsetBefore) + let targetDelta = abs(fixture.state.viewOffsetPixels.target() - offsetBefore) + + #expect(currentDelta <= 1) + #expect(targetDelta > 1) + #expect(fixture.state.viewOffsetPixels.isAnimating) + } + + // MARK: - Fixture + + /// Four 1150pt columns with a 20pt gap at x = 0, 1170, 2340, 3510, seen through a + /// 2560pt viewport — the destination workspace geometry from the reproduction. + /// The moved window lands in the last column. + private let movedColumn = 3 + + private struct Fixture { + let engine: NiriLayoutEngine + let columns: [NiriContainer] + let gap: CGFloat + let viewportWidth: CGFloat + var state: ViewportState + + var context: ViewportSnapContext { + state.snapContext(columns: columns, gap: gap, viewportWidth: viewportWidth) + } + + func visibility(of columnIndex: Int) -> ColumnVisibility { + let context = context + return context.visibility( + of: columnIndex, + viewportOffset: context.currentViewStart(in: state), + in: state + ) + } + + mutating func reveal( + _ columnIndex: Int, + trigger: RevealTrigger, + motion: MotionSnapshot = .disabled + ) -> Bool { + engine.scrollToReveal( + columnIndex: columnIndex, + isFFM: false, + state: &state, + context: context, + motion: motion, + trigger: trigger + ) + } + } + + /// The pre-transfer destination state: selection on column 2, viewport at -20. + private func makeFixture() -> Fixture { + let engine = NiriLayoutEngine() + engine.revealStyle = .auto + engine.animationClock = AnimationClock() + let workspaceId = UUID() + + var previous: NiriNode? + for pid in pid_t(801) ... pid_t(804) { + previous = engine.addWindow( + handle: makeTestHandle(pid: pid), + to: workspaceId, + afterSelection: previous?.id + ) + } + + let columns = engine.columns(in: workspaceId) + for column in columns { + column.width = .fixed(1150) + column.cachedWidth = 1150 + } + + var state = ViewportState() + state.animationClock = engine.animationClock + state.activeColumnIndex = 2 + state.viewOffsetPixels = .static(-2360) + + return Fixture( + engine: engine, + columns: columns, + gap: 20, + viewportWidth: 2560, + state: state + ) + } + + /// `makeFixture()` after the view-neutral rebase onto the moved window's column: + /// `activeColumnIndex` 3 with `viewOffsetPixels` -3530, i.e. `viewStart` -20. + private func rebasedFixture() -> Fixture { + var fixture = makeFixture() + fixture.state.activeColumnIndex = movedColumn + fixture.state.viewOffsetPixels = .static(-3530) + return fixture + } +} From 7acf5de6f04db3e0fd563da25ce6ad2b33711643 Mon Sep 17 00:00:00 2001 From: Luke Amdor Date: Thu, 20 Aug 2026 08:46:45 -0500 Subject: [PATCH 2/2] Observe an animated reveal when deciding the viewport moved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A window moved to another workspace could stay parked off the destination viewport while still holding keyboard focus, so the user saw a different window than the one they moved. ensureSelectionVisible rebases the viewport offset so retargeting activeColumnIndex does not shift what is on screen, then calls scrollToReveal to bring the newly selected column into view. The rebase is view-neutral by design, so that reveal is the only step that can move the viewport onto the moved window's column. How it moves depends on the motion snapshot. With animations disabled animateToOffset writes a static offset and the current offset changes immediately. With animations enabled — the value MotionPolicy.snapshot() returns, so the one this path uses — it installs a spring: the target moves and the current offset stays put for the display-link driver to interpolate. The plan-build step decided whether the viewport had moved by comparing only the current offset against its pre-reveal value. For an animated reveal that comparison is false, so viewportNeedsRecalc stayed unset and the .startNiriScroll directive gated on it was never emitted. Nothing called startScrollAnimation, nothing registered in scrollAnimationByDisplay, and nothing ever drove the spring — leaving the target pointing at the revealed column while the viewport sat where it started. Compare both the current and target offsets against their pre-reveal values, via one shared helper used by both gate sites so they cannot drift apart. A static reveal is still observed through the current-offset delta; an animated one is now observed through the target delta. This is a candidate fix: the mismatch it removes is measured by tests, but the runtime defect is timing-dependent and has not yet been confirmed fixed in a real reproduction. --- ...iewport-scroll-that-reveals-a-window-mo.md | 6 +++ .../Core/Controller/NiriLayoutHandler.swift | 19 ++++++++- ...dowRevealAfterWorkspaceTransferTests.swift | 42 +++++++++++++------ 3 files changed, 52 insertions(+), 15 deletions(-) create mode 100644 .changeset/20260820100000-drive-the-viewport-scroll-that-reveals-a-window-mo.md diff --git a/.changeset/20260820100000-drive-the-viewport-scroll-that-reveals-a-window-mo.md b/.changeset/20260820100000-drive-the-viewport-scroll-that-reveals-a-window-mo.md new file mode 100644 index 00000000..a41e5c08 --- /dev/null +++ b/.changeset/20260820100000-drive-the-viewport-scroll-that-reveals-a-window-mo.md @@ -0,0 +1,6 @@ +--- +"nehir": patch + +--- + +Fix a window moved to another workspace sometimes staying off screen. When the destination workspace needed to scroll to bring the moved window into view, the scroll was worked out but never started, so the window stayed parked off the edge of the screen while still receiving keyboard input. diff --git a/Sources/Nehir/Core/Controller/NiriLayoutHandler.swift b/Sources/Nehir/Core/Controller/NiriLayoutHandler.swift index 635ae646..4dc19d11 100644 --- a/Sources/Nehir/Core/Controller/NiriLayoutHandler.swift +++ b/Sources/Nehir/Core/Controller/NiriLayoutHandler.swift @@ -822,8 +822,23 @@ enum NiriWindowMoveResult { } let offsetBefore = state.viewOffsetPixels.current() + let targetOffsetBefore = state.viewOffsetPixels.target() var viewportNeedsRecalc = removal.removalResult.viewportNeedsRecalc + // A reveal moves the viewport in one of two ways depending on the motion + // snapshot. With animations disabled `animateToOffset` writes a static + // offset, so the *current* offset changes immediately. With animations + // enabled it installs a spring: the *target* moves and the current offset + // stays put for the display-link driver to interpolate. Watching only the + // current offset therefore misses an animated reveal entirely, and the + // `.startNiriScroll` directive gated on this flag is never emitted — so + // nothing ever drives the spring and the viewport never reaches the + // revealed column. + func viewportMoved() -> Bool { + abs(state.viewOffsetPixels.current() - offsetBefore) > 1 + || abs(state.viewOffsetPixels.target() - targetOffsetBefore) > 1 + } + for col in pass.engine.columns(in: pass.wsId) { if col.cachedWidth <= 0 { col.resolveAndCacheWidth(workingAreaWidth: pass.insetFrame.width, gaps: pass.gap) @@ -869,7 +884,7 @@ enum NiriWindowMoveResult { fromContainerIndex: removal.removalResult.fromIndexForVisibility, revealTrigger: .automatic ) - if abs(state.viewOffsetPixels.current() - offsetBefore) > 1 { + if viewportMoved() { viewportNeedsRecalc = true } } @@ -914,7 +929,7 @@ enum NiriWindowMoveResult { reason: "resolveSelection.centeredViewportCorrection" ) state.preservesUnsnappedGestureOffset = false - if abs(state.viewOffsetPixels.current() - offsetBefore) > 1 { + if viewportMoved() { viewportNeedsRecalc = true } } diff --git a/Tests/NehirTests/MovedWindowRevealAfterWorkspaceTransferTests.swift b/Tests/NehirTests/MovedWindowRevealAfterWorkspaceTransferTests.swift index d58e16c8..97e378fd 100644 --- a/Tests/NehirTests/MovedWindowRevealAfterWorkspaceTransferTests.swift +++ b/Tests/NehirTests/MovedWindowRevealAfterWorkspaceTransferTests.swift @@ -122,26 +122,42 @@ import Testing #expect(abs(fixture.state.viewOffsetPixels.current() - currentBefore) <= 1) } - /// The plan-build step in `NiriLayoutHandler` decides whether to emit a - /// `.startNiriScroll` directive by testing - /// `abs(viewOffsetPixels.current() - offsetBefore) > 1`. Under the production - /// motion value the reveal moves only the target, so that comparison does not - /// observe the scheduled scroll — while `isAnimating` and the target delta do. - /// - /// This records the asymmetry between the three signals rather than asserting a - /// desired directive outcome. - @Test func currentOffsetDeltaDoesNotObserveAnAnimatedScroll() { + /// The plan-build step in `NiriLayoutHandler` decides whether the viewport moved + /// — and so whether to emit a `.startNiriScroll` directive that drives the + /// animation — by comparing offsets against their pre-reveal values. An animated + /// reveal moves only the target, so a current-offset comparison alone does not + /// observe it. Both deltas must be considered, or nothing drives the spring and + /// the viewport never reaches the revealed column. + @Test func animatedRevealIsObservableFromTheOffsetDeltas() { var fixture = rebasedFixture() - let offsetBefore = fixture.state.viewOffsetPixels.current() + let currentBefore = fixture.state.viewOffsetPixels.current() + let targetBefore = fixture.state.viewOffsetPixels.target() _ = fixture.reveal(movedColumn, trigger: .automatic, motion: .enabled) - let currentDelta = abs(fixture.state.viewOffsetPixels.current() - offsetBefore) - let targetDelta = abs(fixture.state.viewOffsetPixels.target() - offsetBefore) + let currentDelta = abs(fixture.state.viewOffsetPixels.current() - currentBefore) + let targetDelta = abs(fixture.state.viewOffsetPixels.target() - targetBefore) + // The current offset alone is not enough: it has not moved yet. #expect(currentDelta <= 1) + // The target delta is what makes the scheduled scroll observable. #expect(targetDelta > 1) - #expect(fixture.state.viewOffsetPixels.isAnimating) + #expect(max(currentDelta, targetDelta) > 1) + } + + /// The same combined check must also observe a static reveal, so the gate keeps + /// working when animations are disabled. + @Test func staticRevealIsObservableFromTheOffsetDeltas() { + var fixture = rebasedFixture() + let currentBefore = fixture.state.viewOffsetPixels.current() + let targetBefore = fixture.state.viewOffsetPixels.target() + + _ = fixture.reveal(movedColumn, trigger: .automatic, motion: .disabled) + + let currentDelta = abs(fixture.state.viewOffsetPixels.current() - currentBefore) + let targetDelta = abs(fixture.state.viewOffsetPixels.target() - targetBefore) + + #expect(max(currentDelta, targetDelta) > 1) } // MARK: - Fixture