(#620) Enabling keyboard LEDs when the modifier state changes #3441

mattkae · 2024-06-24T14:42:53Z

fixes #620

What's new?

XkbMapper takes a callback for when the leds need to be updated for a device
Whenever a modifier is changed for a device, XkbMapper notifies the corresponding device of a possible change to its LEDs using Device::set_leds
In the case if a libinput device,LibInputDevice::set_leds calls libinput_device_led_update to update the LEDs

codecov · 2024-06-24T14:56:39Z

Codecov Report

Attention: Patch coverage is 73.53760% with 95 lines in your changes missing coverage. Please review.

Project coverage is 77.14%. Comparing base (317060e) to head (de5f7f5).

Files	Patch %	Lines
src/server/input/xkb_mapper_registrar.cpp	75.26%	71 Missing ⚠️
src/platforms/evdev/libinput_device.cpp	30.76%	18 Missing ⚠️
...ude/mir/test/doubles/mock_led_observer_registrar.h	0.00%	2 Missing ⚠️
tests/mir_test_doubles/mock_libinput.cpp	0.00%	2 Missing ⚠️
src/server/input/default_configuration.cpp	90.00%	1 Missing ⚠️
tests/include/mir/test/doubles/mock_libinput.h	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3441      +/-   ##
==========================================
- Coverage   77.45%   77.14%   -0.32%     
==========================================
  Files        1073     1076       +3     
  Lines       68782    69118     +336     
==========================================
+ Hits        53278    53323      +45     
- Misses      15504    15795     +291

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AlanGriffiths · 2024-06-27T10:47:03Z

include/platform/mir/input/input_device.h

+
+    virtual void set_leds(KeyboardLeds leds) = 0;


Just noting that this is a platform ABI break

I think this change, and the ABI break are now unnecessary

I still think this is unnecessary here (as is the ABI break)

AlanGriffiths

My first thought is that this should be more explicit.

Keyboard devices register for modifier state updates
Modifier updates are passed through that registrar

The only question then is whether the register is a new object or just a new interface supported by XKBMapper

(Maybe you can shoot this idea down though?)

src/platforms/evdev/libinput_device.cpp

AlanGriffiths · 2024-06-27T11:31:01Z

src/server/input/default_configuration.cpp

+           {
+               the_default_input_device_hub()->for_each_mutable_input_device([&](mir::input::Device& device)
+               {
+                    if (device.id() == device_id)
+                        device.set_leds(leds);
+               });
+           });


We should probably capture the_default_input_device_hub(), not call it every time the mapping state changes.

And, surely, there could be a more direct way from XKBMapper to the corresponding device than scanning everything registered on the device hub? (E.g. a map MirInputDeviceId -> callback)

The problem is that the_default_input_device_hub() also relies on the the_key_mapper(), so it's a circular dependency. I will see if there is some other way around this, perhaps with the "registration" idea that you said earlier.

AlanGriffiths

After we spoke I thought you were going to propose a solution that preserved ABI. This isn't that

AlanGriffiths · 2024-06-28T11:37:38Z

src/common/input/xkb_mapper.cpp

-#include "mir/input/xkb_mapper.h"
+#include "mir/input/xkb_mapper_deprecated.h"


Why would we rename the file and break downstreams that use it?

So the tricky part is that the import will be the same when we include mir/input/xkb_mapper.h. So one of them has to be named differently, and I chose the old one.

Would it be a better idea to rename the new one to something slightly different? Or is there a better solution?

So, I thought "we don't change old stuff, just deprecate it". New stuff can be called by new names, old stuff cannot

AlanGriffiths · 2024-06-28T11:44:51Z

src/include/common/mir/input/key_mapper_deprecated.h

I don't see why KeyMapper is being changed. Especially when there are now two different definitions

AlanGriffiths · 2024-06-28T11:46:30Z

include/platform/mir/input/input_device.h

+
+    virtual void set_leds(KeyboardLeds leds) = 0;


I think this change, and the ABI break are now unnecessary

AlanGriffiths · 2024-06-28T11:48:23Z

