Wynntils

Wynntils

611k Downloads

UserFeatures config problems

kristofbolyai opened this issue ยท 6 comments

commented

I was going to create a PR fixing this, but this one is a controversial topic, so I thought an issue might be better to start with.

ConfigManager has a requirement that a Feature must be annotated with FeatureInfo to get the Feature's category or else config options won't load. This is obviously not a good thing.

Here is the things we could do:

  • Make FeatureInfo annotation required by throwing Exception on feature registration
  • Make ConfigManager assume blank ("") category for features without annotation (the annotation already has "" as default category).
commented
commented

maybe i'm missing something, but i don't really see what the issue is - like you said, the annotation already has a default category defined, so config options will always be loaded as long as it's present (which is required for other feature loading code anyway iirc)

to deal with translations and hierarchy definitions, the category field will probably become an enum eventually, so the default could just be UNCATEGORIZED or something similar

commented

maybe i'm missing something, but i don't really see what the issue is - like you said, the annotation already has a default category defined, so config options will always be loaded as long as it's present (which is required for other feature loading code anyway iirc)

to deal with translations and hierarchy definitions, the category field will probably become an enum eventually, so the default could just be UNCATEGORIZED or something similar

No, actually the annotation is not required as of right now and not every feature has it. So for example, many UserFeatures don't load the userEnabled config (and you can't change it hence).

commented

No, actually the annotation is not required as of right now and not every feature has it. So for example, many UserFeatures don't load the userEnabled config (and you can't change it hence).

ah, i didn't realize that.

the userEnabled switch should be categorized even if there's no other options, so i think requiring FeatureInfo for loading configs is fine. i'm not sure if it should be enforced though, because it would be unnecessary for a lot of InternalFeatures - maybe only enforce it for UserFeature

commented

the userEnabled switch should be categorized even if there's no other options, so i think requiring FeatureInfo for loading configs is fine. i'm not sure if it should be enforced though, because it would be unnecessary for a lot of InternalFeatures - maybe only enforce it for UserFeature

Enforcing it if a feature has configs should be the way to go imo.

commented

Do youreally need to ask what I think? I thought I've made my views on FeatureInfo clear by now. ๐Ÿ˜ƒ

To iterate: I don't think FeatureInfo provides much value. I don't wish to see it required. I'd rather see it removed. :-) I have still hard to see really how the category will help us. Maybe it will be more obvious once we have a config GUI. Or a config GUI will show us that we need a different kind of grouping than what the category provides, so we'll have to redo it anyway.

So my stance here is that if a FeatureInfo is missing, we should still be able to use @Config, and you can assume the empty category (or whatever default it might be replaced with in the future).