Skip to content

Report failed display brightness changes - #454

Open
resolvicomai wants to merge 1 commit into
omacom:quattrofrom
resolvicomai:codex/propagate-brightness-failures
Open

resolvicomai wants to merge 1 commit into
omacom:quattrofrom
resolvicomai:codex/propagate-brightness-failures

Conversation

@resolvicomai

Copy link
Copy Markdown

When brightnessctl set fails, omarchy brightness display --no-osd currently returns success. With OSD enabled, it also continues to the brightness overlay despite the failed change.

Exit nonzero immediately on a failed generic backlight write. The existing backend error remains visible, and no success OSD is shown. Extend the existing brightness regression test to cover failure with and without --no-osd, plus the successful OSD path.

Fixes #429.

Validation:

  • Reproduced the failure against quattro@79b074a using an isolated brightnessctl stub.
  • Focused regression: 14 checks passed; independent QA and review passed.
  • Bash syntax, command metadata (469 commands), and git diff --check passed.
  • Local host is macOS with Bash 5; the broader local shell suite encounters existing Linux/GNU-tool assumptions. Linux CI is required. No physical brightness or graphical OSD test was performed.

AI-assisted implementation, QA and review using Codex.

@malik-na malik-na added the lvl 0 Core userspace: display, audio, video, graphics, memory, storage, battery, keyboard, network, BT. label Sep 18, 2026
@laihenyi

Copy link
Copy Markdown

Independent review (AI-assisted, Claude). Small change, so this is mostly a confirmation.

Fix

|| exit 1 on the brightnessctl set call is the right minimal change: the DDC branch already does || exit 1 on its setter, so the internal-backlight branch now behaves the same, and the OSD call after it is skipped on failure. brightnessctl's own stderr stays visible, matching what #429 asked for.

Test changes

  • The new omarchy-osd stub is needed because the success path was never exercised before (PATH includes $ROOT/bin, so without the stub the real omarchy-osd would run).
  • The flock stub turns the lock into a no-op. Harmless for a sequential test and it is what lets the file run on macOS, but on Linux the real flock would also have been fine, so the stub is a portability choice rather than a requirement. Worth a one-line comment so nobody removes it thinking it hides a bug.
  • Running the script via bash "$ROOT/bin/…" instead of executing it directly is fine (same interpreter as the shebang on Linux).

The osd_count baseline plus the "no new OSD call" assertion pins the actual regression (OSD after a failed write), not just the exit code.

Out of scope but worth noting for a follow-up: omarchy-brightness-display-apple is called without checking its exit status either, so the Apple-display branch has the same gap.

Local verification (Ubuntu 24.04 arm64 container)

  • Syntax loop and shellcheck on the changed files: clean
  • bin/omarchy commands --check: 469 commands pass
  • test/shell.d/brightness-display-test.sh: 14 checks pass

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

Labels

lvl 0 Core userspace: display, audio, video, graphics, memory, storage, battery, keyboard, network, BT.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

omarchy brightness display exits 0 when brightnessctl cannot change brightness

3 participants