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

Make clientside config editable with Configured on Forge 1.19.2 #3148

Open
wants to merge 8 commits into
base: 1.19
Choose a base branch
from

Conversation

stepa2
Copy link

@stepa2 stepa2 commented Mar 6, 2023

Fixes #3139.

I exported IClientConfigs to public API to make it accessible from ForgeGuiPlugin and FabricGuiPlugin.
FabricGuiPlugin has a hack with casting to concrete implementation of IClientConfigs, when proper configuration support on fabric will be added, that hack will be not needed.

Also added Configured as development dependency for Forge and updated development version of Forge.

@CLAassistant
Copy link

CLAassistant commented Mar 6, 2023

CLA assistant check
All committers have signed the CLA.

@stepa2
Copy link
Author

stepa2 commented Mar 6, 2023

All tests are passed, spotless check is passed

@mezz
Copy link
Owner

mezz commented Mar 7, 2023

Thank you for contributing!
Unfortunately, I think this may be a duplication of effort.
I have been in contact with the author of Configured and written a different solution here: MrCrayfish/Configured#83
I will be able to backport the solution to 1.19.2 once the PR is accepted by MrCrayfish.

@stepa2
Copy link
Author

stepa2 commented Mar 7, 2023

My solution is not Configured-dependent, it will work with all Forge mods that access configs of other mods.
Also, as you support Fabric as well, you'll still need to do refactoring similar to mine to register clientside configs in Fiber.

@mezz
Copy link
Owner

mezz commented Mar 10, 2023

I think that sounds beneficial. There are some things to overcome here in the review here though.

For PRs, we must target the latest version of JEI and then we can backport to any version you would like. In this case, we need this first PR to target 1.19.3.

The dependency structure of JEI is like this:

             /\
            /  \
           /    \
          /      \
         / forge  \
        / & fabric \  (These depend on everything below)
       /------------\
      /      |       \
     /  gui  |library \  (These depend on the lower levels, but are not connected to each other)
    /------------------\
   /      common        \  (Depends on core, and Minecraft is available here)
  /----------------------\
 /         core           \  (mostly plain Java)
/--------------------------\

So for anything you need accessible in forge and fabric, it doesn't need to go in the API at all.
Core should have no dependencies on high levels like common api.
I think the config files are organized a little better in the 1.19.3 branch.

@Daraphin
Copy link

Daraphin commented Mar 11, 2023

Hello, sorry to bother, but do i need to change something manually or will it be implemented in next update?
If its the first one option, how should i do it...sorry i am not very good in that "programming" stuff xD

@zerebos
Copy link

zerebos commented Apr 16, 2023

@Daraphin We will have to wait for the developer to backport the change 4128bf4 to 1.19.2. The Configured change referenced earlier #3148 (comment) has already been merged and released for 1.19.2. For now you can edit configs manually through files or use an alternative.

@Daraphin
Copy link

Daraphin commented May 14, 2023

@Daraphin We will have to wait for the developer to backport the change 4128bf4 to 1.19.2. The Configured change referenced earlier #3148 (comment) has already been merged and released for 1.19.2. For now you can edit configs manually through files or use an alternative.

(Oh i figured it out so no need for help anymore)

Yeah problem is i need this fix for 1.18.2 :X and sorry for late response completly forgot i asked here...

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.

5 participants