Key binding context #3286
Open
Key binding context #3286
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
modmuss50
reviewed
Aug 29, 2023
...ng-api-v1/src/client/java/net/fabricmc/fabric/api/client/keybinding/v1/KeyBindingHelper.java
Show resolved
Hide resolved
...api-v1/src/client/java/net/fabricmc/fabric/mixin/client/keybinding/KeyBindingEntryMixin.java
Outdated
Show resolved
Hide resolved
Comment on lines
23
to
28
| public class CustomKeyBindingContext implements KeyBindingContext { | ||
| @Override | ||
| public boolean isActive() { | ||
| return MinecraftClient.getInstance().currentScreen == null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this not the same as IN_GAME? Might be better to have a test that does something slightly different. Just this is is isActive and not conflictsWith. I think the test mod needs expanding to cover more cases.
Maybe also look into automated testing, but that likely requires me to finish: #3168 Unit tests might be possible.
There was a problem hiding this comment.
This is to check whether 2 keys with different context instance work simultaneously.
...pi-v1/src/testmodClient/java/net/fabricmc/fabric/test/client/keybinding/KeyBindingsTest.java
Outdated
Show resolved
Hide resolved
| */ | ||
| @ApiStatus.OverrideOnly | ||
| default boolean conflictsWith(KeyBindingContext other) { | ||
| return this == other; |
There was a problem hiding this comment.
This doesnt feel right for some reason. If I understand this correctly both keybinds must not conflict with each other. So evem if you did return false to have a keybind that could run everywhere it still wouldnt work.
...g-api-v1/src/client/java/net/fabricmc/fabric/api/client/keybinding/v1/KeyBindingContext.java
Outdated
Show resolved
Hide resolved
...ng-api-v1/src/client/java/net/fabricmc/fabric/api/client/keybinding/v1/KeyBindingHelper.java
Show resolved
Hide resolved
provide client instance on `isActive` require explicit impl for `conflictsWith`
modmuss50
approved these changes
Dec 27, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add
KeyBindingContextto register multiple key binds that won't conflict with others.This PR doesn't change vanilla behavior in which only one conflicting key (same context and key) will be registered as pressed.
The order in which what key to functions depends on when the
KeyBindinginstance is created.Do we wantNow it also makes it usable on screen.KeyBinding#isPressedandwasPressedto work when a screen is open?