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.

Sign up for GitHub

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

Jump to bottom

Add support for config option update listener #87

Closed
wants to merge 12 commits into from
Closed

Add support for config option update listener #87

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9f15606
Add support for config option update listener
P0keDev Jun 5, 2022
e058f0a
Merge branch 'main' into config-on-update
P0keDev Jun 5, 2022
ca6d548
Make UserFeature implement Configurable
P0keDev Jun 5, 2022
82e26fb
Merge branch 'main' into config-on-update
kristofbolyai Jun 5, 2022
c5e40d7
Merge branch 'main' into config-on-update
kristofbolyai Jun 5, 2022
d840c05
Merge branch 'main' into config-on-update
kristofbolyai Jun 5, 2022
404b4bb
Improve update method, add enable/disable handling to UserFeature
P0keDev Jun 5, 2022
d9f7c7b
Merge remote-tracking branch 'origin/config-on-update' into config-on…
P0keDev Jun 5, 2022
ba9177b
Add @ConfigurableInfo to PlayerGhostTransparencyFeature
P0keDev Jun 5, 2022
f0fd8d1
Add userEnable name check
P0keDev Jun 5, 2022
de08908
Add annotation to ensure super method is always called
P0keDev Jun 5, 2022
3af8235
Merge branch 'main' into config-on-update
Incompleteusern Jun 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 6 additions & 6 deletions common/src/main/java/com/wynntils/core/config/ConfigManager.java
View file
Open in desktop
Expand Up @@ -15,7 +15,7 @@
import com.wynntils.core.config.objects.ConfigOptionHolder;
import com.wynntils.core.config.objects.ConfigurableHolder;
import com.wynntils.core.config.properties.ConfigOption;
import com.wynntils.core.config.properties.Configurable;
import com.wynntils.core.config.properties.ConfigurableInfo;
import com.wynntils.mc.utils.McUtils;
import java.io.File;
import java.io.FileInputStream;
Expand All @@ -39,7 +39,7 @@ public class ConfigManager {

private static Gson gson;

public static void registerConfigurable(Object configurableObject) {
public static void registerConfigurable(Configurable configurableObject) {
ConfigurableHolder configurable = configurableFromObject(configurableObject);
if (configurable == null) return; // not a valid configurable

Expand Down Expand Up @@ -121,11 +121,11 @@ public static void saveConfig() {
}
}

private static ConfigurableHolder configurableFromObject(Object configurableObject) {
private static ConfigurableHolder configurableFromObject(Configurable configurableObject) {
Class<?> configurableClass = configurableObject.getClass();

Configurable metadata = configurableClass.getAnnotation(Configurable.class);
if (metadata == null || metadata.category().isEmpty()) return null; // not a valid config container
ConfigurableInfo metadata = configurableClass.getAnnotation(ConfigurableInfo.class);
if (metadata == null || metadata.category().isEmpty()) return null; // not a valid configurable

List<ConfigOptionHolder> options = new ArrayList<>();

Expand All @@ -142,7 +142,7 @@ private static ConfigurableHolder configurableFromObject(Object configurableObje
return new ConfigurableHolder(configurableClass, options, metadata);
}

private static ConfigOptionHolder optionFromField(Object parent, Field field) {
private static ConfigOptionHolder optionFromField(Configurable parent, Field field) {
ConfigOption metadata = field.getAnnotation(ConfigOption.class);
if (metadata == null || metadata.displayName().isEmpty()) return null; // not a valid config variable

Expand Down
12 changes: 12 additions & 0 deletions common/src/main/java/com/wynntils/core/config/Configurable.java
View file
Open in desktop
@@ -0,0 +1,12 @@
/*
* Copyright © Wynntils 2022.
* This file is released under AGPLv3. See LICENSE for full license details.
*/
package com.wynntils.core.config;

import com.wynntils.core.config.objects.ConfigOptionHolder;

/** Used to indicate that a class contains config options */
public interface Configurable {
default void onConfigUpdate(ConfigOptionHolder configOption) {}
}
6 changes: 4 additions & 2 deletions common/src/main/java/com/wynntils/core/config/objects/ConfigOptionHolder.java
View file
Open in desktop
Expand Up @@ -5,19 +5,20 @@
package com.wynntils.core.config.objects;

import com.wynntils.core.Reference;
import com.wynntils.core.config.Configurable;
import com.wynntils.core.config.properties.ConfigOption;
import java.lang.reflect.Field;
import org.apache.commons.lang3.reflect.FieldUtils;

public class ConfigOptionHolder {
private final Object parent;
private final Configurable parent;
private final Field optionField;
private final Class<?> optionType;
private final ConfigOption metadata;

private final Object defaultValue;

public ConfigOptionHolder(Object parent, Field field, ConfigOption metadata) {
public ConfigOptionHolder(Configurable parent, Field field, ConfigOption metadata) {
this.parent = parent;
this.optionField = field;
this.optionType = field.getType();
Expand Down Expand Up @@ -56,6 +57,7 @@ public Object getValue() {
public void setValue(Object value) {
try {
FieldUtils.writeField(optionField, parent, value, true);
parent.onConfigUpdate(this);
} catch (IllegalAccessException e) {
Reference.LOGGER.error("Unable to set config option " + getJsonName());
e.printStackTrace();
Expand Down
8 changes: 4 additions & 4 deletions common/src/main/java/com/wynntils/core/config/objects/ConfigurableHolder.java
View file
Open in desktop
Expand Up @@ -4,15 +4,15 @@
*/
package com.wynntils.core.config.objects;

import com.wynntils.core.config.properties.Configurable;
import com.wynntils.core.config.properties.ConfigurableInfo;
import java.util.List;

public class ConfigurableHolder {
private final Class<?> configurableClass;
private final List<ConfigOptionHolder> options;
private final Configurable metadata;
private final ConfigurableInfo metadata;

public ConfigurableHolder(Class<?> configurableClass, List<ConfigOptionHolder> options, Configurable metadata) {
public ConfigurableHolder(Class<?> configurableClass, List<ConfigOptionHolder> options, ConfigurableInfo metadata) {
this.configurableClass = configurableClass;
this.options = options;
this.metadata = metadata;
Expand All @@ -22,7 +22,7 @@ public List<ConfigOptionHolder> getOptions() {
return options;
}

public Configurable getMetadata() {
public ConfigurableInfo getMetadata() {
return metadata;
}

Expand Down
2 changes: 1 addition & 1 deletion .../core/config/properties/Configurable.java → ...e/config/properties/ConfigurableInfo.java
View file
Open in desktop
Expand Up @@ -11,7 +11,7 @@

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
public @interface Configurable {
public @interface ConfigurableInfo {
/** The parent category for options within this container. Also defines file structure. */
String category();

Expand Down
6 changes: 3 additions & 3 deletions common/src/main/java/com/wynntils/core/features/FeatureRegistry.java
View file
Open in desktop
Expand Up @@ -7,7 +7,7 @@
import com.wynntils.core.Reference;
import com.wynntils.core.WynntilsMod;
import com.wynntils.core.config.ConfigManager;
import com.wynntils.core.config.properties.Configurable;
import com.wynntils.core.config.Configurable;
import com.wynntils.core.features.overlays.Overlay;
import com.wynntils.core.features.properties.EventListener;
import com.wynntils.core.features.properties.RegisterKeyBind;
Expand Down Expand Up @@ -99,8 +99,8 @@ private static void initializeFeature(Feature feature) {

// register & load config options
// this has to be done after the userEnabled handling above, so the default value registers properly
if (featureClass.isAnnotationPresent(Configurable.class)) {
ConfigManager.registerConfigurable(feature);
if (feature instanceof Configurable configurable) {
ConfigManager.registerConfigurable(configurable);
}

// initialize & enable
Expand Down
18 changes: 17 additions & 1 deletion common/src/main/java/com/wynntils/core/features/UserFeature.java
View file
Open in desktop
Expand Up @@ -4,12 +4,28 @@
*/
package com.wynntils.core.features;

import com.wynntils.core.config.Configurable;
import com.wynntils.core.config.objects.ConfigOptionHolder;
import com.wynntils.core.config.properties.ConfigOption;
import javax.annotation.OverridingMethodsMustInvokeSuper;

/**
* A feature that is enabled & disabled by the user.
*/
public abstract class UserFeature extends Feature {
public abstract class UserFeature extends Feature implements Configurable {
P0keDev marked this conversation as resolved.
Show resolved Hide resolved
@ConfigOption(displayName = "Enabled", description = "Should this feature be enabled?")
protected boolean userEnabled = true;

/** This handles the user enabling/disabling a feature in-game */
@Override
@OverridingMethodsMustInvokeSuper
public void onConfigUpdate(ConfigOptionHolder option) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because if it's not user enabled, the feature is supposed to be disabled

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't respect the condition system in place

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like the super calls will absolutely be forgotten when actually writing code, although I can't think of something better atm.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, looks like the annotation isn't fully implemented - although it will still cause a warning in intellij

Copy link
Collaborator

Choose a reason for hiding this comment

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

Warning should be enough

Copy link
Member

Choose a reason for hiding this comment

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

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.

if (!option.getField().getName().equals("userEnabled")) return;

if (userEnabled) {
tryEnable();
} else {
tryDisable();
}
}
}
4 changes: 2 additions & 2 deletions common/src/main/java/com/wynntils/features/user/ItemGuessFeature.java
View file
Open in desktop
Expand Up @@ -6,14 +6,14 @@

import com.google.common.collect.ImmutableList;
import com.wynntils.core.config.properties.ConfigOption;
import com.wynntils.core.config.properties.Configurable;
import com.wynntils.core.config.properties.ConfigurableInfo;
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")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, this won't be a problem if we use translations for config stuff.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Further tracked in #94.

Copy link
Member

Choose a reason for hiding this comment

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

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.

public class ItemGuessFeature extends UserFeature {

@ConfigOption(displayName = "Show Guess Price")
Expand Down
4 changes: 2 additions & 2 deletions common/src/main/java/com/wynntils/features/user/ItemStatInfoFeature.java
View file
Open in desktop
Expand Up @@ -6,14 +6,14 @@

import com.google.common.collect.ImmutableList;
import com.wynntils.core.config.properties.ConfigOption;
import com.wynntils.core.config.properties.Configurable;
import com.wynntils.core.config.properties.ConfigurableInfo;
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")
public class ItemStatInfoFeature extends UserFeature {

@ConfigOption(displayName = "Show Stars")
Expand Down
2 changes: 2 additions & 0 deletions common/src/main/java/com/wynntils/features/user/PlayerGhostTransparencyFeature.java
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@
package com.wynntils.features.user;

import com.wynntils.core.config.properties.ConfigOption;
import com.wynntils.core.config.properties.ConfigurableInfo;
import com.wynntils.core.features.UserFeature;
import com.wynntils.core.features.properties.EventListener;
import com.wynntils.core.features.properties.FeatureInfo;
Expand All @@ -17,6 +18,7 @@

@EventListener
@FeatureInfo(stability = Stability.STABLE)
@ConfigurableInfo(category = "Utilities")
public class PlayerGhostTransparencyFeature extends UserFeature {

@ConfigOption(
Expand Down
4 changes: 2 additions & 2 deletions common/src/main/java/com/wynntils/features/user/TooltipScaleFeature.java
View file
Open in desktop
Expand Up @@ -6,7 +6,7 @@

import com.mojang.blaze3d.vertex.PoseStack;
import com.wynntils.core.config.properties.ConfigOption;
import com.wynntils.core.config.properties.Configurable;
import com.wynntils.core.config.properties.ConfigurableInfo;
import com.wynntils.core.features.UserFeature;
import com.wynntils.core.features.properties.EventListener;
import com.wynntils.core.features.properties.FeatureInfo;
Expand All @@ -19,7 +19,7 @@
import net.minecraftforge.eventbus.api.SubscribeEvent;

@EventListener
@Configurable(category = "Item Tooltips")
@ConfigurableInfo(category = "Item Tooltips")
@FeatureInfo(stability = Stability.STABLE)
public class TooltipScaleFeature extends UserFeature {

Expand Down