Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Key Binds API #59

Draft
wants to merge 57 commits into
base: 1.20
Choose a base branch
from
Draft

Key Binds API #59

wants to merge 57 commits into from

Conversation

EnnuiL
Copy link
Contributor

@EnnuiL EnnuiL commented Dec 2, 2021

A built-from-scratch API that covers Fabric Key Binding API's functionality and more

The PR was planned in two stages: Blue and Pink. The implemented feature set of Blue was:

  • Key binds now support being dynamically disabled, making them completely invisible to the game's eyes
  • The Key Binds screen now shows a tooltip on conflicting keys showing the other key binds that use the same key, and adds a (!) indicator to the key text itself for accessibility reasons
  • The API exposes some methods meant to help with mod intercompatibility

Currently, work is being done on the Pink feature set:

  • A custom config file at ./config/qsl/key_binds.json allowing for our own key bind extensions (WIP)
  • Key binds now accepts key chords, expanding the possibilities for modded key binds (WIP)
  • Key binds will contain key predicates, which essentially points all the use cases where it will be used. This will allow multiple key binds that never overlaps to share the same key

Outdated but relevant information about the stages is available in #61

@TheGlitch76
Copy link
Member

Now is the time for any last-minute bikesheds on where to put the keybindings API, since I know people are unhappy with gui

@OroArmor
Copy link
Member

OroArmor commented Dec 2, 2021

gui isn't perfect, but its the closest large group in my opinion.

you could make an input or client group, but then we end up with 30 different modules

Copy link
Member

@OroArmor OroArmor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I know you talked about supporting some combination keybinds, not sure if that should be an extra PR or not, but its definitely something to discuss further

@TheGlitch76 TheGlitch76 mentioned this pull request Dec 4, 2021
3 tasks
@LambdAurora LambdAurora added this to the Initial release milestone Dec 6, 2021
@LambdAurora LambdAurora added documentation enhancement New feature or request new: library A pull request which adds a new library. new: module A pull request which adds a new module labels Dec 11, 2021
@EnnuiL EnnuiL changed the title Key Bindings API v1 Key Bindings API Dec 25, 2021
@LambdAurora LambdAurora added the library: gui Related to the GUI library. label Jan 24, 2022
@EnnuiL EnnuiL changed the title Key Bindings API Key Binds API Feb 2, 2022
*
* <pre>
* {@code
* public static final KeyBinding EXAMPLE_KEY_BIND = KeyBinding.registerKeyBinding(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* public static final KeyBinding EXAMPLE_KEY_BIND = KeyBinding.registerKeyBinding(
* public static final KeyBinding EXAMPLE_KEY_BIND = KeyBindingRegistry.registerKeyBinding(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh whoops, I'll fix that

import net.minecraft.text.Text;

@Environment(EnvType.CLIENT)
public interface ConflictTooltipOwner {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could either be exposed as API as it could have some functionality or make it slightly clearer that it's internal with @ApiStatus.Internal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I was thinking of replacing the interface mixin with the two accessors that I've done for resource pack tooltips; I think it's much cleaner than the current one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and nevermind, while the accessor does simply a bit of code, ConflictTooltipOwner will still be needed;
I'll mark it as internal

@LambdAurora LambdAurora added the t: new api This adds a new API. label Feb 18, 2022
* @param includeVanilla {@code true} if vanilla entries should be included, else {@code false}
* @return a map containing all modded (and optionally vanilla) key binds
*/
public static Map<KeyBind, Boolean> getAllKeyBinds(boolean includeVanilla) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really should use fastutil's Object2BooleanMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll probably abandon the Map<KeyBind, Boolean> approach once on the Pink stage, because having some sort of QuiltKeyBinds would probably be better than a map between keybinds and the extra info

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably would be a good idea to encapsulate the inner map so the api can be a little more resistant to change.

@LambdAurora
Copy link
Contributor

A custom config file at ./config/qsl/key_binds.json allowing for our own key bind extensions (WIP)

Now that I'm thinking of it, probably should be ./config/quilt/key_binds.json.

@EnnuiL
Copy link
Contributor Author

EnnuiL commented Mar 13, 2022

A custom config file at ./config/qsl/key_binds.json allowing for our own key bind extensions (WIP)

Now that I'm thinking of it, probably should be ./config/quilt/key_binds.json.

I thought about it (after all, FAPI used ./config/fabric/), however, I don't know; I'd think ./config/quilt/ would instead be used by something like the Quilt Loader, and not QSL

@LambdAurora LambdAurora added s: wip This pull request is being worked on. and removed new: library A pull request which adds a new library. labels May 20, 2022
@EnnuiL EnnuiL changed the base branch from 1.18 to 1.19 June 17, 2022 04:46
@EnnuiL EnnuiL removed the enhancement New feature or request label Sep 30, 2022
Oh, this is definitely not ready yet, I just needed an easy way to sync progress between computers
If everything goes right, tomorrow we'll have a PR!
@EnnuiL EnnuiL changed the base branch from 1.19 to 1.20 June 24, 2023 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
library: gui Related to the GUI library. new: module A pull request which adds a new module s: wip This pull request is being worked on. t: new api This adds a new API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants