qmk_firmware
qmk_firmware copied to clipboard
Improve test invocation, fix Retro Shift bugs, and add Auto+Retro Shift test cases
Description
This PR:
- Fixes all of the Retro Shift bugs I have run into or been alerted to in the past couple of months
- Includes some small Retro Shift doc additions/fixes
- Adds test cases for Auto and Retro Shift
- Includes test invocation improvements:
- Running
make test:auto_shift
will run all Auto Shift tests - Running
make test:auto_shift:retro_shift:permissive_hold
(or similar) allows running tests in different directories that have the same name, versus the old behavior of whichever's parent directory was later in the alphabet - Running
make test:permissive_hold
will run both Retro Shift'spermissive_hold
test and the normal one
- Running
It also makes RETRO_SHIFT
not default to TAPPING_TERM
, as the behavior can be confusing (trying out the feature -> holding -> not releasing between AUTO_SHIFT_TIMEOUT
and TAPPING_TERM
which are probably close for most users). The old behavior also made it impossible to make holds of any length send the shifted value aside from setting RETRO_SHIFT
to INT_MAX
or something.
Types of Changes
- [X] Core
- [X] Bugfix
- [ ] New feature
- [X] Enhancement/optimization
- [ ] Keyboard (addition or update)
- [ ] Keymap/layout/userspace (addition or update)
- [X] Documentation
Issues Fixed or Closed by This PR
- #15458
- Made Retro Shift respect
DYNAMIC_TAPPING_TERM
- Miscellaneous Retro Shift issues that do not have issues
- Updated out-of-date default function in docs
Checklist
- [X] My code follows the code style of this project: C, Python
- [X] I have read the PR Checklist document and have made the appropriate changes.
- [X] My change requires a change to the documentation.
- [X] I have updated the documentation accordingly.
- [X] I have read the CONTRIBUTING document.
- [X] I have added tests to cover my changes.
- [X] I have tested the changes and verified that they work and don't break anything (as well as I can manage).
get_hold_on_other_key_press
isn't exposed, last push added it to action_tapping.h
.
Last commit made record->event.pressed
not false
where there was no good reason for it to be. Not sure what I was thinking when I did that one.
Conflicts once base branch swapped to develop
.
Conflict fixed.
Last commit fixed having IGNORE_MOD_TAP_INTERRUPT
set and rolling starting from a Retro Shifted key not allowing the Retro Shifted key to be shifted.
Since this hasn't been merged yet I'm adding unit tests. Shouldn't be long at all but don't review/merge in the meantime.
Conflicts with #15741 now but that's just because I found things that could be deleted so should be easy :)
I had to make some changes to the way tests are run to allow Retro Shift and the Tap Hold configurations to have the same folder names and both be runnable. I think the glob-like behavior is pretty good, if we name things appropriately one can run relevant test cases easily and without doing them all during development. It also sort of mirrors the keyboard commands and generated files.
I need to look into the failing combo tests (E: they didn't add their keys to Retro Shift), give this actual physical board testing (E: looks good so far), and check if I've managed to fix one last bug unintentionally (E: I did) or if it needs a workaround, but there shouldn't be many changes what with the tests passing.
Resolved conflicts and removed duplicate #18063
Conflicts fixed, kicked off CI.
Thanks, I had stopped checking for conflicts on this as often.
If this is being eyed for this cycle, there was never any discussion here about how this changes how tests can be invoked (see description) or review of those changes. It only adds functionality, but I just wanted to note that.
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.
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]
Will get around to fixing those sometime but this month will be busy (maybe spring break). Shouldn't be bad but IGNORE_MOD_TAP_INTERRUPT by default was merged so there will be some non-obvious conflicts and it will need re-testing.
Will get around to fixing those sometime but this month will be busy (maybe spring break). Shouldn't be bad but IGNORE_MOD_TAP_INTERRUPT by default was merged so there will be some non-obvious conflicts and it will need re-testing.
The PR you refer to has been severely changed and greatly reduced in scope. The global IGNORE_MOD_TAP_INTERRUPT
still exists and the default behavior of mod-taps has not been changed. The only notable change is that IGNORE_MOD_TAP_INTERRUPT_PER_KEY
/bool get_ignore_mod_tap_interrupt
no longer exists and is superseded by bool get_hold_on_other_key_press
.
Yeah, I actually didn't run into any conflicts from it. The unnecessary conditions have been removed so if this is merged before the PR that fully removes IMTI there hopefully shouldn't be much if anything to do for Retro Shift other than swap the IMTI and default test cases.
Most conflicts were from #17282, thanks a lot for that cleanup @fxkuehl. action_tapping.c
is hard to wrap your head around on its own and I made it a good deal worse, it is much better now.
Unless someone thinks otherwise I'm holding off on removing the now unused TAP_IS_LT/MT/RETRO
as they are small and may still be useful later. Swapping times to let the existing logic work instead of doing things manually worked, but the MT vs LT behavior differences may need to be accounted for in Retro Shift or some other feature in the future (not that I have any clue how many of them are intentional/documented and should be accounted for).
All tests are still passing and didn't need adjustments, got this flashed on my main board and everything seems the same as well. Should be ready for review/merge IMO. Note that the test invocation changes still haven't had any discussion, though again they only add functionality and old commands to run tests still work.
Resolved (not pushed) conflicts with #15847 which weren't too bad, tests are failing now. Other tests and mine are getting a lot more empty reports and mine are getting wrong keys, definitely looks like some stuff in that could be the cause. At a very brief glance, I don't see why moving the IS_EVENT
checks like that did in action_tapping.c
should still work.
Will look into deeper soon:tm:
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.
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.