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

Bilateral combinations #17715

Closed

Conversation

DanielSincere
Copy link

@DanielSincere DanielSincere commented Jul 18, 2022

Description

This PR brings @manna-harbour 's Bilateral Combinations feature. I also implemented Per Key Bilateral Combinations, as requested by @cfarvidson and others. I have updated the docs as well.

I tried to implement unit tests, and the trace's match what I expected. However, the expectation macros couldn't be convinced to pass. If you would like to see the progress tests for this feature please see my branch at https://github.com/FullQueueDeveloper/qmk_firmware/tree/bilateral_combinations_tests

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

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).

@drashna drashna requested a review from a team July 18, 2022 22:43
@zvecr zvecr changed the base branch from master to develop July 18, 2022 22:44
Copy link
Contributor

@filterpaper filterpaper left a comment

Choose a reason for hiding this comment

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

Add an explicit example

docs/tap_hold.md Outdated Show resolved Hide resolved
docs/tap_hold.md Show resolved Hide resolved
Comment on lines +477 to +483
#define BILATERAL_COMBINATIONS
```

If `BILATERAL_COMBINATIONS` is defined to a value, hold times greater than that value will permit same hand combinations. For example:

```c
#define BILATERAL_COMBINATIONS 500
Copy link
Member

Choose a reason for hiding this comment

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

Two different states for the same config option is probably going to make data driven config more complicated.

@manna-harbour
Copy link
Contributor

Nice!

I recommend going through manna-harbour#29 and the issues linked from there. In particular, I suggest to:

Best of luck with the PR!

@zvecr
Copy link
Member

zvecr commented Jul 19, 2022

A few notes for above, mostly on BILATERAL_COMBINATIONS_HANDS_LAYER

Using a keymap layer for configuration is very unlikely to be accepted as this does not align with the existing configuration points. Something like swap hands is probably the closer, but the existing get_enable_bilateral_combinations_per_key is fairly consistent to other features.

@manna-harbour
Copy link
Contributor

A few notes for above, mostly on BILATERAL_COMBINATIONS_HANDS_LAYER

Using a keymap layer for configuration is very unlikely to be accepted as this does not align with the existing configuration points. Something like swap hands is probably the closer, but the existing get_enable_bilateral_combinations_per_key is fairly consistent to other features.

get_enable_bilateral_combinations_per_key is fine for keycode exclusion, but the main point of BILATERAL_COMBINATIONS_HANDS is matrix independent hands determination, and once you have that it can easily also accommodate exclusion. The difference in practice being that get_enable_bilateral_combinations_per_key specifies exclusions per keycode for the purpose of adding cheats, whereas doing it via BILATERAL_COMBINATIONS_HANDS specifies the key position in the keymap as the intention is to support keys that might naturally be used with either hand. Given that, it might actually be worth including both.

The current implementation was definitely a hack, but BILATERAL_COMBINATIONS_HANDS could just as well be implemented with a separate array by re-implementing keymap_key_to_keycode(). The important thing is that the keyboard (and userspace keymap mapping) layout macros can be applied directly so that everything can be specified relative to the keymap, and not to the matrix for each keyboard as is required with swap hands.

DanielSincere and others added 2 commits July 19, 2022 18:24
@getreuer
Copy link
Contributor

I very much like the concept of Bilateral Combinations and Positional Hold and it would be fantastic to add some form of this to QMK core. I wrote a userspace library Achordion that does something similar and have some thoughts regarding interface.

I believe Bilateral Combinations' opposite hands rule for what counts as a hold is a great default. However, it is practical to make a few tweaks on top of it:

  • You might want to prohibit hold behavior for certain cross-handed tap-hold key + other key chords, say because they are very common bigrams. Such bigrams are especially prevalent when using home row mods on Colemak or other alternative keyboard layouts. Precondition's home row mods guide describes how cancelling holds for specific bigrams can be useful to reduce accidental mod triggers.
  • Conversely, you might want to enable hold behavior for certain same-handed tap-hold key + other key chords, say to use particular hotkeys with one hand.
  • Also, you probably want to exempt some keys such as thumb keys from the opposite hands rule.

Those first two points would involve customization based on the tap-hold key + other key chord, not just the tap-hold key alone. Achordion facilitates these chord-adaptive customizations with a callback with the signature

bool achordion_chord(uint16_t tap_hold_keycode,
                     keyrecord_t* tap_hold_record,
                     uint16_t other_keycode,
                     keyrecord_t* other_record);

where returning true settles the chord as a hold and false as a tap. The user can use this callback to define tweaks on top of the opposite hands rule as described here.

Some other thoughts:

  • It may be useful to customize the Bilateral Combinations timeout time individually for each tap-hold key (see achordion_timeout()).
  • A big drawback with Bilateral Combinations is when holding a Shift mod-tap key + clicking the mouse to highlight something. For the Shift modifier to register, you have to first wait out the tapping term and the Bilateral Combinations timeout, which is awfully sluggish. To reduce this delay, I added an option in Achordion that modifiers may be eagerly applied.

@github-actions
Copy link

github-actions bot commented Oct 3, 2022

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 bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Oct 3, 2022
@sunaku
Copy link

sunaku commented Oct 3, 2022

The "Flashing Mods" issue should be fixed in my PR for Miryoku: manna-harbour#48

@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Oct 4, 2022
buztard added a commit to buztard/qmk_firmware that referenced this pull request Oct 31, 2022
…ations-develop

Bilateral combinations

* https://github.com/qmk/qmk_firmware:
  Update docs/tap_hold.md
  Update docs/tap_hold.md
  cleanup
  example for `get_enable_bilateral_combinations_per_key`
  “bilateral_combinations_left” should be weak. For testing and for keyboards to override
  format
  docs
  per key Bilateral combinations
  copied in code and it compiles.
  WIP: include quantam & main implementation
grota pushed a commit to grota/qmk_firmware that referenced this pull request Nov 23, 2022
@github-actions
Copy link

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 bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Dec 29, 2022
@github-actions
Copy link

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.
// [stale-action-closed]

@github-actions github-actions bot closed this Jan 28, 2023
grota pushed a commit to grota/qmk_firmware that referenced this pull request May 12, 2023
@grota
Copy link
Contributor

grota commented May 30, 2023

guys, just a quick question, there is no follow up to this pr right?

I mean, I can still use it (rebasing it in my fork) but wanted to know if you were discussing the development for bilateral combinations to go in a specific direction or not.

@getreuer
Copy link
Contributor

@grota right, this PR has stalled, unfortunately. As @zvecr said (and I agree), the main problem was:

Using a keymap layer for configuration is very unlikely to be accepted as this does not align with the existing configuration points. Something like swap hands is probably the closer, but the existing get_enable_bilateral_combinations_per_key is fairly consistent to other features.

If a new PR in this direction is made, it would be better to follow how swap hands configuration works.

As an alternative to modifying core, you might be interested in Achordion. It is a userspace library with behavior similar to Bilateral Combinations.

grota pushed a commit to grota/qmk_firmware that referenced this pull request Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core documentation stale Issues or pull requests that have become inactive without resolution.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants