Add fine brightness steps for keys and shortcuts - #166
Anywhere-Music-Player wants to merge 2 commits into
Conversation
didriksg
left a comment
There was a problem hiding this comment.
A finer step on the brightness keys is a good idea, thanks for bringing it over from NativeDisplayBrightness. Sending the pointer path through adjustDisplays is a nice cleanup too, and I'd keep it.
I'd like to get there a different way, though. macOS already has a fine step: Option+Shift+brightness moves the built-in in quarter steps, and Option+brightness on its own opens Displays settings (Apple's list). The PR reads Option on every press, even with the setting off, and takes the built-in press while Option is held. So with the pointer on the built-in, Option+brightness stops opening Displays settings, and Option+Shift moves a sixth of a step where macOS moves a quarter. That's from reading the diff, I haven't run the branch.
So rather than a setting of its own, I'd like Crisp to do exactly what macOS does on the displays it already controls: Option+Shift+brightness moves a quarter step, four per stop. With the pointer on the built-in, the press still goes to macOS, which already does the quarter step there. No setting, tooltip, README line or new strings, and the #160 shortcuts keep the whole step. Your rounding to the nearest substep and the tests carry over with 4 in place of 6.
Could you also drop the Swift 5 language mode commit from this PR? It's in #165 as well, and I'll look at it there, on its own.
|
One change to my review: please keep the Swift 5 language mode commit in this PR. I said I'd take it into a PR of its own, but it's simpler to merge it here together with the quarter steps. |
|
Thanks for the review. Using Option alone was intentional: Option+F1/F2 is more convenient for me than Option+Shift+F1/F2. The toggle makes fine steps the default for ordinary brightness-key presses, without holding any modifiers. This is the workflow I used in NativeDisplayBrightness. I understand your concern about overriding the native Option+brightness shortcut, especially while the toggle is off. Would you consider making this behavior explicitly opt-in, with native shortcuts preserved by default? Using quarter steps instead of sixths would be fine with me. The convenience of fewer modifiers is the main reason for this feature. I'll keep the Swift 5 commit in this PR, as requested in your latest comment. Separately, I'm considering migrating my fork fully to Swift 6, including the concurrency fixes that would require. That would be separate work from this PR. |
|
I'm not open to Crisp overriding an existing and native shortcut by default, but I would be open to adding it as a keyboard shortcut opportunity where you can explicitly override existing shortcuts as you please. My suggestion is that you modify this PR to use Option+Shift+F1/F2, and in the same time also add the "fine brightness step" as a keyboard shortcut in Crisp, which opens the possiblity of using Option+F1/F2. I can't guarantee that it wont affect the existing native shortcut though. |
|
Thanks, I understand your point about preserving native shortcuts. I also wanted to share a similar option in MonitorControl: "Use fine OSD scale for brightness and contrast". Its description explains that Shift+Option provides fine control, while the checkbox makes fine control the default. That checkbox is very convenient: once enabled, ordinary brightness-key presses use smaller steps without holding any modifiers. This is the convenience I was trying to bring to Crisp, and I think their approach communicates it more clearly than my current implementation. Would you also consider an optional setting like this, alongside the configurable shortcuts you suggested? |
|
As Crisp has no settings window, only the Settings section in the menu, I'd rather not fill it with switches that only affect a very specific use case and/or people. So I'm going to pass on the setting, but I'd be happy to merge the Option+Shift+brightness modifier, so external monitors get the same quarter steps macOS gives the built-in. About the shortcut I suggested: I checked it out, and media keys like brightness can't be recorded as keybindings in Crisp. It would work if you turn on "Use F1, F2, etc. keys as standard function keys" in System Settings, though: F1 and F2 then send ordinary key presses, so Option+F1 can be bound, and the brightness keys move behind fn. I'm open to the shortcut, but as a separate PR, so this one stays small. |
|
Thanks for clarifying, and for taking the time to review this! I understand that you want to keep Crisp's menu compact and preserve the native shortcuts. For now, I've switched back to MonitorControl and will focus on my fork there. I used it for years, and the updated version is working well for me. I've refreshed the menu, added system Night Shift temperature, and put the options in a dedicated settings window with sidebar navigation. Here's the branch with screenshots in the README. Crisp was useful inspiration for this, and I've credited the adapted system controls and included its MIT license. I'm pausing work on this PR for now, but I appreciate your feedback and the work you've put into Crisp. Thanks again! :) |
|
Understood, and good luck with the fork. A settings window with a sidebar is the right call once the options outgrow a menu, and thanks for keeping the license and credit in order. If Crisp ever grows to the point where it needs a settings window of its own, your PRs would be more than welcome. I'm closing this so it isn't left open with changes requested. Reopen it any time if you come back to it. I'll do the Option+Shift quarter steps myself. You were right that the build modes disagreed. I moved every build path to Swift 6 rather than pin them together at the old one, and that turned up about a dozen real concurrency problems in Crisp. Thanks for pointing at it. |

Brightness keys currently move between 16 stops (6.25% per step). Add an optional Fine Brightness Steps setting that divides each normal step into six substeps, giving approximately 1.04% increments.
I built NativeDisplayBrightness many years ago and used its “Smooth brightness step” option for smaller keyboard adjustments. That was the motivation for bringing the same convenience to Crisp, with its support for multiple displays.
Behavior
Validation
make check: 142 tests passed, strict SwiftLint, x86_64 typecheck, and localization-key check passed.Shared build prerequisite
The separate
Align Xcode language mode with supported buildscommit is also present in #165. It makes the generated Xcode project use the Swift 5 language mode already used by the development/release scripts, and removes redundant language-mode overrides from Makefile checks. It is included here so this branch builds normally in Xcode. Once that fix lands upstream, the duplicate prerequisite can be dropped.