Skip to content

framework: honor process_record_user() return value - #55

Open
MarkAtwood wants to merge 1 commit into
FrameworkComputer:fl16-2026-f9from
MarkAtwood:fix-honor-process-record-user-return
Open

framework: honor process_record_user() return value#55
MarkAtwood wants to merge 1 commit into
FrameworkComputer:fl16-2026-f9from
MarkAtwood:fix-honor-process-record-user-return

Conversation

@MarkAtwood

Copy link
Copy Markdown

process_record_kb() in keyboards/framework/framework.c calls process_record_user() but discards its return value:

bool process_record_kb(uint16_t keycode, keyrecord_t *record) {
  process_record_user(keycode, record);
  ...

so a keymap returning false cannot stop a key from being processed. The usual QMK idiom is to return early when the keymap reports it has handled the key.

Impact

framework.c is shared by every input module (ansi, iso, jis, copilot, numpad, macropad), so this affects all of them. Six shipped keymaps already rely on the return value to consume FN_LOCK:

  • ansi/keymaps/default, ansi/keymaps/advanced
  • iso/keymaps/default, iso/keymaps/copilot
  • jis/keymaps/default
  • copilot/keymaps/default

To be precise about severity: for FN_LOCK specifically the practical effect may be benign, since it is a custom keycode that nothing downstream emits. The general case is not — a keymap that suppresses a standard keycode gets no suppression at all, silently.

How I hit it

I added a raw-HID key-event stream to a framework/macropad keymap, where the firmware reports (row, col, pressed) to a host daemon and must send no keycode for that press. Returning false from process_record_user had no effect, which is what led me here.

Testing

Built framework/macropad:default and flashed it on a Framework Laptop 16 RGB Macropad. With the fix, returning false from process_record_user correctly suppresses the keycode; returning true behaves exactly as before. FN Lock behavior on the ANSI keymap is unchanged.

Base branch

Based on fl16-2026-f9 as the most recently updated branch carrying this code. The same line is present on fl16-2025 and fl16-2026-remap-keys — happy to retarget or open additional PRs if you would prefer it land elsewhere.

🤖 Generated with Claude Code

process_record_kb() called process_record_user() but discarded its result,
so a keymap returning false could not stop a key from being processed.

Every framework input module shares this function, and six shipped keymaps
already return false to consume FN_LOCK (ansi/iso/jis/copilot defaults,
ansi/advanced, iso/copilot) — so keymap-level suppression silently does
nothing today.

Return early when the keymap reports it has handled the key, matching the
standard QMK idiom.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@MarkAtwood

Copy link
Copy Markdown
Author

One trade-off worth raising myself, since it's a judgement call rather than a clear-cut fix.

Returning early is the conventional QMK shape, but it means that when a keymap does suppress a key, the remainder of process_record_kb() no longer runs for that press:

  • detected_host_os() / set_bios_mode(true)
  • the keymap_config.nkro = 1 enable
  • handle_bios_hotkeys()

In practice that housekeeping runs on every non-suppressed keypress, so state corrects itself on the very next key, and today only FN_LOCK returns false in-tree. But if the discard was deliberate — to keep that housekeeping unconditional on every press — then the preferred shape is probably to keep running and honor the result at the end instead:

bool process_record_kb(uint16_t keycode, keyrecord_t *record) {
  bool handled = process_record_user(keycode, record);

  /* ... existing OS detection, NKRO, BIOS hotkeys ... */

  return handled ? true : false;   /* combined with the existing return paths */
}

That variant is a slightly larger diff because the function has several intermediate return false paths that would need to compose with it, which is why I sent the smaller idiomatic version first.

Happy to switch to whichever you prefer, or to drop the change entirely if the current behavior is intentional and I've misread it.

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.

2 participants