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

[New Feature] Key Overrides #11422

Merged
merged 41 commits into from
Jul 13, 2021
Merged

[New Feature] Key Overrides #11422

merged 41 commits into from
Jul 13, 2021

Conversation

JonasGessner
Copy link

Description

I originally pitched this feature in #11301 and on the qmk discord, and now my code is ready to be reviewed, tested and merged. I wrote a detailed documentation, and I will refer to this document for an explanation of this feature. This document covers the basic idea of the feature, how it is used, why it is different from combos and more.

This seems to be a frequently requested feature (see here, here, here, #108, #1495), but no solid solution has yet been implemented yet (see #2900, #4795, and my comments about #4795 here: #11301).

Also see #11301 for additional details, including some of the implementation decisions. This PR contains a few core changes, but these are purely additive.

I put a lot of thought and effort into this feature, as will be evident from the documentation, and I think people will really like using it. I have been using and testing it myself for over half a year, and I have been continuously improving it.

While precise benchmarking is hard due to the limited granularity of milliseconds from the timer.h functions, I performed some benchmarks. I measured the process_key_override function, which is executed from process_record_quantum, and it is clocking in at 0ms-1ms per key press on my keyboard, which contains 28 key overrides. These numbers don't provide much insight, but at least they show that key overrides add a minimal overhead. To compare, a call to layer_on takes about 10ms on my keyboard.

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: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • 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).

@dnaq
Copy link

dnaq commented Jan 5, 2021

This is an amazing feature, thanks for the work you have put into it. It will greatly simplify having custom keymaps for different languages.

I have found one issue while trying to port my keymap to it, when the replacement key is a custom keycode, then a bunch of modifiers are added to the keypress. I'll give a minimal example of what I mean:

enum custom_keycodes {
  CU_GRV = SAFE_RANGE,
  CU_CIRC,
}

const key_override_t override_6 = ko_make_basic(MOD_MASK_SHIFT, KC_6, CU_CIRC);

const key_override_t **key_overrides = (const key_override_t *[]){ &override_6, NULL};

