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

Add Retro Shift (Auto Shift for Tap Hold via Retro Tapping) and Custom Auto Shifts #11059

Merged
merged 2 commits into from Nov 25, 2021

Conversation

IsaacElenbaas
Copy link
Contributor

@IsaacElenbaas IsaacElenbaas commented Nov 29, 2020

This pull request will allow one to use Auto Shift (AS) on a key but also use it as a modifier - three keys in one, and it feels like they're completely separate! I've been using a very janky hardcoded version of this for months, it's great. It will also add support for custom AS keys, as things like . and ! are clearly a better match than those they currently have.

Description

This is a recode of #9889, as upon further testing it didn't work when TAPPING_TERM was not exceeded (and Auto Shift now supports keyrepeat).

p00ya#2 has also been merged with this pull as there are quite a few conflicts between the two features. I also don't want to have to manage those conflicts once I've forgotten how AS works (I had a custom shifts pull open for months and then helped completely rewrite Auto Shift and got that merged first).

get_custom_auto_shifted_key was added, as get_auto_shifted_key would result in a lot of duplicate and possibly outdated code (e.g. if a #define is added to Retro Shift everything).

AUTO_SHIFT_TIMEOUT_PER_KEY, AUTO_SHIFT_REPEAT_PER_KEY, and AUTO_SHIFT_NO_AUTO_REPEAT_PER_KEY were also added, as they are nearly a necessity with the many different purposes custom shifted keys can have.

HOLD_ON_OTHER_KEY is already handled, so if that is merged it's not an issue.

Known Bugs/Inconsistencies/Todo

Retro Shift:

  • None

Custom Shifts:

  • None

Bugfixes

Fixed rolling from one AS key to another and holding the second sending both unshifted
Stopped tapping an AS key, tapping a non-AS key, and holding the same AS key from keyrepeating
Alternate #12083, as it won't work for Custom Shifts

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

@IsaacElenbaas

This comment has been minimized.

@IsaacElenbaas

This comment has been minimized.

@IsaacElenbaas

This comment has been minimized.

@IsaacElenbaas
Copy link
Contributor Author

IsaacElenbaas commented Dec 23, 2020

Alright, so I'm changing up how IGNORE_MOD_TAP_INTERRUPT and PERMISSIVE_HOLD work. Read the docs for more info, but IMTI only applies to rolling over two keys, PH has no effect, and all nested presses trigger hold action. It is mostly all because holding for longer to exceed TAPPING_TERM makes no sense as your release times should be muscle memory based on whether or not you want that shifted, and lots of Tap Hold config is to change behavior based on TT. Not making a Tap Hold an AS key makes it use normal config behavior, obviously.

@IsaacElenbaas

This comment has been minimized.

@IsaacElenbaas

This comment has been minimized.

@IsaacElenbaas
Copy link
Contributor Author

I plan to rework this to play more nicely with the rest of QMK and hopefully finalize everything + merge custom shifts over Spring Break (by the 13th).

@tzarc tzarc closed this Feb 27, 2021
@tzarc tzarc deleted the branch qmk:develop February 27, 2021 20:28
@tzarc
Copy link
Member

tzarc commented Feb 27, 2021

Sorry about that, GitHub decided to delete the develop branch from upstream when we merged it, despite being told otherwise. Reopened.

@IsaacElenbaas

This comment has been minimized.

@IsaacElenbaas IsaacElenbaas marked this pull request as ready for review March 12, 2021 21:42
@IsaacElenbaas

This comment has been minimized.

@IsaacElenbaas

This comment has been minimized.

@mrlinuxfish
Copy link
Contributor

mrlinuxfish commented Mar 16, 2021

Retro Tapping appears to break with #define IGNORE_MOD_TAP_INTERRUPT in config.h. If I use retro tapping with this setting it outputs enter d instead of # on my symbol layer. This is the tap key on the layer tap followed by the key on the base layer. LT(_SYM, KC_ENT)

The behavior is incorrect if I press the layer tap then the other key in quick succession but if I wait long enough the behavior is correct (ie. it outputs the keycode from the layer as it should)

@IsaacElenbaas
Copy link
Contributor Author

Fixed. Not related to IGNORE_MOD_TAP_INTERRUPT, I had seen that Mod Taps by default had nested presses under TAPPING_TERM act like holds and didn't realize Layer Taps did not. Last commit also fixed PERMISSIVE_HOLD and TAPPING_TERM _PER_KEY possibly breaking Retro Shift behavior.

@IsaacElenbaas
Copy link
Contributor Author

There's waaaaay too much #define hell in this PR for my liking.

@tzarc
Copy link
Member

tzarc commented Aug 7, 2021

Just a heads-up, I intend to push this PR through this breaking changes cycle.

Unfortunately this PR changes areas of the code that have also had merges this breaking changes cycle, and I’m unsure of how the behaviour interacts… otherwise I’d resolve the conflicts myself. At this point it'd be great if you could get these conflicts sorted -- apologies if we create more conflicts during merges of other PRs — at this point the quicker they're sorted the more likely this makes it in first.

@IsaacElenbaas
Copy link
Contributor Author

IsaacElenbaas commented Aug 7, 2021

Those should be handled. No major conflicts, and nothing I wasn't expecting. Glancing over the overall diff everything looks fine. HOLD_ON_OTHER_KEYPRESS should already be handled here, but if it is broken it will only affect people trying to use both that and this feature and I would want to put it in a future pull anyway.

I tested this feature and with Auto Shift disabled again, but not with HOLD_ON_OTHER_KEYPRESS for the same reasons. The left half of my corne wouldn't send any keys, but I haven't touched anything near the split code. Looking into that now, obviously, lol. Everything else seemed fine.

EDIT: I just had to flash both sides, which I usually avoid so I have one working half if I break something.

@IsaacElenbaas
Copy link
Contributor Author

IsaacElenbaas commented Aug 7, 2021

Doesn't have merge conflicts with #11036 but either will need to be updated when the other is merged.
See action_tapping.c and https://github.com/qmk/qmk_firmware/blob/865f58086c43d66dab2dcd7d2f32025ea3d8ba16/quantum/process_keycode/process_auto_shift.c#L171-L176

@tzarc
Copy link
Member

tzarc commented Aug 26, 2021

Deferring to next cycle due to conflicts at the time we're locking the branch.
From a risk perspective, probably best to get it into develop early in the cycle anyway.

@IsaacElenbaas
Copy link
Contributor Author

@tzarc Conflicts fixed and I re-tested, working with early combos as well. Squashed this time as I hate every rebase taking forever to do. Notes above for #11036 still apply as that was delayed as well.

quantum/action_tapping.c Show resolved Hide resolved
@tzarc tzarc merged commit d9393b8 into qmk:develop Nov 25, 2021
KarlK90 added a commit to KarlK90/qmk_firmware that referenced this pull request Nov 26, 2021
that broke compilation because of a typo: KEYPRESS should have been
KEY_PRESS
tzarc pushed a commit that referenced this pull request Nov 27, 2021
that broke compilation because of a typo: KEYPRESS should have been
KEY_PRESS
cadusk pushed a commit to cadusk/qmk_firmware that referenced this pull request Dec 2, 2021
* qmk/develop:
  More headroom. (qmk#15302)
  More headroom. (qmk#15301)
  Documentation typo fix (qmk#15298)
  New feature: `DYNAMIC_TAPPING_TERM_ENABLE` (qmk#11036)
  Tidy up adjustable ws2812 timing (qmk#15299)
  Add ifndef to WS2812 timing constraints (qmk#14678)
  Add Retro Shift (Auto Shift for Tap Hold via Retro Tapping) and Custom Auto Shifts (qmk#11059)
# ifdef RETRO_TAPPING_PER_KEY
get_retro_tapping(tapping_keycode, keyp) &&
# endif
(RETRO_SHIFT + 0) != 0 && TIMER_DIFF_16(event.time, tapping_key.event.time) < (RETRO_SHIFT + 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a good reason why 0 is added to RETRO_SHIFT here and on line 363?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One can #define RETRO_SHIFT without a value, which just makes holds without interrupting keypresses send the shifted key. Defining it to a value makes mod taps' hold actions trigger after that many ms, like the default behavior with TAPPING_TERM to allow using mod taps with the mouse.

Adding zero makes the former compile. I suppose those conditionals could not have the +0 and be in another #if, but there are quite enough preprocessor macros in this haha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume you saw that when working on #15741 - I'll try to test that with Retro Shift (base Auto Shift won't have any issues) too soon, but if/when you have a go at it note that you have to add mod/layer taps to Auto Shift.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants