Repository navigation
fix(hidden-bar): pause rehide for native popovers - #978
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Window bounds and menu detection Sources/OmniWM/UI/HiddenBar/HiddenBarMenuGuard.swift, Tests/OmniWMTests/HiddenBarMenuGuardTests.swift |
The guard checks owned status-level windows outside supplied menu-bar frames, while retaining popup-level detection. Tests cover owner and layer mismatches, panel titles, and displays with differing menu-bar geometry. |
Screen frames and reconcealment Sources/OmniWM/UI/HiddenBar/MenuBarItemService.swift, Tests/OmniWMTests/HiddenBarLifecyclePolicyTests.swift |
The service computes a menu-bar frame for each screen and passes the frames to the guard. A lifecycle test checks that the app remains revealed while a simulated panel is open and is concealed after it closes. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Possibly related PRs
- OmniNull/OmniWM#976: Covers a native app panel closing when Hidden Bar re-hides its item, the behavior addressed by this change.
Suggested reviewers: barutsrb, dappermint
Merge Risk: 
d2940
-
In apps that keep a separate status-level window open, Hidden Bar may remain revealed for up to a minute after the popover closes. This bounded edge case does not appear to block merging.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title names the main change: rehide is paused for native popovers in the hidden-bar feature. It follows the conventional commit format and is short and specific. The changeset adds status-level wi… Description check The description follows the template. It has Problem, Result, and Verification sections. The Problem section states the bug and links related issues ( #976,#944). The Result section explains user-visi…Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the main change: rehide is paused for native popovers in the hidden-bar feature. It follows the conventional commit format and is short and specific. The changeset adds status-level window recognition to HiddenBarMenuGuard and MenuBarItemService, so the title matches the primary change.
Full details: Description check
-
Explanation
The description follows the template. It has Problem, Result, and Verification sections. The Problem section states the bug and links related issues (
#976,#944). The Result section explains user-visible behavior: status-level windows from revealed apps outside the menu-bar strips hold the countdown, while status items, ordinary windows, and other processes do not. The Verification section lists the commands run, test counts, a negative control, and a live AppKit probe, and it notes that Vorssaint was not run. The template's optional screenshot section is not used, which is acceptable because it is optional. The description covers the required information.
-
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
BarutSRB
left a comment
BarutSRB
left a comment
There was a problem hiding this comment.
Thanks, this is a really nice follow-up to the panel that kept closing in #976. The virtual-clock tests and the live NSPopover check make it easy to trust.
The added status-level check counts only windows owned by revealed apps, with nonempty bounds that are not wholly contained in any menu-bar strip. The existing 60 s watchdog still bounds reconcealment. OmniWM’s own bars and popouts cannot trigger this check through the production owner filter, including when its bar is full width.
A few optional things, only if you touch it again:
- Defaults that switch the check off.
WindowInfo.boundsandmenuBarFrameshave defaults that turn the new check off. Production always passes both, so making them required, and updating the old call sites, would keep a caller from skipping it by accident. Right nowtestStatusAndNormalLayersIgnoredstays green only through those defaults. Reshaping it to the real rule, that a status-level window with empty bounds or inside a strip is ignored, would make it say what it checks. - Strip geometry. The tests don't reach the strip calculation in
MenuBarItemService.isMenuOpen(thickness, safe-area top, visible-frame gap, thentoWindowServer), because they pass hand-written rects. A small pure helper with a notched case and an offset case would cover the geometry the description mentions. - Retain cycle. In the lifecycle test, the sleeper closure captures
hiddenBarstrongly throughreconcealment, so the controller isn't freed aftercleanup().[weak hiddenBar]fixes it.
None of these needs to hold this up. Approving.
Bring the style branch up to date with main. Hidden Bar now keeps a revealed item while its app shows a native popover at the status window level outside the menu bar, so panels such as an app's popover no longer close when the rehide countdown runs out. The countdown resumes once the panel closes, and the existing watchdog still bounds it (#978).
Problem
Hidden Bar can re-conceal an item while its native popover is still open. AppKit popovers can use the status-window level rather than the popup-menu levels that the rehide guard recognized.
Follow-up to the remaining panel-dismissal problem reported in #976; relates to #944.
Result
Recognize status-level windows belonging to the revealed apps when they extend outside the displays' native menu-bar strips. Native popovers then use the existing menu countdown and watchdog rules. Status-item windows, ordinary application windows, and windows from other processes do not hold the countdown.
The strips are captured on the main actor using each display's current menu-bar reservation, status-bar thickness, and notch inset; the window query remains off the main actor.
Verification
make verify: passed.swift test: 6718 tests, 26 skipped, 0 failures.NSPopovercheck on both physical displays: the native window used level 25. With a five-second countdown, the baseline re-concealed its anchor at about 5.25 seconds while the popover was open. The patched guard retained the anchor through seven seconds; after the probe closed the popover at eight seconds, it re-concealed at about 13.5 seconds.Vorssaint itself was not run on this machine. The live probe checks native window classification and rehide gating, rather than the third-party app's complete activation and dismissal flow.
Summary by CodeRabbit