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

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

Merged
merged 2 commits into from
Sep 25, 2023

Conversation

IsaacElenbaas
Copy link
Contributor

@IsaacElenbaas IsaacElenbaas commented Jan 15, 2022

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

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

Issues Fixed or Closed by This PR

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

quantum/process_keycode/process_auto_shift.c Outdated Show resolved Hide resolved
quantum/action_tapping.c Outdated Show resolved Hide resolved
quantum/action_tapping.c Outdated Show resolved Hide resolved
@IsaacElenbaas
Copy link
Contributor Author

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

@IsaacElenbaas
Copy link
Contributor Author

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.

@drashna drashna requested a review from a team April 13, 2022 05:42
@tzarc tzarc changed the base branch from master to develop April 19, 2022 11:10
@tzarc
Copy link
Member

tzarc commented Apr 19, 2022

Conflicts once base branch swapped to develop.

@IsaacElenbaas
Copy link
Contributor Author

Conflict fixed.

@IsaacElenbaas
Copy link
Contributor Author

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
Copy link
Contributor Author

IsaacElenbaas commented Jul 9, 2022

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 IsaacElenbaas force-pushed the retro_shift_fixes branch 2 times, most recently from 5663128 to 55c52dc Compare July 28, 2022 02:04
@IsaacElenbaas IsaacElenbaas changed the title Close #15458 and make Retro Shift respect DYNAMIC_TAPPING_TERM Improve test invocation, fix Retro Shift bugs, and add Auto+Retro Shift test cases Jul 28, 2022
@IsaacElenbaas IsaacElenbaas marked this pull request as ready for review July 28, 2022 02:19
@IsaacElenbaas
Copy link
Contributor Author

IsaacElenbaas commented Jul 28, 2022

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.

if (IS_TAPPING_KEY(waiting_buffer[i].event.key) && !waiting_buffer[i].event.pressed && (
WITHIN_TAPPING_TERM(waiting_buffer[i].event)
# if defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT)
|| (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This being missing made so much weird behavior and caused 2-3 months of on-and-off playing with one bug and then getting frustrated and quitting for a while.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Jul 5, 2023
@@ -497,7 +497,7 @@ void process_action(keyrecord_t *record, action_t action) {
default:
if (event.pressed) {
if (tap_count > 0) {
# ifdef HOLD_ON_OTHER_KEY_PRESS_PER_KEY
# ifdef HOLD_ON_OTHER_KEY_PRESS
Copy link
Contributor Author

@IsaacElenbaas IsaacElenbaas Jul 8, 2023

Choose a reason for hiding this comment

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

Life got crazy but I finally got around to looking at this again after stale-bot reminded me. Should be good to go again, other combo tests are failing right now so the action won't run but they're passing. E: combo tests were my fault, fixed.

The issues weren't actually from that PR, they were from a couple of the things I had thrown in in the last PR to make Retro Shift ready for HOLD_ON_OTHER_KEY_PRESS before it happened not being quite correct. I also fixed the above, which I think wasn't noticed in the three months since the switch because an earlier evaluation was also added (which I had to exclude Retro Shift from).

@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Jul 9, 2023
@github-actions
Copy link

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 github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Aug 24, 2023
Makes multiple tests of the same name work
Allows calling all tests under a organizational directory
@IsaacElenbaas IsaacElenbaas force-pushed the retro_shift_fixes branch 2 times, most recently from 535a659 to b3cb29d Compare September 6, 2023 21:08
@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Sep 7, 2023
@tzarc tzarc merged commit dd94877 into qmk:develop Sep 25, 2023
4 checks passed
mechlovin pushed a commit to mechlovin/qmk_firmware that referenced this pull request Oct 25, 2023
christrotter pushed a commit to christrotter/qmk_firmware that referenced this pull request Nov 28, 2023
zgagnon pushed a commit to zgagnon/qmk_firmware_waterfowl that referenced this pull request Dec 15, 2023
future-figs pushed a commit to future-figs/qmk_firmware that referenced this pull request Dec 27, 2023
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

4 participants