Repository navigation
fix(macos): restore donut window class on hide and before close - #808
Conversation
|
Thanks for the fix, @wavever. I went through it and checked a few things on my Mac. Some notes:
One small request: next time, a short comment on the issue when you start on a fix would be much appreciated. I'd started looking into this one too, and it helps avoid two people working on the same thing. |
KnifeLemon
left a comment
There was a problem hiding this comment.
Thanks for the quick PR, @wavever! The diagnosis is spot on, and the test doubles make a nice regression test for "the class stays the same".
The catch is the trade-off you describe: removing the swap brings back #251. With Filee in the Dock (the default), the donut no longer appears over a full-screen app, which is how many Mac users work. I'd rather keep both working, so before taking the removal, could you try the smaller fix?
Restore the class before the window is released
The abort happens because the window is deallocated as an AvnPanel that never ran NSPanel's initialization. If it is turned back into an AvnWindow before AppKit releases it, dealloc runs the path matching its init:
- On close: add something like
NativeMethods.RestoreWindow(nint window). Ifobject_getClass(window) == AvnPanel, it clears the nonactivating style bit and sets the class back toAvnWindow. Call it when the donut window closes on macOS (theClosing/Closedpath at quit). Your harness can check that release then exits 0. - Simpler alternative: restore it every time the donut hides, and swap only while it's on screen. The window is then never released or reused as a panel.
Please check
- Your minimal AppKit program: swap, then restore, then release exits 0.
- In Filee on macOS: show and hide the donut a few times, then quit (⌘Q, Dock → Quit, and logging out). No crash report.
- #251: with the Dock icon on, Ctrl-drag over a full-screen app (Safari or Finder in full screen) still shows the donut there.
If restoring turns out not to be enough, say so and we'll weigh the removal against #251 again. I've approved the CI run. Thanks!
|
Thanks for checking this. You're right: removing the swap regresses #251, so this isn't a complete fix. I also got the PopupImpl/NSPanel behavior wrong—thanks for catching that. Restoring the original class and style before teardown seems worth testing, while preserving full-screen behavior with both Dock settings. I can revise the PR in that direction. Does that align with what you're already working on? Sorry for the duplicated effort; I'll coordinate on the issue before further changes. |
|
Yes, please go ahead with that direction, @wavever. Your branch is the one to continue, and @SeoJCalive can test it on his Mac. Change: put the window back to Please check:
Keep the regression test you wrote, adjusted: the class may be @SeoJCalive, since you couldn't reproduce it on 26.6.2, could you run the #251 full-screen check on the revised build when it's up? |
|
CI is green on the revised commit (restore the class on hide and before close). Thanks, @wavever. @SeoJCalive 수정된 빌드가 준비됐고 CI도 모두 통과했어요. Mac에서 확인 부탁드려요:
결과 알려주시면 리뷰 상태 정리하고 머지할게요. |
|
@KnifeLemon 수정본(
|
|
Thanks for testing this, @SeoJCalive. @KnifeLemon, I've also tested the revised build on macOS 27.0.1 and haven't seen the crash recur. Since the original crash was intermittent and I don't have reliable reproduction steps, I can't confirm it's fully resolved yet, but so far it looks good. I'll report back if it happens again. |
KnifeLemon
left a comment
There was a problem hiding this comment.
Thanks, @wavever and @SeoJCalive. SeoJ's run on macOS 26.6.2 covers #251 (the donut over a full-screen Space, with and without the Dock icon) and the four quit cases, and wavever's run on macOS 27.0.1 hasn't shown the crash again. CI is green on every platform.
Since the crash was intermittent, this closes #805 on merge. @wavever, if it happens again, please reopen #805 with the crash report.


What does this change?
Fixes #805. Keep the donut as
AvnPanelwhile visible for full-screen support (#251). When it hides and before it closes, clear the nonactivating style and restoreAvnWindowso native destruction matches its initialization. Restoration is idempotent.The controller handles actual visibility changes and
Closing, including quitting while the donut is still on screen. Tests cover both hooks and repeated native class/style transitions.Type of change
How was it tested?
RadialController/RadialWindow: repeated show/hide, then quit while visible. Dock on/off × managed shutdown, native application termination and Quit Apple Event: all six cases exit 0.Full-screen Finder/Safari drag check (#251): awaiting the collaborator's check on the revised build, as arranged in the review.
Checklist