Other keys "virtually" being held until TapHold resolves

[dariogoetz]

This might be related to what @TeXitoi wrote in #25.

I observe multiple key activations in my OS after finishing words, e.g. "timeee ".
After lots of debugging, I think that I can pinpoint the problem:

If a key is pressed and a TapHold key pressed afterwards (in my case, the "space" key), but before releasing the previous key, the release of the previous key gets delayed until the TapHold is resolved.

Example (TapHold timeout of 200 ms, OS repeat after 220 ms):

  0 ms: press "a"       -> "a" appears in the OS
 70 ms: press "space"   -> no change in OS
 80 ms: release "a"     -> no change in OS
220 ms: ---             -> another "a" appears
225 ms: ---             -> another "a" appears
...                     -> multiple "a" appearing
...
240 ms: release "space" -> "space" appears in the OS after the multiple "a"s

I use a rather low repeat delay in my OS (220 ms, I believe), so if I am a little lazy on the space key, I get the aforementioned multiple quite easily.

[dariogoetz]

I did some research...
The problem can be avoided by checking all stacked events for releases corresponding to existing states before processing the self.waiting in the tick method.
I am not entirely sure of the consequences this has on other logic and I am quite sure that it is not efficient and kind of dirty, but the following at least solves my issue (insert after self.tap_hold_tracker.tick(); and before match &mut self.waiting { in layout.rs):

        // release already pressed keys with stacked events
        let mut filtered_stacked: Stack = ArrayDeque::new();
        let mut instant_release: Option<CustomEvent<_>> = None;
        for s in self.stacked.iter() {
            match s.event {
                Event::Release(i, j) => {
                    // check if the stacked release corresponds to an existing state
                    let mut custom = CustomEvent::NoEvent;
                    self.states = self
                        .states
                        .iter()
                        .filter_map(|s| s.release((i, j), &mut custom))
                        .collect();

                    if let CustomEvent::Release(_) = custom {
                        // there is an existing state that can be released by the stacked event
                        // -> do not put on stack again
                        instant_release = Some(custom);
                        break;
                    } else {
                        // no existing state can be released by the stacked event
                        // -> put back on stack
                        filtered_stacked.push_back(s.clone());
                    }
                }
                Event::Press(_, _) => {
                    // we only care about releases for existing states
                    // -> put back on stack
                    filtered_stacked.push_back(s.clone());
                }
            }
        }
        // the filtered_stack does not contain the first release event corresponding to an existing state
        self.stacked = filtered_stacked;
        if let Some(event) = instant_release {
            // each tick only generates one event
            // -> return early
            return event;
        };

Also, this requires Stacked to implement Clone.

[TeXitoi]

I'll have to read that carefully. I'm on vacation right now. I think you're right on the diagnostic, but I must think on your proposition.

[dariogoetz]

No hurry. Enjoy your vacation.

For when you have time:
I found a somewhat more elegant solution:

• add another Stack to Layout (e.g. called stacked_no_wait) for those events that shall not be waited for (in particular those that release existing states)
• in the event method, check if a new event releases an existing state and in that case put it into the stacked_no_wait queue instead of the stacked queue

    pub fn event(&mut self, event: Event) {
        let releases_existing_state = match event {
            Event::Release(i, j) => self
                .states
                .iter()
                .find(|s| s.release((i, j), &mut CustomEvent::NoEvent).is_none()) // check if it releases an existing state
                .is_some(),
            Event::Press(_, _) => false,
        };

        if releases_existing_state {
            if let Some(stacked) = self.stacked_no_wait.push_back(event.into()) {
                self.unstack(stacked);
            }
        } else {
            if let Some(stacked) = self.stacked.push_back(event.into()) {
                self.waiting_into_hold();
                self.unstack(stacked);
            }
        }
    }

• in the tick method pop the stacked_no_wait queue and potentially exit early before processing the self.waiting event (instead of the complicated logic from my previous post)

        // unstack events from the "stacked_no_wait" queue
        if let Some(s) = self.stacked_no_wait.pop_front() {
            return self.unstack(s);
        }

This way, the search through the self.states only happens when a new event is registered. Also, Stacked does not need to implement Clone. Instead, the already existing unstack logic is used.

Edit: It does fail a test, though.
The failing test makes sense, although it expects the exact opposite of what I am trying to achieve (only for a key, that acts as a modifier - the hold-action of a tap-hold).

When two hold-taps are consecutively pressed and the first one turns out to be a "hold" and is released before the second one is decided whether it will be a "tap" or a "hold", the "hold" action of the first one will be "held" until the second one is decided. And exactly that "held until the second one is decided" is, what I am trying to avoid for "non-modifier" keys, but which arguably makes sense in the test-case of modifier-keys (the "hold"-action of the hold-tap).

[dariogoetz]

If one only puts those events into the "no wait" queue that correspond to KeyCode states that are not a modifier, the tests do not fail.
That means that modifiers (and LayerModifiers and Custom states) are held until the hold-tap resolves, whereas "normal" keys are released immediately.

    pub fn event(&mut self, event: Event) {
        let releases_existing_state = match event {
            Event::Release(i, j) => self
                .states
                .iter()
                .find(|s| {
                    s.release((i, j), &mut CustomEvent::NoEvent).is_none() // state can be released
                        && s.keycode().map_or(false, |k| !k.is_modifier()) // state is a KeyCode that is not a modifier
                })
                .is_some(),
            _ => false,
        };
        ...

I'll set up a WIP PR for it once the PR #104 is merged (my sample code is based on the "Queue"-variant).