[GMMK Pro] Fix unintentional taps to the volume keys when using the encoder

[andrebrait]

Description

In a similar vein to #16830, this fixes the double-tapping or unintentional tapping of the volume keys when using the encoder, introduced in #16721.

While #16830 took care of fixing the behavior, it still kept the user encoder handling code in the default keymaps. I have removed it, since it's not necessary, given pro.c already contains the handling for the media keys (and in an even better way, since it can be more compatible when NKRO is enabled).

I took the time to look for any instances in which user layouts would end up having a different behavior due to #16721.
Some of them were:

• Introducing a Volume Up/Down key tap where there was none, previously;
  • This happened when the user handled the encoder for other functions in encoder_update_user and then returned true
• Double-tapping Volume Up/Down due to both the keyboard and user encoder functions both sending taps
  • This happened when the user would send a tap on their encoder_update_user implementation and then return true

I realize going about changing other people's code is not a good thing, but the code was effectively broken, so here it is.

I also took care of moving an outdated keymap into the ansi subdirectory and fixing a small typo in the docs. The whitespace-related changes were done by my text editor and I don't have time to undo them right now. If that's an issue, I'll try to push a new commit later to revert them.

Sign-off

• @alexmarmon
• @byungyoonc
• @cedrikl
• @coryginsberg
• @Gigahawk
• @hachetman
• @JackKenney
• @Jonavin (both ansi and iso)
• @mattgauf
• @mike1808
• @stickandgum
• @tguinan
• @ViToni
• @WholesomeDucky
• @willwm
• @zvuc

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

• None that have been reported, but it's a better fix for [Bug] Rotary encoder on GMMK Pro always sends KC_VOL[U,D] in addtion to what's set by the keymap #16832, IMHO.

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

[zvecr]

Changing user keymaps requires sign-off from the affected users.

[andrebrait]

@zvecr how is this a breaking change?

And ok. Should I add them all on this PR and ask them if they are okay with the change then?

[zvecr]

@zvecr how is this a breaking change?

Its the label we use to manage PRs which touch user code, without prior sign-off. https://docs.qmk.fm/#/breaking_changes_instructions.

Should I add them all on this PR and ask them if they are okay with the change then?

Correct.

[andrebrait]

@alexmarmon, @byungyoonc, @cedrikl, @coryginsberg, @Gigahawk, @hachetman, @JackKenney, @Jonavin (both ansi and iso), @zvuc, @mattgauf, @mike1808, @tguinan, @stickandgum, @wholesomeducky, @willwm, @ViToni

My bad for spamming all of you, but essentially there is a breaking change that affects your keymaps and I'm fixing it. Just need you guys to check if the changes I made reflect the intent here (meaning keeping the behavior your keymaps had before #16721).

[coryginsberg]

@andrebrait My keymap keeps the default encoder settings so everything should be fine for my stuff.

[cedrikl]

Default mapping for me as well if it breaks I'll fix it at that point. Thanks for letting us know.

[Jonavin]

Should be fine on my keymaps

[DuckyAtSea]

My keymap uses the encoder for scroll left/right anyway (at least I believe the version on GitHub does) so I'm completely good with this

[stickandgum]

I certainly use the encoder but don't know how to verify if this will break any specific functionality. With that said, I trust the community so feel free to modify my code as you see fit.

Thank you.

[tzarc]

Added list of required sign-offs to the description.

[andrebrait]

I certainly use the encoder but don't know how to verify if this will break any specific functionality. With that said, I trust the community so feel free to modify my code as you see fit.

Thank you.

You can pull the code from master and flash to your keyboard. As far as I can tell, before this PR, you'll always get a volume up/down tap in addition to whatever your code is doing. Best case scenario you'd get two taps to the same volume change button. I think you're using your encoder to scroll with mouse buttons. In this case you'd get a volume up/down every time you scrolled up/down.

[hachetman]

Hi, also default mapping for me. If anything breaks I will fix it.

[stickandgum]

https://github.com/qmk/qmk_firmware/blob/master/keyboards/gmmk/pro/ansi/keymaps/stickandgum/keymap.c#:~:text=%23ifdef%20ENCODER_ENABLE,endif%20//ENCODER_ENABLE
Is the section - I will likely be able to test this over the weekend.

[tguinan]

Looks fine to me

[andrebrait]

https://github.com/qmk/qmk_firmware/blob/master/keyboards/gmmk/pro/ansi/keymaps/stickandgum/keymap.c#:~:text=%23ifdef%20ENCODER_ENABLE,endif%20//ENCODER_ENABLE Is the section - I will likely be able to test this over the weekend.

@stickandgum yeah, you'd definitely get a tap to KC_VOLU or KC_VOLU in addition to PgUp/PgDn (if holding CTRL) or horizontal mouse scrolling (if holding shift), or two taps to the volume keys if none.

[zvuc]

Just re-compiled firmware with the changed bit and tested encoders. Seems fine!

[byungyoonc]

LGTM, thanks for caring!

[andrebrait]

@alexmarmon @Gigahawk @JackKenney sorry to bother you, but I still need your blessing here 🙃

[JackKenney]

Looks good to me!

[Gigahawk]

@alexmarmon @Gigahawk @JackKenney sorry to bother you, but I still need your blessing here 🙃

LGTM, but I haven't tested it. Haven't had a reason to reflash my board. TY for the fix!

[andrebrait]

@drashna @zvecr it seems the only sign-off missing is @alexmarmon, who seems to be active but unresponsive.

[drashna]

ALso, because this targets user keymaps, this either needs to go to the develop branch (you can edit the title to change the target... weird yes, but that's how it handled), or you need each user to sign off on this change.

[andrebrait]

ALso, because this targets user keymaps, this either needs to go to the develop branch (you can edit the title to change the target... weird yes, but that's how it handled), or you need each user to sign off on this change.

I switched the base branch to develop and resolved a conflict.

[andrebrait]

@drashna let me know if this is good enough now