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

Simple extended space cadet #5277

Merged
merged 3 commits into from
Apr 30, 2019
Merged

Conversation

XScorpion2
Copy link
Contributor

@XScorpion2 XScorpion2 commented Mar 1, 2019

Description

Extending the space cadet concept to have versions for alt and ctrl and to pack all the options into a single macro, SC_KEY where you define which mod to send normally, what mod to send on tap, and what keycode to send on tap, thus: SC_KEY(mod, tapmod, keycode).

  • If a user previously used DISABLE_SPACE_CADET_MODIFIER, in this new world, they would just define their SC_KEY with KC_TRNS as the tapmod.
  • If a user previously used DISABLE_SPACE_CADET_ROLLOVER, this is now just automatic as this define was a bug fix plain and simple when you step back and think about what Space Cadet at it's core was designed to do.

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

@XScorpion2 XScorpion2 marked this pull request as ready for review March 1, 2019 02:49
@XScorpion2 XScorpion2 changed the title Simple space cadet Simple extended space cadet Mar 1, 2019
@drashna
Copy link
Member

drashna commented Mar 1, 2019

Travis CI failure is due to feature creep.

@XScorpion2
Copy link
Contributor Author

ya, even with this simpler version I just couldn't get it below the limit on those 3 boards.

@XScorpion2
Copy link
Contributor Author

Merged docs for Space Cadet Shift & Space Cadet Shift Enter and updated them with the new configuration defines.

@XScorpion2
Copy link
Contributor Author

Force push rebase on latest master

@ghost
Copy link

ghost commented Mar 31, 2019

This looks like it fulfils #2249, so feel free to contact me once it's merged :)

@XScorpion2
Copy link
Contributor Author

@Krakob sure, I'll let you know when it lands, but I won't take the bounty

@drashna
Copy link
Member

drashna commented Mar 31, 2019

@Krakob when it gets merged, if you really want to send money somewhere, there's always this: https://donorbox.org/qmk

@XScorpion2
Copy link
Contributor Author

But only do so if they get around to approving / merging this in the next 5 days =p

@ghost
Copy link

ghost commented Mar 31, 2019

But only do so if they get around to approving / merging this in the next 5 days =p

Ok! €30 goes to QMK's donorbox if this is merged within five days :)

@XScorpion2
Copy link
Contributor Author

@Krakob I pushed a small change that exposed the main function:
void perform_space_cadet(keyrecord_t *record, uint8_t normalMod, uint8_t tapMod, uint8_t keycode)
So you will be able to use it in your keymaps directly in desired.

@XScorpion2
Copy link
Contributor Author

Not entirely sure the CI failures are my fault planck & preonic compile errors in keymaps

@XScorpion2
Copy link
Contributor Author

XScorpion2 commented Apr 20, 2019

@skullydazed reverted the change from KC_ to MODS_ as MODS_ is compressed down to 5 bits while KC_LCTRL through KC_RGUI are 8 bits. This causes an issue when you use MODS_R versions as they cannot be used directly, IE: register_mods(KC_RSHIFT); and have to be converted. In addition, this also means things like MODS_LSHIFT | MODS_RCTRL (10011) won't work as expected as the result would be indistinguishable from MODS_RSHIFT | MODS_RCTRL (10011).

@XScorpion2
Copy link
Contributor Author

Rebased on latest master, any update on merging this?

@drashna drashna merged commit c745d9b into qmk:master Apr 30, 2019
akrob pushed a commit to akrob/qmk_firmware that referenced this pull request Apr 30, 2019
* upstream/master: (779 commits)
  [Keyboard] Signum3.0 remove sortedcontainers (qmk#5679)
  Simple extended space cadet (qmk#5277)
  Removed forced in lining for lib8tion functions (qmk#5670)
  Change lib8tion library to be usable in user keymaps (qmk#5598)
  [Keyboard] Fixing drag-and-drop (qmk#5728)
  [Keyboard] Adding ortho_4x12 & planck_mit layouts for KBD4X (qmk#5729)
  [Keyboard] Minor fixes for Baguette (qmk#5737)
  Updated rgb_led struct field modifier to flags (qmk#5619)
  RGB Matrix: Custom effects on a kb/user level (qmk#5338)
  Fix Planck and Preonic builds (qmk#5658)
  [Keymap] dz60 keymap w/ hhkb-esque default layer (qmk#5708)
  [Keymap] Added compatibility for Planck rev6 (qmk#5706)
  [Keyboard] Satisfaction75 i2c fix and VIA layout (qmk#5726)
  A better new_project.sh (qmk#5191)
  Fix sendstring "#" producing "£" instead (qmk#5724)
  [Keyboard] Added WT69-A PCB (qmk#5721)
  [Keymap] Fix typo and function layer image for Quefrency (qmk#5719)
  [Keymap] Initial keyboard layout for KBD67 (qmk#5720)
  [Keymap] New keymap for Quefrency 65% with split backspace, RGB, media keys, mouse keys (qmk#5717)
  [Keyboard] Update Gergo to use newer Ergodox Matrix code (qmk#5703)
  ...
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Apr 30, 2019
* Simplifying and Extending Space Cadet to work on Ctrl and Alt keys

* PR Review feedback

* Reverting back to keycodes
fdidron added a commit to zsa/qmk_firmware that referenced this pull request Apr 30, 2019
@XScorpion2 XScorpion2 deleted the simple_space_cadet branch May 1, 2019 00:49
foosinn pushed a commit to foosinn/qmk_firmware that referenced this pull request May 6, 2019
* Simplifying and Extending Space Cadet to work on Ctrl and Alt keys

* PR Review feedback

* Reverting back to keycodes
Timbus pushed a commit to Timbus/qmk_firmware that referenced this pull request Jun 23, 2019
* Simplifying and Extending Space Cadet to work on Ctrl and Alt keys

* PR Review feedback

* Reverting back to keycodes
ridingqwerty pushed a commit to ridingqwerty/qmk_firmware that referenced this pull request Jan 10, 2020
* Simplifying and Extending Space Cadet to work on Ctrl and Alt keys

* PR Review feedback

* Reverting back to keycodes
JeffreyPalmer pushed a commit to JeffreyPalmer/qmk_firmware that referenced this pull request Feb 27, 2020
* Simplifying and Extending Space Cadet to work on Ctrl and Alt keys

* PR Review feedback

* Reverting back to keycodes
@dRoskar
Copy link

dRoskar commented May 26, 2020

@skullydazed reverted the change from KC_ to MODS_ as MODS_ is compressed down to 5 bits while KC_LCTRL through KC_RGUI are 8 bits. This causes an issue when you use MODS_R versions as they cannot be used directly, IE: register_mods(KC_RSHIFT); and have to be converted. In addition, this also means things like MODS_LSHIFT | MODS_RCTRL (10011) won't work as expected as the result would be indistinguishable from MODS_RSHIFT | MODS_RCTRL (10011).

I'm trying to make my space cadet key do LALT on key down and RALT+V on tap. Why was this changed to not work anymore? RALT has a different functionality on some layouts than LALT and this breaks space cadet for those layouts.

@XScorpion2
Copy link
Contributor Author

XScorpion2 commented May 26, 2020

@dRoskar you might be misunderstanding that comment. Skully asked for a change to use the MODS values instead of using KC values for modifiers. As I mentioned in the comment that this actually causes a problem and prevents folks from distinguishing Left vs Right modifiers and that I was reverting the change so that users can continue to distinguish Left vs Right just like they do today.

For what you described, you would set up your space cadet define as:
#define LAPO_KEYS KC_LALT, KC_RALT, KC_V
Which is, send KC_LALT when held, the mod KC_RALT with the key KC_V when tapped.

@dRoskar
Copy link

dRoskar commented May 26, 2020

@XScorpion2 Oh yeah.. come to think of it, my assumption didn't make sense. But I did discover what the issue is. My set up is exactly as you describe. In my locale, RALT+V produces an '@'. But it only works in some text editors. And the reason is that the above definition triggers CTRL+LALT+V which is almost the same as RALT+V but not quite. And in certain editors this triggers a hotkey action instead of writing the '@'.
Thoughts?

@XScorpion2
Copy link
Contributor Author

hmm, that setup should trigger on tap in order: LALT down, LALT up, RALT down, V down, V up, RALT up
ctrl should not be involved in this unless you have the ctrl key down when you do this. Have you looked at Tap Dance? Space cadet was designed around the principal of keeping things simple and responsive, while Tap Dance is a lot more configurable and can delay sending keys until it resolves the full action. It's a bit larger in firmware space, but for your use case it might turn out better. I use a combination of space cadet for simple things, like [ or ] for Left or Right shifts, and more advanced combinations I find Tap Dance is a better fit.

@dRoskar
Copy link

dRoskar commented May 27, 2020

@XScorpion2 Thank you for taking your time to give me the exact key combination that happens. The fact that LALT is effectively tapped before the RALT+V combo is what causes the issue, which in certain editors triggers an altered state (In intelliJ it ends up behaving the same as if you pressed CTRT+LALT+V which really was a red herring here).
Also in browsers it flashes the menu, which, if nothing else, is annoying. I'll look into Tap Dance to see if I can get the wanted behavior without getting an ALT tap infront of my RALT+V combo.

@XScorpion2
Copy link
Contributor Author

XScorpion2 commented May 27, 2020

Ya, that initial tap behavior throws things a little off in some applications, but it is responsive. Generally I've found that space cadet works fine with CTRLs and Spaces, but ALT is problematic due to that. Tapdance however delays sending all keycodes until it can determine the action you are taking, which is usually determined by the delay set by TAP_TERM in ms. So for example if your delay is set to 150 (ms), and you have tapdance setup to do that combo here's what it feels like for hold vs tap:
Hold: You press LALT, 150ms delay, LALT down triggers, you release LALT, LALT up triggers.
Tap: You tap LALT (press & release are faster than 150ms), on your release RALT down, V down, V up, RALT up triggers.

@dRoskar
Copy link

dRoskar commented May 27, 2020

@XScorpion2 Thank you, sir!

@JayceFayne JayceFayne mentioned this pull request Sep 4, 2021
14 tasks
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.

6 participants