manna-harbour / qmk_firmware

See the "forkreadme" branch or the following link for a description of branches maintained in this fork.
https://github.com/manna-harbour/qmk_firmware/blob/forkreadme/readme.org
GNU General Public License v2.0
281 stars 64 forks source link

unilateral mod-tap shouldn't send KC_GUI to computer #48

Closed sunaku closed 2 years ago

sunaku commented 2 years ago

This fixes a glitch in Miryoku's bilateral-combinations implementation where the GUI modifiers in a mod-tap key combination are always sent to the computer alongside the combination, interrupting the user's typing.

For example, suppose you held LGUI_T(KC_A) and then pressed KC_R with the same hand (assuming your left hand on the standard QWERTY layout).

Before this patch, the combination sends KC_LGUI to the computer when LGUI_T(KC_A) is held, followed by KC_A and KC_R when finally released. This inadvertently triggers whatever action is associated with KC_LGUI on the computer, such as opening the "Start Menu" in Microsoft Windows.

With this patch applied, the combination doesn't send KC_LGUI to the computer when LGUI_T(KC_A) is held. Instead, it applies the KC_LGUI modifier internally for bilateral_combinations_hold() and therefore only sends out KC_A and KC_R to the computer as the user would expect.

However, note that non-GUI modifiers such as Shift/Ctrl/Alt are still sent to the computer for unilateral mod-tap combinations to allow for modified mouse clicks such as Shift-click, Ctrl-click, Alt-click, etc.

manna-harbour commented 2 years ago

Thanks for this PR!

I believe flashing of other mods can also be an issue, so perhaps this can be applied to all mods. For mouse use, register the mod after a timeout. That could be a new timer, or it could be integrated into this feature: https://github.com/manna-harbour/qmk_firmware/blob/b8f3651d353389bfce2b837f20511a1857e65b26/docs/tap_hold.md?plain=true#L194

sunaku commented 2 years ago

I believe flashing of other mods can also be an issue, so perhaps this can be applied to all mods.

Yes, I think Shift and Control are harmless and only Super and Alt are troublesome (e.g. Windows 10 use Alt to highlight keyboard shortcuts for ribbon/menu access). This could be solved by exposing a #define BILATERAL_COMBINATIONS_FLASHMODS setting to the user so they can decide which mods to enable for mouse usage. I will revise this PR accordingly. :heavy_check_mark: https://github.com/sunaku/qmk_firmware/commit/8f43b62a78bbd9d49139248c986e8d0257b5c04a

For mouse use, register the mod after a timeout. That could be a new timer, or it could be integrated into this feature:

Setting a timer would introduce additional delay on top of TAPPING_TERM. In practice, I often find that even TAPPING_TERM is too long when mod-clicking instinctively: I'm too quick and the mod key doesn't register, so I have to redo the operation and pause for a moment after holding down the mod-tap key before I click. Such mistakes and abrupt re-dos interrupt my flow. :disappointed:

sunaku commented 2 years ago

I've made the definition of what keys are considered "flashing mods" configurable to whatever the user wants, as follows.

To suppress "flashing mods" such as the GUI keys (which pop up the "Start Menu" in Microsoft Windows) during bilateral combinations, add the following to your config.h:

#define BILATERAL_COMBINATIONS_FLASHMODS MOD_MASK_GUI

In addition, to also suppress the Alt keys (which pop up the "Ribbon Menu" in Microsoft Office) during bilateral combinations, specify a compound mask. For example:

#define BILATERAL_COMBINATIONS_FLASHMODS (MOD_MASK_GUI|MOD_MASK_ALT)
sunaku commented 2 years ago

Hey @manna-harbour, you were right: I'm having a much better typing experience after treating all modifiers as flashmods:

#define BILATERAL_COMBINATIONS_FLASHMODS (~0) /* everything! */

The only problem remaining is the lack of mouse-click awareness for home row mods, which your timer suggestion would solve. For now, I'm working around the problem by pressing one of the Miryoku layer activation keys on the thumb cluster (i.e. normal home row mods plus Miryoku layer key) to get the home row mods to "register" to the computer when mod-clicking the mouse.

sunaku commented 2 years ago

Hey @manna-harbour, I have implemented your timeout suggestion now using QMK's deferred execution facility. This replaces the BILATERAL_COMBINATIONS_FLASHMODS mask with a BILATERAL_COMBINATIONS_DEFERMODS timeout, documented as follows.

To delay the registration of modifiers (such as KC_LGUI and KC_RGUI, which are considered to be "flashing mods" because they suddenly "flash" or pop up the "Start Menu" in Microsoft Windows) during bilateral combinations:

  1. Add the following line to your config.h and define a timeout value (measured in milliseconds). Hold times greater than this value will permit modifiers to be registered.
#define BILATERAL_COMBINATIONS_DEFERMODS 100
  1. Add the following line to your rules.mk file to enable QMK's deferred execution facility, which is needed by the BILATERAL_COMBINATIONS_DEFERMODS setting mentioned above.
DEFERRED_EXEC_ENABLE = yes

Please try it out and let me know what you think. For me, it's working quite well so far, especially in combination with PR #54.

Note: QMK's deferred execution feature was introduced 1 year later (on 16 November 2021 in commit 36d123e9c5a9ce0e29b9bc22ef87661bf479e299) after the latest in Miryoku's bilateral-combinations branch! So in order to try out my latest changes, you'll need to update this branch to a more recent version of mainline QMK. I've already done this (resolving merge conflicts) in a separate branch based on 0.18.6, so simply check out that branch and configure your config.h and rules.mk files (as I've documented previously) to try it out.

sunaku commented 2 years ago

Sorry for the noise: I've been trying to modernize this branch by merging the latest QMK mainline so that we can use QMK's deferred execution feature cleanly. However, it ended up bloating this PR, so I moved it to a separate independent PR #55.

sunaku commented 2 years ago

For your testing convenience, I've created a modernized version of this PR that is updated to a recent QMK mainline (0.18.6).

sunaku commented 2 years ago

After poring over my current implementation at great depth and with relentless fervor, I have made substantial improvements in my miryoku_bilateral branch at commit 308a9bfce4f5c8366bb51a62cf724a15c2543f5a for smarter handling of hold and release events on secondary mods:

Here is a flowchart of the improved implementation, illustrating the complexity of this interconnected re-entrant monstrosity:

flowchart

Also, I found that QMK's deferred execution isn't preemptively scheduled, so I'm now setting my defermods timeout to 1ms:

/* QMK */
#define TAPPING_TERM 200
#define IGNORE_MOD_TAP_INTERRUPT

/* Miryoku */
#define BILATERAL_COMBINATIONS
#define BILATERAL_COMBINATIONS_CROSSOVER 75
#define BILATERAL_COMBINATIONS_DEFERMODS 1
sunaku commented 2 years ago

Superseded by PR #56.