WaylandKeyboard: Handle key repeat state [chromium/src : main]
AbdAlRahman Gad (Gerrit)
AbdAlRahman Gad added 1 comment![]( "Open in Gerrit")
The same change with some logs is here: https://chromium-review.googlesource.com/c/chromium/src/+/6973437/1
Related details
- Max Ihlenfeldt
- Orko Garai
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AbdAlRahman Gad (Gerrit)
AbdAlRahman Gad added 1 comment![]( "Open in Gerrit")
When `WL_KEYBOARD_KEY_STATE_REPEATED` is sent, do we need to send `DispatchKey` with `repeat` is `true` so that `EF_IS_REPEAT` flag is set?
I read the docs for the flags but couldn't figure out if I should set `repeat` or not: https://source.chromium.org/chromium/chromium/src/+/main:ui/events/event.h;l=783;drc=717fed8880afe2cba710d198019b9fbf25a9c4e7
Related details
- Max Ihlenfeldt
- Orko Garai
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Max Ihlenfeldt (Gerrit)
Max Ihlenfeldt added 2 comments![]( "Open in Gerrit")
When `WL_KEYBOARD_KEY_STATE_REPEATED` is sent, do we need to send `DispatchKey` with `repeat` is `true` so that `EF_IS_REPEAT` flag is set?
I read the docs for the flags but couldn't figure out if I should set `repeat` or not: https://source.chromium.org/chromium/chromium/src/+/main:ui/events/event.h;l=783;drc=717fed8880afe2cba710d198019b9fbf25a9c4e7
Please check what flags `EventAutoRepeatHandler` sets on the events it emits.
auto_repeat_handler_.IsAutoRepeatEnabled()) {I think we need to handle two cases in this class:
1. client-side key repeat is in use, i.e. we receive a `wl_keyboard.repeat_info` event with `rate != 0`.
2. compositor-side key repeat is in use, i.e. we receive a `wl_keyboard.repeat_info` event with `rate == 0`.
For (1), we want to use `auto_repeat_handler_` with the provided configuration. For (2), we want to disable using `auto_repeat_handler_` and instead emit events in this method (`ProcessKey()`) when we get an event with the `repeated` state.
`WaylandKeyboard::OnRepeatInfo()` already does most of this, it seems to me the only missing thing is correctly handling repeat events in this method. We should also include a check to ignore repeat events (maybe with a warning log) if `auto_repeat_handler_` is enabled.
Related details
- AbdAlRahman Gad
- Orko Garai
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Max Ihlenfeldt (Gerrit)
Max Ihlenfeldt added 1 comment![]( "Open in Gerrit")
auto_repeat_handler_.IsAutoRepeatEnabled()) {I think we need to handle two cases in this class:
1. client-side key repeat is in use, i.e. we receive a `wl_keyboard.repeat_info` event with `rate != 0`.
2. compositor-side key repeat is in use, i.e. we receive a `wl_keyboard.repeat_info` event with `rate == 0`.For (1), we want to use `auto_repeat_handler_` with the provided configuration. For (2), we want to disable using `auto_repeat_handler_` and instead emit events in this method (`ProcessKey()`) when we get an event with the `repeated` state.
`WaylandKeyboard::OnRepeatInfo()` already does most of this, it seems to me the only missing thing is correctly handling repeat events in this method. We should also include a check to ignore repeat events (maybe with a warning log) if `auto_repeat_handler_` is enabled.
Ugh, please excuse my Monday brain and disregard almost everything of the above comment. Please add a `LOG(WARNING)` before the return here.
Orko Garai (Gerrit)
Orko Garai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Orko Garai
| Code-Review | +1 |
1 comment
lgtm % nit about the warning log suggested by max
Related details
- AbdAlRahman Gad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AbdAlRahman Gad (Gerrit)
AbdAlRahman Gad added 3 comments![]( "Open in Gerrit")
Max IhlenfeldtWhen `WL_KEYBOARD_KEY_STATE_REPEATED` is sent, do we need to send `DispatchKey` with `repeat` is `true` so that `EF_IS_REPEAT` flag is set?
I read the docs for the flags but couldn't figure out if I should set `repeat` or not: https://source.chromium.org/chromium/chromium/src/+/main:ui/events/event.h;l=783;drc=717fed8880afe2cba710d198019b9fbf25a9c4e7
Please check what flags `EventAutoRepeatHandler` sets on the events it emits.
It sends `repeat` with `true` which eventually sets `EF_IS_REPEAT` flag when creating the event: [OnRepeatCommit](https://source.chromium.org/chromium/chromium/src/+/main:ui/events/ozone/keyboard/event_auto_repeat_handler.cc;l=111) and eventually here at [OnKeyboardKeyEvent](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/host/wayland_event_source.cc;l=266;drc=717fed8880afe2cba710d198019b9fbf25a9c4e7)
1. The case when `repeat == 0` works correctly and the repeat rate will be controlled by the compositor.
2. But when `repat != 0` on my machine, the nested `KWin` session will send this:
```c
[3520410.143] {Default Queue} wl_keyboard#28.repeat_info(31, 500)
[3520410.332] wl_keyboard#41.repeat_info(0, 500)
```
- The auto repeat is not enabled: `auto_repeat_handler_.IsAutoRepeatEnabled() = 0`
- But the keys are going to be repeated at higher rate like the one in the settings.
I think this doesn't violate the protocol but it's kinda weird.
Max IhlenfeldtI think we need to handle two cases in this class:
1. client-side key repeat is in use, i.e. we receive a `wl_keyboard.repeat_info` event with `rate != 0`.
2. compositor-side key repeat is in use, i.e. we receive a `wl_keyboard.repeat_info` event with `rate == 0`.For (1), we want to use `auto_repeat_handler_` with the provided configuration. For (2), we want to disable using `auto_repeat_handler_` and instead emit events in this method (`ProcessKey()`) when we get an event with the `repeated` state.
`WaylandKeyboard::OnRepeatInfo()` already does most of this, it seems to me the only missing thing is correctly handling repeat events in this method. We should also include a check to ignore repeat events (maybe with a warning log) if `auto_repeat_handler_` is enabled.
Ugh, please excuse my Monday brain and disregard almost everything of the above comment. Please add a `LOG(WARNING)` before the return here.
- Max Ihlenfeldt
- Orko Garai
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orko Garai (Gerrit)
Orko Garai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Orko Garai
| Code-Review | +1 |
Related details
- AbdAlRahman Gad
- Max Ihlenfeldt
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Max Ihlenfeldt (Gerrit)
Max Ihlenfeldt voted and added 3 comments![]( "Open in Gerrit")
Votes added by Max Ihlenfeldt
| Code-Review | +1 |
3 comments
1. The case when `repeat == 0` works correctly and the repeat rate will be controlled by the compositor.
2. But when `repat != 0` on my machine, the nested `KWin` session will send this:
```c
[3520410.143] {Default Queue} wl_keyboard#28.repeat_info(31, 500)
[3520410.332] wl_keyboard#41.repeat_info(0, 500)
```
- The auto repeat is not enabled: `auto_repeat_handler_.IsAutoRepeatEnabled() = 0`
- But the keys are going to be repeated at higher rate like the one in the settings.
I think this doesn't violate the protocol but it's kinda weird.
It looks like there are two event queues, and one is probably the one used by GTK? So ozone/wayland might be the one receiving the second event with a rate of 0, and thus correctly disable `auto_repeat_handler_`. Maybe Orko can give some additional context on whether my theory makes sense.
lgtm % nit
state == WL_KEYBOARD_KEY_STATE_REPEATED ? true : false /*repeat*/,nit: This can be simplified to just `state == WL_KEYBOARD_KEY_STATE_REPEATED` without the ternary operator.
Related details
- AbdAlRahman Gad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AbdAlRahman Gad (Gerrit)
AbdAlRahman Gad added 2 comments![]( "Open in Gerrit")
Max Ihlenfeldt1. The case when `repeat == 0` works correctly and the repeat rate will be controlled by the compositor.
2. But when `repat != 0` on my machine, the nested `KWin` session will send this:
```c
[3520410.143] {Default Queue} wl_keyboard#28.repeat_info(31, 500)
[3520410.332] wl_keyboard#41.repeat_info(0, 500)
```
- The auto repeat is not enabled: `auto_repeat_handler_.IsAutoRepeatEnabled() = 0`
- But the keys are going to be repeated at higher rate like the one in the settings.
I think this doesn't violate the protocol but it's kinda weird.
It looks like there are two event queues, and one is probably the one used by GTK? So ozone/wayland might be the one receiving the second event with a rate of 0, and thus correctly disable `auto_repeat_handler_`. Maybe Orko can give some additional context on whether my theory makes sense.
@or...@igalia.com Could you please take a look at this comment?
state == WL_KEYBOARD_KEY_STATE_REPEATED ? true : false /*repeat*/,nit: This can be simplified to just `state == WL_KEYBOARD_KEY_STATE_REPEATED` without the ternary operator.
- Orko Garai
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orko Garai (Gerrit)
Orko Garai added 1 comment![]( "Open in Gerrit")
Max Ihlenfeldt1. The case when `repeat == 0` works correctly and the repeat rate will be controlled by the compositor.
2. But when `repat != 0` on my machine, the nested `KWin` session will send this:
```c
[3520410.143] {Default Queue} wl_keyboard#28.repeat_info(31, 500)
[3520410.332] wl_keyboard#41.repeat_info(0, 500)
```
- The auto repeat is not enabled: `auto_repeat_handler_.IsAutoRepeatEnabled() = 0`
- But the keys are going to be repeated at higher rate like the one in the settings.
I think this doesn't violate the protocol but it's kinda weird.
AbdAlRahman GadIt looks like there are two event queues, and one is probably the one used by GTK? So ozone/wayland might be the one receiving the second event with a rate of 0, and thus correctly disable `auto_repeat_handler_`. Maybe Orko can give some additional context on whether my theory makes sense.
@or...@igalia.com Could you please take a look at this comment?
Yes there are 2 queues. One is the default one, which we don't use. We use our own queue created [in WaylandConnection](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/host/wayland_connection.cc;l=210;drc=1e665a659d8fac8e71a0263132d9dc9cdabb2fc6;bpv=0;bpt=0). So we can ignore the "Default Queue".
I found some additional context [here](https://wayland.freedesktop.org/docs/html/apb.html#Client-classwl__display):
A wl_display has at least one event queue, called the default queue. Clients can create additional event queues with wl_display_create_queue()
Related details
- AbdAlRahman Gad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AbdAlRahman Gad (Gerrit)
AbdAlRahman Gad added 1 comment![]( "Open in Gerrit")
Max Ihlenfeldt1. The case when `repeat == 0` works correctly and the repeat rate will be controlled by the compositor.
2. But when `repat != 0` on my machine, the nested `KWin` session will send this:
```c
[3520410.143] {Default Queue} wl_keyboard#28.repeat_info(31, 500)
[3520410.332] wl_keyboard#41.repeat_info(0, 500)
```
- The auto repeat is not enabled: `auto_repeat_handler_.IsAutoRepeatEnabled() = 0`
- But the keys are going to be repeated at higher rate like the one in the settings.
I think this doesn't violate the protocol but it's kinda weird.
AbdAlRahman GadIt looks like there are two event queues, and one is probably the one used by GTK? So ozone/wayland might be the one receiving the second event with a rate of 0, and thus correctly disable `auto_repeat_handler_`. Maybe Orko can give some additional context on whether my theory makes sense.
Orko Garai@or...@igalia.com Could you please take a look at this comment?
Yes there are 2 queues. One is the default one, which we don't use. We use our own queue created [in WaylandConnection](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/host/wayland_connection.cc;l=210;drc=1e665a659d8fac8e71a0263132d9dc9cdabb2fc6;bpv=0;bpt=0). So we can ignore the "Default Queue".
I found some additional context [here](https://wayland.freedesktop.org/docs/html/apb.html#Client-classwl__display):
A wl_display has at least one event queue, called the default queue. Clients can create additional event queues with wl_display_create_queue()
Thanks, I think this makes the CL ready for review. Let me know if I misunderstood anything or if we need to change something.
Related details
- Max Ihlenfeldt
- Orko Garai
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Max Ihlenfeldt (Gerrit)
Max Ihlenfeldt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Max Ihlenfeldt
| Code-Review | +1 |
Max Ihlenfeldt1. The case when `repeat == 0` works correctly and the repeat rate will be controlled by the compositor.
2. But when `repat != 0` on my machine, the nested `KWin` session will send this:
```c
[3520410.143] {Default Queue} wl_keyboard#28.repeat_info(31, 500)
[3520410.332] wl_keyboard#41.repeat_info(0, 500)
```
- The auto repeat is not enabled: `auto_repeat_handler_.IsAutoRepeatEnabled() = 0`
- But the keys are going to be repeated at higher rate like the one in the settings.
I think this doesn't violate the protocol but it's kinda weird.
AbdAlRahman GadIt looks like there are two event queues, and one is probably the one used by GTK? So ozone/wayland might be the one receiving the second event with a rate of 0, and thus correctly disable `auto_repeat_handler_`. Maybe Orko can give some additional context on whether my theory makes sense.
Orko Garai@or...@igalia.com Could you please take a look at this comment?
AbdAlRahman GadYes there are 2 queues. One is the default one, which we don't use. We use our own queue created [in WaylandConnection](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/host/wayland_connection.cc;l=210;drc=1e665a659d8fac8e71a0263132d9dc9cdabb2fc6;bpv=0;bpt=0). So we can ignore the "Default Queue".
I found some additional context [here](https://wayland.freedesktop.org/docs/html/apb.html#Client-classwl__display):
A wl_display has at least one event queue, called the default queue. Clients can create additional event queues with wl_display_create_queue()
Thanks, I think this makes the CL ready for review. Let me know if I misunderstood anything or if we need to change something.
- AbdAlRahman Gad
- Orko Garai
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orko Garai (Gerrit)
Orko Garai voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- AbdAlRahman Gad
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AbdAlRahman Gad (Gerrit)
AbdAlRahman Gad voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
WaylandKeyboard: Handle key repeat state
- bump wayland version to 1.24.0
- M DEPS
- M third_party/wayland/README.chromium
- M third_party/wayland/include/config.h
- M third_party/wayland/include/src/wayland-version.h
- M third_party/wayland/src
- M third_party/wayland/wayland_version.gni
- M ui/ozone/platform/wayland/host/wayland_keyboard.cc
- M ui/ozone/platform/wayland/host/wayland_pointer.cc
- M ui/ozone/platform/wayland/host/wayland_pointer.h
- M ui/ozone/platform/wayland/host/wayland_seat.cc
Code-Review: +1 by Max Ihlenfeldt, +1 by Orko Garai
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |