Fix CombinationHandler releasing

[sezanzeb]

notify of CombinationHandler has a forward_release method, which iterates over keys_to_release and sends all of them into a single forward_to device

notify passes a single "forward" uinput down.

some of the events in keys_to_release originate from other devices though

I'm trying to add a property to the inputEvent that has a reference to the correct forwarding device

[jonasBoss]

I think it is a better solution to add all uinputs and sources to the Context. this way we can remove the Source and forward parameters from the notify method. yesterday did some of the changes to the context already: https://github.com/jonasBoss/key-mapper/tree/fix_release_combination_keys

[sezanzeb]

To then get the correct device for forwarding from the context via origin_hash?

[jonasBoss]

each InputEvent and InputConfig already has a origin_hash so we can use that to in the forward_release method something like this:

    def forward_release(self) -> None:
        """Forward a button release for all keys if this is a combination

        this might cause duplicate key-up events but those are ignored by evdev anyway
        """
        if len(self._pressed_keys) == 1 or not self.mapping.release_combination_keys:
            return

        for input_config in self.mapping.input_combination:
            device = self.context.get_uinput(input_config.origin_hash)
            device.write(*input_config.type_and_code, 0)
        forward.syn()

[jonasBoss]

This also allows us to simplify all the AbsTo...Handler.notify() methods, because we can retrieve the absinfo during the __init__

[sezanzeb]

device = self.context.get_uinput(input_config.origin_hash)

I think get_uinput gets the target uinput though

[sezanzeb]

ah, get_uinput kinda does two things. I'll try to split it

[sezanzeb]

I don't have a device that triggers that edge case, can you please try it?

[jonasBoss]

I don't have a device that triggers that edge case, can you please try it?

I don't either

[jonasBoss]

ah, get_uinput kinda does two things. I'll try to split it

Treating the global_uinputs and the forward_uinputs the same opens up the possibility to have mappings where the target uiput is the forwarded device. We just need to change the MappingHandler's to not use the global_uinputs, but instead the context.get_uinput()

[jonasBoss]

And also the source. And provide the correct origin_hash instead.

[sezanzeb]

Thanks.

About the source: I'd generally prefer passing the original objects whenever possible, instead of derived information or members.

[jonasBoss]

hash_a = "some hash"

context.get_source(hash_a)  # returns the source device
context.get_uinput(hash_a)  # returns the forward_to device

Since we need the origin hash to retrieve the correct forward_to device we can reduce the number of arguments to notify() by providing only the hash.

This also eliminates the possibility to create a bug where the source has a different origin_hash than the one provided.

We could instead not provide the origin_hash and calculate it during runtime, but that is expensive.

[sezanzeb]

Since we need the origin hash to retrieve the correct forward_to device we can reduce the number of arguments to notify() by providing only the hash.

the correct forward_to device is resolved by looking at the InputConfig objects though

[jonasBoss]

That is correct, so we don't even need the hash.

It is not relevant to this PR, but I was looking through all the notify() methods, and the source is only ever used to do some init stuff like finding out the absinfo.

Now that the context knows about all the sources and it gets passed to all MappingHandlers we should probably remove the source argument form notify and do the init stuff inside __init__ that would clean up a few mapping handlers.

[sezanzeb]

To reiterate, because I might have forgotten what this was actually all about

The point of this is, that the input that triggers a combination is released correctly, when a device has sub-devices with similar capabilities

a mouse "A" that has two devices "A_1" and "A_2" in evdev. Both of them list BTN_EXTRA and BTN_SIDE in their capabilities for whatever funny reason.

• BTN_EXTRA and BTN_SIDE arrive via "A_2"
• they are sent into "A_2 forwarded"
• the mapping triggers
• the release events for those two go into device "A_2 forwarded", and not into "A_1 forwarded"

Was that it?

Do you know which github-issues are going to be closed with this?

[jonasBoss]

this closes #577

The issue is more general than what you described.
If a mapping has inputs from two different sub devices, it will always fail to send the release events to the correct one. Regardless if both sub devices have similar capabilities or not.

Currently the notify method looks like:

def notify(
    self,
    event: InputEvent,

    # the source of the event, relevant to get the absinfo for AbsTo...Handler
    source: evdev.InputDevice,

    # the forward device which is blindly used to forward the release events.
    # On mappings with multiple input devices, this is guaranteed to be the wrong one 
    # for at least one input
    forward: evdev.UInput = None, 
    suppress: bool = False,
) -> bool:

If we manage the forward devices in the Context we can use the origin_hash from the mapping
to find the correct forward device, and remove the forward argument from notify.

That requires all Handlers to have access to the context.

It would also make senses store references to the source devices and the virtual uinputs in the context. Than we can also remove the source argument and we can hide the fact that there is a global_uinputs object.

[sezanzeb]

Thanks!

CombinationHandler has the context, and it figures out the device to send the release to by looking at the origin_hash. Sounds good.

I just need to write a few tests for this and document the reasoning and then this is done. All old tests are passing.