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

[Bug] Combos have inconsistent triggering #15911

Open
johnwilmes opened this issue Jan 17, 2022 · 1 comment
Open

[Bug] Combos have inconsistent triggering #15911

johnwilmes opened this issue Jan 17, 2022 · 1 comment

Comments

@johnwilmes
Copy link

Currently, combos have somewhat surprising timing and triggering behavior. I'd like to propose and discuss refactoring the combo code to achieve more consistent and dependable combo triggering. Particularly, there is no guarantee that a combo will trigger even if all keys are pressed within COMBO_TERM of each other, and there is no guarantee that a combo will fail to trigger even if the keys are not pressed within COMBO_TERM of each other.

Description

Suppose Combo1 is triggered by a+b and Combo2 is triggered by c+d.

  • With COMBO_STRICT_TIMER, if a, b, and c are all pressed within COMBO_TERM, and d is pressed within COMBO_TERM of c, but not within COMBO_TERM of a, then Combo1 fires but Combo2 does not. Since a+b were pressed within COMBO_TERM, and also c+d were pressed within COMBO_TERM, it would be natural to expect both to fire
  • Without COMBO_STRICT_TIMER, if a, c, and b are pressed, in that order, with each press coming just a bit less than COMBO_TERM after the previous one, then Combo1 will trigger, even though a and b were not pressed together within COMBO_TERM. Even with COMBO_STRICT_TIMER, if COMBO_TERM_PER_COMBO is also set and Combo2 has a longer term than Combo1, we still trigger Combo1 even though a and b were not pressed together within Combo1's combo term.
  • Any key release event resets the whole buffer, so quickly typing c followed by a+b fails to trigger Combo1 (above) if c happens to be released between pressing a and b.
  • Pressing a, then x, then b, all within COMBO_TERM, will trigger Combo 1 if x belongs to some other combo, but not if x belongs to no combo.

The general issue is that the combo key buffer is all-or-nothing: there is no way to release some key presses or combos while keeping other key presses in the buffer and preserving partial activation state of other combos.

Proposed solution

I suggest that combos should provide a behavior guarantee such as the following:

A combo will trigger if and only if

  1. all of its keys are pressed within its combo term, and
  2. none of the following occurs
  • One of the combo's keys was released before the final key was pressed
  • Two events (press and/or release) for the same keycode occur between the first and last triggering key presses
  • The combo has the "tap" requirement, but no triggering key is released within the hold term
  • The combo has the "hold" requirement, but a triggering key is released within the hold term
  • The combo is disabled
  • An overlapping combo with higher priority triggers instead
  • The key buffer overflows between the first and last key presses of the combo

Relatedly, it would be nice to have a new feature that allows specifying whether a combo must have all its key presses contiguous or not. A contiguous combo (the default?) would fail to trigger if any unrelated key (whether or not it is associated to another combo) is pressed between the first and last key presses. A non-contiguous combo would have no such condition. Contiguous combos would be useful for avoiding accidental combo triggers, and non-contiguous combos would be useful for combos that do things like trigger mods, where one might want to fire multiple combos simultaneously.

I hope to submit a PR in the next few days addressing this issue.

@johnwilmes
Copy link
Author

What is the "right" way to deal with overlapping combos? As far as I can tell, the motivation for the current approach to overlapping combos is to allow a combos a+b+c to trigger, even if a+b is also a combo. In other words, the idea is that nested combos should work, where the keys of one combo are a proper subset of the keys of another combo.

The current code prefers longer combos, and otherwise decides arbitrarily for same-size combos which one should be preferred. When a longer combo completes, the shorter combos are disabled and will not fire, even if the longer combo also doesn't end up firing. For example, if a+b and a+b+c are both combos with hold requirements, then pressing a+b+c and quickly releasing c would cause all key strokes to be emitted normally without triggering combos under the current code. This is arguably undesirable - it might be more natural to let the combo a+b trigger in this case.

Naively, a better rule than the current code would be to wait until the end of the hold period to see which combo should trigger. For nested combos, this would work okay because the combo term for the combo a+b+c+d starts at the same time as the combo term for the combo a+b. But for non-nested combos it requires arbitrarily long delays until keypresses resolve. E.g., if a+b, b+c+d, c+d+e+f, etc. are all combos, then to see whether we trigger a+b we need to wait to see whether we trigger b+c+d, and therefore also wait to see whether we trigger c+d+e+f. So this rule would seem to add a lot of complexity for rather dubious benefit.

I suggest that instead, we allow nested combos to work by waiting until the end of the hold period to see if tap/hold requirements are met, before disabling the smaller combos. But for non-nested but overlapping combos, we instead greedily use the first combo to satisfy all its triggering requirements (key presses + tap/hold).

johnwilmes added a commit to johnwilmes/qmk_firmware that referenced this issue Jan 23, 2022
- Resolves issue qmk#15911. Combos now trigger if and only if
    1. all combo keys pressed within combo term, and
    2. no ennumerated exceptions apply (interruption of combo sequence,
       tap/hold requirements, higher priority combo triggers, buffer
       overflow)
- Adds new combo features: detailed combo events, and
  contiguous/noncontiguous combos
- Breaking changes:
    - EXTRA_SMALL_COMBOS now limited to length 4 rather than 6
    - If COMBO_COUNT not defined but user has many combos, they must
      define MANY_COMBOS
    - no COMBO_NO_TIMER or COMBO_STRICT_TIMER options
    - For overlapping combos, priority is determined by combo index:
      lower index is higher priority. Ripe combos only wait for high
      priority combos that involve a superset of trigger keys
    - Combos now trigger in sequence with their first key press rather than last
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant