Add media key example hotkeys to sxhkdrc

[jallbrit]

The example sxhkdrc is pretty bare bones and could use some simple, system-independent changes. I've added hotkeys for some common laptop media keys like adjusting volume and brightness. This helps bspwm work more out-of-the-box. New users may not expect having to manually map their brightness control/volume adjustment.

[emanuele6]

I don't get why you say that these are system-independent changes; amixer only works with ALSA (which you could say it's very popular and used by a lot of desktop systems), but, even if we ignore that, the /sys API you are using (like all the other /sys APIs) is Linux-specific and won't work on anything that isn't Linux.

[jallbrit]

I've updated my comment to remove that phrase. The current sxhkdrc includes commands that are system-dependent but work on a lot of machines (e.g. urxvt, dmenu_run), so I still see value in adding more system-dependent examples (like amixer) even if they aren't present on 100% of target machines.

[SeerLite]

I like the idea of these examples! But I feel like making them system-independent isn't really necessary (cause I mean if you're using some BSD you should be able to figure this stuff out by yourself, I think).
Personally, I'd just title the section "media keys (Linux)" and only focus on that.

Also, I think using light for brightness might be a little bit clearer for new people. Just a suggestion!

[jallbrit]

I think we should definitely make the commands as portable as possible, while also being realistic. That being said, in my opinion it is more portable to use the echo > /sys method that I currently have over something like light or xbacklight or brightnessctl or whatever, simply because most (if not all) Linux systems can use echo but you have to manually install all those other programs (and most people don't).

I knew that the echo > sys method would be a pain point for this PR, but I feel that it's important to include some form of brightness control because it's such an important feature for laptop bspwm users.

[SeerLite]

Makes sense!
I still find it kinda hard to read at first though. What about splitting it up into multiple lines, like this?

brightness_file="/sys/class/backlight/*/brightness"; \
original_brightness=$(cat "$brightness_file"); \
echo $((original_brightness + 15 * {1,5} * {-2,2})) > "$brightness_file"

It could even work as an example for "pseudo multi-line" hotkeys. Again, it's just a suggestion!

[emanuele6]

@SeerLite Have you tried that before suggesting it? That won't work!

You quoted the *, it won't expand: brightness_file="/sys/class/backlight/*/brightness", you need to do something like this:

brightness_file=/sys/class/backlight/*/brightness; \
original_brightness="$(cat $brightness_file)"; \
echo "$((original_brightness + 15 * {1,5} * {-2,2}))" > $brightness_file

---

Even if you fix the mistake you made in your script, this approach is still very problematic.

On my laptop, a 2011 thinkpad T420 with ArchLinux using kernel "Linux 5.4.68-1-lts", I have two matches for /sys/class/backlight/*/brightness:

$ printf "%s\n" /sys/class/backlight/*/brightness
/sys/class/backlight/acpi_video0/brightness
/sys/class/backlight/intel_backlight/brightness

If we want to make this script not "laptops which only have one backlight device"-specific, we need more changes (the previous script won't work on my laptop for example):

brightness_file="$(printf '%s\n' /sys/class/backlight/*/brightness | head -1)"; \
original_brightness="$(cat "$brightness_file")"; \
echo "$((original_brightness + 15 * {1,5} * {-2,2}))" > "$brightness_file"

---

But hold on, this still won't work on my laptop.
My user doesn't have permissions to write to these file (it only has read permission):

$ getent passwd "$USER"
emanuele6:x:1000:998::/home/emanuele6:/bin/bash
$ id "$USER"
uid=1000(emanuele6) gid=998(wheel) groups=998(wheel),5(tty)
$ echo 20 > /sys/class/backlight/acpi_video0/brightness
dash: 3: cannot create /sys/class/backlight/acpi_video0/brightness: Permission denied
$ echo 20 > /sys/class/backlight/intel_backlight/brightness
dash: 4: cannot create /sys/class/backlight/intel_backlight/brightness: Permission denied

So, this approach will only work if you run sxhkd as root or, if you mess with privileges of your user or with permissions of your filesystem (which @jallbrit probably did and forgot about?).

---

There's still another problem that prevents this from working on my laptop:

$ echo 20 | sudo tee /sys/class/backlight/acpi_video0/brightness
20
tee: /sys/class/backlight/acpi_video0/brightness: Invalid argument
$ echo 20 | sudo tee /sys/class/backlight/intel_backlight/brightness
20

Only one of the two devices actually works (acpi_video0 probably doesn't accept message because it's being controlled by intel_backlight which is a virtual driver?); and since, alphabetically, acpi_video0 comes before intel_backlight, the wrong device will get picked by head -1.

---

Note that xbacklight uses the Xrandr API to get/set the brightness of displays and that API will work on any system with Xorg and doesn't require special privileges/permissions; example:

XF86MonBrightness{Up,Down}{_, + shift}
	xbacklight -{inc,dec} {15, 30}

---

The fact that this don't work on my laptop shows that this method is not as "portable" as you think since it highly relies on the specific kind of hardware your laptop has.

Also note that as soon as you connect an external monitor which has a backlight, a new devices will be created in /sys/class/backlight which will mess with your script even if you don't normally have two backlights like I do.

Also, I think that LED displays don't have a backlight device, so this approach won't work with them. (NOTE: this one is just speculation, I don't know for sure.)

Using the /sys API in this usecase is really not smart. (It won't work on my Linux laptop!).

---

EDIT:
Also note that my laptop already reacts to XF86MonBrightnessUp and XF86MonBrightnessDown by increasing and decreasing the brightness for some reasons (at hardware level?). So the binding which uses xbacklight that I showed here are pretty glitchy, but work correctly (because the automatic change is probably just at hardware level and gets overwritten).

[jallbrit]

@emanuele6 thank you for your extensive testing. I took a peek at the brightness file permissions:

-rw-rw-r-- 1 root video 4.0K Jan 12 12:34 brightness

Since my user is part of the video group, I am able to edit the file. I don't recall doing this manually.

xbacklight unfortunately does not work out of the box on my machine (Thinkpad T440s, Linux 5.7.0-3-amd64): xbacklight -get and xbacklight -set 50 both exit with exit code 0 and do not give any output. I will look into why this is.

Unfortunately the XF86MonBrightnessUp/Down keys on my machine also do not do anything in the absence of sxhkd.

That being said, is xbacklight the most portable option?

[SeerLite]

You quoted the *, it won't expand: brightness_file="/sys/class/backlight/*/brightness"

whoops :S

Also, I completely forgot about the whole group/permissions thing. I remember having to do it manually on Arch, but maybe other distros come with it by default?

That being said, is xbacklight the most portable option?

I think light is a better option. It has always worked fine for me. Quoting from the README itself:

Light is a program to control backlights and other lights under GNU/Linux:

• Works where other software has proven unreliable (xbacklight etc.)
• Works even in a fully CLI-environment, i.e. it does not rely on X
• Provides functionality to automatically control backlights with the highest precision available
• Extra features, like setting a minimum brightness value for controllers, or saving/restoring the value for poweroffs/boots.

Careful, though: It also requires the user to be in the video group.
IIRC xbacklight requires a group too, but it's a different one. I think there just isn't a way to make this work out of the box on all distros.

What I'd do is just use light and put a comment saying something like "note: add your user to the video group for this to work!".
@emanuele6 What are your thoughts on that?

[jallbrit]

light is available in the apt repo and works out of the box on my machine. It seems clear at this point that either light or xbacklight would be better options than the /sys method.

I think there just isn't a way to make this work out of the box on all distros.

That may be true.

[emanuele6]

@SeerLite, as you can see from the output of id "$USER" here #1202 (comment), my user is only in the wheel group and in the tty group:

$ id "$USER"
uid=1000(emanuele6) gid=998(wheel) groups=998(wheel),5(tty)

I can use xbacklight just fine.

---

xbacklight shouldn't need any specific group for privileges.

Could the problem here possibly be that you guys are running a display manager or something similar as root and that that causes Xorg to only accept xbacklight requests from root?

If that's the case, can you guys try to start X with a simple startx with the same user that supposedly can't use xbacklight using e.g. this .xinitrc file:

#!/bin/sh
exec bspwm

and then try to run xbacklight -dec 20 and see if it works?

---

Also note that light also requires to be called from a user in the video group or root:

$ light -U 10; echo "$?"
1
$ sudo light -U 10; echo "$?"
0
$ sudo usermod -a -Gvideo emanuele6
$ # after a reboot
$ id
uid=1000(emanuele6) gid=998(wheel) groups=998(wheel),5(tty),986(video)
$ light -U 10; echo "$?"
0

[jallbrit]

Could the problem here possibly be that you guys are running a display manager or something similar as root and that that causes Xorg to only accept xbacklight requests from root?

I am running a display manager (I guess that runs as root?) but even when i try using sudo xbacklight -get or other xbacklight commands, I have the same problem (no output, exit code 0). Unfortunately I have too many windows/tasks going on that I can't really restart X right now, but I would imagine using sudo would have the same effect as running X via startx since I would have all the correct privileges?

Perhaps light requires the video group for a specific purpose, so that it is possible for administrators to control who can adjust the backlight? I'm not sure if this is intentional or just an annoyance. The README only says:

Note: make sure that your user is part of the video group, otherwise you will not get access to the devices.

Also I checked and light does not use the SUID bit (if that affects anything).

[SeerLite]

@jallbrit The Arch wiki says that there are some limitations with xbacklight. (I think those are the main reasons light exists too).

I haven't mentioned this, but my current display actually doesn't support backlight control at all. I've only been speaking from my experience of when I used to have a laptop. This means I can't really test if xbacklight works with startx, sorry about that :

[jallbrit]

I went ahead and committed the change to use light.

[emanuele6]

I don't get why there has to be a separate commit for the switch to using light since it's changing something that was introduced by the same PR and why these commits can't just be squashed into a single one, but, ignoring that..

..in the example sxhkd configuration that can be found in baskerville/sxhkd/README.md, there is already an example hotkey for changing the brightness:

{_,shift + ,super + }XF86MonBrightness{Down,Up}
	bright {-1,-10,min,+1,+10,max}

This furhter shows how futile this PR is: this example has nothing to do with bspwm and the same example can already be found in the sxhkd repo.

[SeerLite]

Right, but that's the sxhkd README on the sxhkd repo. Not example/base configuration files that are (AFAIK) installed with packages or as documentation. I don't think many people see the examples on the sxhkd repo.
Also, I wouldn't call this PR futile. There's no harm in having more examples, right?

That said, I have never heard of bright and can't find it as any binary/package (on the Arch repos at least :p). It does work as a good example, but I think light is fine.

@jallbrit Btw, you can do light -{U,A} directly

[jallbrit]

I don't get why there has to be a separate commit for the switch to using light since it's changing something that was introduced by the same PR and why these commits can't just be squashed into a single one, but, ignoring that..

It is my understanding that squashing commits changes git history, which would require a git push -f which is generally not recommended. What is the best practice here?

..in the example sxhkd configuration that can be found in baskerville/sxhkd/README.md, there is already an example hotkey for changing the brightness:

While it's great that sxhkd's README includes that helpful info, there is still value in including keybindings in the default sxhkdrc that comes packaged with bspwm. Otherwise we may as well just remove the whole default sxhkdrc and tell people "oh, well just look at the sxhkrc repo for an example!" you know?

Btw, you can do light -{U,A} directly

@SeerLite I appreciate the recommendation, but the current syntax...

{_, shift +}XF86MonBrightness{Down,Up}
	echo "{2,10}" | xargs light -{U,A}

works because {_, shift +} comes before {Down,Up} so I must first decide between 2 and 10 then decide between -U and -A. Essentially, ordering of the sets of {} is really important here, so basically I reversed the arguments using xarg. If there's a better way to do so, tell me!

[emanuele6]

It is my understanding that squashing commits changes git history, which would require a git push -f which is generally not recommended. What is the best practice here?

Can you justify why git push --force is "bad practice"? Force-pushing on baskerville/bspwm's master of course is bad, but why do you think deleting the history of a PR branch or of a local branch is bad? You are not changing the history of master...

Once it gets added to master, it is going to have different hashes anyways, no? It will be commited by baskerville and authored by you, it will be commited at a different time, the man page will be recompiled with the new latest commit, &c...

---

just remove the whole default sxhkdrc

the default sxhkdrc

example :)

Otherwise we may as well just remove the whole default sxhkdrc and tell people "oh, well just look at the sxhkrc repo for an example!" you know?

There are useful examples related to bspwm in that example file: the only non-bspwm-specific are the ones for opening a urxvt window and the one to launch dmenu_run (for usability) and the one to reload sxhkd for convenience.
I don't get why you say that since the sxhkd example hotkeys found in baskerville/bspwm are not also in baskerville/sxhkd.
There is already an example hotkey for changing the backlight brightness of LCD monitors in baskerville/sxhkd and it is not bspwm-specific.

I personally wouldn't like looking on the internet for bspwm example and finding generic sxhkd examples, that's just useless information! (or looking for example for something and finding examples for something else along with the examples I want.)
That file is an example, not an actual configuration file that gets used by default if you install bspwm or sxhkd.

---

If there's a better way to do so, tell me!

In this comment of mine #1202 (comment), you can see that I didn't need to use xargs for the xbacklight, you can use the exact same method with light:

XF86MonBrightness{Up,Down}{_, + shift}
	light -{A,U} {15,30}

An alternative solution would be:

{_,shift + }XF86MonBrightness{Up,Down}
	light {-A 15,-A 30,-U 15,-U 30}

[jallbrit]

Can you justify why git push --force is bad practice? on force-pushing on baskerville/bspwm's master of course is bad, but why do you think deleting the history of a PR branch or of a local branch is bad? You are not changing the history of master...

It's something I've read before but don't have experience with, so that's why I left the question open-ended. I see now that I should squash the commits.

XF86MonBrightness{Up,Down}{_, + shift}
	light -{A,U} {15,30}

This is a better way.