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

Antecedent Morph Behavior aka Adaptive Keys #2042

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

voidyourwarranty2
Copy link

The Antecedent-Morph behavior (adaptive keys) sends different behaviors, depending on which key was most recently
released before the antecedent-morph behavior was pressed, if this occurs within a configurable time period.

Details in the docs.

@voidyourwarranty2 voidyourwarranty2 requested a review from a team as a code owner November 28, 2023 20:15
Copy link
Contributor

@caksoylar caksoylar left a comment

Choose a reason for hiding this comment

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

I reviewed the docs portion noting some improvements, but overall I noticed that the page organization isn't quite ideal. In particular:

  • You will notice that any behavior page that has a "Behavior Binding" actually refers to pre-defined node labels, not labels from an example like &ad_a here
  • The descriptions of properties like antecedents and bindings can go under "Configuration" header
  • The example you have, along with its usage in the keymap, can go under an "Example Usage" section.

I think a good example to look at is tap dance page here, since that is similarly a behavior type that doesn't have a pre-defined instance.

Other docs-related things:


## Summary

The Antecedent-Morph behavior (adaptive keys) sends different behaviors, depending on which key was most recently
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The Antecedent-Morph behavior (adaptive keys) sends different behaviors, depending on which key was most recently
The Antecedent-Morph behavior (adaptive keys) sends different behaviors depending on which key was most recently

Comment on lines 13 to 17
The configuration of the behavior consists of an array of `antecedents`, key codes with implicit modifiers, as well as
of a delay `max-delay-ms` in milli-seconds. If none of the `antecedents` was released during the `max-delay-ms` before
the antecedent-morph behavior is pressed, the behavior invokes the `defaults` binding. If, however, the `n`-th of the
key codes (with implicit modifiers) listed in the array `antecedents` was released within `max-delay-ms`, the behavior
invokes the `n`-th of the bindings of the `bindings` property.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The configuration of the behavior consists of an array of `antecedents`, key codes with implicit modifiers, as well as
of a delay `max-delay-ms` in milli-seconds. If none of the `antecedents` was released during the `max-delay-ms` before
the antecedent-morph behavior is pressed, the behavior invokes the `defaults` binding. If, however, the `n`-th of the
key codes (with implicit modifiers) listed in the array `antecedents` was released within `max-delay-ms`, the behavior
invokes the `n`-th of the bindings of the `bindings` property.
The configuration of the behavior consists of an array of `antecedents` which consist of keycodes including [implicit modifiers](../codes/modifiers.md#modifier-functions), as well as
a delay `max-delay-ms` in milliseconds. If none of the `antecedents` were released during the `max-delay-ms` before
the antecedent-morph behavior is pressed, the behavior invokes the `defaults` binding. If, however, the `n`-th of the
keycodes (with implicit modifiers) listed in the array `antecedents` was released within `max-delay-ms`, the behavior
invokes the `n`-th binding in the `bindings` property.

Finally, if the user is still holding down a Shift key when pressing the A that triggers the above antecedent-morph
behavior, then this results in an upper case O rather than a lower case o.

### Dead Antecedents
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this section is complicated and exposes too much of the ZMK internals for an end user. If dead keys functionality is really something that people need to be using, I would put it in a :::tip box where you can briefly mention using e.g. K_CANCEL as a dummy keycode.

RA(Y)`. Here, right Alt is called an *implicit modifier*. Antecedents are always considered with implicit modifiers. For
example,

```dts
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to make sure to use 4 spaces instead of tabs consistently in examples.

example, A is replaced by O if preceded by Z, regardless of which modifier keys were held down while tapping the Z,
i.e. in particular after a lower case z or after an upper case Z.

Finally, if the user is still holding down a Shift key when pressing the A that triggers the above antecedent-morph
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems rather obvious to me, but I am not strictly opposed to having it if you feel differently.


### Explicit Modifiers

The entire function of the antecedent-morph behavior is independent of the *explicit modifiers*. These are the modifiers
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that this can also go into a :::note box rather than a dedicated header.

shawnohare added a commit to shawnohare/zmk that referenced this pull request Mar 8, 2024
* Adaptive keys by the Antecedent Morph Behavior.

* Allow rolling in Antecedent Morph.

* Allow dead keys in Antecedent Morph.

* Multiple antecedent morph compiles.

* Documentation

* Fix errors.

---------

Co-authored-by: Void Your Warranty <[email protected]>
@davmarksman
Copy link

This PR is broken but it is fixed in https://github.com/klausweiss/zmk/tree/fix/adaptive-keys

@Aproxia-dev
Copy link

i hate to be that kinda person, but any updates on this?

@andreyadrian
Copy link

Please merge this 🙏

@ssbb
Copy link

ssbb commented Aug 19, 2024

If anyone is interested, I wrapped this PR in a module to use without needing to maintain a fork - https://github.com/ssbb/zmk-antecedent-morph

@proostas
Copy link

proostas commented Sep 1, 2024

It may be normal but I have noticed that Antecedent Morph Behavior clashes a bit with #2027 by introducing a small but very unpleasant delay for certain scroll and mouse move behaviors.

It would be great if somebody could confirm that it is really so and it could or couldn't be fixed.

After some debugging, it looks like there's nothing wrong with Antecedent Morph Behavior. It was just the last straw that made it possible to detect that I accidentally overlapped global combos with Mouse behaviors and those combos were the main lag contributors.

@smo999
Copy link

smo999 commented Sep 2, 2024

I cannot make it work together with hold-tap. I'm using the module made by @ssbb with the following configuration.

Am I missing something ?

    behaviors{
        adapt_key:adapt_key{
            compatible = "zmk,behavior-antecedent-morph"; 
            #binding-cells = <0>;
            defaults = <&kp T>; 
            bindings = <&kp N>; 
            antecedents = <K>; 
            max-delay-ms = <250>; 
        };
        test: test { 
            compatible = "zmk,behavior-hold-tap";        
            #binding-cells = <2>;
            flavor = "balanced"; 
            tapping-term-ms = <280>; 
            bindings = <&kp>, <&adapt_key>;
        }; 
    };

It fails /behaviors/test POST_KERNEL 34 < /behaviors/adapt_key APPLICATION 2.

NOTE: same error when trying to mix antecedent and mod morph.

@ssbb
Copy link

ssbb commented Sep 2, 2024

@smo999 I just merged PR by @haasn so it should be fixed now.

@haasn
Copy link

haasn commented Sep 10, 2024

Hi, I'm very interested in having this feature available by default. What can I do to help this MR out?

@caksoylar
Copy link
Contributor

Hi, I'm very interested in having this feature available by default. What can I do to help this MR out?

Do you have a specific issue with using the module version as linked above? This is a good candidate of a behavior that fits in the modular framework, rather than supported in tree.

@haasn
Copy link

haasn commented Sep 10, 2024

Do you have a specific issue with using the module version as linked above?

Yes - I would like to use the upstream Glove80 layout editor, which only supports the official/vanilla ZMK firmware. (It also uses a custom build process, which makes adding modules a bit annoying)

On a more philosophical note, I personally think it's much better for end users to have reasonably popular functionality included by default, rather than having to track down mods. On top of being less discoverable and more of a hassle to install, out of tree code tends to bitrot.

There is also the argument to be made that this style of behavior is incredibly flexible and can be used to implement a wide range of use cases (e.g. magic keys, adaptive letters. alternative repeat keys, etc); I consider it no less useful and versatile than other mainline feature such as combos or tap dances. If your goal is to minimize the size of the core code, surely these would be the first to go?

@ssbb
Copy link

ssbb commented Sep 10, 2024

ZMK is moving towards a modular approach, so making module usage easier is probably something that should be handled on the Glove80 build process side, I think.

The main problem with moving such features upstream before they are stable enough is that you might end up needing something that isn’t supported yet. As a result, you’ll have three PRs with the desired changes, and you’ll need to maintain your fork and resolve merge conflicts, which is far more of a hassle compared to just using the modules.

@caksoylar
Copy link
Contributor

caksoylar commented Sep 10, 2024

Let me try to address with my thoughts, as just one contributor to the project.

Yes - I would like to use the upstream Glove80 layout editor, which only supports the official/vanilla ZMK firmware. (It also uses a custom build process, which makes adding modules a bit annoying)

I think modular is the future of ZMK, and MoErgo might eventually want to account for that. But right now you can use the west-based workflow they provide.

IMHO it would be great if they make their custom tweaks and publish them as a module, so they don't have to maintain a bespoke fork.

On a more philosophical note, I personally think it's much better for end users to have reasonably popular functionality included by default, rather than having to track down mods. On top of being less discoverable and more of a hassle to install, out of tree code tends to bitrot.

In tree code needs constant maintenance, which is harder when there isn't an interested contributor (as evidenced by the lack of updates in this PR, this author isn't). So if merged it will certainly increase maintenance burden on core maintainers. This justification is also related to the popularity argument, addressed below.

The hassle to install is something that is planned to be improved with better tooling, such as ZMK CLI (which already has rudimentary support for configuring modules).

There is also plans to implement a proper versioning scheme so bitrot can be managed better in external modules.

There is also the argument to be made that this style of behavior is incredibly flexible and can be used to implement a wide range of use cases (e.g. magic keys, adaptive letters. alternative repeat keys, etc); I consider it no less useful and versatile than other mainline feature such as combos or tap dances. If your goal is to minimize the size of the core code, surely these would be the first to go?

Edit: reworded below section.

I think comparing the popularity of this feature to combos and tap dances might be a bit of an overestimation, but without data I won't comment much on it. Using the module and thus popularizing it further might be a good way to prove that it is popular enough to warrant inclusion in core, though.

@haasn
Copy link

haasn commented Sep 10, 2024

In tree code needs constant maintenance, which is harder when there isn't an interested contributor (as evidenced by the lack of updates in this PR, this author isn't). So if merged it will certainly increase maintenance burden on core maintainers. This justification is also related to the popularity argument, addressed below.

I think this argument grossly neglects the asymmetry of maintenance burden. Most maintenance tasks are significantly easier for the person doing the maintenance / refactor in question right now. rather than the person who contributed the module X years ago. (Or worse, an unaffiliated end user who knows nothing about the codebase prior to being forced to update the module in question)

For a case study of exactly what I mean, consider commit d6de8a3. It would have been utterly trivial to extend this rote replacement task to one additional file. (sed -i s/APPLICATION/POST_KERNEL/ behaviors/*.c even automates it). What happened instead is that I hit a cryptic error message, spent a good chunk of time both understanding what the error even means (and tracking down its source from deep within the build log), consulted with another developer, drafted a commit, forked the project and then had to wait for a developer to review and merge the changes. The amount of effort involved - a good half hour of my time - is surely not a worthwhile tradeoff for the 1 additional second it would have taken the original author of the change to apply it to one additional behavior definition.

What happens in my experience with highly modular projects is that random modules keep randomly breaking or requiring changes, usually hitting completely unaffiliated end users, asking them to either research and patch (or crudely work around) the breakage in question themselves, stop using the module (or finding a more updated replacement), or simply stop updating the core software to avoid this fate.

YMMV, but this is the reason I personally tend to avoid modular software these days - maintainer burden being shifted onto me, the uninformed and unaffiliated end user, does not lead to a pleasant software experience. (Tell me honestly, who has never cursed loudly at a sea of red error messages filling up the screen when launching vim or any equivalent software? Never mind the significant, regular maintenance burden I experience trying to use a Linux system that requires just one out-of-tree kernel module to boot...)

@Nick-Munnich
Copy link
Contributor

My two cents:

Modules are absolutely the future of ZMK, purely for board/shield definition reasons. Continuously adding keyboard definitions to ZMK would be a terrible approach, as we can see from QMK's example. The alternatives which aren't modules have proven to be inelegant and cause things to be messier than they should be.

That said, your (justified) points are aimed more towards the usage of modules for features, rather than for keyboards (though they do apply to both). ZMK has been using modules for some time now, but their presence and the ecosystem has been small and lacking, both of which are expected to change with time. Rather than arguing against modules, I think it would be more productive to try and focus on how we can design/improve the ecosystem to minimize the pain points you raised. Caksoylar has already mentioned some of these in his comment.

I am of course fully on board with including new popular features into ZMK directly. For behaviours specifically though, I personally think it is unlikely new behaviours (which aren't related to some additional feature such as studio unlock) will be added to ZMK prior to the above being worked out.

@voidyourwarranty2
Copy link
Author

voidyourwarranty2 commented Oct 7, 2024

Apologies for the leaving the patch unmaintained for so long, and thanks for all the fixes and updates that people provided in the meantime. I now found some time to rebase the patch to the current main branch, and so I hope the patch has become useful again. (Unfortunately, automatic rebasing screwed up, and I had to force push a current version).

@voidyourwarranty2 voidyourwarranty2 force-pushed the devel-adaptive branch 5 times, most recently from 2851ea2 to 9790a48 Compare October 15, 2024 15:52
@voidyourwarranty2 voidyourwarranty2 force-pushed the devel-adaptive branch 6 times, most recently from 934b441 to 6d42782 Compare October 23, 2024 08:11
@voidyourwarranty2 voidyourwarranty2 force-pushed the devel-adaptive branch 2 times, most recently from df5daac to 7558895 Compare October 27, 2024 22:04
@voidyourwarranty2 voidyourwarranty2 force-pushed the devel-adaptive branch 4 times, most recently from 77a133c to a47f497 Compare November 7, 2024 07:31
@voidyourwarranty2 voidyourwarranty2 force-pushed the devel-adaptive branch 5 times, most recently from f969ff1 to dd6b7c1 Compare November 16, 2024 12:26
@voidyourwarranty2 voidyourwarranty2 force-pushed the devel-adaptive branch 2 times, most recently from 32fcdda to cada819 Compare November 21, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants