Skip to content

Add exact keyboard backlight CLI controls - #330

Open
CptPanko wants to merge 2 commits into
omacom:quattrofrom
CptPanko:feat/keyboard-backlight-cli-control
Open

Add exact keyboard backlight CLI controls#330
CptPanko wants to merge 2 commits into
omacom:quattrofrom
CptPanko:feat/keyboard-backlight-cli-control

Conversation

@CptPanko

@CptPanko CptPanko commented Sep 2, 2026

Copy link
Copy Markdown

Summary

  • add available, get, and exact set 0..100 controls to omarchy-brightness-keyboard
  • preserve the existing up, down, cycle, off, and restore behavior, including bare --no-osd
  • fail safely when no keyboard LED is present or sysfs values cannot be read
  • cover device selection, percentage rounding, backend failures, OSD failures, and legacy actions with a stateful focused test

Test plan

  • /bin/bash test/shell.d/brightness-keyboard-test.sh (116 assertions)
  • /opt/homebrew/bin/bash test/shell.d/brightness-keyboard-test.sh (116 assertions)
  • focused test repeated under both Bash 3.2 and Bash 5.3
  • bash -n bin/omarchy-brightness-keyboard test/shell.d/brightness-keyboard-test.sh
  • shellcheck --severity=warning bin/omarchy-brightness-keyboard
  • command metadata test (454 commands)
  • git diff --check upstream/quattro...HEAD

Scope

This is the CLI portion of #260 and does not close the settings-panel request. The panel remains deferred until its QML behavior can be exercised through a real compositor-aware harness.

Physical Apple keyboard-LED behavior and interaction with the ambient-light writer in #303 remain integration checks.

@scottjones scottjones left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good PR. The legacy actions are preserved exactly, bare --no-osd still works, find_device keeps the *kbd_backlight* glob so it stays portable off Apple hardware, and 484 lines of test for a 169-line script is a ratio I'm happy to see. Reviewed against the current head (a5f3582, after the quattro merge) on an M2 Max, where the LED is /sys/class/leds/kbd_backlight with max_brightness=255.

The ask: --no-osd on available and get is rejected, and that's the flag a wrapper will pass.

Verified against a stubbed LED tree:

available                rc=0  out=[]
--no-osd available       rc=2  out=[Usage: omarchy-brightness-keyboard ...]
--no-osd get             rc=2  out=[Usage: omarchy-brightness-keyboard ...]

--no-osd is a no-op for a read, so rejecting it is surprising on its own terms. It matters concretely right now: #303 and #340 both add an ambient-light writer that shells out to this command, and passing --no-osd uniformly is exactly what such a wrapper does. Whichever of those lands will hit this. Treating the flag as a harmless no-op on reads costs nothing and removes a trap that will be found by someone else's integration rather than by this PR's tests.

suggestion: get and the OSD compute the same percentage differently. percent_for_current rounds:

percent=$(( (current_brightness * 100 + max_brightness / 2) / max_brightness ))

while the up|down|cycle path truncates:

show_osd "$((new_brightness * 100 / max_brightness))"

At max_brightness=255 the step is 25, so one up from zero shows 9% on the OSD while omarchy brightness keyboard get reports 10%.

To be clear about severity, since I initially filed this as a bug and that was too strong: nothing breaks. get/set round-trips are unaffected because set hands a percentage to brightnessctl, which does its own math, and nothing in the repo reads the OSD value back. It's cosmetic today. The reason I'd still take the one-line fix is that this PR's purpose is to make get the canonical read for keyboard brightness, and a second differently-rounded computation two functions away is what drifts later - particularly if the settings panel from #260 ever shows get's number beside the OSD for the same keypress. Reusing percent_for_current (assigning current_brightness=$new_brightness first) makes them agree and removes a line.

nit: available is documented as a probe but still writes to stderr when nothing is found:

available (no device)    rc=1  out=[No keyboard backlight device found]

Callers testing for presence will need 2>/dev/null. The exit code already carries the answer.

nit: MAX_SAFE_BRIGHTNESS=90000000000000000 plus the digit-length overflow guard in normalize_decimal is around 30 lines defending a value whose real range here is 0-255. The logic looks correct; it's just a lot of surface area for a hazard sysfs can't produce, and it's the part of this file most likely to confuse the next reader. Not blocking.

Worth noting for sequencing: this is the CLI half of the keyboard-backlight work and it doesn't conflict with #303 or #340 - different file, and its writes are precisely the manual override those services are built to yield to.

The --no-osd change is what I'd like before merge; everything else is yours to take or leave.

@malik-na

malik-na commented Sep 9, 2026

Copy link
Copy Markdown
Member

The existing blocker is still present: --no-osd available and --no-osd get are rejected. Accept the flag as a harmless no-op for reads and add both cases to the focused test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants