Implement keyboard LEDs #549

Manually merged
dwl merged 1 commit from thanatos/dwl:keyboard_leds into main 2023-12-18 19:52:59 +01:00
Owner

This PR implements keyboard LEDs to indicate caps lock, num lock, and scroll lock.

This PR implements keyboard LEDs to indicate caps lock, num lock, and scroll lock.
Owner

Haven't checked, but what's wlr_keyboard.name for virtual devices?, NULL?

Haven't checked, but what's wlr_keyboard.name for virtual devices?, NULL?
Author
Owner

The name is part of wlr_input_device rather than wlr_keyboard (that's why I'm keeping track of it separately in the Keyboard struct), so it's not applicable to virtual devices as far as I'm aware.

The name is part of wlr_input_device rather than wlr_keyboard (that's why I'm keeping track of it separately in the Keyboard struct), so it's not applicable to virtual devices as far as I'm aware.
Owner

I asked in #wlroots and turns out that the correct way to handle this is using wlr_keyboard_group, are you interested in implement it

I asked in #wlroots and turns out that the correct way to handle this is using `wlr_keyboard_group`, are you interested in implement it
Author
Owner

Sure, I'll take a crack at implementing that.

Sure, I'll take a crack at implementing that.
Author
Owner

OK, that commit has the keyboard groups implemented. All keypresses are now handled through groups rather than individual devices, and virtual keyboards are each being placed in a group by themselves. This saves us having to handle both groups and individual device key events, since everything is grouped. The LEDs work out of the box when the keyboards are grouped, so I removed the LED logic I had added.

OK, that commit has the keyboard groups implemented. All keypresses are now handled through groups rather than individual devices, and virtual keyboards are each being placed in a group by themselves. This saves us having to handle both groups and individual device key events, since everything is grouped. The LEDs work out of the box when the keyboards are grouped, so I removed the LED logic I had added.
Owner

Is Keyboard used at all?, if not we can remove it (the data field for wlr_input_device.destroy is the input device itself)

The virtual keyboards need to be in a exclusive group?

What about having a global KeyboardGroup?

Is `Keyboard` used at all?, if not we can remove it (the data field for wlr_input_device.destroy is the input device itself) The virtual keyboards need to be in a exclusive group? What about having a global KeyboardGroup?
Owner

Is Keyboard used at all?, if not we can remove it (the data field for wlr_input_device.destroy is the input device itself)

To be more specific, we have Keyboard to handle key presses for different keyboards, now that all is managed by wlr_keyboard_group we don't have to distinguish between different keyboards thus making it useless, for example, pointer events are managed by wlr_cursor and not for each different pointer

> Is `Keyboard` used at all?, if not we can remove it (the data field for wlr_input_device.destroy is the input device itself) > To be more specific, we have Keyboard to handle key presses for different keyboards, now that all is managed by wlr_keyboard_group we don't have to distinguish between different keyboards thus making it useless, for example, pointer events are managed by wlr_cursor and not for each different pointer
Author
Owner

Is Keyboard used at all?, if not we can remove it (the data field for wlr_input_device.destroy is the input device itself)

On further inspection, the stuff I put in cleanupkeyboard is redundant, so we can remove the Keyboard struct.

The virtual keyboards need to be in a exclusive group?

That would be a good one to ask in the wlroots IRC. I did it that way based on what the implementations I was looking at did, but I don't know if there's a specific reason for sectioning them off. I tested adding a virtual keyboard (wf-osk) to the group with my physical boards, and it works just fine as far as I can tell, but I can't say definitively one way or the other.

What about having a global KeyboardGroup?

That would be possible, and it would simplify things, as long as virtual keyboards are OK to include.

> Is Keyboard used at all?, if not we can remove it (the data field for wlr_input_device.destroy is the input device itself) On further inspection, the stuff I put in cleanupkeyboard is redundant, so we can remove the Keyboard struct. >The virtual keyboards need to be in a exclusive group? That would be a good one to ask in the wlroots IRC. I did it that way based on what the implementations I was looking at did, but I don't know if there's a specific reason for sectioning them off. I tested adding a virtual keyboard (wf-osk) to the group with my physical boards, and it works just fine as far as I can tell, but I can't say definitively one way or the other. >What about having a global KeyboardGroup? That would be possible, and it would simplify things, as long as virtual keyboards are OK to include.
Author
Owner

I can't find any reason for virtual keyboards to need to be kept separate, so I went ahead and changed to using a global KeyboardGroup instance for everything.

I can't find any reason for virtual keyboards to need to be kept separate, so I went ahead and changed to using a global KeyboardGroup instance for everything.
sevz force-pushed keyboard_leds from ad10902ed3 to 55f7c8b52c 2023-12-15 20:55:08 +01:00 Compare
Owner

I fixed a typo and deleted a extra blank line.

I'll be testing a few days before merging.

I fixed a typo and deleted a extra blank line. I'll be testing a few days before merging.
Owner

We could probably also remove KeyboardGroup but I'll leave that for another time.

Also, I feel like we could squash all the commits, or at least the first and second ones

We could probably also remove KeyboardGroup but I'll leave that for another time. Also, I feel like we could squash all the commits, or at least the first and second ones
Author
Owner

I think we should squash them all, personally, unless there's some benefit to keeping the 2nd around (the 1st and 2nd should probably be squashed regardless, since the 1st was entirely replaced)

I think we should squash them all, personally, unless there's some benefit to keeping the 2nd around (the 1st and 2nd should probably be squashed regardless, since the 1st was entirely replaced)
Owner

Ah, almost forgot, please also update the docs for keypressmod and remove the calls to wlr_seat_set_keyboard() leaving only one call when creating the group (you'll need to move the seat creation above it)

Ah, almost forgot, please also update the docs for keypressmod and remove the calls to wlr_seat_set_keyboard() leaving only one call when creating the group (you'll need to move the seat creation above it)
Author
Owner

Which docs are you referring to? I'm removing the block comment about the seat only being able to have one keyboard, as that's no longer relevant, is there something else related to keypressmod that needs an update?

Which docs are you referring to? I'm removing the block comment about the seat only being able to have one keyboard, as that's no longer relevant, is there something else related to keypressmod that needs an update?
Owner

Which docs are you referring to? I'm removing the block comment about the seat only being able to have one keyboard, as that's no longer relevant, is there something else related to keypressmod that needs an update?

Hm
Only that, I wanna keep the comment about only one keyboard, but I'm not quite sure where put it, remove it and I'll re-add it later

> Which docs are you referring to? I'm removing the block comment about the seat only being able to have one keyboard, as that's no longer relevant, is there something else related to keypressmod that needs an update? Hm Only that, I wanna keep the comment about only one keyboard, but I'm not quite sure where put it, remove it and I'll re-add it later
Author
Owner

I could add one about the limitation where I'm assigning the group keyboard to the seat in setup, if that would be a good place to document it.

I could add one about the limitation where I'm assigning the group keyboard to the seat in setup, if that would be a good place to document it.
Owner

Sounds good

Sounds good
sevz left a comment
Owner

Just these little things and everything is done

Just these little things and everything is done
dwl.c Outdated
@ -1437,3 +1397,1 @@
* wlr_seat handles this transparently.
*/
wlr_seat_set_keyboard(seat, kb->wlr_keyboard);
* pressed. We simply communicate this to the client.*/
Owner

Missing space before */

Missing space before `*/`
dwl.c Outdated
@ -2337,0 +2331,4 @@
*/
wlr_seat_set_keyboard(seat, &kb_group.wlr_group->keyboard);
Owner

Extra blank line

Extra blank line
sevz force-pushed keyboard_leds from 0bba6420f1 to 023efce6eb 2023-12-18 00:10:16 +01:00 Compare
Owner

Fixed another typo and squashed all the commits

Do you want to add more details to the commit message?

Fixed another typo and squashed all the commits Do you want to add more details to the commit message?
Author
Owner

No, I think that commit message covers it.

No, I think that commit message covers it.
dwl manually merged commit 023efce6eb into main 2023-12-18 19:52:59 +01:00
thanatos deleted branch keyboard_leds 2024-03-27 00:55:06 +01:00
Sign in to join this conversation.
No description provided.