Simplify how mods use config.json

Pathoschild commented 8 years ago

Consider simplifying how mods deal with config.json.

Current system

Here's how mods use config.json now:

The mod creates a subclass of StardewModdingAPI.Config with an overridden method:

internal class SampleConfig : StardewModdingAPI.Config
{
   public bool ExampleBoolean { get; set; }
   public float ExampleFloat { get; set; }

   public override T GenerateDefaultConfig<T>()
   {
       this.ExampleBoolean = true;
       this.ExampleFloat = 0.5;
       return this as T;
   }
}

The mod loads it by calling a method on the subclass with the mod's config path:
```
var config = new SampleConfig().InitializeConfig(this.BaseConfigPath);
```

If the mod wants to use a separate JSON file (e.g. Lookup Anything's data.json):

Reference the Newtonsoft.Json package. (Make sure to use the same version as SMAPI to avoid issues.)

Read the file:

string path = Path.Combine(this.PathOnDisk, "data.json");
string data = File.ReadAllText(path);
var customModel = JsonConvert.DeserialiseObject<Model>(data);

Optionally resave the file if you want to use SMAPI's auto-create logic:

string data = JsonConvert.SerialiseObject(customModel);
File.WriteAllText(path, data);

Pros & cons of current system

Advantages:
- ✓ Existing mods already use it.
- ✓ It's well-tested, since it's been in use for seven months now.
Disadvantages:
- ✘ It ignores C# conventions.
  This makes it harder for new mod developers. For example, it's easy to forget to override Config.GenerateDefaultConfig<T>, which will break. It's also not clear how it should be used — should you set the defaults in the constructor and return the instance, or set the defaults in that method and return the instance, or return a new object? Why not use a constructor?
- ✘ The interface is confusing and exposes SMAPI internals.
  For example, what's the difference between Config.InitializeConfig<T>(), Config.LoadConfig<T>(), Config.ReloadConfig<T>(), and Config.Instance<T>()? How about between Config.UpdateConfig<T>() and Config.WriteConfig<T>()?
- ✘ It adds a lot of boilerplate.
  Ideally the mod author shouldn't need to care how the config system works (e.g. implementing an internal base class or explicitly calling an initialiser).
- ✘ If the mod wants to use custom JSON files, it needs to reference the underlying library and do it manually.
- ✘ The internal implementation is a bit complicated, since it reinvents some things that C# and the JSON library do out of the box (like merging defaults).

Proposed system

Here's one idea for a new config system:

The mod creates a plain class and sets defaults in the usual C# way:

internal class SampleConfig
{
   public bool ExampleBoolean { get; set; } = true;
   public float ExampleFloat { get; set; } = 0.5;
}

(A constructor would work too.)

The mod loads it using a mod method:

var config = this.Helper.ReadConfig<SampleConfig>();

If the mod wants to use a separate JSON file (e.g. Lookup Anything's data.json):

The mod loads it the same way:

var model = this.Helper.ReadJson<CustomModel>("data.json");

Pros & cons of proposed system

Advantages:
- ✓ It uses C# conventions.
  This reduces the learning curve, since mod developers can apply their existing knowledge. They already know how to set default values in C#; why reinvent the wheel?
- ✓ The interface is discoverable.
  The mod's base class only has two properties: this.Manifest and this.Helper. It's easy to discover what features are available without needing to look up separate documentation. The SMAPI internals are hidden away to avoid confusion.
- ✓ It eliminates boilerplate.
  The mod author just creates a simple class — no base class, overridden methods, or passing paths into a certain method.
- ✓ If the mod wants to use custom JSON files, that's easy to do.
- ✓ The ModHelper is easy to extend with more methods in the future.
- ✓ It's easy to implement internally, since that's how the Json.NET library is meant to be used.
Disadvantages:
- ✘ For backwards compatibility with existing mods, SMAPI would need to support the old system too (and mark it deprecated).

Pathoschild commented 8 years ago

This is just an idea in the backlog, and isn't planned for any upcoming release. Feedback or other ideas are welcome (even if your feedback is "I like the current system" 😉).

Zoryn4163 commented 8 years ago

I do like the idea of this system more, but I'd like to see more about its implementation. Overall though it's quite nice, much simpler than my original config implementation.

AdvizeGH commented 8 years ago

You're right and I can't argue with your logic, but like Zoryn I would have to see more about its implementation first. Your proposed changes might actually break functionality in GetDressed. Currently, extending the Config class allows me greater control over what is serialized and what isn't. Would this still work?

public class LocalConfig : Config, IComparable
{
    public bool firstRun { get; set; }

    public int[] chosenAccessory = new int[numOfFavs];
    public int[] chosenFace = new int[numOfFavs];
    public int[] chosenNose = new int[numOfFavs];
    public int[] chosenBottoms = new int[numOfFavs];
    public int[] chosenShoes = new int[numOfFavs];

    public int[] chosenSkin = new int[numOfFavs];
    public int[] chosenShirt = new int[numOfFavs];
    public int[] chosenHairstyle = new int[numOfFavs];
    public uint[] chosenHairColor = new uint[numOfFavs];
    public uint[] chosenEyeColor = new uint[numOfFavs];
    public uint[] chosenBottomsColor = new uint[numOfFavs];

    [JsonIgnore]
    public int saveTime { get; set; }
    [JsonIgnore]
    private const int numOfFavs = 37;

    public override T GenerateDefaultConfig<T>()
    {
        firstRun = true;

        return this as T;
    }

    public int CompareTo(object obj)
    {
        return ((LocalConfig)obj).saveTime - saveTime;
    }
}

Because of how the mod reads per-save configurations for all saves at the game's loading screen (in order to appropriately render characters in the load game menu), I need to initialize a collection of configs for every local save and sort them in order of which was most recently played. This involved adding additional functionality to the config class itself.

Pathoschild commented 8 years ago

@AdvizeGH yep, that should work fine. Here's your code with the new system:

public class LocalConfig : IComparable
{
    public bool firstRun { get; set; } = true;

    public int[] chosenAccessory = new int[numOfFavs];
    public int[] chosenFace = new int[numOfFavs];
    public int[] chosenNose = new int[numOfFavs];
    public int[] chosenBottoms = new int[numOfFavs];
    public int[] chosenShoes = new int[numOfFavs];

    public int[] chosenSkin = new int[numOfFavs];
    public int[] chosenShirt = new int[numOfFavs];
    public int[] chosenHairstyle = new int[numOfFavs];
    public uint[] chosenHairColor = new uint[numOfFavs];
    public uint[] chosenEyeColor = new uint[numOfFavs];
    public uint[] chosenBottomsColor = new uint[numOfFavs];

    [JsonIgnore]
    public int saveTime { get; set; }
    [JsonIgnore]
    private const int numOfFavs = 37;

    public int CompareTo(object obj)
    {
        return ((LocalConfig)obj).saveTime - saveTime;
    }
}

Even if we made this change, anything using the old Config base class would continue to work for backwards compatibility.

Pathoschild commented 8 years ago

@Zoryn4163 sure, here's a sample implementation. (This is an example I just wrote up. It might change based on this discussion, testing, or other requirements.)

Initial version

Create a ModHelper class that encapsulates interaction with a mod directory:

/// <summary>Provides methods for interacting with a mod directory.</summary>
public class ModHelper
{
  /*********
  ** Accessors
  *********/
  /// <summary>The mod directory path.</summary>
  public string DirectoryPath { get; }

  /*********
  ** Public methods
  *********/
  /// <summary>Construct an instance.</summary>
  /// <param name="modDirectory">The mod directory path.</param>
  public ModHelper(string modDirectory)
  {
      // validate
      if (string.IsNullOrWhiteSpace(modDirectory))
          throw new InvalidOperationException("The mod directory cannot be empty.");
      if (!Directory.Exists(modDirectory))
          throw new InvalidOperationException("The specified mod directory does not exist.");

      // initialise
      this.DirectoryPath = modDirectory;
  }

  /****
  ** Mod config file
  ****/
  /// <summary>Read the mod's configuration file (and create it if needed).</summary>
  /// <typeparam name="TConfig">The config class type.</typeparam>
  public TConfig ReadConfig<TConfig>()
      where TConfig : class, new()
  {
      var config = this.ReadJsonFile<TConfig>("config.json") ?? new TConfig();
      this.WriteConfig(config); // create file or fill in missing fields
      return config;
  }

  /// <summary>Save to the mod's configuration file.</summary>
  /// <typeparam name="TConfig">The config class type.</typeparam>
  /// <param name="config">The config settings to save.</param>
  public void WriteConfig<TConfig>(TConfig config)
      where TConfig : class, new()
  {
      this.WriteJsonFile("config.json", config);
  }

  /****
  ** Generic JSON files
  ****/
  /// <summary>Read a JSON file.</summary>
  /// <typeparam name="TModel">The model type.</typeparam>
  /// <param name="path">The file path relative to the mod directory.</param>
  public TModel ReadJsonFile<TModel>(string path)
      where TModel : class
  {
      path = Path.Combine(this.DirectoryPath, path);
      return JsonConvert.DeserializeObject<TModel>(File.ReadAllText(path));
  }

  /// <summary>Save to a JSON file.</summary>
  /// <typeparam name="TModel">The model type.</typeparam>
  /// <param name="path">The file path relative to the mod directory.</param>
  /// <param name="model">The model to save.</param>
  public void WriteJsonFile<TModel>(string path, TModel model)
      where TModel : class
  {
      path = Path.Combine(this.DirectoryPath, path);

      // create directory if needed
      string dir = Path.GetDirectoryName(path);
      if (!Directory.Exists(dir))
          Directory.CreateDirectory(dir);

      // write file
      string json = JsonConvert.SerializeObject(model, Formatting.Indented);
      File.WriteAllText(path, json);
  }
}

Assign a helper to each mod as it's initialised via Program::LoadMods.

(optional) Use it to simplify the Manifest logic:

var manifest = mod.Helper.ReadJsonFile<Manifest>("manifest.json");

That's pretty much it. Mods would use it like this:

var config = this.Helper.ReadConfig<SampleConfig>();
var custom = this.Helper.ReadJsonFile<CustomModel>("data.json");

Longer term

We could easily add more functionality to the mod helper. For example, we could inject the current save ID and use it to add simple per-save wrappers like this:

var config = this.Helper.ReadPerSaveConfig();
var data = this.Helper.ReadPerSaveJson<DataModel>("data.json");

Zoryn4163 commented 8 years ago

Looks good to me @Pathoschild

Entoarox commented 8 years ago

I feel that even with the new system, the option to extend a base class, and thus have all the methods available directly on the config class would be much nicer.

That way, you could even replace this.Helper.LoadConfig<ConfigClass>(file) with new ConfigClass(file)

Zoryn4163 commented 8 years ago

@Entoarox It should be easily possible to apply the virtual tag to all of the Helper methods, and have a class still be able to inherit from Helper. This would allow anyone to extend or redefine their own variant of Helper whilst still having its base functionality, and anyone that doesn't need those features could still easily use the Helper in the way that is offered in the original post.

It may also be an option to define an IConfig interface which could be used by the helper and any mod, but I do not personally have an extensive knowledge of interfaces and how they work.

Entoarox commented 8 years ago

@Zoryn4163 I am afraid that interfaces would not help simplify things in this case, they are a in/out blueprint, but the actual logic is left up to the implementing class, and the whole point behind a base class here would be to simplify access to methods, rather then reimplement them with identical in/out properties.

Pathoschild commented 8 years ago

@Entoarox While new ModConfig(this.BaseConfigPath) would work, I don't think it's simpler. My explanation will be long, so please bear with me. :)

What's wrong with the constructor approach?

At a high level:

It's not discoverable. A developer would need to read the manual to know config settings are supported and how they work. This one is important — I'll explain more below.

Config classes would still need boilerplate code:

internal class SampleConfig : StardewModdingAPI.Config
{
  public bool ExampleBoolean { get; set; } = true;
  public float ExampleFloat { get; set; } = 0.5f;

  public SampleConfig(string configPath)
      : base(configPath)
  { }
}

It violates the official design guidelines and it's an object-oriented anti-pattern. Constructors shouldn't have side-effects — they're supposed to construct the object, not act as a function.

And you only save one character for the most common scenario anyway:

var config = new ModConfig(this.BaseConfigPath); // your proposal
var config = this.Helper.ReadConfig<ModConfig>(); // my proposal

Discoverability

Discoverability is my main objection to both the existing system and the constructor proposal.

What is discoverability?

For our purposes, discoverability is a measure of how easily a developer can learn (and remember) to use SMAPI. If you need to look up how to do something each time, you have low discoverability.

As a rough guideline, you can rank discoverability in this order:

★★★ Developers can figure it out from the interface and its intellisense. For example, it's fairly clear what bool File.Exists(string path) does without reading the manual, and the code documentation explains any ambiguities. It's also easy to find other file operations by autocompleting on File.
★★☆ Developers need a high-level guide to understand the basic concepts that are unique to the project, but otherwise can discover the code from its interface.
★☆☆ Developers need a guide to understand each component. For example, they need to look up how to use a config file independently of the high-level concepts.
☆☆☆ The developer needs to read the framework code to understand how to use it.

Here's how I would compare the three config systems being discussed by discoverability.

★☆☆ current system

The current system works, but a new developer won't know how to use it without reading the config guide. Developers typically copy & paste the sample code to do what they want, without necessarily understanding what the different methods do. (Some of those methods should probably be hidden by SMAPI, too.)

★☆☆ constructor approach

This approach is very similar to the current system; it just hides some of the methods. A new developer won't know they can subclass Config without reading a guide, nor will they know what they need to do to actually read the file. (Calling the constructor is unintuitive, since it violates C# guidelines and design patterns.)

★★★ ModHelper approach

The ModHelper was designed specifically to improve discoverability. This approach gives you autocompletion and intellisense at every step. Here's how a developer could learn how to create a configuration class without ever opening the manual:

What are the mod's available properties and methods?
What does this.Helper do?
How do you use this.Helper.ReadConfig<T>()?

And bam! The developer just wrote the configuration parsing line, without even opening a guide. The guide still exists for reference, but it's no longer required reading. Since they now know the paradigm, they can quickly do it again in the future without looking it up (just navigate the intellisense directly).

Entoarox commented 8 years ago

@Pathoschild While I can agree that the constructor approach might not be as intuitive, I must disagree about it being a case of having side-effects, you are passing in the file that contains the values you wish the object to have, in essence, it is no different then passing those same values as arguments, in this case the constructor simply performs the reading of those values from a file for us.

I can agree that learning about the constructor itself can be annoying, it is just as difficult to grasp as a generic method on the Helper instance that is available through this, especially if you do not know to be looking there in the first place, in such a case, passing the Helper as a argument for Entry would be much better, since it makes it clear it exists for a reason.

And while I can agree that using the helper if made clearly obvious as having a use during Entry will be much more intuitive then a constructor, I still have to disagree for methods that are inherited from a base class as those are much more intuitive then having to use a helper to do things like that. While having people implement such things themselves is more then possible, it also goes directly against the purpose of having a easy and intuitive config system.

Pathoschild commented 8 years ago

@Entoarox I agree that passing it in as an entry argument would be even better, and that shouldn't cause any problems for backwards compatibility:

public override void Entry(ModHelper helper)
{
   var config = helper.ReadConfig<MyConfig>();
}

I think that would be a good compromise; what do you think?

Entoarox commented 8 years ago

I can agree on that yes :) Combined with the ability to perform save and reload commands on the instance itself directly, that would make it do everything the current setup does, but make it much more intuitive to do so.

Pathoschild commented 8 years ago

@Entoarox We could add instance methods with a bit more framework code, while making it optional for mods that don't need it.

Sample implementation

Summary

At a high level, here's the proposed approach:

Create an IConfigFile interface which has the Reload() and Save() methods, along with the properties needed to implement them.
Create a default ConfigFile implementation.
When reading a JSON file, check if the model is IConfigFile. If so, inject the needed properties.
Mod authors can optionally extend ConfigFile or implement IConfigFile on their model.

Code inside SMAPI

We'd add this code to the previous implementation:

/// <summary>Wraps a configuration file with IO methods for convenience.</summary>
public interface IConfigFile
{
   /*********
   ** Accessors
   *********/
   /// <summary>Provides methods for interacting with the mod directory, including read/writing the config file.</summary>
   ModHelper ModHelper { get; set; }

   /// <summary>The file path from which the model was loaded, relative to the mod directory.</summary>
   string FilePath { get; set; }

   /*********
   ** Methods
   *********/
   /// <summary>Reparse the underlying file and update this model.</summary>
   void Reload();

   /// <summary>Save this model to the underlying file.</summary>
   void Save();
}

/// <summary>Wraps a configuration file with IO methods for convenience.</summary>
public abstract class ConfigFile : IConfigFile
{
   /*********
   ** Accessors
   *********/
   /// <summary>Provides methods for interacting with the mod directory, including read/writing the config file.</summary>
   public ModHelper ModHelper { get; set; }

   /// <summary>The file path from which the model was loaded, relative to the mod directory.</summary>
   public string FilePath { get; set; }

   /*********
   ** Public methods
   *********/
   /// <summary>Reparse the underlying file and update this model.</summary>
   public void Reload()
   {
      string json = File.ReadAllText(Path.Combine(this.ModHelper.DirectoryPath, this.Filename));
      JsonConvert.PopulateObject(json, this);
   }

   /// <summary>Save this model to the underlying file.</summary>
   public void Save()
   {
      this.ModHelper.WriteJsonFile(this.Filename, this);
   }
}

In ModHelper:

   /// <summary>Read a JSON file.</summary>
   /// <typeparam name="TModel">The model type.</typeparam>
   /// <param name="path">The file path relative to the mod directory.</param>
   public TModel ReadJsonFile<TModel>(string path)
       where TModel : class
   {
      string fullPath = Path.Combine(this.DirectoryPath, path);
      TModel model = JsonConvert.DeserializeObject<TModel>(File.ReadAllText(fullPath));
      if (model is IConfigFile)
      {
         var wrapper = (IConfigFile)model;
         wrapper.ModHelper = this;
         wrapper.FilePath = path;
      }
      return model;
   }

(We'd probably need some reflection logic in ConfigFile to reset all properties to the default values before repopulating. It's omitted here for simplicity.)

Basic config usage

Most mods would create a simple config class:

internal class SampleConfig
{
    public bool ExampleBoolean { get; set; } = true;
    public float ExampleFloat { get; set; } = 0.5;
}

...and use the helper:

public override void Entry(ModHelper helper)
{
   // common
   var config = helper.ReadConfig<MyConfig>();

   // uncommon
   config = helper.ReadConfig<MyConfig>(); // reload
   helper.SaveConfig(config); // save
}

This is fully discoverable through the interface, and especially easy for most mods which only read their config settings once.

Advanced config usage

Mods that often read/write their configuration, or which need to customise the read/write logic, would extend ConfigFile (for default I/O) or implement IConfigFile (for custom I/O):

internal class SampleConfig : ConfigFile
{
    public bool ExampleBoolean { get; set; } = true;
    public float ExampleFloat { get; set; } = 0.5;
}

They'd use the helper to instantiate the config, then use the instance methods:

public override void Entry(ModHelper helper)
{
   var config = helper.ReadConfig<MyConfig>();
   config.Reload();
   config.Save();
}

Would that address your use cases?

Entoarox commented 8 years ago

Yes, this looks like it resolves all use cases, from the very basic, to the advanced, to the expert level needs!

Pathoschild commented 8 years ago

Yay! Any objections to adding this to SMAPI 1.0?

Zoryn4163 commented 8 years ago

None from me. Looks like an overall improvement.

StefanOssendorf commented 8 years ago

As said in the chat. I don't see the benefit of providing the parameter. Yes, I see it immediatly, but I don't need it in the Entry method. It's the place where I hook up al things and then handle everything else in my event-handlers. It's really likely that I save the Helper in a backfield which is already provided by a property. I think it should be stated in the very basic tutorial that such functionality is provided by the base-class and can be access via this. or base.

Entoarox commented 8 years ago

@StefanOssendorf The whole point behind this change is to make it more intuitive to do things, relying on a tutorial goes against that point.

StefanOssendorf commented 8 years ago

@Entoarox Yes, but I won't start developing a Mod without a basic tutorial. Especially when I'm new to developing a mod.

Entoarox commented 8 years ago

@StefanOssendorf Despite the tutorial, I still have to every so often use autocomplete with this because I've forgotten the exact name, if all that mod-related information is going to be put into a single property, I'd rather not have to go this.propertynameImighthaveforgotten.actualthingIwannaknowIbutforgot but just go entryarg.propertyIforgot...

StefanOssendorf commented 8 years ago

I'm a fan of SRP so everything in one property would be "overkill". Despite that I don't use this because it's redundant, but that's not my point. My point is, I don't need the provided functionality from the parameter in the Entry method. Here I'm assuming I'm not handling the game events in one big lambda. So when I need access to "helpers" I need them in the event handlers and then I will write a nice backing field and store it there. And this makes the property useless. On the other hand, when I get the parameter why not simplifiying the implementation from inheritance of Mod to something like

IModEntry {
   void Entry(ModHelper modHelper)
}

So I only have to provide these Entry-Method and the rest has to be handled from the mod itself.

Entoarox commented 8 years ago

Because of how loading is done.

As to using things in event handlers, most of us init our configs and such during the Entry and save the instance to a static, The only times I needed a mod property outside of Entry is because I needed to be able to access files relative to the dll, everything else I do during Entry.

Pathoschild commented 8 years ago

For the record, SMAPI will probably add a property either way (if only for its own use); the question is mainly whether to also pass it in as an argument. I guess we could make the property internal so mods can't use it, but why?

Pathoschild commented 8 years ago

Done in the upcoming 1.0 release; see the draft 1.0 guide to writing mods.

Pathoschild / SMAPI