MixinKeyboard leaks the pressed event.

[blackd]

I'm not sure if this is the right place to report this or I need to go to all mods that rely on it but here we go.

So I got this issue reported blackd/Inventory-Profiles#66 which states my keyboard shortcuts stop working when MiniHUD is shown or hidden. I've also tested with Litematica and Tweakeroo.
I've narrowed it down to the GLFW_PRESS event not being cancelled. So any mixin attached at latter point in Keyboard.onKey, like mine at TAIL, will receive the GLFW_PRESS event but not GLFW_RELEASE.

May be the best approach is to not cancel the events to allow other mods to use the same keys. It seams vanilla works that way and you can have multiple actions bind to the same key.

[maruohon]

From you issue report, it sounds like using a MiniHUD hotkey for example "permanently" breaks your keybinds? Can you elaborate how those work? Although I'm not sure it's relevant to this issue.

From looking at my Mixin and input handling code, it seems to be pretty simple and looks correct ie. how I would have expected it to be implemented. The way the malilib keybinds work, is that they will usually cancel only the single event where the full keybind combination is completed and the callback gets triggered. So if the keybind is set to trigger on press, then the press event for the last key in the hotkey combination will be cancelled, but any of the previous keys or the release event will not.

All the individual keybinds also have the "advanced" keybind settings where they can be configured as to whether or not to cancel further processing. And in the rewrite I'm working on (of all the client mods, but here related to malilib code) I did already change the most common/default advanced keybind setting to not cancel further processing, whereas in the current release versions the default setting is to cancel further processing (of that last triggering key event).

So from my perspective things are working as designed... but whether or not that is a good thing I'm not sure of now? Should I move the cancellation point somewhere so that it would more likely only cancel vanilla handling, and let other mods easier inject their handling hooks before the cancellation point? I don't remember off the top of my head what the vanilla code there looks like and how tricky it would be to cancel it somewhere else.

[blackd]

Yes the MiniHUD and permanently breaks my hotkeys because I also have support for multiple keys for hotkey binding. For example r+c by default opens my configuration (I guess the original author got inspired by your mods).

So because I receive GLFW_PRESS for "h" in this example but don't receive the GLFW_RELEASE my handler continues to think "h" is pressed which breaks all of my hotkeys.

[maruohon]

Looking at the vanilla onKey method, it would be very annoying and messy and require several mixins to not cancel the rest of the onKey() method but only the vanilla actions that happen from there. So I don't really want to change how my mixin is set up.

So then there are a few different potential solutions to this. Let me know what you think of these:

• Your mod could use the malilib keybind system. Maybe only if malilib is present to not hard depend on it?
• You could inject at the same point malilib does, but with a higher priority (I always forget if higher or lower priority goes first?) to go before malilib's injection
• malilib uses a method to re-check the held keys, in case it misses the release events sometimes. Maybe you could also use such a method? See:

  malilib/src/main/java/fi/dy/masa/malilib/hotkeys/KeybindMulti.java

  Line 466 in 6444a1d

  public static void reCheckPressedKeys()

  called from:

  malilib/src/main/java/fi/dy/masa/malilib/mixin/MixinMinecraftClient.java

  Line 43 in 6444a1d

  KeybindMulti.reCheckPressedKeys();

Although specifically for example to the MiniHUD main toggle key that triggers on release, that key could probably just be set to not cancel further processing (actually kinda surprised if it does cancel by default, I'd say that's unintentional default settings for it). I take it the issue is mainly keys that trigger on release and cancel further processing?

[blackd]

Some of my functionality works in conjunction with vanilla's actions so moving my processing before vanilla will change this behaviour. For this reason MaliLib and moving the injection point are not options.
So I guess I'm left with the check.
This handles my problem but I still thing it's best to either cancel both press and release despite which of them will trigger the action.

[maruohon]

Canceling on both press and release would probably complicate the malilib key handling code somewhat. And in fact may not even really be possible. For example if the keybind would trigger on release, then you would want to also cancel the press even tof the last key of the combo, but what if the user presses another key after that last key before releasing the keys, and the keybind is set to not allow extra keys. Then it would not be possible to know at the point the key press arrives, whether or not the release will actually trigger the callback.

I'm more thinking that I should simply never cancel any release events. The main point of canceling further processing is to be able to bind stuff to keys that are also used in vanilla, but prevent the vanilla action if the malilib keybind combination triggers. I don't remember anything in vanilla triggering on key releases, so I can't think of any use case where canceling the release event would be useful. Maybe there is some such case somewhere though, so I probably won't hard code/change the handling code. Rather just make sure at least all the default advanced keybind settings are set such that if it triggers on release, then the cancellation is disabled by default.

[blackd]

To be honest the whole input handling with mixins can become quite a mess.

Anyway I have "fixed" it in my mod by tracking press/release events and checking key status if discrepancies are detected. I guess it won't catch 100% of cases but I prefer to do as little as possible processing in critical areas as user input.

I guess this issue can be closed unless you want to keep it for your decision on cancelling release.