feat: add per-display inner gap overrides#477
Conversation
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMonitor gap settings now support optional per-monitor inner-gap overrides. Resolved values flow through layout operations, settings UI, IPC display queries, CLI output, serialization, and tests. ChangesMonitor gap configuration and resolution
Per-monitor settings and display projection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsView
participant SettingsStore
participant WMController
participant LayoutHandler
participant IPCQueryRouter
SettingsView->>SettingsStore: update monitor inner-gap override
SettingsStore->>WMController: resolve and publish gap settings
WMController->>LayoutHandler: provide monitor-specific inner gap
IPCQueryRouter->>SettingsStore: request resolved display gap
SettingsStore-->>IPCQueryRouter: return innerGap
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Thank you will take a look at this tomorrow and if all good will merge it |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Sources/OmniWM/UI/SettingsView.swift (1)
47-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRefresh
connectedMonitorswhen displays change. It’s only seeded fromMonitor.current()and reloaded in.onAppear, so hot-plugged displays won’t show up in the picker until Settings is reopened. Subscribe this tab toNSApplication.didChangeScreenParametersNotificationor the controller’s display-change event and repopulate the list live.🤖 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 `@Sources/OmniWM/UI/SettingsView.swift` around lines 47 - 48, Update SettingsView’s connectedMonitors state and lifecycle handling so it refreshes when display parameters change, not only during initialization or .onAppear. Subscribe to NSApplication.didChangeScreenParametersNotification or the existing controller display-change event, repopulate connectedMonitors from Monitor.current(), and preserve the current picker behavior.Sources/OmniWM/Core/Controller/WMController.swift (2)
490-508: 🚀 Performance & Scalability | 🔵 TrivialConfirm
publishDisplayChanged()won't fire excessively; consider coalescing.
setGapSize/setOuterGapspublish.displayChangedby default, andupdateMonitorGapSettings()now unconditionally publishes on every call — including the unconditional call fromapplyPersistedSettings(Line 385), which runs whenever settings are (re)applied, not only when gap settings changed. If any UI call site invokessetGapSize/setOuterGapsper intermediate slider-drag value (not shown in this batch), each tick spawns a fire-and-forgetTaskpublishing an IPC event, and every unrelated settings reload also emits a spuriousdisplayChanged.Consider gating
publishDisplayChanged()behind an actual-change check (e.g., compare against previous resolved values) or debouncing the publish, so IPC subscribers aren't flooded by unrelated settings churn or continuous drags.Suggested direction
func updateMonitorGapSettings() { layoutRefreshController.requestRelayout(reason: .monitorSettingsChanged) - publishDisplayChanged() + // Only publish when this call is actually gap-related / triggered by an explicit user edit, + // not on every applyPersistedSettings() pass. + publishDisplayChanged() }Also applies to: 722-732, 346-353
🤖 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 `@Sources/OmniWM/Core/Controller/WMController.swift` around lines 490 - 508, Update setGapSize, setOuterGaps, and updateMonitorGapSettings to publish displayChanged only when the resolved gap values actually change; preserve applying settings without emitting events for unchanged values, and coalesce or otherwise suppress redundant publications during repeated slider updates.
901-914: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSimplify
innerGap(for:)to avoid a duplicate lookup and a second source of truth.
settings.resolvedGapSettings(for: monitor).innerGapalready falls back to the global default when there's no override (seeresolvedInnerGap(override?.innerGap)), so the "no override →workspaceManager.gaps" branch here is redundant and forces a second, separate lookup on the override path (gapSettings(for:)is called once here, then again insideresolvedGapSettings(for:)). This is called on hot interactive paths (focus navigation, mouse drag, scroll ticks), and — since it's brand-new code — no test exercises the no-override branch, so a latent divergence betweenworkspaceManager.gapsandsettings.gapSize's resolved value wouldn't be caught.Suggested simplification
func innerGap(for monitor: Monitor) -> CGFloat { - guard settings.gapSettings(for: monitor)?.innerGap != nil else { - return CGFloat(workspaceManager.gaps) - } - return settings.resolvedGapSettings(for: monitor).innerGap + settings.resolvedGapSettings(for: monitor).innerGap }If
workspaceManager.gapsis intentionally used here for a "live preview" value that differs from the persistedsettings.gapSizeduring in-progress edits, please confirm — otherwise this simplification removes the divergence risk.🤖 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 `@Sources/OmniWM/Core/Controller/WMController.swift` around lines 901 - 914, Simplify the monitor overload of innerGap(for:) to directly return settings.resolvedGapSettings(for: monitor).innerGap, removing the preliminary gapSettings(for:) lookup and workspaceManager.gaps fallback. Keep the workspaceId overload unchanged so it continues resolving the monitor before delegating to the monitor-based innerGap(for:).Tests/OmniWMTests/GapSettingsTests.swift (1)
225-292: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest-only coverage gap: no-override fallback path in
WMController.innerGap(for:)is never exercised.Both
Left/Rightmonitors here get explicitinnerGapoverrides, so the "no override" fallback branch inWMController.innerGap(for:)(which returnsworkspaceManager.gapsinstead ofsettings.resolvedGapSettings(for:).innerGap) is untested. Worth adding a case with one monitor left un-overridden to confirm the fallback still matches the globally resolved value.🤖 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 `@Tests/OmniWMTests/GapSettingsTests.swift` around lines 225 - 292, Extend assertLayoutRoutesInnerGapByWorkspaceDisplay to leave one monitor without a MonitorGapSettings override while retaining the other monitor’s explicit override, then assert that the un-overridden workspace’s frame separation matches the global workspaceManager.gaps inner-gap value. Keep the existing explicit-override assertion to cover both paths.
🤖 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 `@Sources/OmniWMCtl/CLIRenderer.swift`:
- Around line 357-368: Update appendDisplayColumn to format whole-number Double
values without a trailing “.0”, while preserving decimal output for fractional
values and “-” for nil values. Reuse the file’s existing numeric formatting
approach used by frameDescription or sizeDescription rather than converting
every value directly with String.
---
Nitpick comments:
In `@Sources/OmniWM/Core/Controller/WMController.swift`:
- Around line 490-508: Update setGapSize, setOuterGaps, and
updateMonitorGapSettings to publish displayChanged only when the resolved gap
values actually change; preserve applying settings without emitting events for
unchanged values, and coalesce or otherwise suppress redundant publications
during repeated slider updates.
- Around line 901-914: Simplify the monitor overload of innerGap(for:) to
directly return settings.resolvedGapSettings(for: monitor).innerGap, removing
the preliminary gapSettings(for:) lookup and workspaceManager.gaps fallback.
Keep the workspaceId overload unchanged so it continues resolving the monitor
before delegating to the monitor-based innerGap(for:).
In `@Sources/OmniWM/UI/SettingsView.swift`:
- Around line 47-48: Update SettingsView’s connectedMonitors state and lifecycle
handling so it refreshes when display parameters change, not only during
initialization or .onAppear. Subscribe to
NSApplication.didChangeScreenParametersNotification or the existing controller
display-change event, repopulate connectedMonitors from Monitor.current(), and
preserve the current picker behavior.
In `@Tests/OmniWMTests/GapSettingsTests.swift`:
- Around line 225-292: Extend assertLayoutRoutesInnerGapByWorkspaceDisplay to
leave one monitor without a MonitorGapSettings override while retaining the
other monitor’s explicit override, then assert that the un-overridden
workspace’s frame separation matches the global workspaceManager.gaps inner-gap
value. Keep the existing explicit-override assertion to cover both paths.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 004b3a23-db52-4bfd-8775-3d06450e95af
📒 Files selected for processing (18)
Sources/OmniWM/Core/Config/MonitorGapSettings.swiftSources/OmniWM/Core/Config/MonitorSettingsType.swiftSources/OmniWM/Core/Config/SettingsStore.swiftSources/OmniWM/Core/Controller/CommandHandler.swiftSources/OmniWM/Core/Controller/DwindleLayoutHandler.swiftSources/OmniWM/Core/Controller/MouseEventHandler.swiftSources/OmniWM/Core/Controller/NiriLayoutHandler.swiftSources/OmniWM/Core/Controller/WMController.swiftSources/OmniWM/Core/Controller/WindowActionHandler.swiftSources/OmniWM/Core/Controller/WorkspaceNavigationHandler.swiftSources/OmniWM/IPC/IPCQueryRouter.swiftSources/OmniWM/UI/SettingsView.swiftSources/OmniWMCtl/CLIRenderer.swiftSources/OmniWMIPC/IPCAutomationManifest.swiftSources/OmniWMIPC/IPCModels.swiftTests/OmniWMTests/GapSettingsTests.swiftTests/OmniWMTests/SettingsTOMLCodecTests.swiftdocs/IPC-CLI.md
Minor review cleanup: BarutSRB#477 (comment)
|
Sorry will look at this tomorrow as tonight I'll be pushing a new release that targets one specific problem that is a bit risky and might break stuff, just wanted to let you know that I'm not ignoring and that I appreciate the PR and help |
2 participants
Related Ideas discussion: #476
This draft makes the tested implementation available while the interaction and IPC scope are discussed. I am happy to adjust the shape of the change before marking it ready for review.
Problem
General inner gaps currently apply to every display, while outer margins can already be overridden per display. Mixed-display setups therefore cannot tune the full General spacing model coherently. A value that works well on a large external display may feel excessive on a built-in panel.
What changed
monitorGapOverridesrecord with an optionalinnerGapvalue.0as an explicit override, and remove records after their final override is reset.inner-gapfield to display IPC queries and render requested gap fields in non-JSON CLI output.display-changedevent when global or monitor-specific gap settings change.Defaults and compatibility
Existing configurations need no migration. The new field is optional and absent values continue to inherit the current global inner gap. New installations keep the existing global default, and selecting a display does not create an override until a value changes.
The existing runtime
0...64gap bounds remain unchanged. The General UI continues to expose its existing0...32range.Verification
Automated checks:
git diff --check0/462files require formattingGapSettingsTests: 16 passedmake check: passed with the required SwiftFormat and SwiftLint versionsTwo-display acceptance against the packaged candidate:
query displays --fields inner-gap --format tablereturned the resolved valuesdisplay-changedeventToolchain note: the locally installed stable compiler is Swift 6.3.2. Build, test, and packaging used temporary compatibility-only edits to
Package.swiftand one unchanged upstream FoundationModels call. Those edits are not part of this diff, and all feature files in the tested candidate match this PR. The available Swift 6.4 development snapshot crashes while compiling unchanged upstream code, so I could not repeat the build with that snapshot.Screenshots
Global defaults
Built-in display override
AI disclosure
I used Codex while investigating and implementing this change. I reviewed the resulting code and verified it manually.
Summary by CodeRabbit
inner-gapto display query snapshots and CLI output, with updated supported display fields.inner-gap.