Fix vertical centering of the stacked menu bar title - #942
Conversation
NSStatusBarButton is an NSButton whose cell only reliably centers a single line, so the two-line "time under title" status-bar layout was rendered as a multi-line attributedTitle that the cell top-aligns. It was nudged into place with hardcoded point constants (lineHeightMultiple 0.7 and baselineOffset -3) tuned to one menu-bar height, which drift off-center on other displays, macOS versions, and notch/Retina scales. Render the stacked layout as a single image instead: draw the icon and both lines into one NSImage sized to at least the menu-bar height and center it ourselves. The button centers a single image by its bounds, so it is correct on every menu-bar height and scale. The image is non-template (colored meeting-service icons are preserved) with dynamic system text colors, and template icons are tinted at draw time so it adapts to light/dark; the tooltip and accessibility label carry the full title and time for VoiceOver. Remove the now-unused stackedTitle(for:) and its orphaned baselineOffset parameter, reusing the shared titleAttributes helper for the stacked title styling; update the status-bar tests accordingly.
dosubot
Bot
added
the
size:L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR replaces stacked menu-bar title attributed text with a vertically centered image containing the icon, title, and countdown. The controller uses image-only rendering, accessibility labeling is updated, regression tests cover sizing and centering, and the changelog records the fix. ChangesStacked Menu Bar Title Rendering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant StatusBarItemController
participant StatusBarTitleRenderer
participant StatusItemButton
StatusBarItemController->>StatusBarItemController: renderStatusBar()
StatusBarItemController->>StatusBarTitleRenderer: stackedImage(title, time, icon, style)
StatusBarTitleRenderer-->>StatusBarItemController: combined NSImage
StatusBarItemController->>StatusItemButton: set image and imageOnly positioning
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MeetingBar/UI/StatusBar/StatusBarItemController.swift`:
- Around line 250-264: The accessibility label in StatusBarItemController’s
stacked nextEvent branch can end with a dangling comma when presentation.time is
empty. Update the label construction around button.setAccessibilityLabel so it
only includes the time separator when presentation.time is present, using
presentation.tooltip or presentation.title as the base and formatting the final
string conditionally.
- Around line 236-241: The status bar fallback logic in StatusBarItemController
is only checking for a nil image, so when the sentinel “no_online_session” icon
is hidden via imagePosition = .noImage the item can still render blank. Update
ensureStatusBarButtonIsVisible to treat the sentinel icon as unavailable as
well, and centralize the raw “no_online_session” asset name into a shared
constant used by the sentinel check in this code path.
- Around line 605-620: The stacked title layout in StatusBarItemController’s
sizing/drawing logic still treats an empty title string as a real line, which
leaves extra vertical space and shifts the block off center. Update the block
height calculation and the rect placement in the same draw/measure path so empty
strings are treated like absent lines, using the existing title/time handling in
the StatusBarItemController rendering code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0220033c-6289-4d5e-b223-33cde885a008
📒 Files selected for processing (3)
CHANGELOG.mdMeetingBar/UI/StatusBar/StatusBarItemController.swiftMeetingBarTests/StatusBarItem/MenuBuilderTests.swift
| case .none: | ||
| break | ||
| iconImage = nil | ||
| } | ||
| iconImage?.size = MenuStyleConstants.iconSize | ||
| // "no_online_session" is the sentinel asset meaning "show no icon". | ||
| let hasNoSessionIcon = iconImage?.name() == "no_online_session" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where "no_online_session" is defined/used to confirm intended contract
rg -n "no_online_session" --type=swiftRepository: leits/MeetingBar
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the surrounding implementation.
ast-grep outline MeetingBar/UI/StatusBar/StatusBarItemController.swift --view expanded || true
wc -l MeetingBar/UI/StatusBar/StatusBarItemController.swift
sed -n '220,310p' MeetingBar/UI/StatusBar/StatusBarItemController.swift
# Find related status bar item/icon/title handling.
rg -n "ensureStatusBarButtonIsVisible|imagePosition|attributedTitle|button\.image|MenuStyleConstants\.iconSize|iconNamed|no_online_session|stacked" MeetingBar -g '*.swift'Repository: leits/MeetingBar
Length of output: 11988
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect title rendering and presentation states that could make title/attributedTitle empty.
sed -n '520,690p' MeetingBar/UI/StatusBar/StatusBarItemController.swift
sed -n '1,420p' MeetingBar/UI/StatusBar/StatusBarPresentation.swift
sed -n '1,120p' MeetingBar/Utilities/Constants.swiftRepository: leits/MeetingBar
Length of output: 25552
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,70p' MeetingBar/UI/StatusBar/StatusBarItemController.swift
sed -n '1,240p' MeetingBar/Meetings/MeetingServices.swiftRepository: leits/MeetingBar
Length of output: 7490
Keep the fallback active when the sentinel icon is hidden
imagePosition = .noImagehidesno_online_session, butensureStatusBarButtonIsVisibleonly falls back onbutton.image == nil. In the.noneicon + empty title path, the status item can render blank.no_online_sessionis duplicated as a raw string; centralize it as a constant so the sentinel check stays in sync with the asset name.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MeetingBar/UI/StatusBar/StatusBarItemController.swift` around lines 236 -
241, The status bar fallback logic in StatusBarItemController is only checking
for a nil image, so when the sentinel “no_online_session” icon is hidden via
imagePosition = .noImage the item can still render blank. Update
ensureStatusBarButtonIsVisible to treat the sentinel icon as unavailable as
well, and centralize the raw “no_online_session” asset name into a shared
constant used by the sentinel check in this code path.
There was a problem hiding this comment.
Real issue, but out of scope for this PR — two notes:
-
The described trigger (
.noneicon) isn't the actual path:.nonesetsiconImage = nil, sohasNoSessionIconstays false and the icon isn't suppressed. The real blank-item path is.meetingService(nil), which returns theno_online_sessionsentinel image and setsimagePosition = .noImage; becauseensureStatusBarButtonIsVisibleonly falls back whenbutton.image == nil, the item can render blank. -
This is pre-existing on
master— this PR only reworks the stacked-title branch and doesn't touch that code path — so I'll track it in a separate follow-up rather than widen this PR. The suggestion to reuseEventTitleIconFormat.none.rawValueinstead of the raw literal will go into that same follow-up.
There was a problem hiding this comment.
@WGlegola Thanks for the correction — you're right, .none maps to iconImage = nil, so hasNoSessionIcon stays false in that branch. The actual blank-item path is .meetingService(nil) returning the no_online_session sentinel with imagePosition = .noImage, which ensureStatusBarButtonIsVisible doesn't currently catch since it only checks button.image == nil. Appreciate you pinning down the precise trigger.
Agreed on deferring this to a follow-up since it's pre-existing on master and unrelated to the stacked-title rework here. Want me to open a GitHub issue for it now (covering both the fallback fix and switching the raw "no_online_session" literal to EventTitleIconFormat.none.rawValue)? Just let me know and I'll file it with a link back to this PR and comment.
Treat an empty title/time line as zero height so a lone line stays centered instead of being pushed off-center by space reserved for the missing line. Add ink-margin tests asserting the stacked block is vertically centered.
Resolve StatusBarItemController conflict with leits#952: only .asset icons are normalized to the menu-bar icon size; meeting-service icons keep the provider's own size. Stacked-title centering fix is preserved.
1 participant
Status
READY
Description
The two-line "time under title" menu-bar layout (event name over the countdown)
rendered off-center — the text sat high/cramped and drifted differently across
displays. Root cause: it was drawn as a multi-line
NSAttributedStringon thestatus item's button, but
NSStatusBarButton(anNSButton/NSButtonCell) onlyreliably vertically-centers a single line. The two-line title was top-aligned
and nudged into place with hardcoded point constants (
lineHeightMultiple 0.7,baselineOffset -3) tuned to one menu-bar height, so it broke on other menu-barheights, macOS versions, and notch/Retina scales.
This renders the stacked layout as a single image instead: the icon and both
lines are drawn into one
NSImagesized to at least the menu-bar height andcentered ourselves. The button centers a single image by its bounds, so it's
correct on every menu-bar height and display scale. The image is non-template, so
colored meeting-service icons are preserved (as before); text uses dynamic system
colors and template icons (e.g. the calendar) are tinted at draw time, so it
adapts to light/dark (
cacheMode = .never). The tooltip and accessibility labelcarry the full (untruncated) title + time for VoiceOver. Only the stacked layout
changes; the single-line/inline layout is untouched. The now-unused
stackedTitle(for:)and its orphanedbaselineOffsetparameter are removed, andthe stacked title styling reuses the existing
titleAttributeshelper.No dependency, entitlement, signing, URL-scheme, script, workflow, or
base-localization changes.
Validation:
make lint(0 violations),make test-logic(208 pass), fullapp-hosted suite (399 pass).
Checklist
Steps to Test or Reproduce
event.
monitor, notch Mac) and in light and dark menu bars.
Summary by CodeRabbit
Bug Fixes
Tests