fix(menubar): stop double-counting NSStatusItemSpacing in notch overflow budget#573
Merged
Merged
Conversation
…low budget The notch overflow check in `applyProfileLayout` was subtracting `(count - 1) * userSpacing` from `availableWidth` while the per-item widths it summed into `profileBaseline` already had the spacing baked in by macOS. The deduction therefore over-counted layout cost by `(count - 1) * userSpacing` pixels. The error scaled linearly with the spacing value, so it was invisible at `offset=-12` (spacing `4`) but ejected several profile items into the hidden section at `offset=0` (the macOS default of `16`) even when the menu bar visibly had room. Applying the Built-in Retina Display profile at default spacing pushed six items into hidden while leaving roughly 200px of empty bar between the rightmost visible item and Control Center. Empirical measurements taken from the `Notch overflow budget:` diagnostic log with the same 11 profile items at three spacing values (`-12`, `-6`, `0`) showed the average reported `item.bounds.width` rising linearly from `24.8` to `31.2` to `36.8`, an implied icon-only width of about `20.8` plus exactly the current spacing value. The Control Center item's `bounds.minX` (used as `rightBoundary`) also shifted left by about `2px` per `1px` of spacing for the same reason. Both inputs to the budget already reflect the spacing, so the explicit `(count - 1) * userSpacing` deduction was pure double-counting. This change removes that deduction and rewrites the comment block above `userSpacing` to record the empirical relationship instead of the inverted assumption that was there before. `userSpacing` itself is retained because the `Notch overflow budget:` diagnostic line continues to log it. With the fix in place the Built-in Retina apply at `offset=0` reports `availableWidth=424` against `profileBaseline=419`; the profile fits, no overflow is triggered, and the visible section matches what the bar actually has room for. Signed-off-by: Amir Zarrinkafsh <3339418+nightah@users.noreply.github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughFix double-counting of NSStatusItemSpacing in notch-overflow budgeting by treating status item bounds as already including inter-item spacing and removing the extra spacing subtraction; also add an explicit ChangesNotch-Overflow Budgeting
DMG Build Workflow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Specify Xcode version to resolve compatibility issues.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Removes a double-count of
NSStatusItemSpacingin the notch overflow budget so that profile applies at the macOS default spacing (offset=0) no longer push items into the hidden section while the menu bar visibly has room.PR Type
Does this PR introduce a breaking change?
If yes, please describe the impact and migration path:
N/A
What is the current behavior?
When
applyProfileLayoutruns on a notched display with the overflow toggle enabled, it computesavailableWidth = rightBoundary - notchGap - nonProfileFootprint - (count - 1) * userSpacingand compares against aprofileBaselinesummed from each item'sbounds.width. macOS bakesNSStatusItemSpacinginto each status item's frame, sobounds.widthalready includes the spacing padding, andrightBoundary(Control Center'sbounds.minX) already shifts to absorb CC's own spacing growth. The explicit(count - 1) * userSpacingsubtraction therefore deducts the spacing a second time, under-countingavailableWidthby(count - 1) * userSpacingpixels. Atoffset=-12(spacing4) the error is small (around 48px on the typical layout) and absorbed by historical slack; atoffset=0(spacing16) the error grows to roughly 192px and forces six profile items into the hidden section despite a visibly empty bar between the rightmost visible item and Control Center.Issue Number: N/A
What is the new behavior?
The
(count - 1) * userSpacingdeduction is removed. The budget now relies onitem.bounds.widthandccItem.bounds.minXto carry the spacing footprint, which the diagnostic log shows they already do. With the fix the Built-in Retina apply atoffset=0reportsavailableWidth=424againstprofileBaseline=419, well within budget, and the previously-ejected items remain in the visible section. Atoffset=-12andoffset=-6the budget gains additional slack but profiles that already fit continue to fit, so existing layouts are unchanged.PR Checklist
Other information
Empirical width measurements collected from the
Notch overflow budget:diagnostic log, same 11 profile items each run:Average
item.bounds.widthrises linearly at 1:1 withuserSpacing, implying an icon-only width of about20.8for this profile plus the current spacing. With the fix,availableWidthatoffset=0becomes232 + 12 * 16 = 424, exceedingprofileBaseline=419by 5px and matching what the screen actually has room for.Notes for reviewers
Focus areas:
userSpacingis retained so the existingNotch overflow budget:log line continues to report the system spacing for diagnostics.applyProfileLayouttoday; verification was manual atoffset = -12,-6, and0on the Built-in Retina display, with theNotch overflow budget:diagnostic line captured at each step (values above).0.Notch overflow budget:logsavailableWidth=232.Notch overflow budget:logsavailableWidth=424.Summary by CodeRabbit
Bug Fixes
Chores