fix issue with retro tapping

[AckslD]

Description

Fixes #6557

Types of Changes

This is just the patch by @ales and updated by @rautesamtr made into a PR

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

Issues Fixed or Closed by This PR

• fast Tapped Key not registering with RETRO_TAPPING and TAPPING_TERM < 100 #6557

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

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

[drashna]

This still have the issue from #6557 (comment) ?

[AckslD]

@drashna Yes, unfortunately! @sigprof have some code (see develop...sigprof:robust-retro-tapping) which could possibly be incorporated with this to solve both issues but I haven't had time to look into it yet

[drashna]

Oaky, thank you for confirming that.

I'd prefer to wait on making sure that it doesn't have issues.

[AckslD]

@drashna Yeah for sure, I thought it might be better to have this in a PR rather than some patches circling around :) So others might find it also. I added WIP to the title

[drashna]

Sounds good. I've added the "in progress" label so that stale-bot won't close it, until working verified code has landed.

[AckslD]

@drashna The issue I had is now fixed. But I suspect that this fix might only work the way I'm using retro tapping and probably not for the normal use. I now simply changed the check of the counter from equality to greater of equal to one. In my case with:

#undef TAPPING_TERM
#define TAPPING_TERM 10
#define RETRO_TAPPING

it now does exactly what I want, ie this effectively gives permissive hold also for layers.

I'm not sure I have the knowledge for how to fix this properly in all cases, so maybe someone with more insight should look into it. But for anyone wanting to use the above settings this branch now supports that.

[sigprof]

This change is still questionable:

1. Because retro_tapping_counter++ was moved inside if (event.pressed) { … } and && event.pressed was added, retro tapping will ignore key release events. Before the change a sequence like “press X (a normal key), press A, release X, release A after tapping term” would not trigger the retro tapping behavior for A, because the “release X” event would increment the counter, making it not match the expected value at the “release A” time. With this change the same sequence will trigger the retro tapping behavior for A, because the release event for X will be ignored.
   This new behavior might actually be useful as an option, because the above sequence can arise just from fast typing, but it also might not be acceptable for some users.
2. The retro_tapping_counter >= 1 comparison does not seem strict enough — now a sequence like “press A, press B, release A after tapping term” may trigger the retro tapping behavior for A, because the extra press event for B would be accepted by that comparison. However, the same change also fixes the “press A, press B, release B after tapping term” sequence (in this case the original retro tapping code did not trigger for B, even though it looks like it should). And it is impossible to distinguish these sequences without actually comparing the event.key values for press and release events.
3. Even if the retro_tapping_counter >= 1 comparison is made strict again, a more complicates sequence like “press A, tap X (a normal key), press B, release A after tapping term” would still trigger the retro tapping behavior for A, because the tap of X will reset the counter, and then the code would handle “press B, release A” as if it was “press A, release A”.

[AckslD]

Thanks for explaining @sigprof! Any idea what a proper fix would be? Do you think some modification do your current branch could work?

[github-actions]

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]