Add support for config option update listener #87
Conversation
common/src/main/java/com/wynntils/features/user/TooltipScaleFeature.java
Outdated
Show resolved
Hide resolved
| import com.wynntils.core.features.UserFeature; | ||
| import com.wynntils.core.features.properties.FeatureInfo; | ||
| import com.wynntils.core.features.properties.FeatureInfo.Stability; | ||
| import com.wynntils.core.webapi.WebManager; | ||
|
|
||
| @FeatureInfo(stability = Stability.STABLE) | ||
| @Configurable(category = "Item Tooltips") | ||
| @ConfigurableInfo(category = "Item Tooltips") |
There was a problem hiding this comment.
What do you think about making categories string constant in some files? What if we would want to change Item Tooltips to whatever new name and you would need to do that manually (or using IDE's find and replace). I think something like Categories.ITEM_TOOLTIPS would fit here better.
There was a problem hiding this comment.
Actually, this won't be a problem if we use translations for config stuff.
There was a problem hiding this comment.
Further tracked in #94.
There was a problem hiding this comment.
Even if we use translations, I think it makes sense to have this an enum or a const defined in a class. It just makes it easier to make sure a typo don't ruin things, or do quickly go to all members of the category from the IDE. In general, using "raw" strings like this is not a good idea.
common/src/main/java/com/wynntils/core/config/objects/ConfigOptionHolder.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/wynntils/core/config/Configurable.java
Outdated
Show resolved
Hide resolved
|
|
||
| /** This handles the user enabling/disabling a feature in-game */ | ||
| @Override | ||
| public void onConfigUpdate(ConfigOptionHolder option) { |
There was a problem hiding this comment.
Don't you need to check if the current option is the enabled one?
Either that or I don't get something about this method
There was a problem hiding this comment.
This will run every time an update happens, yes, but tryEnable and tryDisable don't do anything if the feature is already in the correct state. I can add a check for the field's name though
There was a problem hiding this comment.
Why would we try to enable a feature by changing an unrelated setting?
Also, if it is not user enabled, why do we try to disable it?
There was a problem hiding this comment.
Because if it's not user enabled, the feature is supposed to be disabled
There was a problem hiding this comment.
This doesn't respect the condition system in place
There was a problem hiding this comment.
I feel like the super calls will absolutely be forgotten when actually writing code, although I can't think of something better atm.
There was a problem hiding this comment.
The javax library contains the annotation @OverridingMethodMustInvokeSuper, which raises an error if the super method isn't called. We should be able to use this for peace of mind
There was a problem hiding this comment.
Hm, looks like the annotation isn't fully implemented - although it will still cause a warning in intellij
There was a problem hiding this comment.
Warning should be enough
There was a problem hiding this comment.
Once again, I'm not convinced the "turn feature on or off" thing really should be a config like the others. It could just as well be seen as configurations on the FeatureRegistry. It has a bit of an odd ring to it, that a feature is configured to turn itself on or off. I can very much imagine a GUI where the "on/off" functionality of a feature is shown by a switch or checkbox next to the feature name in a list, but the configuration variables for that feature is shown on a config pane to the right of the list. Like the common model for how you enable/disable mods/plugins etc in one place, but configure them elsewhere.
Especially if this switch is making the "real" configuration options more complicated, by forcing the use of a super() call. I think this is a sign that feature-enabled is a property that should have special treatment, and not be considered a general config option.
|
what config listener does the item guess feature need? |
|
It was mentioned in the previous config pr, but it needs to load the relevant web api for |
|
why does that need an additional config listener? it's already handled by the feature's condition and its |
|
It needs to make sure that the itemlist api is loaded, which is solely dependent on showGuessPrice being true. |
|
@P0keDev Why did you close this? |
|
i no longer have motivation to work on this, it would be best for someone else to pick it up |
|
Oh, that's too bad. :-( I think this was right on track... |
Classes with config options must now implement the
Configurableinterface, which contains anonUpdate()method that gets called any time a config value changes. It's also called when the class is first loaded, to make sure the method runs at least once even if all the contained options are set to default.