[1.16.x] Re-implement the Config GUI #7395

Cadiboo · 2020-10-04T15:25:57Z

Supersedes #6467.

Re-implements the config GUI
Adds a test mod for the config system
Makes some changes to other Forge classes

This was designed to be super easily extensible by other mods wanting to add extra stuff to their Config GUI.
The test mod serves as an example of how to do this.
More thought was put into this than writing clean or highly performant GUI code.

Forge API changes (no breaking changes)

Increase visibility of ForgeConfigSpec.Range from private to public (To allow use in the Config GUI)
Add a public method to ConfigTracker to get all ModConfig registered to a mod
Add a public method to ModConfig to save and reload it's contents (see <> for why)
Makes some changes/improvements to ForgeConfigSpec (Needs a talk with someone from the Forge team, preferably cpw, preferably on discord)
- Need to talk with one of the Forge team about how translation keys for config categories should be handled
- Remove explicit comment handling and use CommentedConfig instead
- (Minor) Add "allowed values" to the comments of entries created with defineInList

Other changes

Adds the mods list button to the in game menu screen
Add back MessageDialogScreen, a copy of the DisconnectedScreen but with custom text
Changes/improvements to ForgeConfigSpec
- Comment cleanup & typo fixes
- Store "allowed values" for entries created with defineInList and defineEnum for use by the Config GUI
- Make ConfigValues implement their respective Supplier counterparts (Discussed on discord and was super easy to implement)
- Implement the rest of ForgeConfigSpec.Range#toString()
Improve the comments on ModConfig.Type (from [1.15.x] Extend the config system #6464)
Add doc/info comment to FMLConfig (from [1.15.x] Extend the config system #6464)
Formatting fixes to ForgeConfig.java
Comment typo fixes to DistExecutor
Cleanup to the Slider widget
Change UnicodeGlyphButton from rendering the result of getNarrationMessage as it's title to using getMessage
Fixes up some translations for Forge's config

Screenshots:

ichttt

Some early notes. I know this is still a draft and heavy WIP (I saw many Todos :P), so it's just a very rough review from a very quick look

ichttt · 2020-10-04T19:06:01Z

src/main/java/net/minecraftforge/client/gui/config/CategoryConfigScreen.java

+        return new CategoryConfigElementList(this, field_230706_i_, categoryInfo);
+    }
+
+    public interface ConfigCategoryInfo {


Maybe move the inner classes it into their own files? Or is there a good reasoning this is an inner class?

It's a mostly internal thing that is likely change a bit before I merge this PR. I don't see any reason for this to be in it's own class as it's just used like a Record to pass around functions. I should probably make this into a POJO rather than an interface though...

src/main/java/net/minecraftforge/client/gui/config/CategoryConfigScreen.java

ichttt · 2020-10-04T19:12:00Z

src/main/java/net/minecraftforge/client/gui/config/ConfigScreen.java

+ * -> Change indices instead of reloading the entire view
+ * -> Drag and drop movement?
+ * -> Maybe try and refactor it all to be less confusing and twisted
+ * - Client -> Server syncing?


If a server config is being changed, it needs to be synced to the server and all clients. It should also (IMO) only be changeable by owners. So yes, we need syncing, but we already have a packet for this (although we might need a new one for in-game changes to config). Most likely it should also resync when the file is changed by the filesystem, which IIRC it doesn't do right now as well.

If a server config is being changed, it needs to be synced to the server and all clients.
Most likely it should also resync when the file is changed by the filesystem, which IIRC it doesn't do right now as well.

Yes, I was assuming that I would just need to send a packet to the server, it would save & reload the config and then that the server -> all clients syncing was already implemented. If it isn't, it shouldn't be too hard to implement.

It should also (IMO) only be changeable by owners

Definitely, I was thinking changeable by OPs

we already have a packet for this

We have one for Server -> Client sync during the handshake, I think this one could be repurposed for in-game updates but I'm not sure... We definitely need a new one for Client -> Server config update request.
I've left out the code for syncing right now because I know it will be a bit of work to implement.

Definitely, I was thinking changeable by OPs

Would be cool if you would utilize the forge permission api for that, so that people are encouraged to use it more

src/main/java/net/minecraftforge/client/gui/config/ControlCreator.java

maxanier · 2020-10-04T19:58:48Z

I didn't look into the code yet, but I tested this with a mod of mine:
It works great out of the box, only:

If you have multiple configuration specs/files per type only the latter are displayed in the menu.

        ModLoadingContext.get().registerConfig(ModConfig.Type.SERVER, serverSpec);
        ModLoadingContext.get().registerConfig(ModConfig.Type.SERVER, balanceSpec, "vampirism-balance.toml");

Here only options from balanceSpec are displayed

Cannot edit server configuration while hosting a SP world to LAN (maybe that is intentional or would require to much work/hacks)

I might look into the code when have more time

Cadiboo · 2020-10-04T23:25:17Z

If you have multiple configuration specs/files per type only the latter are displayed in the menu.

I'll look into fixing this. A glance at ConfigTracker shows that it wasn't set up to handle multiple configs. In fact, If you run the command /config showfile SERVER you'll see that only one file is printed. I'm not sure you're meant to be using multiple configs like that.

Cannot edit server configuration while hosting a SP world to LAN (maybe that is intentional or would require to much work/hacks)

It's intentional right now. SP non-lan works because there's only one player and I can ignore the syncing (maybe, it might be an issue there too but it appears to work) and just reach across logical sides (ew!). I need to actually add some packets n stuff to make it work in non-singleplayer. See ichittt's comment #7395 (comment).

ChampionAsh5357 · 2020-10-05T16:30:13Z

Are the allowed values only specified on the tooltip and then validated on save? That's what I see anyways. If so, then maybe something similar to ISuggestionHelper in commands should be used on the text field widgets to allow autofill on the fields and to notify the user if an entered text is invalid.

Cadiboo · 2020-10-06T01:41:52Z

Are the allowed values only specified on the tooltip and then validated on save?

The widgets content is validated whenever it changes (and then also on save) but semantics. If it’s invalid, the widget will turn red. I can add some code to also make the tooltip say it’s invalid and make it’s label turn red (addUpdateResponder in the config category list class).

If so, then maybe something similar to ISuggestionHelper in commands should be used on the text field widgets to allow autofill on the fields and to notify the user if an entered text is invalid.

Sounds great to me. Would require a couple minor changes to ForgeConfigSpec, but I’m not sure how hard it would be to implement in the text widgets. I’m not going to do it if it’s very hard to make auto completion work.

ichttt

This should be a fairly complete review of this PR.
About the changes to ForgeConfigSpec: Those seem mostly fine to me, I added a suggestion on how to handle translation keys for categories.
Mostly looking good IMO though

ichttt · 2020-10-13T10:44:38Z

src/main/java/net/minecraftforge/client/gui/config/CategoryConfigScreen.java

@@ -233,6 +273,42 @@ protected void addExtraTooltipInfo(ValueSpec valueInfo, List<ITextProperties> to
            }
        }

+        protected void addInvalidityWarningHandler(Interactor<?> interactor, List<ITextProperties> tooltip) {
+            final ITextComponent tooltipWarning = new TranslationTextComponent("forge.configgui.tooltip.invalidValue").func_240701_a_(TextFormatting.BOLD, TextFormatting.RED);
+            MutableObject<Boolean> warningActive = new MutableObject<>(false);


MutableBoolean and similar classes exist for primitive types

ichttt · 2020-10-19T05:15:20Z

src/main/java/net/minecraftforge/client/gui/config/CategoryConfigScreen.java

+         *
+         * @return Either a {@link Config} (if the key corresponds to a category) or a {@link ConfigValue} (if the key corresponds to a value).
+         */
+        Object getValue(String key);


Can we use DFU's Either here to make things a little cleaner?

I can’t find out what a DFU is, I assume it’s not a Device Firmware Update which is all google will show me.

Data Fixer-Upper, it's a Mojang tool that... is hard to explain.

There is a class in the DFU lib that's called Either, it can be used instead of raw object if exactly two completly different types are possible

ichttt · 2020-10-19T05:25:29Z

src/main/java/net/minecraftforge/client/gui/config/ControlCreator.java

+    }
+
+    public void createBooleanInteractionWidget(Interactor<Boolean> interactor) {
+        MutableObject<Boolean> state = new MutableObject<>();


Again, MutableBoolean

ichttt · 2020-10-19T05:29:22Z

src/main/java/net/minecraftforge/client/gui/config/ControlCreator.java

+     */
+    @SuppressWarnings("unchecked")
+    @Nullable
+    public <T extends Number, C extends Number & Comparable<? super C>> Range<C> getRangeForSliderCreation(Interactor<T> interactor) {


Should be able to inline T

ichttt · 2020-10-19T05:30:43Z

src/main/java/net/minecraftforge/client/gui/config/ControlCreator.java

+            return null;
+        double min = range.getMin().doubleValue();
+        double max = range.getMax().doubleValue();
+        if (max - min > 10)


This seems like an arbitary bound... At least put it into a private static final int and add some clarification why 10 seems like a good value

It’s very arbitrary, I’m not unhappy about it though - if people don’t like it I want them to change it by supplying their own ControlCreator for their config screen. The aim then is to make it as easy as possible to change which I think I have done.

ichttt · 2020-10-19T05:45:13Z

src/main/java/net/minecraftforge/client/gui/config/ModConfigScreen.java

+                    String translationKey = "forge.configgui.modConfigType." + modConfig.getType().name().toLowerCase();
+                    IFormattableTextComponent title = CategoryConfigScreen.translateWithFallback(translationKey, StringUtils.capitalize(modConfig.getType().name().toLowerCase()));


Always use Locale.ROOT when using toLowerCase is such context

ichttt · 2020-10-19T05:46:23Z

src/main/java/net/minecraftforge/common/ForgeConfig.java

@@ -52,62 +52,62 @@

        Server(ForgeConfigSpec.Builder builder) {
            builder.comment("Server configuration settings")
-                   .push("server");


Please leave all the indentation as-is, it just bloats the already huge diff

Heh, you should see my old PR

ichttt · 2020-10-19T05:49:40Z

src/main/java/net/minecraftforge/common/ForgeConfigSpec.java

        }

        //boolean
        public BooleanValue define(String path, boolean defaultValue) {
            return define(split(path), defaultValue);
        }
        public BooleanValue define(List<String> path, boolean defaultValue) {
-            return define(path, (Supplier<Boolean>)() -> defaultValue);


These casts may be needed for eclipse compiler, please leave them

ichttt · 2020-10-19T05:50:59Z

src/main/java/net/minecraftforge/fml/DistExecutor.java

@@ -46,7 +46,7 @@
    private DistExecutor() {}

    /**
-     * Run the callable in the supplier only on the specified {@link Side}.
+     * Run the callable in the supplier only on the specified {@link Dist}.


Please leave this class alone, it is not in the scope of this PR

I figured it would be easier to bundle these small docs changes in this PR, Is it really worth splitting it off into its own PR?

ichttt · 2020-10-19T05:54:23Z

src/main/java/net/minecraftforge/common/ForgeConfigSpec.java

 {
-    private Map<List<String>, String> levelComments = new HashMap<>();


About translations for categories: We could add a map for cat path -> translation key, just like we had with levelComments before

My thoughts as well

JamesTheAwesomeDude · 2020-12-29T19:09:41Z

Will this PR break compatibility with current mods' programmed configs?

e.g. this currently blocks squeek502/AppleSkin#85 — how much (if any) refactoring will this and similar devs have to do of their existing code to get their config GUIs working again?

Will you support the "legacy" API?

GirafiStudios · 2020-12-29T19:20:49Z

Will this PR break compatibility with current mods' programmed configs?

e.g. this currently blocks squeek502/AppleSkin#85 — how much (if any) refactoring will this and similar devs have to do of their existing code to get their config GUIs working again?

Will you support the "legacy" API?

I think you've misunderstood something. There is currently NO code for the in-game config in Forge, so nothing will break.
Mods will have to add support for this on their end, when the PR gets accepted though.

JamesTheAwesomeDude · 2020-12-29T23:28:10Z

Will you support the "legacy" API?

I think you've misunderstood something. There is currently NO code for the in-game config in Forge, so nothing will break.
Mods will have to add support for this on their end, when the PR gets accepted though.

I believe I have not misunderstood anything:

I'm referring to the GUI code that was dropped in 1.13 — the "legacy" config GUI API from 1.12 — some mods (e.g. AppleSkin, linked previously) retain their GUI config declarations which were coded against this API.

I am asking if this PR will resume support, "drop-in", non-breaking (or minimally-breaking) for such legacy code [that is still coded against the 1.12 config GUI API], or if all mod developers will be required to refactor their GUI config declarations against the new config GUI API.

ChampionAsh5357 · 2020-12-29T23:55:11Z

The GUI is associated with the configuration toml. If they haven't already rewritten their configs, then of course they would need to be rewritten. The GUI itself has no bounds on the config as its dependent on it. Comparing pre-1.13 to post-1.13 will most likely include a full rewrite of the mod anyways.

So, yes, I believe you did misunderstand. This it not to mention the fact that Forge doesn't support 'legacy' versions.

XFactHD · 2020-12-30T00:06:07Z

@JamesTheAwesomeDude The code you are referencing can not be used with any 1.13+ version of Forge because the config system itself has been rewritten completely. Therefore any "existing" config GUI code will need to be rewritten.

@GirafiStudios That is not true. Forge provides an ExtensionPoint where you can register your own implementation of a Config GUI, you do not need this PR for that. The "only" thing this PR does is providing a default implementation of a Config GUI.

Cadiboo · 2021-02-01T10:13:19Z

I’ll fix up the merge conflicts on this but IIRC I’m still waiting for Lex/cpw’s comments about the changes to ForgeConfigSpec and about how translation keys on categories should be supported.

LexManos · 2021-02-16T00:50:51Z

Honestly, this is a major thing that will break every Minecraft version.
It's better to be split off into a custom mod/library that is managed by someone who has the time and the patience to deal with rendering crap.
The config systems that we use are designed to have all of the metadata necessary for these generic systems to exist.
If we do not have the hooks in place {I believe we do} to allow such a mod to provide the GUI's for other mods that don't provide their own. Then we can add that.
Also, IF this mod does come into existence, it could be published with our installer as a default option. But that's something we'd have to talk to the author about.

Basically, Config GUIs are nice. But Mojang likes to break their rendering system to much for us to sanely manage them.

ichttt reviewed Oct 4, 2020

View reviewed changes

Cadiboo added 25 commits October 10, 2020 17:56

ChangeList & tests

d49954c

Just starting off

162d0b8

Mock

3628888

Messing around

bbd8449

Hacks already

aba108d

Fix position

14456c4

Cleanup + improvements

85ac678

Make server only load in-world

9e3f228

Booleans and integers

478b298

Add mods list to ingame menu

c8f6f37

Reset kinda works

6ec8b77

Refactor, not sure if it's in a good way

a05da35

Very unsure that this is a good refactor

b6794e2

Fix text fields

e316455

Cleanup

c5b989e

Cleanup

cd3a969

Cleanup ForgeConfigSpec

f435fb6

Feels like they should be in this order

a582e5e

Fixup

44bf795

Debug: Add testing config

210bdea

More text field fixes

8da64a1

Tooltip

418b2c4

Refactoring on the way to lists

778e703

Comments

72d01db

Working on saveAndReload

69e5708

Cadiboo added 14 commits October 10, 2020 17:56

Refactor to be a bit cleaner

b21970f

Double round instead of cast

542a870

Cleanup

fe4810a

Add license header

53c6c93

Move to test mod

a187170

Oops forgot this

8160fb4

Add some missing translations

24d8cbe

Polishing

2cf84b2

Nvm, it just works:tm:

9550409

Couple more fixes

e155668

Update TODO

b454300

Cleanup

4b13d30

Cleanup & responding to PR feedback

4a50f66

Remove unnecesary import

4452ab8

Cadiboo force-pushed the config-gui-1.16 branch from 85b19c8 to 4452ab8 Compare October 10, 2020 06:57

Cadiboo added 2 commits October 18, 2020 15:17

Add undo button to list elements

a74d75a

This reads better

b6db98d

ichttt suggested changes Oct 19, 2020

View reviewed changes

pupnewfster mentioned this pull request Oct 21, 2020

Configuration is not accessible through the forge mod menu mekanism/Mekanism#5794

Open

JamesTheAwesomeDude mentioned this pull request Dec 29, 2020

Configuration GUI is broken squeek502/AppleSkin#85

Open

lukawarren mentioned this pull request Jan 30, 2021

Stamina recovery rate wants changing lukawarren/Lorien-Legacies#110

Closed

LexManos closed this Feb 16, 2021

Fuzss mentioned this pull request Jul 17, 2022

[Feature] Config GUI #8874

Closed

3 tasks

		String translationKey = "forge.configgui.modConfigType." + modConfig.getType().name().toLowerCase();
		IFormattableTextComponent title = CategoryConfigScreen.translateWithFallback(translationKey, StringUtils.capitalize(modConfig.getType().name().toLowerCase()));

		{
		private Map<List<String>, String> levelComments = new HashMap<>();

[1.16.x] Re-implement the Config GUI #7395

[1.16.x] Re-implement the Config GUI #7395

Conversation

Cadiboo commented Oct 4, 2020 • edited

Forge API changes (no breaking changes)

Other changes

Screenshots:

ichttt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Minecraftschurli Oct 5, 2020 • edited

Choose a reason for hiding this comment

maxanier commented Oct 4, 2020 • edited

Cadiboo commented Oct 4, 2020 • edited

ChampionAsh5357 commented Oct 5, 2020

Cadiboo commented Oct 6, 2020

ichttt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cadiboo Oct 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesTheAwesomeDude commented Dec 29, 2020 • edited

GirafiStudios commented Dec 29, 2020 • edited

JamesTheAwesomeDude commented Dec 29, 2020 • edited

ChampionAsh5357 commented Dec 29, 2020

XFactHD commented Dec 30, 2020

Cadiboo commented Feb 1, 2021

LexManos commented Feb 16, 2021

Cadiboo commented Oct 4, 2020 •

edited

Minecraftschurli Oct 5, 2020 •

edited

maxanier commented Oct 4, 2020 •

edited

Cadiboo commented Oct 4, 2020 •

edited

Cadiboo Oct 19, 2020 •

edited

JamesTheAwesomeDude commented Dec 29, 2020 •

edited

GirafiStudios commented Dec 29, 2020 •

edited

JamesTheAwesomeDude commented Dec 29, 2020 •

edited