-
-
Notifications
You must be signed in to change notification settings - Fork 40.2k
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
[New Feature] Key Overrides #11422
Conversation
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:
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):
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. |
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 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 |
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. |
583ecaf
to
edcaa3d
Compare
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):
|
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 ( 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 // / -> !
const key_override_t slsh_override = ko_make_basic(MOD_MASK_SHIFT, SE_7, SE_EXLM); |
Alright, thank you. As for my code snippet I do have to use |
@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 I thought you had the keycode |
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. |
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 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! |
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 I would consider leaving this PR as-is for now, and perhaps adding a flag like 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/ |
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! |
Hi Jonas, thanks for your work ! Is it possible to override a mod-tap key ? |
Sorry about that, GitHub decided to delete the |
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 |
3852993
to
11ef7d8
Compare
Thanks, works fine !
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 : |
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. |
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. |
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, // 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! |
Also, could you add the function here: |
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 So two suggestions:
(Sorry if this comment comes a bit late. It looks like this might be coming close to approval. Hopefully food for thought though.) |
Thanks for the feedback! I made the docs on |
FYI, that CI error is on includes for a specific keyboard, that was reportedly fixed in #13108. |
Pinging because cut off for next breaking changes merge is in one month. Would be nice to see this make it for that merge. |
Could you run |
The remaining linter errors are on code that this PR doesn't touch. |
Thanks! |
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 But with the key override above, when I try to press shift+eacu, here is what happens:
(and the same happens when looking at the So it looks like my custom keycode's 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! |
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 |
@dnaq hello, can you maybe share an example of how did you solve your problem? I think I'm trying to do the same. |
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 fromprocess_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 tolayer_on
takes about 10ms on my keyboard.Types of Changes
Issues Fixed or Closed by This PR
Checklist