Recording sequences including OneShot modifiers does not work correctly
cdisselkoen opened this issue · 10 comments
If you attempt to record a keystroke sequence which uses a OneShot modifier (from Kaleidoscope-OneShot), the OneShot will not fire during playback. I'm not sure whether the problem is during recording (that it's not recorded), or during playback (that it's not played back). The problem occurs whether you 'tap' the OneShot, or whether you hold the OneShot - in either case, it will have no effect during playback.
The problem is during playback - specifically, when we inject an event for something like OSM(LeftShift), OneShot doesn't handle it properly because OneShot simply doesn't handle injected keys. I'm not sure yet what the correct fix for this is, or even whether the correct fix is on our end or theirs (or both).
The dynamic macros in QMK work ok with one-shot modifiers, perhaps because it's recording events rather than keys. I'm not yet familiar enough with the Kaleidoscope code to offer more help.
For ease of terminology let's consider a "trigger" (real keypress) which passes through MacrosOnTheFly's hook and then later some other plugin injects one or more keys in response. The current policy is to record "triggers" (which to us just look like any other keypress), and not injected keys, with the idea that during playback we inject the triggers and they are handled just as before.
The general problem here is that some plugins (such as OneShot) won't handle the "trigger" the same way when we inject it, because of the INJECTED flag. For instance, OneShot simply ignores all injected keys.
This problem probably isn't limited to OneShot, it's just only been reported for OneShot. There are probably other plugins that inject keys whose interaction with MacrosOnTheFly is broken. (I haven't thoroughly tested interactions with a lot of other standard plugins.) Worse, the fixes for each of these interactions might not all be the same, or might not even all be compatible. We might need some kind of more general solution.
Some ideas I'm thinking about currently: (see also my brainstorming in IRC)
- Record and play back injected keys as well as their triggers. This should fix this particular issue - during playback if we inject both the trigger and the resulting modifier press, OneShot will just ignore the trigger; and since the trigger in this case is a reserved key, it won't have any other effect. However, this solution would probably break other things.
- OneShot could just always handle its own reserved keys, injected or not. This seems like it would solve at least this special case; maybe other interactions could be fixed similarly, if any others are broken. A downside is this requires all upstream patches rather than fixes here in MacrosOnTheFly - if it's even possible.
- Seems like the ideal solution would be the opposite of the current policy - instead of recording triggers and not injected keys, we should record the injected keys and not their triggers. However, this doesn't seem possible because we can't know what the trigger was for a given injected key. The trigger could even be multiple keys, as in Kaleidoscope-Leader.
I should really do testing to make a concrete list of which interactions with which standard plugins are currently broken. I don't have time today, but hopefully I can get to it soon.
I continue the example of Kaleidoscope-Leader as a representative plugin which uses injected keys.
Kaleidoscope-Leader flat-out ignores injected keys, just like Kaleidoscope-OneShot. This means that, without testing, I'm guessing that our interaction with Leader is broken in the current case and under solutions (1) and (2). In the current case, the behavior is that the leader sequence is played back and not captured by Leader, so it registers as normal keys without invoking the desired action. Under solution (1), same behavior except it also plays back the desired action (in addition to the leader sequence verbatim). Solution (2) only affects OneShot, so interaction with Leader would be the same as in the current case.
It seems like Leader is actually an interesting use-case for automating with MacrosOnTheFly - I could imagine users wanting to record a macro sequence including a Leader invocation, to have it available again at the touch of a button. However, that use-case seems to currently be broken.
I only ran through your IRC brainstorming and the descriptions above, but from these, my impression is that for this particular plugin, dropping the INJECTED flag when re-injecting events would be the way forward. As in, keep the flag if it was set when recording, but don't add it if it wasn't.
This is just a hunch, mind you, and may or may not be a viable solution. Long-term, it makes sense to rethink the whole INJECTED thing, and figure out if we can do better.
Is injecting events without the INJECTED flag not taboo? That could work. I'll have to be careful how MacrosOnTheFly handles its own injected events, but it should work.
No, it's not taboo. Have a look at TopsyTurvy for an example of how to handle only your own injected events. (TT still uses INJECT when inserting its own keys, but does not ignore keys with that flag, and uses its own instead)
Turns out there is one gotcha with respect to injecting keys without the INJECTED flag. Namely, at https://github.com/keyboardio/Kaleidoscope/blob/master/src/key_events.cpp#L40, we see that keyToggledOff events only generate HID releaseKey calls for INJECTED keys; non-injected keys simply don't get release events passed through to the HID layer. Not sure why this behavior? Correct macro playback seems like it needs both press and release events to be reflected to the HID layer; as it is, injecting release events without INJECTED means they're functionally ignored.
All right, so I think the solution here is to mimic the "new scan cycle" process inside MacrosOnTheFly - specifically, releaseAllKeys then re-press held keys. Unfortunately that means instead of blindly replaying key events, we have to track what's down so we can re-press every "cycle", which is going to increase RAM usage.
More complicated fix than I'd like, but it seems to work, at least for OneShot. Commit incoming.