Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing release events when keys are released in order of pressing #965

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 64 additions & 61 deletions inputremapper/injection/mapping_handlers/combination_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class CombinationHandler(MappingHandler):
_pressed_keys: Dict[Hashable, bool]
# the last update we sent to a sub-handler. If this is true, the output key is
# still being held down.
_output_active: bool
_output_previously_active: bool
_sub_handler: InputEventHandler
_handled_input_hashes: list[Hashable]
_requires_a_release: Dict[Tuple[int, int], bool]
Expand All @@ -60,7 +60,7 @@ def __init__(
logger.debug(str(mapping))
super().__init__(combination, mapping)
self._pressed_keys = {}
self._output_active = False
self._output_previously_active = False
self._context = context
self._requires_a_release = {}

Expand Down Expand Up @@ -109,74 +109,77 @@ def notify(
is_pressed = event.value == 1
self._pressed_keys[event.input_match_hash] = is_pressed
# maybe this changes the activation status (triggered/not-triggered)
is_activated = self.is_activated()
changed = self.is_activated() != self._output_previously_active

if changed:
if is_pressed:
return self._handle_freshly_activated(suppress, event, source)
else:
return self._handle_freshly_deactivated(event, source)
else:
if is_pressed:
return self._handle_no_change_press(event)
else:
return self._handle_no_change_release(event)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea, but I think this is a bit misleading. If I read this correctly _handle_no_change_release will be called when the state changes from suppressed to passive. I guess a simple comment should be fine.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit lost, could you please propose a change with the comment?


def _handle_no_change_press(self, event: InputEvent) -> bool:
"""A key was pressed, but this doesn't change the combinations activation state.
Can only happen if either the combination wasn't already active, or a duplicate
key-down event arrived (EV_ABS?)
"""
# self._output_previously_active is negated, because if the output is active, a
# key-down event triggered it, which then did not get forwarded, therefore
# it doesn't require a release.
self.require_release_later(not self._output_previously_active, event)
# output is active: consume the event
# output inactive: forward the event
return self._output_previously_active

def _handle_no_change_release(self, event: InputEvent) -> bool:
"""One of the combinations keys was released, but it didn't untrigger the
combination yet."""
# Negate: `False` means that the event-reader will forward the release.
return not self.should_release_event(event)

if is_activated == self._output_active:
return self._handle_state_did_not_change(is_pressed, event)
def _handle_freshly_activated(
self,
suppress: bool,
event: InputEvent,
source: evdev.InputDevice,
) -> bool:
"""The combination was deactivated, but is activated now."""
if suppress:
return False

# This depends on whether the key was pressed or released, therefore those are
# equal
assert is_activated == is_pressed
return self._handle_state_changed(is_activated, suppress, event, source)
# Send key up events to the forwarded uinput if configured to do so.
self.forward_release()

def _handle_state_did_not_change(self, is_pressed: bool, event: InputEvent) -> bool:
"""Handle the event when it didn't change the combination activation state.
logger.debug("Sending %s to sub-handler", self.mapping.input_combination)
self._output_previously_active = bool(event.value)
sub_handler_result = self._sub_handler.notify(event, source, suppress)

The combination was previously triggered, and is still triggered,
or it was not yet triggered and still isn't triggered.
"""
if is_pressed:
# self._output_active is negated, because if the output is active, a
# key-down event triggered it, which then did not get forwarded, therefore
# it doesn't require a release.
self.require_release_later(not self._output_active, event)
# output is active: consume the event
# output inactive: forward the event
return self._output_active

# Else if it is released: Returning `False` means that the event-reader
# will forward the release.
return not self.should_release_event(event)
# Only if the sub-handler return False, we need a release-event later.
# If it handled the event, the user never sees this key-down event.
self.require_release_later(not sub_handler_result, event)
return sub_handler_result

def _handle_state_changed(
def _handle_freshly_deactivated(
self,
is_activated: bool,
suppress: bool,
event: InputEvent,
source: evdev.InputDevice,
) -> bool:
"""Handle a changed combination-activation state.
"""The combination was activated, but is deactivated now."""
# We ignore the `suppress` argument for release events. Otherwise, we
# might end up with stuck keys (test_event_pipeline.test_combination).
# In the case of output axis, this will enable us to activate multiple
# axis with the same button.

Either it was previously triggered, but not anymore, or it was not yet
triggered, but is now.
"""
if not is_activated:
# We ignore the `suppress` argument for release events. Otherwise, we
# might end up with stuck keys (test_event_pipeline.test_combination).
# In the case of output axis, this will enable us to activate multiple
# axis with the same button.
suppress = False

if not suppress:
if is_activated:
# Send key up events to the forwarded uinput if configured to do so.
self.forward_release()

logger.debug("Sending %s to sub-handler", self.mapping.input_combination)
self._output_active = bool(event.value)
sub_handler_result = self._sub_handler.notify(event, source, suppress)

if is_activated:
# Only if the sub-handler return False, we need a release-event later.
# If it handled the event, the user never sees this key-down event.
self.require_release_later(not sub_handler_result, event)
return sub_handler_result

# Else if it is released: Returning `False` means that the event-reader
# will forward the release.
return not self.should_release_event(event)

return False
logger.debug("Sending %s to sub-handler", self.mapping.input_combination)
self._output_previously_active = bool(event.value)
self._sub_handler.notify(event, source, suppress=False)

# Negate: `False` means that the event-reader will forward the release.
return not self.should_release_event(event)

def should_release_event(self, event: InputEvent) -> bool:
"""Check if the key-up event should be forwarded by the event-reader.
Expand Down Expand Up @@ -205,7 +208,7 @@ def reset(self) -> None:
for key in self._pressed_keys:
self._pressed_keys[key] = False
self._requires_a_release = {}
self._output_active = False
self._output_previously_active = False

def is_activated(self) -> bool:
"""Return if all keys in the keymap are set to True."""
Expand Down
Loading