Fixing release events when keys are released in order of pressing

[sezanzeb]

1.

a + b -> c

1. press a
2. press b
3. release a
4. release b

the release event for a was never injected

---

2.

"release_input" "on" released events, for which no key-down event was written before

[jonasBoss]

I think there are still some bugs in the notify() method. There are 3 different states and 6 different state transitions:

• P Passive
• A Active
• S Suppressed

1. P->P with key down
2. P->P with key up
3. P->A key down
4. A->P key up
5. P->S key down
6. S->P key up

I think the first 4 transitions are good, but not the transitions 5 and 6.

On transition 5 self.forward_release() will be executed (line 129). Basically releasing the input even though the mapping does not execute. A test for this would be two mappings:
a+b+c->1 and a+c->2 on mapping 1 set release_combination_keys=False and then input a+b+c the first mapping will be active, the second one suppressed but the key a is still released.

On transition 6 the line return not self.should_release_event(event) (line 125) is executed. Now there are two possibilities: the released key is the exact same key that triggered transition 5 -> Return False
the released key is a different one -> Return True

I think the second possibility can cause a stuck key like so (assuming transition 5 is fixed):
mappings: a+b+d->1 and c+d->2
press a+b+c+d -> mapping 1 is in sate A and 2 in state S
release c -> mapping 2 does transition 6 but the release of c is not forwarded because line 125 returns True (i think)

[sezanzeb]

Thanks. I'm pretty impressed that you figured that out by looking at the code.

I reproduced transition 5 in test_suppressed_doesnt_forward_releases and fixed it.

I couldn't reproduce transition 6. Did I test that correctly in test_no_stuck_key?

[jonasBoss]

I had to draw a state diagram in order to wrap my head around this and see all the possible state transitions.

Going to look at transition 6 again, as I could not check this without the fix for transition 5.

[sezanzeb]

Sounds like something that I should do as well sometimes. I'm curious, could you please share the state diagram?

[jonasBoss]

[jonasBoss]

I think I was wrong with the transition 6. should_release_event will always return True (assuming there are no duplicate key-up events).

The only key that is not in _requires_a_release is the one that triggered transition 5, and that key must have triggered transition 3 in a mapping with higher priority. Therefore it did not get forwarded and cannot get stuck.

[sezanzeb]

Ok, thank you. Shall we merge?

[sezanzeb]

Should we keep test_no_stuck_key?

[jonasBoss]

Should we keep test_no_stuck_key?

I think it is fine to keep

Ok, thank you. Shall we merge?

You can change the things I commented or not, I am not sure what is more readable

[sezanzeb]

I think we can remove the calls to event.modify. tests are still passing. key_handler handles it correctly by doing bool(value), so any value > 0 is true. The abs_to_btn handler modifies the value already based on the threshold anyway.

[sezanzeb]

With the recent changes and discoveries, I managed to split it into 4 separate methods

• _handle_freshly_activated
• _handle_freshly_deactivated
• _handle_no_change_press
• _handle_no_change_release

Is this more maintainable? I guess so

[jonasBoss]

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.

[sezanzeb]

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

[sezanzeb]

Thank you so much @jonasBoss, after merging I realized I probably should have added you as a co-author of that commit. Great review.

[sezanzeb]

I'll make a new 2.0.2 release in a few weeks