[phraktal]

Objective

Fixes #26188

Bevy 0.20.0 was releasing remapped modifier keys (different physical position and logical meaning) immediately.

Solution

1. Use key's logical meaning during modifier-state reconciliation.
2. Preserve key codes
3. Recovery of missed presses and releases preserved.

Out of scope

1. ordinary non-modifier remappings, such as A -> B

Testing

1. Tests pass on Linux and macOS (M3)
2. An early workaround version of this heavily tested in game (manual testing).
3. Tested this PR in game manually.

AI Disclosure

Code and automated tests were written using AI. Human reviewed by rust-challenged dev.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide, as well as our policy regarding AI usage, and we look forward to reviewing your pull request shortly ✨

[alice-i-cecile]

I want this fixed, and from what I can tell this does work. I think we can meaningfully simplify though :)

The extra module is fine, because bevy_winit needs to be less of a ball of tangled mess.

However:

1. We should simplify and cut down the tests here to reduce line count and overlap.
2. reconcile_modifier_group is buried in state.rs rather than living with its sibling code.
3. We can use ButtonState instead of bool to better communicate intent.
4. The map walking logic is over-elaborate (and no longer uses a deterministic order because of the HashMap). Here's a draft from my own text elemental (Opus 5.5) for the body of reconcile_pending_modfiers:

let state = mods.state();
let groups = [
    (Key::Super, state.super_key(), [(KeyCode::SuperLeft, mods.lsuper_state()), (KeyCode::SuperRight, mods.rsuper_state())]),
    (Key::Control, state.control_key(), [(KeyCode::ControlLeft, mods.lcontrol_state()), (KeyCode::ControlRight, mods.rcontrol_state())]),
    (Key::Alt, state.alt_key(), [(KeyCode::AltLeft, mods.lalt_state()), (KeyCode::AltRight, mods.ralt_state())]),
    (Key::Shift, state.shift_key(), [(KeyCode::ShiftLeft, mods.lshift_state()), (KeyCode::ShiftRight, mods.rshift_state())]),
];

// Recover presses we never saw (only macOS attests per-side state).
for (logical_key, _, sides) in &groups {
    // A remapped key (e.g. CapsLock -> Control) may already hold this group,
    // and macOS may attest it as a canonical side: don't duplicate it.
    let held_by_remap = pressed_keys.0.iter().any(|(code, key)| {
        key == logical_key && !sides.iter().any(|(side, _)| side == code)
    });
    if held_by_remap {
        continue;
    }
    for &(key_code, side_state) in sides {
        if side_state == ModifiersKeyState::Pressed && !pressed_keys.0.contains_key(&key_code) {
            pressed_keys.0.insert(key_code, logical_key.clone());
            synthesized.push(KeyboardInput {
                key_code,
                logical_key: logical_key.clone(),
                state: ButtonState::Pressed,
                repeat: false,
                window,
                text: None,
            });
        }
    }
}

// Release recorded keys whose *logical* modifier is provably up.
let released = pressed_keys.0.extract_if(|&key_code, logical_key| {
    // Non-modifiers, AltGraph and character remaps are never reconciled.
    let Some((_, group_pressed, sides)) = groups.iter().find(|(key, ..)| key == logical_key) else {
        return false;
    };
    let any_side_pressed = sides.iter().any(|&(_, s)| s == ModifiersKeyState::Pressed);
    // Per-side state only describes the canonical key of this group.
    let side_released = sides.iter().any(|&(side, s)| {
        side == key_code && s == ModifiersKeyState::Unknown && any_side_pressed
    });
    !group_pressed || side_released
});
synthesized.extend(released.map(|(key_code, logical_key)| KeyboardInput {
    key_code,
    logical_key,
    state: ButtonState::Released,
    repeat: false,
    window,
    text: None,
}));

The core idea is pretty simple: we can just rely on logical keys, and save the repeated, complex scans.

[phraktal]

Most of what you've suggested in your draft code was implemented. Two points that changed:

1. Recovered presses - Collect them first, but apply and emit them after releases - Ensures the final logical modifier state is correct when switching between left and right keys.
2. Release order - Sort releases by physical key code - HashMap iteration has no guaranteed order. Sorting makes the emitted sequence predictable.

[alice-i-cecile]

Release order - Sort releases by physical key code - HashMap iteration has no guaranteed order. Sorting makes the emitted sequence predictable.

Would it be possible to use a BTreeMap or something here? Those are ordered by default, but IDK if the underlying type is Ord off the top of my head.

[alice-i-cecile]

Proper private type with named fields please, not type alias.

[alice-i-cecile]

Better! There's a type alias style nit to fix up (type aliases and complex tuple types suck to try and reason about / maintain), and I think we can make our testing setup better still:

1. We should pick one of the state.rs integration test or the module-level InputPlugin test, not both: these overlap heavily.
2. The unit tests do a bad job isolating failures here, because they're basically just a pile of exhaustive test cases. I don't think they map well onto the shape of real bugs as a result. Can we have a few simpler tests, focusing on the precise invariants we want to test, with less "test every permutation"? Something like "releasing keys works properly", "works regardless of side" etc.

[alice-i-cecile]

Awesome, thanks for your patience on this :)

[phraktal]

Awesome, thanks for your patience on this :)

Great project - Happy to help !