Skip to content

Classic/Original have no off-screen window rescue (gated out of feat/winamp-modern; restore it corner-consistently) #431

Description

@ad-repo

Summary

Classic and Original have no recovery for a window that ends up off screen. A rescue for exactly
this shipped on feat/winamp-modern (f0e5dc14"a window that launches off screen finds its own
way back"
), but it was written for .wal window management, applied to all four UI modes ungated,
and carried a defect. It has now been gated to Winamp Modern only, so Classic and Original are
back to the pre-branch behaviour: a window stranded by a resolution change, an unplugged monitor, or
a resized Dock stays stranded, and Snap To Default is the only way back.

This issue tracks bringing that rescue to Classic and Original properly — on its own PR against
main, corner-consistent, with its own tests and live pass.

Why it was gated rather than kept

Found during the pre-release review of feat/winamp-modern
(docs/winamp-modern/branch-review-stage-1.md, Finding 1).

The placement rule the rescue is built on defines "on screen" as the window's top-left corner
(WindowPlacement.isReachable), and WindowPlacement.swift is explicit about why:

it leaves the classic habit of parking a window mostly past the bottom or right edge intact — that
window is placed, not stranded, and a sweep that yanked it back would be the bug.

One path did not honour that rule. AppStateManager.correctedRestoredFrames(_:screens:force:) takes
a force flag from savedScreenIsMissing, which compares the saved visibleFrame to the current
ones with exact NSRect equality. When force is set, the correction runs over frames that are
all perfectly reachable and routes them through WindowPlacement.rescued, which measures the whole
rect
— so it moves them.

Measured on a 1440×850 screen, with nothing stranded:

resizing the Dock alone (visibleFrame 850 → 800)  ⇒  savedScreenIsMissing = true

force = true, both windows already reachable:
  main      {100,  400} 275×116  →  {100, 580}      whole cluster shifted +180
  playlist  {100, -180} 275×232  →  {100,   0}

force = true, right-parked, already reachable:
  main      {1300, 700} 275×116  →  {1165, 700}     shifted −135

So a user who resized or hid their Dock would have their whole saved layout moved on the next
launch, undoing deliberate window placement. Two things bounded it and neither removed it:
mainScreenVisibleFrame is a new key, so the effect starts on the second launch after upgrade; and
the force: true path had no test at all — the three existing tests all pass force: false.

This is the shape of B56, where a screen clamp added for .wal placement moved Classic's
sub-windows too. Per the rule in CLAUDE.md and skills/winamp-modern-skin-guide/SKILL.md, shared
code is gated on the mode rather than justified as a no-op, so it was gated.

What is worth keeping

The rest of the family is well-built and is the model to follow. ensureAllWindowsOnScreen(),
rescuedOrigin(for:) and restoreWindowPositions()'s onScreen() helper are all consistently
corner-based
: they skip any window that already passes isReachable, so a deliberately parked
window is left alone. Only the force path breaks the rule.

The docked-cluster handling is also worth keeping as-is: when one window in a cluster is stranded,
the whole cluster moves by one shared offset, so docking survives. Clamping window by window is what
would break it — two windows flush against each other, clamped independently against the same edge,
come back overlapping instead of touching.

Proposed work

  1. Make the force path corner-consistent — under force, still only move frames that fail
    isReachable. Note this narrows force to nearly nothing, since a session saved on a screen that
    is genuinely gone already has unreachable frames and trips stranded without help. It may be
    simpler to drop force and savedScreenIsMissing entirely and decide on stranded alone; that
    choice should be made deliberately rather than inherited.
  2. Ungate for Classic and Original: WindowManager.appliesWinampModernPlacement currently gates
    ensureAllWindowsOnScreen (internally and at all four call sites), the restore-frame correction,
    restoreWindowPositions's rescue, and the didChangeScreenParameters hook.
  3. Decide separately whether snapToDefaultPositions should also come back for Classic. Four
    changes were reverted there — screen.framescreen.visibleFrame, stack-overrun top-anchoring,
    side-window clamps, and a rescue pass. The visibleFrame change in particular reverses a
    deliberate documented choice
    ("Use full screen frame (not visibleFrame) so windows aren't
    constrained by menu bar/dock"
    ) and is a visible change to every Classic snap result, so it wants
    its own decision rather than riding along.

Acceptance criteria

  • A window stranded off screen in Classic finds its way back after a resolution change, an
    unplugged monitor, a Dock resize, and a session restore.
  • A window deliberately parked mostly past the bottom or right edge is left exactly where the
    user put it, in every one of those cases.
  • A docked cluster stays docked through a rescue — members still flush, not overlapping.
  • Tests cover the force path (or its replacement) in Classic, including the parked-window case.
    Tests/NullPlayerAppTests/WinampModernPlacementGatingTests.swift pins the current gated
    behaviour and will need updating as part of this.
  • Live pass in Classic and Original, per winamp-modern-skin-guide's "verify in the running app,
    not in your head" — window geometry has no useful armchair form.

References

  • docs/winamp-modern/branch-review-stage-1.md — Findings 1, 2 and 5, and the Dispositions table.
  • Sources/NullPlayer/App/WindowPlacement.swift — the corner rule and why it exists.
  • f0e5dc14 on feat/winamp-modern — the original fix.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions