Kaleidoscope
Kaleidoscope copied to clipboard
keyboardio
Separate layer shifts and locks
This (hopefully) makes layer changes more intuitive for users, and more straightforward to explain and reason about. I believe this is a natural extension of the activation-order layer stack, and away from the fixed-order stack of old.
Specifically, it changes things so that layer lock keys will deactivate the target layer if it is at the top of the active layer stack, but otherwise it will bring the target layer to the top of the stack, regardless of whether or not it was previously active. This ought to provide less surprising behaviour in the case where a layer was active, but hidden (possibly for long enough that the user can't recall its status). Pressing the layer lock key is now guaranteed to produce a user-visible change (unless the target layer is 100% transparent).
Layer shift behaviour is also changed, but not quite in the same way. A layer shift key will now activate the target layer, as before, but pressing a second layer shift key for the same target layer won't bring the target layer to the top of the stack. The idea is that pressing a second identical layer shift should be interpreted as a continuation of the already-active shift, probably so that the user can change hands. A user could still forget about a shifted layer, but it would have to be a sticky OneShot layer shift, so I think this is reasonable.
The major change is that locked and shifted layers are now tracked separately on the layer stack (using an offset), which means that locking a layer, then pressing and releasing a layer shift key for that layer will not result in the layer being unlocked.
Closes #1009
This PR constitutes a small design change that affects behaviour in a user-visible way. It does not affect user sketches at all, and in the vast majority of expected use cases there should be no noticeable difference. In some corner cases, it should result in less surprising behaviour, in my opinion, and it does solve at least one problematic issue (the release of a layer shift key deactivating a layer activated by a layer move or lock).
In some of the use cases (see the included testcases for details), there is no unambiguous best behaviour, but I believe that layer changes in this PR will be generally more consistent and less surprising than they are in current master.
This PR saves a total of 180 bytes of PROGMEM and 19 bytes of RAM, incidentally. The PROGMEM savings come entirely from the first commit, that simply removes the vestigial layer_state_ variable, but doesn't change any behaviour. (Not that I'm not hoping the minor behaviour changes will be accepted, too, but we've been looking for ways to save bytes.)
Looking through the open issues, I realized that I had forgotten about #910, which contains some useful and relevant discussion of the design change here. In particular, though this PR addresses most of what was discussed there (in my opinion), it does not differentiate the existing LockLayer() and UnlockLayer() macros, leaving them both with the same "raise or deactivate layer" behaviour (which is very nearly the same as "toggle layer" from the perspective of a typical user).
While we could redefine UnlockLayer(N) to give it an unconditional "deactivate layer" behaviour, and assign a new range of Key values to represent it, I don't think doing so serves much functional purpose. If the user has fully-specified layers, any layer that's not on top is effectively already unlocked, and a user who is employing layer transparency, it's a simple matter to press the key twice to deactivate it.
If I'm wrong, and there are people who frequently deactivate non-top layers in order to access different lower layers via transparency, then it would be worth further discussion, but even in that case, having separate "lock" and "unlock" keys in the keymap is probably not worth the complexity (in the keymap, not the code).
I just pushed a rebase of these changes to bring it up to date and run all the tests.
This is, indeed, a natural extension of the activation-order stack, and it does make using layer keys far less surprising, because the edge cases are handled in a more natural way.
Lets pull this in.
@obra — do you want to weigh in on this?
I maintain my approval, but would like @obra to chime in too (once he has some spare cycles) before hitting the merge button.
Assuming this does what the NEWS update says it does, I’m +1 to mergeOn Oct 5, 2022, at 4:54 PM, Gergely Nagy @.***> wrote: I maintain my approval, but would like @obra to chime in too (once he has some spare cycles) before hitting the merge button.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>
Re-reading my notes in NEWS.md, I see some typos. I will fix that up shortly.
I see updates were pushed, code is approved, docs are great, tests were added, acked by Jesse too. Merging!
Thanks a lot for all the hard work that went into this!