Skip to content

NSWindow: skip the screen-parameters remap when the frame already fits the new screen - #1008

Merged
fredkiefer merged 2 commits into
gnustep:masterfrom
e4779:nswindow-remap-guard
Oct 18, 2026
Merged

fredkiefer merged 2 commits into
gnustep:masterfrom
e4779:nswindow-remap-guard

Conversation

@e4779

@e4779 e4779 commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Implements the fix proposed in #1006.

applicationDidChangeScreenParameters: remaps the window frame by the old→new screen delta, reading oldScreenFrame from the window's cached _screen. As analyzed in #1006, that cached screen can already reflect the new state when the handler reads it: the screen list is rebuilt during the notification dispatch, and -screen's recompute — reachable e.g. via [NSScreen mainScreen] → [keyWindow screen] — refreshes later observers' _screen to the new frame before their own handlers run. The delta then collapses to zero exactly when a remap was needed, and the previous-screen frame is re-applied against the new screen height — the window moves by exactly the height difference.

This adds the minimal guard from #1006: if the frame already fits the new screen, skip both the remap and the re-apply — the window has already been realigned against it (or sits on an unaffected screen). The delta path is unchanged for frames that do not fit.

Validation (Xvfb + RANDR, WM-less, paired with the backend-side realignment from gnustep/libs-back#251): a window's title bar across a 1280×800 → 1024×768 → 1280×800 cycle moves 271 → 270 → 270 px (a 1 px rounding difference), stable across repeated runs and multi-cycle sequences; without the guard the window jumps ±32 px on every cycle.

applicationDidChangeScreenParameters: remaps the frame by the
old->new screen delta, but the window's cached _screen can already
reflect the new screen when the handler reads it (the screen list is
rebuilt during the notification dispatch, and -screen recomputes
against it), so the delta collapses to zero exactly when a remap is
needed and the previous-screen frame is re-applied against the new
screen. If the frame already fits the new screen it has been realigned
already (or sits on an unaffected screen): skip both the remap and the
re-apply in that case. Details and analysis: gnustep#1006.
@e4779
e4779 requested a review from fredkiefer as a code owner October 16, 2026 04:51
@rfm

rfm commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

If I understad the general idea, isn't NSContainsRect() the wrong test?
I would expect the test to be a check for equality of the two frames (with a small margin to allow for floating point errors).

@rfm

rfm commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

(with a small margin to allow for floating point errors)
I haven't checked: that would only be necessary if the two frames have passed through different could which could have introduced different floating point rounding errors. If they can't have different errors then a simple equality test would be all that's needed.

@e4779

e4779 commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor Author

Yes, NSContainsRect() is the wrong test — a frame can fit the new
screen and still need the remap (a mid-screen window keeps its top
anchor on a screen that grows, and the skip would change that). The
frame-equality test you describe also has a hole on the other side, and
unfortunately both holes are reachable — we have traces of each:

  1. The zero-delta case your test catches: the window's cached _screen
    can already hold the NEW frame when the handler reads it (the screen
    list is rebuilt during the notification dispatch and -screen's
    recompute refreshes it), so old == new, the delta collapses, and the
    previous-screen frame is re-applied against the new screen — the
    window moves by the height difference. An old == new screen-frame
    test skips this correctly.

  2. The double-compensation case it does not: with the backend-side
    realignment from RandR screen change: collapse the notify burst onto the newest event, refresh screen data, realign windows from server truth libs-back#251 the frame is already corrected
    (the realign sends GSAppKitWindowMoved) before this handler runs,
    while _screen may still be the pre-change instance — the screen
    frames differ, the delta is nonzero, and applying it on top of the
    corrected frame moves the window by the height difference again (we
    measured exactly this while developing the pair).

The unambiguous signal seems to be "this frame was already realigned",
not a geometric relation. The GSAppKitWindowMoved handler in NSWindow
would mark the window's frame as current (an ivar flag), and
applicationDidChangeScreenParameters: would skip the remap for marked
windows and clear the mark. A moved event means "this origin is
authoritative" — including user drags — so trusting the latest frame
over the remap seems safe.

I could rework the PR in that direction — or whichever you prefer.

Comment thread Source/NSWindow.m Outdated
[self _applyFrame: newFrame];
/* Skip the remap when the frame already fits the new screen:
it has already been realigned against it. */
if (NSContainsRect(newScreenFrame, newFrame) == NO)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, it would be clearer to have newFrame only inside the if and use _frame here.

@fredkiefer fredkiefer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just move the variable, the rest is fine.

Per review: clearer to have newFrame only inside the if.
@e4779

e4779 commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

Done — newFrame now lives inside the if and the condition tests _frame directly (deddd72).

@e4779
e4779 requested a review from fredkiefer October 16, 2026 16:28

@fredkiefer fredkiefer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some cases we now will do to little and in others to much. But as you pointed out the screen the window holds may already be updated. So there is no way to clearly tell, how much the frame should be changed.
Richard's idea to see, whether the frame of the old and the new screen differ at all, will also not work.

@fredkiefer
fredkiefer merged commit e169449 into gnustep:master Oct 18, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants