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

Some redesigning of the settings API #408

Merged
merged 6 commits into from Apr 21, 2020
Merged

Some redesigning of the settings API #408

merged 6 commits into from Apr 21, 2020

Conversation

SquidDev
Copy link
Member

@SquidDev SquidDev commented Apr 18, 2020

This is based on some of the comments in #351, and some other things I've wanted to do. There's a couple of things going on in this change:

  • Add the ability to describe a specific setting. settings.define("my_setting", { ... }) allows you to provide a default value and description about a specific setting. This has several benefits:

    • /.settings now only stores options which have been changed, as the bios registers default values, rather than stating their value exactly.

    • settings.clear and settings.unset reset something to its default value, rather than clearing it entirely.

    • The set program can now print whether an option has been changed, and its description:

      The fancy new set program

  • settings.load/settings.save now default to /.settings

  • settings.set and any other modifying function will fire a settings_changed event, with the arguments name:string, new_value, old_value.


I'd like to get some feedback from others before merging with this - there's definitely some design decisions I'm not happy with. As always, the PR can be tried online using cloud-catcher.

  • settings.add is a bit of a confusing name. We probably also need a way to remove a setting's metadata, and really don't want to go with settings.remove.

  • Firing an event whenever a setting changes feels somewhat inefficient, especially when doing bulk actions such as loading from /.settings. On one hand, this is exactly what the event system is for, but given that 99% of programs won't care about this, I wonder if there's another way. Ideas?

    The original issue (Settings API Improvements #351) suggested batching all changes into one single event. It works, but it also feels inelegant in its own way. Ughr, I don't know :).

 - The store is now split into two sections:
   - A list of possible options, with their description and default
     value.
   - A list of values which have been changed.
 - settings.{set,unset,clear,load,store} operate using this value list.
   This means that only values which have been changed are stored to
   disk.
   Furthermore, clearing/unsetting will reset to the /default/ value,
   rather than removing entirely.
 - settings.add can be used to register a new option. We have migrated
   all existing options over to use it.
 - The set program will now display descriptions.
@SquidDev SquidDev added enhancement An extension of a feature or a new feature. feedback wanted Tell me what you want, what you really really want. area-CraftOS This affects CraftOS, or any other Lua portions of the mod. labels Apr 18, 2020
@Lemmmy
Copy link
Contributor

Lemmmy commented Apr 18, 2020

I wonder if settings.define and settings.undefine would be better names. This makes it clear that you need to explicitly define a setting for the computer to know about it's description.

I agree that batching things into a table doesn't feel very CC. Two alternative (but perhaps not any better) options come to mind, as food for thought:

  • Register callback handlers instead of events (very not CC), e.g. settings.onChanged(function(setting, oldValue, newValue) ... end)
  • A setting, off by default, for emitting settings events - settings.notifyChanges or settings.emitChangeEvents perhaps.

@SquidDev SquidDev linked an issue Apr 18, 2020 that may be closed by this pull request
@SquidDev SquidDev marked this pull request as ready for review April 19, 2020 18:34
@JakobDev
Copy link
Contributor

We could also add a type check for settings

@SquidDev
Copy link
Member Author

Yeah, I did wonder about allowing some basic type = "string" field, but wasn't sure as some settings have dynamic types (such as string or nil). But even something simple would be a good start for now.

@JakobDev
Copy link
Contributor

You can use a table type={"string","nil"}. it would also be nice, if we have custom types e.g. color.

@Lupus590
Copy link
Contributor

Tables like the one @JakobDev described is how my arg validator works, if you want to take bits of that then here's a link to it.

I'm leaving off anything more complex for now, as I want to have a think
about design. This is good enough for CC:T itself, which I guess is the
main thing.
@SquidDev SquidDev merged commit 1fc0214 into master Apr 21, 2020
@SquidDev SquidDev deleted the feature/new-settings branch April 21, 2020 10:38
@SquidDev
Copy link
Member Author

I'm not entirely with some of the error messages, especially the ones that the set program spits out, but this'll do for now.

@SquidDev SquidDev mentioned this pull request May 4, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CraftOS This affects CraftOS, or any other Lua portions of the mod. enhancement An extension of a feature or a new feature. feedback wanted Tell me what you want, what you really really want.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Settings API Improvements
4 participants