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

Autoconfig Requirement API discussion #217

Draft
wants to merge 3 commits into
base: v8
Choose a base branch
from

Conversation

MattSturgeon
Copy link
Contributor

I'm opening this Draft PR hoping to get some feedback and spark discussion on the user-facing annotation API, hopefully before I spend too much time implementing the backend for it...

Each commit takes a slightly different approach. I also have some other examples in some different branches (see below)...

1

The first commit takes a simplistic approach.

All requirement annotations must reference a handler method and optionally provide arguments to it.
Additionally, DefaultRequirements handlers are provided to prevent users needing to re-implement common requirement handlers such as simple equality checks.

This can lead to somewhat clunky annotation declarations to do fairly simple requirements:

// Enabled when coolToogle is `true`
@ConfigEntry.Requirements.EnableIf(
        @Requirement(
                value = @Ref(cls = DefaultRequirements.class, value = DefaultRequirements.TRUE),
                refArgs = @Ref("coolToggle")
        )
)

2

The second commit attempts to improve the UX by introducing a concept of "simple" requirements. These are requirements that reference a Config Entry GUI instead of an actual handler method.

Simple requirements would be built into actual requirements during dependency setup. We would need to follow the requirement reference looking for a method handler, and then try again looking for a config entry field if no method exists.

// Same example as above
@ConfigEntry.Requirements.EnableIf(
        @Requirement(@Ref("coolToggle"))
)

3

The third commit removes the separate @Requirement annotation that was used by both @EnableIf and @DisplayIf and instead moves all of its parameters into each of those.

This is a bit of a painful compromise since it leads to having to maintain the same interface in two places, however it significantly improves the user experience by reducing boilerplate from dependency annotations.

In my opinion it's well worth it:

// Same again
@ConfigEntry.Requirements.EnableIf(@Ref("coolToggle"))

Other work

Other examples are in the original draft PR and my newer autoconfig dependencies branch

A `Requirement` can now reference either a Config Entry or a handler method.
Instead of having `Requirement` be a separate annotation, simplify the UX by copying its parameters directly to both `EnableIf` and `DisplayIf`.

This is kinda painful, since it leads to effectively duplicated annotation code, however it dramatically cleans up the user experience (as seen in `ExampleConfig`)...
@MattSturgeon MattSturgeon changed the title Example requirement api Autoconfig Requirement API discussion Aug 2, 2023
@MattSturgeon
Copy link
Contributor Author

@shedaniel do you have any thoughts on this and #215 or should I assume this is heading in the right direction and continue?

@KingContaria
Copy link

would it be possible to add some way to define a different translation key for when the option is disabled by requirement?

something like this:

@ConfigEntry.Requirements.EnableIf(
        @Requirement(@Ref("coolToggle"))
)
@ConfigEntry.Requirements.CustomDisabledTranslation
public boolean coolOption;

with a language file like this:

"text.autoconfig.modid.option.coolOption.@CustomDisabledTranslation": "Disabled"

additionally cloth config could even just add its own translation string for "Disabled" (in grey i suppose) which could be used with @ConfigEntry.Requirements.DisabledTranslation

@MattSturgeon
Copy link
Contributor Author

@KingContaria something like that would be possible, but it'd be better tackled as a separate issue because it'll also need changed on the cloth-config side, not just autoconfig.

@KingContaria
Copy link

that makes sense, i might make a fork for it once this pr is finished then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants