fix(settings): align icon refresh rate with capture floor - #928
fix(settings): align icon refresh rate with capture floor#928CamilleGuillory wants to merge 2 commits into
Conversation
The slider offered up to 30 fps while SCK capture is floored at 4 fps, wasting main-actor wakeups. Snap stored intervals to the expressible grid, clamp sleep, and correct the URI docs/range.
github-actions
Bot
added
layout
|
Labels: Caution Convention issues found — please fix before merge.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR standardizes icon refresh intervals around a 30 FPS maximum and a 1/30-second minimum. Settings values now normalize to supported reciprocal FPS values. The UI, URI handler, documentation, and tests use the updated limits. ChangesIcon refresh interval controls
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 1
🤖 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 `@Thaw/Utilities/SettingsURIHandler.swift`:
- Line 115: Update the iconRefreshInterval handling in SettingsURIHandler.swift
at lines 115-115 to normalize values through
AdvancedSettings.normalizedIconRefreshInterval before writing Defaults and
posting the notification; add URI tests covering 0.1 → 0.25 and 0.6 → 0.5.
Update docs/URI_SCHEMES.md at lines 157-157 to document that positive values
snap to 1/n for n in 1...4, alongside the existing range behavior.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e1c85642-7dd8-4f73-8b43-1419e4992abd
📒 Files selected for processing (9)
Thaw/MenuBar/MenuBarItems/MenuBarItemImageCache.swiftThaw/Settings/Models/AdvancedSettings.swiftThaw/Settings/SettingsPanes/MenuBarLayoutSettingsPane.swiftThaw/Utilities/SettingsURIHandler.swiftThawTests/Settings/Models/AdvancedSettingsTests.swiftThawTests/Settings/Models/IconRefreshIntervalNormalizationTests.swiftThawTests/Settings/Models/ProfileDecodingDefaultsTests.swiftThawTests/Settings/URI/SettingsURIHandlerApplyTests.swiftdocs/URI_SCHEMES.md
| "showOnHoverDelay": (0, 5), | ||
| "tooltipDelay": (0, 5), | ||
| "iconRefreshInterval": (0, 5), | ||
| "iconRefreshInterval": (0, 1), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Normalize URI values before persistence and document the discrete grid.
iconRefreshInterval supports only 0, 1, 1/2, 1/3, and 1/4 seconds after normalization. The current URI range accepts values such as 0.1 and 0.6. handleDoubleSet stores those raw values before posting the notification. This makes correct persistence depend on an active AdvancedSettings observer.
Thaw/Utilities/SettingsURIHandler.swift#L115-L115: NormalizeiconRefreshIntervalwithAdvancedSettings.normalizedIconRefreshIntervalbefore writingDefaultsand before posting the notification. Add URI tests for0.1 -> 0.25and0.6 -> 0.5.docs/URI_SCHEMES.md#L157-L157: State that positive values snap to1/nfornin1...4, rather than only describing continuous range clamping.
📍 Affects 2 files
Thaw/Utilities/SettingsURIHandler.swift#L115-L115(this comment)docs/URI_SCHEMES.md#L157-L157
🤖 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 `@Thaw/Utilities/SettingsURIHandler.swift` at line 115, Update the
iconRefreshInterval handling in SettingsURIHandler.swift at lines 115-115 to
normalize values through AdvancedSettings.normalizedIconRefreshInterval before
writing Defaults and posting the notification; add URI tests covering 0.1 → 0.25
and 0.6 → 0.5. Update docs/URI_SCHEMES.md at lines 157-157 to document that
positive values snap to 1/n for n in 1...4, alongside the existing range
behavior.
The SCK floor was a deliberate CPU throttle, not an API limit. Match the historical slider top so users who want smoother panel icons can opt in; SkyLight offscreen stays at 1/s.
|
Follow-up: raised the SCK capture ceiling from 4 fps to 30 fps so it matches the historical slider top. Default remains 0.25 s (~4 fps). SkyLight offscreen path stays at 1/s. Tradeoff: leaving Search/Layout/Thaw Bar open at 30 fps can pin a core again — that was why the 250 ms floor existed. |
|
Superseded by a fresh single-commit PR that includes DCO sign-off, URI-side grid normalization (CodeRabbit), and the 30 fps ceiling. Closing in favor of the new PR. |
1 participant
Summary
iconRefreshIntervalonto{Off, 1–4 fps}, clamp live-loop sleep, and tighten the URI range/docs so slow or sub-ms values cannot show as Off or spin the main actorTest plan
xcodebuild test -project Thaw.xcodeproj -scheme Thaw -destination 'platform=macOS' CODE_SIGN_IDENTITY="" CODE_SIGNING_REQUIRED=NOthaw://set?key=iconRefreshInterval&value=2.5clamps to 1s (1 fps);value=0stays OffCloses: N/A
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
Enhancements
Documentation