qmk_firmware icon indicating copy to clipboard operation
qmk_firmware copied to clipboard

Improve test invocation, fix Retro Shift bugs, and add Auto+Retro Shift test cases

Open IsaacElenbaas opened this issue 2 years ago • 11 comments

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's permissive_hold test and the normal one

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

IsaacElenbaas avatar Jan 15 '22 14:01 IsaacElenbaas

get_hold_on_other_key_press isn't exposed, last push added it to action_tapping.h.

IsaacElenbaas avatar Mar 13 '22 22:03 IsaacElenbaas

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.

IsaacElenbaas avatar Apr 08 '22 15:04 IsaacElenbaas

Conflicts once base branch swapped to develop.

tzarc avatar Apr 19 '22 11:04 tzarc

Conflict fixed.

IsaacElenbaas avatar Apr 21 '22 14:04 IsaacElenbaas

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.

IsaacElenbaas avatar May 13 '22 20:05 IsaacElenbaas

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.

IsaacElenbaas avatar Jul 09 '22 21:07 IsaacElenbaas

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.

IsaacElenbaas avatar Jul 28 '22 02:07 IsaacElenbaas

Resolved conflicts and removed duplicate #18063

IsaacElenbaas avatar Aug 24 '22 03:08 IsaacElenbaas

Conflicts fixed, kicked off CI.

tzarc avatar Oct 06 '22 10:10 tzarc

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.

IsaacElenbaas avatar Oct 06 '22 13:10 IsaacElenbaas

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[bot] avatar Jan 05 '23 02:01 github-actions[bot]

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[bot] avatar Feb 05 '23 02:02 github-actions[bot]

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.

IsaacElenbaas avatar Feb 06 '23 20:02 IsaacElenbaas

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.

precondition avatar Feb 10 '23 12:02 precondition

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.

IsaacElenbaas avatar Mar 12 '23 19:03 IsaacElenbaas

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:

IsaacElenbaas avatar Apr 23 '23 15:04 IsaacElenbaas

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[bot] avatar Jul 05 '23 02:07 github-actions[bot]

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[bot] avatar Aug 24 '23 01:08 github-actions[bot]