Add Niri scroll snap toggle#350
Open
Guria wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughA new ChangesScroll Snap Configuration and Control
Sequence Diagram(s)sequenceDiagram
participant SettingsView
participant SettingsStore
participant NiriLayoutHandler
participant NiriLayoutEngine
participant ViewportState
SettingsView->>SettingsStore: set niriScrollSnapEnabled(value)
SettingsStore->>NiriLayoutHandler: updateNiriConfig(scrollSnapEnabled: value)
NiriLayoutHandler->>NiriLayoutEngine: updateConfiguration(scrollSnapEnabled: value)
ViewportState->>SettingsStore: resolvedNiriSettings(for: monitor)
SettingsStore-->>ViewportState: ResolvedNiriSettings(scrollSnapEnabled)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Guria
force-pushed
the
disable-niri-scroll-snap
branch
from
0eeb864 to
4b0173e
Compare
There was a problem hiding this comment.
Pull request overview
Adds a user-configurable “Niri Scroll Snap” setting (global + per-monitor override), persists it through the settings export/TOML pipeline, and exposes it in both the Settings UI and the status bar Controls menu. This aligns with issue #336 by allowing scroll gestures to finalize without forced snapping when the setting is disabled.
Changes:
- Add persisted global
niriScrollSnapEnabledplus per-monitor overrideMonitorNiriSettings.scrollSnapEnabled, including canonical TOML wiring. - Respect the resolved per-monitor
scrollSnapEnabledwhen finalizing trackpad gestures (snapToColumn). - Expose a toggle in Settings (global + per-monitor) and in the status bar Controls popup; add/extend tests and fixture TOML.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Tests/OmniWMTests/StatusBarMenuTests.swift | Verifies status bar menu includes the new Niri scroll snap control. |
| Tests/OmniWMTests/SettingsStoreTests.swift | Asserts default export/settings include niriScrollSnapEnabled = true. |
| Tests/OmniWMTests/NiriLayoutEngineTests.swift | Updates fixture helper to include the new resolved setting. |
| Tests/OmniWMTests/Fixtures/canonical-settings.toml | Adds scrollSnapEnabled to canonical Niri config fixture. |
| Sources/OmniWM/UI/StatusBar/StatusBarMenu.swift | Adds a “Niri Scroll Snap” toggle row to the Controls section and keeps it in sync. |
| Sources/OmniWM/UI/SettingsView.swift | Adds global toggle + per-monitor overridable toggle for scroll snap. |
| Sources/OmniWM/Core/Layout/Niri/NiriLayoutEngine+Monitors.swift | Extends resolved settings construction to include scroll snap (but currently hard-coded). |
| Sources/OmniWM/Core/Controller/MouseEventHandler.swift | Uses resolved per-monitor scroll snap setting for gesture finalization (snapToColumn). |
| Sources/OmniWM/Core/Config/SettingsStore.swift | Persists niriScrollSnapEnabled and includes it in export/apply + resolved settings. |
| Sources/OmniWM/Core/Config/SettingsExport.swift | Adds niriScrollSnapEnabled to the settings export and defaults. |
| Sources/OmniWM/Core/Config/MonitorNiriSettings.swift | Adds per-monitor override field and includes it in coding + resolved settings model. |
| Sources/OmniWM/Core/Config/CanonicalTOMLConfig.swift | Adds TOML decode/encode mapping for scrollSnapEnabled within [niri]. |
| Scripts/build-metadata.env | Updates Ghostty archive SHA256. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| maxWindowsPerColumn: maxWindowsPerColumn, | ||
| centerFocusedColumn: centerFocusedColumn, | ||
| alwaysCenterSingleColumn: alwaysCenterSingleColumn, | ||
| scrollSnapEnabled: true, |
Guria
force-pushed
the
disable-niri-scroll-snap
branch
from
4b0173e to
dadd77a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/OmniWM/Core/Layout/Niri/ViewportState+Gestures.swift (1)
435-441: 💤 Low valueConsider extracting positions computation to avoid duplication.
The positions array construction at lines 435-441 duplicates the logic in
normalizedPreservedGestureOffsetat lines 486-492. One option is to return the positions array fromnormalizedPreservedGestureOffsetas part ofPreservedGestureOffset, or extract a shared helper.🤖 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/Layout/Niri/ViewportState`+Gestures.swift around lines 435 - 441, The positions array computation is duplicated; refactor by moving the logic into a single helper or into the PreservedGestureOffset result so callers reuse it. Update normalizedPreservedGestureOffset (and/or the PreservedGestureOffset type) to produce and return the positions array, then replace the local positions construction in ViewportState+Gestures.swift (the loop that builds positions using columns and gap) with a call to that helper or use the positions from PreservedGestureOffset, ensuring callers reference the new positions property instead of recomputing.
🤖 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.
Nitpick comments:
In `@Sources/OmniWM/Core/Layout/Niri/ViewportState`+Gestures.swift:
- Around line 435-441: The positions array computation is duplicated; refactor
by moving the logic into a single helper or into the PreservedGestureOffset
result so callers reuse it. Update normalizedPreservedGestureOffset (and/or the
PreservedGestureOffset type) to produce and return the positions array, then
replace the local positions construction in ViewportState+Gestures.swift (the
loop that builds positions using columns and gap) with a call to that helper or
use the positions from PreservedGestureOffset, ensuring callers reference the
new positions property instead of recomputing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d7f039f-ef44-4e71-bb80-677315898204
📒 Files selected for processing (13)
Sources/OmniWM/Core/Config/CanonicalTOMLConfig.swiftSources/OmniWM/Core/Config/MonitorNiriSettings.swiftSources/OmniWM/Core/Config/SettingsExport.swiftSources/OmniWM/Core/Config/SettingsStore.swiftSources/OmniWM/Core/Controller/MouseEventHandler.swiftSources/OmniWM/Core/Layout/Niri/NiriLayoutEngine+Monitors.swiftSources/OmniWM/Core/Layout/Niri/ViewportState+Gestures.swiftSources/OmniWM/UI/SettingsView.swiftSources/OmniWM/UI/StatusBar/StatusBarMenu.swiftTests/OmniWMTests/Fixtures/canonical-settings.tomlTests/OmniWMTests/NiriLayoutEngineTests.swiftTests/OmniWMTests/SettingsStoreTests.swiftTests/OmniWMTests/StatusBarMenuTests.swift
✅ Files skipped from review due to trivial changes (1)
- Tests/OmniWMTests/SettingsStoreTests.swift
Author
|
Just sharing mine local experiments on it. @BarutSRB Please feel free to decide what parts to use. Branch is open to changes from maintainers too. |
Guria
force-pushed
the
disable-niri-scroll-snap
branch
from
dadd77a to
059226a
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tests/OmniWMTests/SettingsStoreTests.swift (1)
896-931:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd assertion for
niriScrollSnapEnabledto maintain test completeness.The
settingsStoreFallbackDefaultsMatchExportDefaultstest verifies thatSettingsStoredefaults matchSettingsExportdefaults for each setting. The newniriScrollSnapEnabledsetting should be included in this comparison to ensure the defaults remain synchronized.Proposed fix
`#expect`(settings.niriMaxWindowsPerColumn == exportDefaults.niriMaxWindowsPerColumn) `#expect`(settings.niriMaxVisibleColumns == exportDefaults.niriMaxVisibleColumns) +#expect(settings.niriScrollSnapEnabled == exportDefaults.niriScrollSnapEnabled) `#expect`(settings.defaultLayoutType.rawValue == exportDefaults.defaultLayoutType)🤖 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/SettingsStoreTests.swift` around lines 896 - 931, Add an assertion in the test function settingsStoreFallbackDefaultsMatchExportDefaults to compare the new setting: assert that settings.niriScrollSnapEnabled == exportDefaults.niriScrollSnapEnabled; this keeps SettingsStore defaults in sync with SettingsExport defaults and should be added alongside the other `#expect` checks in the same test.
🤖 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.
Outside diff comments:
In `@Tests/OmniWMTests/SettingsStoreTests.swift`:
- Around line 896-931: Add an assertion in the test function
settingsStoreFallbackDefaultsMatchExportDefaults to compare the new setting:
assert that settings.niriScrollSnapEnabled ==
exportDefaults.niriScrollSnapEnabled; this keeps SettingsStore defaults in sync
with SettingsExport defaults and should be added alongside the other `#expect`
checks in the same test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4057d0a2-eaa8-4336-a849-a2cafa37b279
📒 Files selected for processing (16)
Sources/OmniWM/Core/Config/CanonicalTOMLConfig.swiftSources/OmniWM/Core/Config/MonitorNiriSettings.swiftSources/OmniWM/Core/Config/SettingsExport.swiftSources/OmniWM/Core/Config/SettingsStore.swiftSources/OmniWM/Core/Controller/MouseEventHandler.swiftSources/OmniWM/Core/Controller/NiriLayoutHandler.swiftSources/OmniWM/Core/Controller/WMController.swiftSources/OmniWM/Core/Layout/Niri/NiriLayoutEngine+Monitors.swiftSources/OmniWM/Core/Layout/Niri/NiriLayoutEngine.swiftSources/OmniWM/Core/Layout/Niri/ViewportState+Gestures.swiftSources/OmniWM/UI/SettingsView.swiftSources/OmniWM/UI/StatusBar/StatusBarMenu.swiftTests/OmniWMTests/Fixtures/canonical-settings.tomlTests/OmniWMTests/NiriLayoutEngineTests.swiftTests/OmniWMTests/SettingsStoreTests.swiftTests/OmniWMTests/StatusBarMenuTests.swift
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
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.
Closes #336
Summary
Testing
Summary by CodeRabbit
New Features
Behavior
Tests