gedankenexperimenter / Kaleidoscope

Firmware for the Keyboardio Model 01 and other keyboards with AVR or ARM MCUs.
http://keyboard.io
GNU General Public License v3.0
0 stars 1 forks source link

More event handler hooks #21

Open gedankenexperimenter opened 6 years ago

gedankenexperimenter commented 6 years ago

I want at least one more, possibly two more, event handler hooks. Especially one for functions that won't change the event key value (so it won't be passed by reference), meant to run after it's been set. Some plugins will even run more than one event handler hook (e.g. Unshifter, so it can accurately count the number of real shift keys held, though this requires the further guarantee that the event will not be aborted).

gedankenexperimenter commented 6 years ago

There are a number of plugins that will change the Key value of event.key in event handler hook functions. What prompted this was a need for an accurate count of values in active_keys that meet certain criteria. If Unshifter's event handler hook precedes some other plugin's hook, then that plugin changes event.key from some other value to shift (or vice versa), Unshifter would have the wrong count of shift keys held.

This is a specific instance of the more general issue of plugin order as it relates to changing the value of event.key. There are lots of plugins that do (or might) change that value in an event handler, and in some cases, they might change it to a Key variant belonging to another plugin.

This leaves us with a problem, because if we change a TapDance key to a Macros key, but Macros comes first in the event handler order, then TapDance would have to do more than just update the event.key value; it would need to call handleKeyswitchEvent(). But if we use checkCaller(), that still results in Macros ignoring the event. If we don't use checkCaller(), then we open the door to infinite loops.

To address that problem, we would need to do at least two things: First, plugins would need to set a boolean flag to indicate that they've called handleKeyswitchEvent(), so they could treat the next event as one that should be ignored (whereas the plugins before it should be starting over). Second, because of plugins that care about physical events and/or delaying and preserving the order of events (i.e. Qukeys), we might need to add an injected flag again, so Qukeys can ignore those events that are simply starting over. The former flag has a problem, because it needs to be reset every cycle, not just in the event handler, in case a plugin aborts event handling — but then Qukeys is a problem again. Maybe.

gedankenexperimenter commented 6 years ago

An alternative would be a system where the Controller is able to examine event.key, and determine which plugin to call (after Qukeys and similar plugins) before calling other event handlers. This still needs a solution to the infinite-loop problem, but circular Key references could just be considered a Henny Youngman bug.

This might be done by breaking up the event handlers into groups:

  1. physical keyswitch handlers
  2. Key variant resolution
  3. normal event handlers
  4. bookkeeping handlers

During Key variant resolution, the controller would iterate through the plugin array, and call each plugin's handleKeyVariant(event) function. That function would return true if that loop should restart, possibly skipping the current plugin on the next pass to minimize obvious infinite loops (but not preventing them entirely). Maybe it would be good to use an event serial number to let the plugins determine if they're getting the same event twice during Key variant resolution.

gedankenexperimenter commented 6 years ago

Now I'm thinking of punting on the general solution, and declaring that built-in Key variants are supported, but other plugin Key variants will depend on plugin order. Recommended order:

Qukeys comes first because when there's a queue, all keypress events are deferred, and those keys don't even get looked up until they're flushed from the queue, so it's not valid for other plugins to take any action until after the events are flushed.

Next comes Leader — it intercepts keypresses and doesn't pass them along to other plugins, so it's best if they don't get those events. The events it does get do need to have correct values, though. This means that if a TapDance key (for example) is pressed following a Leader key, it won't get interpreted as the user might expect.

Next comes TapDance, which intercepts some events, but instead of simply making them disappear, it compresses multiple events into one. TapDance could come before Leader — they're very similar — but I think it makes more sense for them to be in this order. In both cases, they might change the event.key value to a Macros Key variant, so they should both come before Macros.

Next comes Macros. It doesn't make sense to have Macros change the event.key value to a Qukeys, Leader, or TapDance Key, but it does make sense to end a Macro with a Glukey, and of course any of the other types that result in HID reports. In fact, Macros should perhaps work by calling handleKeyswitchEvent(caller = this) in order to produce output, rather than bypassing the other plugin event handlers. It could also (or alternatively) include a small supplemental virtual keyboard (e.g. an 8-Key version of active_keys[]) to allow Macros to use multiple keys simultaneously and even persistently.

After that comes Glukeys. All it needs to do is change Key values to normal keys; it doesn't make much sense to have a Glukey that evaluates to any of the previous plugin Key variants, but it definitely makes sense to have one that evaluates to a mouse key.

Last comes Squeakeys. This plugin doesn't need to change the value of a key to anything else; it just provides the interface to the various mouse controls, and clearly needs to come after all the aforementioned.

Beyond that should be a bookkeeping hook point, useful at least for Unshifter. Speaking of which, I'm not sure where it fits in yet.

gedankenexperimenter commented 6 years ago

Regarding where to put Unshifter in the event handler order:

I can come up with reasons for placing it ahead of both TapDance and, to a lesser extent, Leader. I think it would be quite reasonable, if a bit odd, to have a key that's a TapDance key unshifted (to output different braces), but some other symbol (or even a different TapDance key) when shift is held.

Likewise, it would be reasonable to define a Leader sequence including an Unshifter key; it behaves just like a normal aspect of the keymap, so the user ought to be able to think of it that way, including in Leader & TapDance sequences. The other plugins are quite different in that regard.

gedankenexperimenter commented 6 years ago

Aha! I've got a better place for the "bookkeeping" hooks: after the report is sent. Then, if the report isn't sent (i.e. sendReport() isn't called), nothing will have been added to active_keys, and it's okay not to call those bookkeeping functions, because some plugin suppressed the event and made it as if it didn't happen. So the new name for this hook is postReportHook. It should work for Unshifter because it can't need to resolve both a new shift event and an Unshifter Key at the same time.

gedankenexperimenter commented 6 years ago

Other plugins (e.g. LED effects based on key events) can also use the postReportHook; they don't need to affect the HID output to the host, so this hook point makes it clear.

gedankenexperimenter commented 6 years ago

…because we still pass the event object as a parameter to postReportHook(), but not by reference, and we don't return true or false — every plugin's function runs unconditionally.

gedankenexperimenter commented 6 years ago

preReportHook should get called from Controller::sendKeyboardReport(), after the report has been built from active_keys, but before the report is sent to the host.

postReportHooks should get called from Controller::handleKeyswitchEvent() so it can get the event info as a parameter. This might be a problem, because sendKeyboardReport() could be called from someplace else, but I still think it's the best system.