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.

Sign up for GitHub

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

Jump to bottom

Fix combo_disable #13988

Merged
merged 1 commit into from
Aug 14, 2021
Merged

Fix combo_disable #13988

merged 1 commit into from
Aug 14, 2021

Conversation

sevanteri
Copy link
Contributor

@sevanteri sevanteri commented Aug 13, 2021 •
edited
Loading

Description

Fixes a couple of issues with combo_disable function calls.

The key buffer was not emptied when disabling combos; I had removed the dump_key_buffer() call as it could lead to infinite loops. Removing the call it seems wasn't the right move.
Now recursive calls to dump_key_buffer start from the next key instead of the key that is "currently being dumped" from the buffer.

Another issues with disabling combos was that any currently active combo wasn't released on key releases.
Now combo key releases are always handled, even if combo processing is disabled.

Easy way to test the current issues:

  • create a combo that resolves to LT(_LAYER, KC_X)
  • When changing to _LAYER, call disable_combo()

Then just hold the combo for a while to activate the layer. On release the layer stays on.

I've messaged the two I know had issues with this to review this.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@sevanteri
Fix combo_disable
fd51826 
- Dump key buffer when combos are disabled.
- Recursive calls to `dump_key_buffer` need to start dumping the buffer
  from index i+1 to avoid possible infinite loops.
- Handle combo key releases even though combo processing is disabled.
@github-actions github-actions bot added the core label Aug 13, 2021
@sevanteri sevanteri mentioned this pull request Aug 13, 2021
Merged
13 tasks
@tzarc tzarc requested a review from a team August 13, 2021 08:12
@drashna
Copy link
Member

drashna commented Aug 13, 2021

@tzarc Since this is a bug fix, I'd feel better if this made it in on this cycle, rather than q3's. Thoughts?

@caksoylar
Copy link

I can confirm that this fixes the issue I described here: #8591 (comment). Thanks @sevanteri!

@tzarc
Copy link
Member

tzarc commented Aug 14, 2021

@tzarc Since this is a bug fix, I'd feel better if this made it in on this cycle, rather than q3's. Thoughts?

This is the q3 cycle :)

@tzarc tzarc merged commit fd3dc3a into qmk:develop Aug 14, 2021
cadusk pushed a commit to cadusk/qmk_firmware that referenced this pull request Aug 14, 2021
Merge remote-tracking branch 'qmk/develop' into develop
b520875 
* qmk/develop: (57 commits)
  [Keyboard] Add Irene Keyboard (qmk#13991)
  Fix `combo_disable` (qmk#13988)
  Fix Line endings in Absinthe VIA keymap (qmk#13997)
  [Keymap] clean up userspace, add XD75 / Keyboardio Atreus (qmk#13121)
  Crin Refactor (qmk#13982)
  Fix pmw3360 code to only output debug info if mouse debugging is enabled (qmk#13993)
  [Keyboard] Add Gud70 (qmk#12575)
  [Keyboard] Adding VIA to Absinthe (qmk#13031)
  [Keyboard] Macropad v2 polling rate fix (qmk#13129)
  [Keyboard] Add eniigmakeyboards/ek60 (qmk#13140)
  [Keyboard] Add n9 macropad (qmk#13168)
  [Keyboard] Updates to ScrabblePad (qmk#13177)
  [Keymap] adds sulrich crkbd (corne) personal keymap (qmk#13186)
  [Keymap] Updates to jdelkins userspace and keymaps (qmk#13216)
  [Keyboard] Add optional colemak layout to keymap (qmk#13217)
  [Keyboard] add support for alternate ortho UD40 PCB (qmk#13491)
  [Keyboard] Add split_3x5_3 layout support to boardsource/microdox (qmk#13323)
  [Keyboard] Interlace matrix scan for performance on Moonlander (qmk#13625)
  [Keybord] Add LS_60 keyboard (qmk#13851)
  [Keyboard] Add cest73/tkm keyboard (qmk#13939)
  ...
@trankillity
Copy link

Can also confirm that this fixes the issue described.

nhongooi pushed a commit to nhongooi/qmk_firmware that referenced this pull request Dec 5, 2021
@sevanteri @nhongooi
Fix combo_disable (qmk#13988)
8cf0763 
- Dump key buffer when combos are disabled.
- Recursive calls to `dump_key_buffer` need to start dumping the buffer
  from index i+1 to avoid possible infinite loops.
- Handle combo key releases even though combo processing is disabled.
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
@sevanteri
Fix combo_disable (qmk#13988)
664f0bd 
- Dump key buffer when combos are disabled.
- Recursive calls to `dump_key_buffer` need to start dumping the buffer
  from index i+1 to avoid possible infinite loops.
- Handle combo key releases even though combo processing is disabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drashna drashna drashna approved these changes

@tzarc tzarc tzarc approved these changes

@fauxpark fauxpark fauxpark approved these changes

Assignees
No one assigned
Labels
bug core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sevanteri @drashna @caksoylar @tzarc @trankillity @fauxpark