bool process_record_user(uint16_t keycode, keyrecord_t *record) {
  switch (keycode) {
    case CU_CIRC:
      return false;
    default:
      return true;
  }

Here a press of Shift-6 won't yield the keycode CU_CIRC, instead it will yield a weird combination of characters, that seems to be dependent on the custom keycode it has. For example using the above code for me yields the following output (from xbindkeys):

"(Scheme function)"
    m:0x11 + c:50
    Shift+Mod2 + Shift_L
"(Scheme function)"
    m:0xd5 + c:105
    Control+Shift+Mod2+Mod4+Mod5 + Control_R
"(Scheme function)"
    m:0xd1 + c:108
    Shift+Mod2+Mod4+Mod5 + ISO_Level3_Shift
"(Scheme function)"
    m:0x51 + c:134
    Shift+Mod2+Mod4 + Super_R
"(Scheme function)"
    m:0x11 + c:21
    Shift+Mod2 + dead_acute
"(Scheme function)"
    m:0x11 + c:50
    Shift+Mod2 + Shift_L

The problem I'm trying to solve using your pull request is to have a US keyboard layout on an international keyboard. I have a much more convoluted solution for this in current keymap, but your approach is a lot nicer to work with.

The example has been stripped down to be minimal, but it should be reproducible.

@JonasGessner
Copy link
Author

Thanks for your feedback @dnaq and I'm glad you find this feature useful! I tested your example and reproduced the issue. Turns out I had to add some more checks to make it work properly with custom keycodes. It should work now!

Keep in mind though that process_record_user will not be called for your custom keycode when you set it up as replacement key in a key override. With the current design of qmk, process_record_user is only called for physical key presses, so it will not be called for the replacement key registered by the key overrides feature. Unfortunately there is no equivalent to process_record_user for keycodes registered in software.

Key overrides are designed with this in mind, and they allow you to set up a function that is called every time the override activates and deactivates. If you look at the key overrides docs, there is an example showing how you can register a custom function (it is the advanced example at the bottom), and there is also documentation text on the custom_action member of key_override_t. Hope this works and any more feedback is welcome!

@dnaq
Copy link

dnaq commented Jan 5, 2021

Thanks for the fix. This almost solves my use-case, there's still some code that I need to write to handle dead keys and shifted dead keys (I used a macro for these previously), but it greatly simplifies the keymap at least.

I think a viable solution is to have a custom keycode that checks the modifiers and returns true if the modifiers that the override triggers on are set, causing a wee bit of code duplication, but still much cleaner than my old solution.

@treeman
Copy link

treeman commented Jan 10, 2021

Very cool feature.

Is it possible to get it working with auto-shift? I tried this kind of override (with a Swedish keyboard in the OS):

// / -> !
const key_override_t slsh_override = ko_make_basic(MOD_MASK_SHIFT, SE_SLSH, SE_EXLM);

@JonasGessner
Copy link
Author

It is possible, but would require modifying the auto shift code. Unfortunately the various features (like auto shift) that are spread throughout the tmk and qmk code are implemented without a unified interface and by different developers, making it really hard to have these different features work together nicely. While there is a clean interface for handling physical key presses (process_record_*), there is no equivalent for handling software-based key presses (i.e. calling functions like register_code, add_mods, etc). Auto shift registers keycodes using these functions, while key overrides only handle physical key presses, since there is no design in place for handling software-based key presses. A fundamental redesign would be necessary here.

So once this PR is accepted, I could look at adding support for auto shift with a separate PR, where I would have to dig into the auto shift code and make it specifically call into the key overrides code.

One comment for your code snippet: The trigger keycode has to be without any modifiers, since you specify the modifiers separately (i.e. MOD_MASK_SHIFT in your code snippet). SE_SLSH is defined as S(SE_7), therefore you should just specify SE_7:

// / -> !
const key_override_t slsh_override = ko_make_basic(MOD_MASK_SHIFT, SE_7, SE_EXLM);

@treeman
Copy link

treeman commented Jan 11, 2021

Alright, thank you.

As for my code snippet I do have to use SE_SLSH as it doesn't seem to work with SE_7?

@drashna drashna requested a review from a team January 11, 2021 07:28
@JonasGessner
Copy link
Author

@treeman ok, I took a look at your keymap and it seems I misunderstood what you are trying to do! You're right to use SE_SLSH as trigger keycode.

I thought you had the keycode SE_7 on your keymap and wanted to map this to SE_EXLM when shift is held, but you actually have SE_SLSH (S(SE_7)) on your keymap, so specifying SE_SLSH as trigger keycode is correct in this case.

@JonasGessner
Copy link
Author

PSA: The name of this feature is not set in stone yet. I don't think it's necessarily the best name, so if anyone has ideas for a better name I'd be happy to hear them.

@theol0403
Copy link

theol0403 commented Jan 17, 2021

This PR looks amazing! I come from #4795, but this implementation looks much better! I look forward to using this new functionality for all sorts of stuff.

However, I am decently tight for firmware space on my board, but this PR absolutely obliterates my available space and goes 1.2k bytes over (and that's with the removal of #4795). I measured this PR to take about 1.8k bytes on my board, just by enabling the feature. Each override takes 20B.

Looking at the diff, I see a ton of functionality that I will probably never use. For example: callbacks, negmods, suppressed mods, options, layers, enabled, key override on/off/toggle/keycodes. They still take up a lot of space regardless of whether I use them or not.

My request to you, is to put as much functionality as possible behind macro config options. If I want to use any of the above-listed features, I need to enable a macro in config.h. This will greatly save on space, and keep this new feature exactly as complex as the user wants it.

I think that by default, this PR should only provide ko_make_basic(modifiers, key, replacement), and as you need more advanced features, you can enable them.

I do however see myself using quite a few of these features (I quite like your layer key example), so any other way to reduce firmware space would be greatly appreciated!

Thank you!

@JonasGessner
Copy link
Author

Thanks for your feedback! Code size-wise I don't see a whole lot than can be done without turning off certain features. Your idea of adding extra flags to enable/disable some of the features could certainly be a way to reduce code size, although it would also make the source code itself more bloated and complicated to follow.

Feedback by reviewers would be helpful on this. I could, for example, add a flag that either turns on or turns off the options, negative_mods, custom_action, context and enabled features. The savings in compiled code size would not be monumental for the feature itself, but it would make the type key_override_t smaller. Depending on how many overrides you use, these savings could add up to provide a reasonable amount of size reduction.

I would consider leaving this PR as-is for now, and perhaps adding a flag like KEY_OVERRIDE_DISABLE_EXTRA at a later point in a separate PR, which allows turning off the mentioned features. That kind of change would be non-breaking.

Also, you've probably already done this, but you can check this guide for ideas to reduce code size: https://thomasbaart.nl/2018/12/01/reducing-firmware-size-in-qmk/

@theol0403
Copy link

Sounds good! Given the amount of space the feature uses, without looking at the code, I can imagine much of it is to provide support for the various features mentioned, so could removing the API for various interfaces also remove much of the internal code? Hopefully implementing that won't be too difficult. There could be other optimizations done too, like removing the keyrepeat functionality if the defer delay is 0.

Thanks again for making this feature however, and I hope the reviewers will have some insight on this problem!

@99epep
Copy link

99epep commented Feb 27, 2021

Hi Jonas, thanks for your work !

Is it possible to override a mod-tap key ?
I mean, in these two ways : overriding the key sent by a mod-tap when tapped, and keeping the modifier functionality of a mod-tap on which an override is defined ?

@tzarc tzarc closed this Feb 27, 2021
@tzarc tzarc deleted the branch qmk:develop February 27, 2021 20:28
@tzarc
Copy link
Member

tzarc commented Feb 27, 2021

Sorry about that, GitHub decided to delete the develop branch from upstream when we merged it, despite being told otherwise. Reopened.

@tzarc tzarc reopened this Feb 27, 2021
@JonasGessner
Copy link
Author

Hi Jonas, thanks for your work !

Is it possible to override a mod-tap key ?
I mean, in these two ways : overriding the key sent by a mod-tap when tapped, and keeping the modifier functionality of a mod-tap on which an override is defined ?

Yes you can use mod tap keys. If you want to use the mod tap key as the trigger key you have to specify the full MT(.., ..) keycode as the trigger key. If you want to use the modifier state of a mod tap key as one of the required modifiers to activate a key override you don't need to do anything special.

@99epep
Copy link

99epep commented Feb 28, 2021

Yes you can use mod tap keys. If you want to use the mod tap key as the trigger key you have to specify the full MT(.., ..) keycode as the trigger key.

Thanks, works fine !

If you want to use the modifier state of a mod tap key as one of the required modifiers to activate a key override you don't need to do anything special.

That is not the point, I would like an overrided mod-tap key to still be a mod-tap key. For example I’d like this to be possible :
const key_override_t a_ko = ko_make_basic(0, MT(MOD_LSFT, BP_A), MT(MOD_LSFT, FR_A));

@JonasGessner
Copy link
Author

I see, you want the replacement key to be a mod-tap. That won't work unfortunately! It's a non-trivial case to handle. Getting these different features to work together nicely is very hard and I am not planning on adding support for MT replacement keys.

@99epep
Copy link

99epep commented Feb 28, 2021

Thanks again Jonas, yes keep it as simple as can be ! I just wanted to know as I’m not able to understand this by myself. The trick I asked better suits a layer change.

I’m looking for the easiest way to "override" or "remap" a layout over another one, in my case I want a bépo layout (kind of french Dvorak) when plugged on a french/azerty device. Your key-overrides allow to obtain this without defining a specific layer (unsure it’s clever), unless there’s mod-taps around.

I tried your feature in combinaison with sevanteri’s early_combo and everything seems to work as expected.

@theol0403
Copy link

theol0403 commented Mar 22, 2021

Hey again, finally made some space on my board to experience the power of this feature 😋.

I made a little helper file to make working with overrides even nicer, inspired by germ's combos helper!

The user-facing code looks like this:

// simple overrides go here
#define OVERRIDE_DEF                                                \
  OVERRIDE_BASIC(delete_key, MOD_MASK_SHIFT, KC_BSPACE, KC_DELETE)  \
  OVERRIDE_BASIC(tilde_esc, MOD_MASK_SHIFT, KC_ESC, S(KC_GRAVE))    \
  OVERRIDE_LAYERS_NEGMODS_OPTIONS(prev_track_override_example, MOD_MASK_CS, KC_MPLY, KC_MPRV, ~0, MOD_MASK_ALT, ko_option_no_reregister_trigger)

// complicated overrides go here (if you want linebreaks, comments, or initializers), like normal use of this PR
const key_override_t vol_up_override = ko_make_with_layers_negmods_and_options(MOD_MASK_ALT, KC_MPLY, KC_VOLU, ~0, MOD_MASK_CS, ko_option_no_reregister_trigger);

const key_override_t vol_down_override = ko_make_with_layers_negmods_and_options(MOD_MASK_SA, KC_MPLY, KC_VOLD, ~0, MOD_MASK_CTRL, ko_option_no_reregister_trigger);

// register the complicated overrides
#define OVERRIDE_EXTRAS &vol_up_override, &vol_down_override,

// alternative syntax for the above *could possibly* be:
#define OVERRIDE_EXTRAS                       \
  OVERRIDE_REGISTER_EXTRA(vol_up_override)    \
  OVERRIDE_REGISTER_EXTRA(vol_down_override)

// where the magic happens
#include "override_helper.h"

And, override_helper.h is

// this helper takes an override definition (defined by OVERRIDE_DEF) and converts it into qmk-compatible override syntax

// helper defines which feed into the `OVERRIDE` define 
#define OVERRIDE_BASIC(name, ...) OVERRIDE(name, ko_make_basic(__VA_ARGS__))
#define OVERRIDE_LAYERS(name, ...) OVERRIDE(name, ko_make_with_layers(__VA_ARGS__))
#define OVERRIDE_LAYERS_NEGMODS(name, ...) OVERRIDE(name, ko_make_with_layers_and_negmods(__VA_ARGS__))
#define OVERRIDE_LAYERS_NEGMODS_OPTIONS(name, ...) OVERRIDE(name, ko_make_with_layers_negmods_and_options(__VA_ARGS__))

// for this step we use `OVERRIDE` to create the objects
#define OVERRIDE(name, initializer) static const key_override_t override_##name = initializer;
OVERRIDE_DEF
#undef OVERRIDE

// extra overrides not in `OVERRIDE_DEF` to be included in the array
#ifndef OVERRIDE_EXTRA
#  define OVERRIDE_EXTRA
#endif

// for this step we use `OVERRIDE` to name the overrides to put in the array
#define OVERRIDE(name, initializer) &override_##name,
const key_override_t **key_overrides = (const key_override_t *[]){OVERRIDE_DEF OVERRIDE_EXTRA NULL};
#undef OVERRIDE

I think it makes the overrides definition much more concise and easy to maintain!

You are welcome to use or modify this in any way you like, if you wish to include it in the pr or documentation.

Thanks again for this awesome feature!

@drashna
Copy link
Member

drashna commented May 22, 2021

Also, could you add the function here:
https://github.com/qmk/qmk_firmware/blob/master/docs/understanding_qmk.md#process-record

@baffalop
Copy link

baffalop commented May 27, 2021

This looks like a great feature, thank you! Actually I can't believe that with all the features QMK has this is only just a PR — it feels like a natural extension.

I just wanted to suggest a couple of things regarding the negmods option. This might be coloured by the use case that I intend for the feature, but before I had read as far as the negmods initialisers and fully absorbed the implications, I would've assumed that the modifiers would be exclusive. For example, currently I want to move my quote into semicolon position, but still get colon when I type it shifted. But if I'm entering some keyboard shortcut involving quote, eg cmd + shift + ', I would still want it to register as such rather than be translated to cmd + shift + ;.

So two suggestions:

  • I think it'd be very helpful to make clear early on in the documentation that this is not how ko_make_basic works by default, rather than having it tacitly implied by the presence of the negmods option.
  • Maybe provide another initialiser, such as ko_make_exclusive, that sets negative_mods to be ~trigger_mods?

(Sorry if this comment comes a bit late. It looks like this might be coming close to approval. Hopefully food for thought though.)

@JonasGessner
Copy link
Author

Thanks for the feedback! I made the docs on ko_make_basic clearer regarding what you mentioned, but I'll keep the code as it is. If more demand for a macro like ko_make_exclusive comes up this can always easily be added later.

@rmurrish
Copy link

rmurrish commented Jun 7, 2021

FYI, that CI error is on includes for a specific keyboard, that was reportedly fixed in #13108.

@drashna drashna self-requested a review June 7, 2021 03:32
@JonasGessner
Copy link
Author

Pinging because cut off for next breaking changes merge is in one month. Would be nice to see this make it for that merge.

@drashna
Copy link
Member

drashna commented Jul 3, 2021

Could you run qmk cformat on the files in question?

@drashna drashna requested a review from a team July 3, 2021 15:19
@JonasGessner
Copy link
Author

The remaining linter errors are on code that this PR doesn't touch.

@drashna
Copy link
Member

drashna commented Jul 13, 2021

Thanks!

@drashna drashna merged commit 52cfc92 into qmk:develop Jul 13, 2021
@Ekleog
Copy link

Ekleog commented Jul 21, 2021

Hello people! I'm new to QMK (currently basing my config off 52ce208) and trying this out (to make “é” capitalize to “É” with my keyboard set as azerty, which means send caps-lock / é / caps-lock so it doesn't give the default “2”).

The way I tried to do it was by using a custom key that outputs “É” with the above-mentioned command sequence in process_record_user, and then use the following code to set my key override:

const key_override_t seacu_key_override = ko_make_basic(MOD_MASK_SHIFT, FR_EACU, EK_SEACU);
const key_override_t **key_overrides = (const key_override_t *[]){
    &seacu_key_override,
    NULL
};

I first checked by having a key have the EK_SEACU effect that my process_record_user did what I want. It looks like it works.

But with the key override above, when I try to press shift+eacu, here is what happens:

  • I press shift, X gets “shift pressed”
  • I press and release eacu, X gets “shift release” “shift pressed” (I assume as a result of the key override)
  • I release shift, X gets “shift released”

(and the same happens when looking at the showkey output)

So it looks like my custom keycode's tap_code calls aren't getting output.

Is it expected that this feature doesn't work with custom keycodes? If no, what am I doing wrong? (Bonus points if there's an easier way to do what I'm trying to do)

Anyway, thank you for QMK, I'm still very new to it (flashed for the first time yesterday) but it looks awesome to me already, and this PR looks like exactly what I was looking for!

@Ekleog
Copy link

Ekleog commented Jul 22, 2021

Oh. I was pointed to the answer, and well, looks like it was hidden in the github-hidden comments, sorry for the noise! The answer is ko_make_basic can't do this and it must go through the full syntax, as explained in #11422 (comment)

@drashna drashna mentioned this pull request Jul 23, 2021
4 tasks
Cu3PO42 pushed a commit to Cu3PO42/qmk_firmware that referenced this pull request Sep 15, 2021
nhongooi pushed a commit to nhongooi/qmk_firmware that referenced this pull request Dec 5, 2021
@Chikowitz
Copy link

Thanks for the fix. This almost solves my use-case, there's still some code that I need to write to handle dead keys and shifted dead keys (I used a macro for these previously), but it greatly simplifies the keymap at least.

I think a viable solution is to have a custom keycode that checks the modifiers and returns true if the modifiers that the override triggers on are set, causing a wee bit of code duplication, but still much cleaner than my old solution.

@dnaq hello, can you maybe share an example of how did you solve your problem? I think I'm trying to do the same.

BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
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.