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

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

Closed
MrAgeo opened this issue Apr 5, 2024 · 6 comments

Comments

@MrAgeo
Copy link
Contributor

MrAgeo commented Apr 5, 2024

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
Copy link
Member

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 added a commit to MrAgeo/BepInEx.ConfigurationManager that referenced this issue Apr 5, 2024
- Added `supportedTypes` Type array (containing the types of `AcceptableValueList` & `AcceptableValueRange`).

- Added method `isTypeSupported` to check if a type is a subclass or an instance of a type in `supportedTypes` via `IsSubclassOfRawGeneric`.

- Added `IsSubclassOfRawGeneric(Type generic, Type toCheck)` method which checks if `toCheck` is a subclass or an instance of `generic`.
@MrAgeo
Copy link
Contributor Author

MrAgeo commented Apr 5, 2024

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
Copy link
Member

ManlyMarco commented Apr 5, 2024

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
Copy link
Contributor Author

MrAgeo commented Apr 5, 2024

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
Copy link
Member

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

@MrAgeo
Copy link
Contributor Author

MrAgeo commented Apr 5, 2024

Sure! ^^

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

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

@MrAgeo MrAgeo closed this as completed Apr 5, 2024
ManlyMarco pushed a commit that referenced this issue Apr 5, 2024
…lues (#89)

This PR removes the ArgumenNullException present in ConfigSettingEntry.GetAcceptedValues(AcceptableValueBase values), enforcing the use of both MinValue and MaxValue for custom range classes (see issue's comments for a in-depth explanation)

Fixes #88
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

No branches or pull requests

2 participants
  翻译: