Allow overlapping combos

[alexspeller]

Description

Add option to only send the longest combo when multiple combos are held down

I wanted to have multiple combos on the home row, that overlapped - e.g. SDF for shift and DF to change layers. However the combos were sent immediately as they were pressed, so I would change layers when I really wanted to shift.

I've added a COMBO_ONLY_SEND_LONGEST_COMBO flag that will debounce the combos and only send the longest one, allowing overlapping independent combos like this.

This still allows combos to be used as modifier keys - it only delays sending the combo by COMBO_TERM.

I am far from a competent C programmer so I might be doing stupid stuff here.

Types of Changes

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

Issues Fixed or Closed by This PR

• fixes Overlapping Combos #4302

Checklist

• My code follows the code style of this project. it follows the code style of the file it's in, which seems not quite to follow the documented project style in terms of indentation
• 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 can't find tests for combos at all - am I missing them?
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[drashna]

Well, you can run git revert 24d0e1ae058d7d89adce6be65917cb40c740ccbf
You may be able to revert it on github, which may work best.

Or you ran run git rebase HEAD~2 -i and "drop" the first commit.

Also, if you did this locally, you probably ran git checkout -B branch_name, when you should have run git checkout -B branch_name master (this creates a new branch based on master, rather than the current working branch).

If it's the fine in question that is causing the issue, you can forcibly disable tracking on it, if it's a git issue.

[alexspeller]

I cleaned this up a little moving to a function and fixed it so the combo works instantly if a non-combo key is pressed.

R.e. the git comments, what I mean is that git is actually completely broken for me without including that commit, which I did intentionally. e.g. git rebase doesn't let you rebase because it thinks you have local modifications. Event git checkout master doesn't work with a clean repo:

➤  git status                                                                                                                                                                                      
On branch allow-overlapping-combos
nothing to commit, working tree clean
➤  git checkout master
error: The following untracked working tree files would be overwritten by checkout:
	keyboards/tada68/keymaps/hhkb68/README.md
Please move or remove them before you switch branches.
Aborting

If #6028 is merged soon, then this will be moot, but if it doesn't get merged soon I can figure out how to rebase this. Likely by doing it in a docker image or something. And then curse OSX for being case insensitive by default :)

[alexspeller]

Oh it did just get merged - I rebased fine now, thanks!

[noroadsleft]

This is potentially a duplicate of #5527.

[noahfrederick]

I've added a COMBO_ONLY_SEND_LONGEST_COMBO flag

I don't think this should be behind a flag—it should just be how the Combo feature works. The previous behavior of triggering all sub-combos is unintuitive.

If there is some use case for the old behavior, then maybe introduce a flag with the opposite meaning.

[drashna]

I agree with @noahfrederick actually.

[alexspeller]

So, thinking about this, I think it might be best to have both.

I.e., update process_combo_event to have the new behaviour, but make a new callback process_combo_event_immediate that calls immediately so that users can react at both points.

Whilst it's hard for me to articulate a concrete use case for process_combo_event_immediate right now, there are definitely things that you might want to be able to do there that you just can't do if the combo is debounced. I can imagine tapping combos, for example, would suffer if you only got the combo press after a COMBO_TERM debounce.

I guess it just feels right, because you can implement the debounced behaviour in userspace if you only have the immediate behaviour in core, but not the other way round.

Either way, I'm going to do some refactoring here and see if I can hash that implementation out a little better. I've also found a little bug in my implementation: although the presses are debounced, the release of the combos still send releases for each combo. This probably isn't an issue most of the time if you're just doing layer_off on release, but it's a bit ugly to get COMBO ASD pressed and then COMBO AS released, COMBO ASD released.

Would you prefer I close this PR and come back with a new one later, or leave it open until I do the refactoring?

[drashna]

Both may be best, but I still think that the "send longest" should be the default behavior. We can hide the older method behind preprocessor lines.

[drashna]

There is a merge conflict here. Could you rebase and fix this?

[drashna]

Any update on this?

[alexspeller]

Hi, I used this a little on my own board and I found that it's a bit unreliable - I've been meaning to come around and figure out why and get it more reliable. I was trying to figure out if I could write some tests that would help me try out all the cases however I didn't find a way to do that kind of testing, and that's about as far down the rabbit hole as I got :(

[stale]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[stale]

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.

[germ]

@alexspeller Did you manage to figure out where the unreliability came from or do any further work? Asking before I take a stab at this. Would like to get this merged into core as a few of my boards a 100+ combos on them :P

[alexspeller]

@germ unfortunately not, I took a different approach to my keymap in the end without any overlapping combos

[germ]

Damn. You mind if I use this PR as a starting point for my own work @alexspeller ?

[alexspeller]

@germ please feel free, if you get it working I'd love to try it out again myself!