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

Improvement of Space Cadet Shift by preventing to automatically apply… #3856

Merged
merged 4 commits into from Feb 5, 2019

Conversation

anthonyrichir
Copy link
Contributor

… a modifier on the key and allow to override the default modifier. Closes #3815.

I added LSPO_MOD and RSPC_MOD which, by default, are set to KC_LSFT and KC_RSFT.
I also added the possibility to define DISABLE_SPACE_CADET_MODIFIER to prevent the firmware to apply a modifier to LSPO_KEY and RSPC_KEY.

… a modifier on the key and allow to override the default modifier. Closes qmk#3815
Copy link
Contributor

@frederik-h frederik-h left a comment

Choose a reason for hiding this comment

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

nice 👍

|`LSPO_MOD` |`KC_LSFT` |The keycode to send when Left Shift is tapped |
|`RSPC_MOD` |`KC_RSFT` |The keycode to send when Right Shift is tapped |
|`DISABLE_SPACE_CADET_ROLLOVER`|*Not defined*|If defined, use the opposite Shift key to cancel Space Cadet |
|`DISABLE_SPACE_CADET_MODIFIER`|*Not defined*|If defined, prevent the Space Cadet to apply a modifier to LSPO_KEY and RSPC_KEY|
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to have corresponding options (DISABLE_LSPO_MOD, DISABLE_LSPC_MOD) for both sides? I am not sure if this is necessary for any language if your goal is to have matching pairs of parentheses or brackets on the left and right shift key.

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 think it's not needed. But it can be changed later if required.

quantum/quantum.c Outdated Show resolved Hide resolved
quantum/quantum.c Show resolved Hide resolved
@@ -627,14 +634,21 @@ bool process_record_quantum(keyrecord_t *record) {
}
else {
#ifdef DISABLE_SPACE_CADET_ROLLOVER
if (get_mods() & MOD_BIT(KC_RSFT)) {
if (get_mods() & MOD_BIT(RSPC_MOD)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested this with DISABLE_SPACE_CADET_ROLLOVER on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yes as well 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

The question was repeated accidentally, not to emphasize it ;-).

…egistering KC_LSFT when equals to LSPO_MOD
@anthonyrichir
Copy link
Contributor Author

@frederik-h if you have some spare time to review, I edited the code to avoid to unregister the mod if KC_SLFT == LSPO_MOD as well as addressed your other remarks.

@frederik-h
Copy link
Contributor

@anthonyrichir Looks fine to me

@anthonyrichir
Copy link
Contributor Author

Thanks a lot.
Regarding the conversations, I don't know who's responsible for resolving the conversations, if it's a maintainer, you or me?

@frederik-h
Copy link
Contributor

I don't know, I guess either you could wait until someone who could merge this notices it or you could ask. @drashna has recently reviewed and merged a PR of mine.

quantum/quantum.c Outdated Show resolved Hide resolved
quantum/quantum.c Outdated Show resolved Hide resolved
quantum/quantum.c Outdated Show resolved Hide resolved
quantum/quantum.c Outdated Show resolved Hide resolved
change #if to if statement
@anthonyrichir
Copy link
Contributor Author

@drashna you seems to be the only one very active around here, how can I can get a review for this PR ?

@drashna
Copy link
Member

drashna commented Sep 26, 2018

That's because I mostly handle keyboards, keymaps, and documentation.
@jackhumbert and @skullydazed are the ones that usually handle most everything (else), but they've been busy.

I'm not as skilled or as good with programming as they are, so I'm hesitant to merge "core" code. Sorry.

@anthonyrichir
Copy link
Contributor Author

Yes, don't be sorry, I understand and I'm not requesting you to do it, I was more wondering what was going on as it's been quite some time since I opened this PR.

@anthonyrichir
Copy link
Contributor Author

@jackhumbert @skullydazed
I'm sorry to bother you guys but could you take a look at this ?
Thanks in advance

@drashna
Copy link
Member

drashna commented Jan 25, 2019

What a difference that time makes.

Just to make sure, this is tested and does work, correct?

@anthonyrichir
Copy link
Contributor Author

I use it on a daily basis on my Planck, I don't know if it's good enough for you guys.

@drashna
Copy link
Member

drashna commented Feb 5, 2019

Thanks!

@drashna drashna merged commit 5c7a31e into qmk:master Feb 5, 2019
@leico
Copy link
Contributor

leico commented Feb 16, 2019

Congrats!

zer09 pushed a commit to zer09/qmk_firmware that referenced this pull request Feb 16, 2019
* Improvement of Space Cadet Shift by preventing to automatically apply a modifier on the key and allow to override the default modifier. Closes qmk#3815

* Improve the use of the DISABLE_SPACE_CADET_MODIFIER flag to avoid unregistering KC_LSFT when equals to LSPO_MOD

* change #if to if statement
dlhextall pushed a commit to dlhextall/qmk_firmware that referenced this pull request May 24, 2019
* Improvement of Space Cadet Shift by preventing to automatically apply a modifier on the key and allow to override the default modifier. Closes qmk#3815

* Improve the use of the DISABLE_SPACE_CADET_MODIFIER flag to avoid unregistering KC_LSFT when equals to LSPO_MOD

* change #if to if statement
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