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

Fixing and cleaning up configs #40

Merged
merged 14 commits into from
Dec 1, 2023
Merged

Fixing and cleaning up configs #40

merged 14 commits into from
Dec 1, 2023

Conversation

dima-dencep
Copy link
Contributor

@dima-dencep dima-dencep commented Nov 19, 2023

The things that keep me from sleeping well. Commits are documented (if possible),

Perhaps after merge this pr I will make an additional one for neoforge, however right now it is not required and does not make any breaking changes (Probably)

Who edits the config via a file when minecraft is already running?
Since `ConfigFileTypeHandler` is extensible and `ModConfig` already has everything ready for it
Some mods add screens to edit configs and they should fire an event via this method
Copy link
Member

@Technici4n Technici4n left a comment

Choose a reason for hiding this comment

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

A few comments since I'm not too familiar with the config code.

@@ -23,10 +23,12 @@
public class FMLConfig
{
public enum ConfigValue {
DISABLE_CONFIG_WATCHER("disableConfigWatcher", Boolean.FALSE, "Disables File Watcher. Used to automatically update config if its file has been modified."),
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of disabling the config watcher? If it is to fix a bug, it might already be fixed in upstream NightConfig. Or at least it was reported to TheElectronWill some time ago and he said a fix was in the pipeline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, there's no point as such, but I'd like to be able to do so

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I'm in favor of this change - there are outstanding issues with the watcher on our end that are not addressed, such as the Loading & Reloading events being able to fire simultaneously, and the whole concept being quite broken with editors that don't save atomically. So I think giving users the option to disable the watcher is a good stopgap until someone has time to properly rewrite that logic.

Copy link
Member

Choose a reason for hiding this comment

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

Should we maybe disable it by default then for now? (at least in production)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about a complete remove?

Copy link
Member

Choose a reason for hiding this comment

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

No - it would be nice to fix it eventually so please leave it in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, what else do I need to do to get this pr merged? (I can't continue working on one mod without these changes)

core/src/main/java/net/neoforged/fml/config/ModConfig.java Outdated Show resolved Hide resolved
core/src/main/java/net/neoforged/fml/config/ModConfig.java Outdated Show resolved Hide resolved
@embeddedt
Copy link
Member

The changes look fine to me but I'd like someone else to take a look.

@Technici4n
Copy link
Member

I'll do some testing later.

@Technici4n Technici4n self-assigned this Nov 30, 2023
@Technici4n Technici4n merged commit a43f00e into neoforged:main Dec 1, 2023
1 check passed
@TheElectronWill
Copy link

Hello
You may be interested in knowing that TheElectronWill/night-config#148 has been merged, and that TheElectronWill/night-config#152 is in the works :)

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