Proper customization at the keymap level is impossible due to _user() functions either being handled incorrectly or not returning a bool.

[guyyst]

Incorrect handling of bool _user() functions

This is the advice given when writing a custom _kb() function, straight from the docs:

For process_record_kb() for example, that description would be roughly equal to this code structure:

bool process_record_kb(uint16_t keycode, keyrecord_t *record) {
    if (!process_record_user(keycode, record)) {
        return false;
    }
    // Do _kb stuff only if process_record_user() didn't already handle the key event.
}

Yet all implementations I could find of process_record_kb() look like this:

bool process_record_kb(uint16_t keycode, keyrecord_t *record) {
    // Alway do _kb stuff, and potentially even skip process_record_user() with early return.
    return process_record_user(keycode, record);
}

As a results you have to change code at the keyboard level if you want proper control at the keymap level.

No option of overwriting non-bool _kb() functions

The second part of this (which might even deserve its own issue), is that a lot of _user() and _kb() functions simply don't return a bool, which makes it impossible for the keyboard level to know if the user level already handled that functionality.

This includes things like void encoder_update_kb(), void matrix_init_kb(), void raw_hid_receive_kb(), void matrix_scan_kb() and a bunch more.

[guyyst]

Regarding the second part of the issue, just grepping qmk for all _kb() functions yields the following:

bool led_update_kb(led_t led_state)
bool music_mask_kb(uint16_t keycode)
bool process_action_kb(keyrecord_t *record)
bool process_record_kb(uint16_t keycode, keyrecord_t *record)
layer_state_t default_layer_state_set_kb(layer_state_t state)
layer_state_t layer_state_set_kb(layer_state_t state)
void eeconfig_init_kb(void)
void encoder_update_kb(uint8_t index, bool clockwise)
void keyboard_post_init_kb(void)
void keyboard_pre_init_kb(void)
void led_set_kb(uint8_t usb_led)
void matrix_init_kb(void)
void matrix_scan_kb(void)
void raw_hid_receive_kb(uint8_t *data, uint8_t length)
void reset_keyboard_kb()
void rgb_matrix_indicators_kb(void)
void suspend_power_down_kb(void)
void suspend_wakeup_init_kb(void)
void via_init_kb(void)

Almost all of the problematic ones return void and could be easily updated to bool without breaking anything. layer_state_set_kb and default_layer_state_set_kb will require some more thought as they already return layer_state_t.

[skullydazed]

This is definitely something ripe for improvements. I don't have comprehensive answers to everything you raise here, but I've started to work on at least one piece of it: https://github.com/qmk/qmk_firmware/tree/refactor_process_record_kb_user

This will ensure that the _user functions are run if the _kb function returns true, which matches the behavior in situ for most keyboards. The rest of the keyboards do not run the _user functions at all, and this branch rectifies that.

However, this doesn't solve the void return problem yet. I want to solve that, but given the size of this branch already that should be a separate PR.

[guyyst]

Looking over your branch, it seems you're intentionally not trying to address the order and dependency of the two functions, right?

Because given the requirement by the docs to only execute _kb if _user returns true, shouldn't quantum.c#L220 change from this:

[ ... ]
            process_record_kb(keycode, record) &&
            process_record_user(keycode, record) &&
[ ... ]

to this:

[ ... ]
            process_record_user(keycode, record) &&
            process_record_kb(keycode, record) &&
[ ... ]

But assuming your branch is meant to be a non-breaking change the current order makes sense.

[skullydazed]

That's part of what we'll need to work out with this branch. I initially had it the second way, but the sheer number of FIXME notes I was having to leave about the behavior changing, combined with the fact I couldn't find a single case where the latter actually happened, led me to codify the situation as it currently exists.

Once we're sure everything is working ok this way we can start to figure out how to change it from there.

Docs are a separate but parallel issue. We'll probably need to change those no matter which direction we go here.

[vomindoraan]

With the current implementation, it make senses for process_record_kb() to depend on process_record_user(), not the other way around. In other words, the former shouldn't run if the latter returns false. The use case is being able to override the behavior of built-in keycodes or do pre-processing on them (post-processing is covered by post_process_record_user()). Changing the order from

process_record_user(keycode, record) &&
process_record_kb(keycode, record) &&

to

process_record_kb(keycode, record) &&
process_record_user(keycode, record) &&

would break this behavior in existing user implementations and remove the ability to do pre-processing for built-in keycodes.

Examples: https://github.com/qmk/qmk_firmware/blob/d7b381128e503a57315261841c66c13d3ab5be25/users/arkag/arkag.c#L515-L525 https://github.com/qmk/qmk_firmware/blob/d7b381128e503a57315261841c66c13d3ab5be25/users/spacebarracecar/spacebarracecar.c#L85-L92 https://github.com/qmk/qmk_firmware/blob/d7b381128e503a57315261841c66c13d3ab5be25/users/konstantin/konstantin.c#L81-L96 https://github.com/qmk/qmk_firmware/blob/d7b381128e503a57315261841c66c13d3ab5be25/keyboards/contra/keymaps/maxr1998/keymap.c#L139-L148 https://github.com/qmk/qmk_firmware/blob/d7b381128e503a57315261841c66c13d3ab5be25/keyboards/ergodox_ez/keymaps/hacker_dvorak/user/process_record_user.c#L51-L58 https://github.com/qmk/qmk_firmware/blob/d7b381128e503a57315261841c66c13d3ab5be25/keyboards/xd60/keymaps/Jos/keymap.c#L179-L227

[skullydazed]

With the current implementation, it make senses for process_record_kb() to depend on process_record_user(), not the other way around. In order words, the former shouldn't run if the latter returns false.

Yes, this is what we want to have happen. The reality as currently implemented by pretty much every keyboard is different. Most keyboards do not make running process_record_kb() conditional on process_record_user() returning true. Either they don't run process_record_user() at all or they run it unconditionally at the end of process_record_kb().

[3geek14]

It seems like it would be a fairly simple (though potentially large) change to modify existing boards to do what the documentation says, for functions which return bools. This would be a breaking change, but might be worthwhile. Would it be useful for me to take that on?