BepInEx / BepInEx.ConfigurationManager

Plugin configuration manager for BepInEx
https://www.patreon.com/ManlyMarco
GNU Lesser General Public License v3.0
227 stars 54 forks source link

Custom classes derived from BepInEx.Configuration.AcceptableValueBase will throw ArgumentNullException #88

Closed MrAgeo closed 4 months ago

MrAgeo commented 4 months ago

I created a custom AcceptedValueMinimum class derived from AcceptableValueBase which has only a minimum value restriction. ConfigurationManager will then treat this custom class as AcceptableValueRange, because it does not have the property AcceptableValues present in AcceptableValueList. This will always throw an ArgumentNullException because ConfigurationManager checks for the existence of both MinValue and MaxValue properties in the custom class, which are not present in this case.

PS: Thanks in advance for your help!

ManlyMarco commented 4 months ago

Why not just use int.MaxValue as the upper bound? You can use a custom setting drawer to change how the setting looks to the user.

MrAgeo commented 4 months ago

That's an option that I also thought about, but the drawback is that I would have to copy (and translate) ConfigurationManager.SettingFieldDrawer.DrawUnknownField() along with some other methods in SettingFieldDrawer into a new custom drawer everytime the upper bound is huge (to avoid a slider with a extreme unusable range). I think this would be like reinventing the wheel, giving that the draw function already exists and its call can be accomplished if GetAcceptableValues(value) isn't called in ConfigurationManager.ConfigSettingEntry's constructor (i.e. if its only called when AcceptableValueList or AcceptableValueRange is present).

PS: I implemented the latter behaviour in fix-for-issue88 branch (in case needed). Please let me know if you want me to make a PR with these changes :)

ManlyMarco commented 4 months ago

The fix doesn't work since it checks only for types inside the ConfigurationManager assembly. Most plugins will not reference the assembly and instead have their own copies. A more robust implementation of GetAcceptableValues would be better, since currently it doesn't handle custom range classes at all.

Edit: Why not rename the min value field in your class?

MrAgeo commented 4 months ago

Renaming the min value field in my class from MinValue to other field name (such as minValue) also fixes the bug XD I didn't thought about it haha Thanks for the idea :D

Now, it's worth noting that, as you said, if any custom class wants to include a custom range via the MinValue and MaxValue fields, then my quick fix that checks only for "vanilla" Types won't work. Now, ignoring that quick fix, if the mod developer wants to support the slider feature, they have to implement converting their custom IComparable type from and to double via IConvertible, and also implement IEquatable.Equals() for int and double if they want the slider to show a percentage too. In the other cases where only MinValue exists in the custom class and the name of the max value field is different to MaxField then GetAcceptableValues will throw the exception.

Maybe the solution is then to enforce the mod devs via a message in the README.md file to use AcceptableValueRange for the custom type and/or implementing both MinValue and MaxValue if they need extra behaviours in a "AcceptableValueRange-like" class? Then GetAcceptableValues would only have to check for the existence of both MinValue and MaxValue at the same time (in case either is not present and also removing the throw ArgumentNullException) as follows:

private void GetAcceptableValues(AcceptableValueBase values)
{
    var t = values.GetType();
    var listProp = t.GetProperty(nameof(AcceptableValueList<bool>.AcceptableValues), BindingFlags.Instance | BindingFlags.Public);
    if (listProp != null)
    {
        AcceptableValues = ((IEnumerable)listProp.GetValue(values, null)).Cast<object>().ToArray();
    }
    else
    {
        var minProp = t.GetProperty(nameof(AcceptableValueRange<bool>.MinValue), BindingFlags.Instance | BindingFlags.Public);
        var maxProp = t.GetProperty(nameof(AcceptableValueRange<bool>.MaxValue), BindingFlags.Instance | BindingFlags.Public);
        if (minProp != null && maxProp != null)
        {
            AcceptableValueRange = new KeyValuePair<object, object>(minProp.GetValue(values, null), maxProp.GetValue(values, null));
            ShowRangeAsPercent = (AcceptableValueRange.Key.Equals(0) || AcceptableValueRange.Key.Equals(1)) && AcceptableValueRange.Value.Equals(100) ||
                                    AcceptableValueRange.Key.Equals(0f) && AcceptableValueRange.Value.Equals(1f);
        }
    }
}

PS: For a better understanding I'll post my original custom class code here :)

public class AcceptableValueMinimum<T> : AcceptableValueBase where T : IComparable
{
    public virtual T MinValue { get; }

    public AcceptableValueMinimum(T minValue) : base(typeof(T))
    {
        if (minValue == null)
        {
            throw new ArgumentNullException("minValue");
        }
        MinValue = minValue;
    }

    public override object Clamp(object value)
    {
        if (MinValue.CompareTo(value) > 0) return MinValue;
        return value;
    }

    public override bool IsValid(object value)
    {
        return MinValue.CompareTo(value) <= 0;
    }

    public override string ToDescriptionString()
    {
        return $"# Acceptable value range: From {MinValue} to infinity and beyond";
    }
}
ManlyMarco commented 4 months ago

The throw could be removed as you've suggested. It'd be nice if you could make a PR for that.

MrAgeo commented 4 months ago

Sure! ^^

Edit: I've made the PR. I'll close this issue as completed.

Thanks for your help! Have a nice day! :D