src/include/server/mir/input/key_mapper.h

+class KeyMapperObserver
+{
+public:
+    virtual ~KeyMapperObserver() = default;
+    virtual void leds_set(MirInputDeviceId id, KeyboardLeds leds) = 0;
+};
+


This isn't a KeyMapper observer. It is a LED state observer.

We should be keeping the KeyMapper interface unchanged. The ObserverRegister can be part of another interface implemented by XkbMapper.

Also, wouldn't it be better to register with the device id and only send notifications to the corresponding device. (And not notify all devices most of which drop the notification.

Yup I agree 👍

AlanGriffiths · 2024-06-28T11:53:55Z

src/server/input/default_input_device_hub.cpp

@@ -39,6 +40,26 @@

 namespace mi = mir::input;

+namespace
+{
+class DefaultInputDeviceHubKeyMapperObserver : public mi::KeyMapperObserver


I don't think the DefaultInputDeviceHub should be registering. Only devices that support LEDs

AlanGriffiths · 2024-07-01T11:49:04Z

src/include/server/mir/input/led_observer_registrar.h

+class LedObserver
+{
+public:
+    virtual ~LedObserver() = default;
+    virtual void leds_set(MirInputDeviceId id, KeyboardLeds leds) = 0;
+};


It is our practice to disable CopyAssign for interfaces

AlanGriffiths

This seems unfinished

AlanGriffiths · 2024-07-01T11:51:12Z

src/include/server/mir/input/led_observer_registrar.h

+class LedObserverRegistrar : public ObserverRegistrar<LedObserver>
+{
+public:
+    virtual ~LedObserverRegistrar() = default;
+};
+}


As previously discussed I think we should be registering with a device id, which isn't supported by ObserverRegistrar<>

AlanGriffiths · 2024-07-01T11:52:03Z

include/platform/mir/input/input_device.h

+
+    virtual void set_leds(KeyboardLeds leds) = 0;


I still think this is unnecessary here (as is the ABI break)

AlanGriffiths · 2024-07-01T11:58:12Z

src/include/server/mir/input/xkb_mapper_registrar.h

+    void register_interest(std::weak_ptr<LedObserver> const& observer) override;
+    void register_interest(
+        std::weak_ptr<LedObserver> const& observer,
+        Executor& executor) override;


I don't see this implemented or used?

mattkae · 2024-07-01T18:42:31Z

This seems unfinished

I have updated it 👍

AlanGriffiths

I think we could simplify further and reduce change

AlanGriffiths · 2024-07-01T16:59:15Z

include/platform/mir/input/device.h

Does this file need to move?

AlanGriffiths · 2024-07-02T08:55:43Z

include/platform/mir/input/device.h

Why does this file need to move?

AlanGriffiths · 2024-07-02T08:58:59Z

include/platform/mir/input/platform.h

-    std::shared_ptr<InputReport> const& report);
+    std::shared_ptr<InputReport> const& report,
+    std::shared_ptr<LedObserverRegistrar> const& led_observer_registrar);


This is an ABI break - I still don't think that is needed

AlanGriffiths · 2024-07-02T09:09:34Z

src/platforms/evdev/libinput_device.cpp

+void mie::LibInputDevice::associate_to_id(MirInputDeviceId id)
+{
+    try_stop_observing_leds();
+    led_observer = std::make_shared<LibInputDeviceLedObserver>(this);
+    led_observer_registrar->register_interest(led_observer, id);
+    device_id = id;
+}


This code looks like initialization, but is not in a constructor. Is there a good reason it is here? (And not, say, in mi::DefaultDevice?)

The reason it is here is to not break the input ABI, but perhaps I'm not seeing something ;)

AlanGriffiths · 2024-07-02T09:19:30Z

src/platforms/evdev/platform.cpp

+        auto weak_device = input_device_registry->add_device(devices.back());
+
+        if (auto device = weak_device.lock())
+            new_device->associate_to_id(device->id());


Oh!

So we're making mi::Device visible and passing LedObserverRegistrar around so that we can do two-phase initialization here?

Can we simplify things by registering the callback from DefaultInputDeviceHub/DefaultDevice?

That would require InputDevice to have set_leds defined on it, which would break the input ABI again 😄 That's how I had it before.

But I guess we broke it anyway my change the create_platform signature 😂

mattkae · 2024-07-02T17:39:27Z

@AlanGriffiths For your AM tomorrow, here's a summary what I have right now:

We have a new interface called LedObserverRegistrar that gets passed to the input Platform
Every platform except evdev ignores that argument. The evdev platform uses it to register on Led events in LibinputDevice
XkbMapperRegistrar will then notify LibInputDevice when an LED changes

In this way, only the evdev platform needs to concern itself with the LEDs, because it is the only one that cares about it.

AlanGriffiths · 2024-07-10T17:33:53Z

@mattkae I've had a quick pass at the approach I prefer with #3471 - it is still a bit rough, but you are allowed to look

AlanGriffiths

I don't have the hardware to test, but this all looks sensible

AlanGriffiths · 2024-07-11T13:37:56Z

This branch has conflicts that must be resolved

Looks like a rebase is in order

mattkae changed the title ~~Feature/620~~ WIP: Do not review! (#620) Enabling keyboard LEDs when the modifier state changes Jun 24, 2024

mattkae changed the title ~~WIP: Do not review! (#620) Enabling keyboard LEDs when the modifier state changes~~ (#620) Enabling keyboard LEDs when the modifier state changes Jun 24, 2024

mattkae requested a review from RAOF June 25, 2024 13:10

mattkae marked this pull request as ready for review June 25, 2024 13:10

mattkae requested a review from a team as a code owner June 25, 2024 13:10

AlanGriffiths reviewed Jun 27, 2024

View reviewed changes

mattkae force-pushed the feature/620 branch 3 times, most recently from 49b787c to b361733 Compare June 27, 2024 17:27

mattkae requested a review from AlanGriffiths June 27, 2024 17:36

AlanGriffiths requested changes Jun 28, 2024

View reviewed changes

mattkae force-pushed the feature/620 branch 2 times, most recently from 4ac484a to 24e5e33 Compare June 28, 2024 20:14

AlanGriffiths reviewed Jul 1, 2024

View reviewed changes

AlanGriffiths requested changes Jul 1, 2024

View reviewed changes

mattkae requested a review from AlanGriffiths July 1, 2024 18:42

AlanGriffiths requested changes Jul 2, 2024

View reviewed changes

mattkae requested a review from AlanGriffiths July 11, 2024 11:48

AlanGriffiths approved these changes Jul 11, 2024

View reviewed changes

(#620) Enabling keyboard LEDs when the modifier state changes

ccb60fb

mattkae force-pushed the feature/620 branch from 9031964 to ccb60fb Compare July 11, 2024 13:51

mattkae added this pull request to the merge queue Jul 11, 2024

Merged via the queue into main with commit 873fea3 Jul 11, 2024
33 of 36 checks passed

mattkae deleted the feature/620 branch July 11, 2024 14:31

AlanGriffiths mentioned this pull request Jul 17, 2024

Caps lock LED lights and stays lit regardless of lock state #3492

Open

AlanGriffiths mentioned this pull request Sep 9, 2024

Release 2.18.0 #3532

Merged

		#include "mir/input/xkb_mapper.h"
		#include "mir/input/xkb_mapper_deprecated.h"

(#620) Enabling keyboard LEDs when the modifier state changes #3441

(#620) Enabling keyboard LEDs when the modifier state changes #3441

Conversation

mattkae commented Jun 24, 2024 • edited Loading

What's new?

codecov bot commented Jun 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkae Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkae commented Jul 1, 2024

AlanGriffiths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkae commented Jul 2, 2024

AlanGriffiths commented Jul 10, 2024

AlanGriffiths left a comment

Choose a reason for hiding this comment

AlanGriffiths commented Jul 11, 2024

mattkae commented Jun 24, 2024 •

edited

Loading

codecov bot commented Jun 24, 2024 •

edited

Loading

mattkae Jun 27, 2024 •

edited

Loading