actkbd module: introduce services.actkbd.brightnessControl option

[ghost]

Motivation for this change

As discussed in #57602

@primeos Please give it a try

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Assured whether relevant documentation is up to date
• Fits CONTRIBUTING.md.

---

[primeos]

Hey, sorry for the late response :o
Unfortunately I cannot test this on my main laptop atm, but I did test brightness.sh with debug output:

$ ./brightness.sh + 10
bl_dev: /sys/class/backlight/intel_backlight
current: 7500
max: 7500
level: 750
next_level: 7500
$ ./brightness.sh - 200
bl_dev: /sys/class/backlight/intel_backlight
current: 7500
max: 7500
level: 15000
next_level: 1
$ ./brightness.sh - 50
bl_dev: /sys/class/backlight/intel_backlight
current: 7500
max: 7500
level: 3750
next_level: 3750
$ ./brightness.sh - 50
bl_dev: /sys/class/backlight/intel_backlight
current: 3750
max: 7500
level: 3750
next_level: 1

And it does work as expected :)

To make the script more robust I would also recommend to add something like the following (especially nounset might be useful):

set -o errexit
set -o nounset

Without nounset:

$ ./brightness.sh - a
bl_dev: /sys/class/backlight/intel_backlight
current: 3750
max: 7500
level: 0
next_level: 3750
$ ./brightness.sh - $
./brightness.sh: line 9: 7500 * $ / 100: syntax error: operand expected (error token is "$ / 100")
./brightness.sh: line 10: 3750 - : syntax error: operand expected (error token is "- ")
./brightness.sh: line 11: [: -gt: unary operator expected
./brightness.sh: line 13: [: -lt: unary operator expected
bl_dev: /sys/class/backlight/intel_backlight
current: 3750
max: 7500
level: 
next_level: 

With nounset:

$ ./brightness.sh - a
./brightness.sh: line 9: a: unbound variable
$ ./brightness.sh - $
./brightness.sh: line 9: 7500 * $ / 100: syntax error: operand expected (error token is "$ / 100")
./brightness.sh: line 10: level: unbound variable

brightness.sh:

#!/usr/bin/env bash

set -o errexit
set -o nounset

bl_dev=`echo /sys/class/backlight/*`
current=$(< $bl_dev/brightness)
max=$(< $bl_dev/max_brightness)
level=$(($max * $2 / 100))
next_level=$(($current $1 $level))
if [ $next_level -gt $max ]; then
  next_level=$max
elif [ $next_level -lt 1 ]; then
  next_level=1
fi
#echo $next_level > $bl_dev/brightness

echo "bl_dev: $bl_dev"
echo "current: $current"
echo "max: $max"
echo "level: $level"
echo "next_level: $next_level"

[primeos]

Looks good otherwise :)
(I did not test the actkbd part though.)

[ghost]

@primeos Thank you much! I've applied your recommendations.

[primeos]

cc @oxij (as the author of the actkbd module) in case you have time to review this

[oxij]

- Commit messages should use "nixos/<module name>: <action>" format. - 4cd3e6c needs ```diff - (mkRenamedOptionModule [ "sound" "enableMediaKeys" ] [ "sound" "mediaKeys" "enable" ]) + (mkRenamedOptionModule [ "sound" "enableMediaKeys" ] [ "services" "actkbd" "volumeControl" "enable" ]) ``` to work properly else very old configurations will break. - 4cd3e6c also changes the semantics of the renamed options, enabling `sound.mediaKeys.enable` enabled `services.actkbd.enable` before, now it does not. To make this way of doing things work you would have to make `services.actkbd.enabled` depend on `volumeControl.enable`, as ugly as it sounds. In which case I would also suggest making `sound.mediaKeys` an alias to `services.actkbd.volumeControl` instead of `mkRenamedOptionModule`, since it makes sense to keep that option there for documentation purposes at the very least. But, personally, I'm 👎 on that patch in the first place. I think keeping these things outside of `actkbd` makes more sense since you could do the same thing using different backends. E.g. PulseAudio could have its own media keys setup with `actkbd` using `pauvcontrol` or whatever. I planned `actkbd` module as a generic modular thing, not as a place to assemble all possible implementations of all possible things, which this patch drives towards. - `brightness.sh` needs some more hacking, e.g. on my laptop `ls /sys/class/backlight` gives several results (it gives both acpi backlight and intel backlight controls), I think that script will do something bad in such a situation. I would consider using some lightweight tool for that instead. E.g. I use `pkgs.light`. Which brings me to `programs.light` option. I would just add the new options to that module instead and keep `actkbd` as module as it is.

[ghost]

@oxij Thank you!

• brightness.sh needs some more hacking, e.g. on my laptop ls /sys/class/backlight gives several results (it gives both acpi backlight and intel backlight controls), I think that script will do something bad in such a situation. I would consider using some lightweight tool for that instead. E.g. I use pkgs.light.
  Which brings me to programs.light option. I would just add the new options to that module instead and keep actkbd as module as it is.

Makes sense. I will make a new PR with changes to programs.light then.