Cleanup and refactoring, mostly prepared for custom values #1
Conversation
|
As far as I can tell behavior is unchanged, but this does not rely on the key's own releases which makes custom values possible. |
IsaacElenbaas
force-pushed
the
auto-shift-cleanup
branch
from
814c94b
to
aee1085
Compare
|
Pressing one autoshift key, pressing another, and releasing the first causes both to be unshifted regardless of the second one's hold time. Not sure how to fix here, but found a solution if custom shifts is merged. |
|
Issues from manual testing (with
You'll have to rebase to get rid of the conflicts (sorry - I'm rebasing over qmk/develop myself).
FYI I couldn't repro the issue you mentioned (if I hold |
|
Shoot, thought I had already fixed the conflicts. Oh well, will try again. Tested with The issue with releasing is when under the tapping term - pressing the second key triggers evaluation of the first, and releasing the first key still in the second one's hold time causes the latter to be evaluated and released as non-shifted, even if you're still holding it. The cause is |
IsaacElenbaas
force-pushed
the
auto-shift-cleanup
branch
from
daa91b8
to
3491f86
Compare
| } | ||
| } else { | ||
| autoshift_end(keycode, record->event.time, false); | ||
| return false; |
There was a problem hiding this comment.
What would it take to rewrite this so that we could return true here (at least for the non-custom case)?
I tried to avoid eating records since the behaviour becomes less transparent to things downstream (like process_record_user).
There was a problem hiding this comment.
Basically just not mergng this. It's near impossible to return true and still have control of keyrepeat to make this not a breaking change, and this refactor relies heavily on the assumption that it will be the one pressing and releasing keys (as I was preparing for custom ones). Your original pull is simpler if you want to go with only-keyrepeat, though I would probably submit another cleanup pull to make it handle shift more like this.
IMO that is more of a QMK-wide issue. We need a way to let other processes know that a key was pressed but that it's been handled and they shouldn't press anything, avoiding return false; everywhere is a pain.
| unregister_code(keycode); | ||
| if (keycode == autoshift_lastkey) { | ||
| // This will only fire when the key was the last auto-shiftable | ||
| // pressed. That prevents aaaaBBBB then releasing a from unshifting |
There was a problem hiding this comment.
nit: do you mind putting quotes around the keys here ("releasing a from" is a bit hard to parse eagerly).
| if (elapsed < TAPPING_TERM && keycode == autoshift_lastkey) { | ||
| // Allow a tap-then-hold for keyrepeat. | ||
| if (!autoshift_flags.lastshifted) { | ||
| register_code(autoshift_lastkey); |
There was a problem hiding this comment.
Would be good if we could return true here rather than registering the key ourselves. Though you'd miss updating autoshift_time below if you returned directly.
I've done my best to condense this to just one evaluate function and remove conditions. It fits more with AutoShift's behavior (record stuff on press, evaluate on release or another key down [and now timeout]) and is easier to wrap your head around.
Adding support for custom values should at this point be a 10-minute task, aside from somehow recording whether the shifted key was the one pressed (for if its release isn't immediately triggered and the shift state could have been changed). Simply releasing both the unshifted and shifted base keys is also an option.