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

Alternative to global static settings dictionary map for config #125

Open
nyanpasu opened this issue Apr 4, 2015 · 15 comments
Open

Alternative to global static settings dictionary map for config #125

nyanpasu opened this issue Apr 4, 2015 · 15 comments

Comments

@nyanpasu
Copy link
Member

nyanpasu commented Apr 4, 2015

Current implementation is a hash map with string keys to string values.

It's simple, but not effective.

Also a hash map is so fucking noob.

I propose creating a new class called SettingsOption, which will contain:

  • Name
  • Value
  • Description (optional)
  • Type (optional)
  • Permitted values (i.e range, string regex)
  • Callback functions that will be called when a change occurs (so that any function could register for changes in option)

This could allow the config file to be self documenting when it's first generated.

Even though most config options should be self documenting by name alone.

Along the way we could consider:

  • SettingSection (settingOption can be grouped by category)

The current Setting class will be replaced with a abstract Setting parent that will contain:

  • init
  • clean up
  • default
  • read/parse
  • write/update/generate

And other modules will inherit from it, adding their own options.

@benwaffle
Copy link
Member

If you're doing simple config files then I'm not sure how you plan to integrate description and all that. As for input validation, etc... start with the list of settings and figure out the minimum needed that's still extensible.

@nyanpasu
Copy link
Member Author

nyanpasu commented Apr 5, 2015

I'm not sure how you plan to integrate description and all that

# Comments
# This option will fuck you're waifu
# Acceptable value(s):
# - True
WaifuNTR=True

@IGI-111
Copy link
Contributor

IGI-111 commented Apr 5, 2015

I do agree that the current implementation is simplistic in that it only handles strings, but there is nothing to be said against the use of std::map here, associative maps are the fastest and most comprehensive way to get options by their handle.

Oh and you're wrong in assuming that std::map is a Hashmap, it's actually implemented as a red-black tree.

Anyhow, what i would recommend is to move to YAML and import a minimal parser for that format that does object mapping, there is no need to reinvent the wheel.

@benwaffle
Copy link
Member

why not ini files and strtok for parsing?

@IGI-111
Copy link
Contributor

IGI-111 commented Apr 5, 2015

Because

  • We're already pretty much parsing INI files except we don't support sections (they're just key=value)
  • strtok() is C not C++ and isn't thread safe

@nyanpasu
Copy link
Member Author

nyanpasu commented Apr 5, 2015

A YAML parser sounds quite good for serializing settings to a config file and back, but my real objective here is to introduce some addtional features and extensibility how we use already use settings.

I'm quite particular about the callback functions, it almost feels essential.

Associative maps are fast and straightforward, but data members of an object would be even faster to access. The issue here being that serialisation/deserialisation wouldn't be dynamic, initialising and cleaning up will involve more manual labour.

@benwaffle

why not strtok for parsing?

Reinventing the wheel. And it's bug-prone approach. By the time you've gotten parsing bug free and usable you might as well have written a YAML/whatever parser that only covers 40% of the spec.

I'm not fond of YAML itself but I want to avoid dealing with serialisation/deserialisation ourselves.

I like ini files, key/value lines with simple comment support are essentially all we need.

Even though /g/ really fapped about it, I don't think anyone out there will modify a text config file for something that has a graphical interface and is meant to be used with said graphical interface.

If we introduce a daemon/remote interface, you would still use a control program, web/whatever interface that goes with it.

@IGI-111
Copy link
Contributor

IGI-111 commented Apr 5, 2015

@nyanpasu so what are you suggesting? In the end C++ is still statically typed so unless you intend to write a lot of boilerplate and standardize the options you won't have objects mappings for the options.

The problem with a key value store is that it is just that, a key value store.
Its memory mapping is just an associative map and as much as you can elaborate on the interpretation of the mapping you can't really optimize it without limiting the options.

If you want more out of the config management, the way to go really is to write the config file in a proper markup language. I suggested YAML because i know it's good at writing config files.

As with not storing options in a textfile, you might as well serialize it and store a binary then, but that's a bit stupid given that we don't need performance and adding a parser is still less complicated than what we have now.

@benwaffle
Copy link
Member

Reinventing the wheel.

strtok for ini, not for yaml - all you do is split on '='

I agree though, let's ignore ini/yaml/whatever and just read/write the simplest way possible (and use GUI to change settings)

@nyanpasu
Copy link
Member Author

nyanpasu commented Apr 5, 2015

As with not storing options in a textfile

I don't mean avoid text altogether (although storing config settings in a database sounds like something), I just meant it as a point about caring less how things will be stored, with the assumption that people are less likely to worry about having to tweak config files manually with a text editor when a more appealing alternative exists.

The problem with a key value store is that it is just that, a key value store.
Its memory mapping is just an associative map and as much as you can elaborate on the interpretation of the mapping you can't really optimize it without limiting the options.

Nor am I arguing against the usefulness of a string-string map, only that it's too limited (simple as in it leaves more to be desired in this context of a config storage medium)

Anyho, some pseudocode will help clarify whether what I'm thinking about is a stupid idea.

I had something like this in mind:

// Abstract settings class...
// so that a serializer can deal with different settings type transparently
class Settings
{
    // Set defaults etc.
    virtual int init();
    // Perhaps a hypothetical SettingsFile class could be used for an abstraction between the values and the parser?
    virtual int parse(SettingsFile file);
    virtual int save(SettingsFile file);
}

// An example of an implementing class
// e.g a section in a config file?
class SettingsCore: public Settings
{
    Option savePath;
    Option maxSeeds;
    Option uploadLimit;
    // etc...

    virtual int init() {
        savePath = new Option("Save path", // Option name
            "Where you dump your fucking hentai to", // Option desc
            OPTION_TYPE_STRING, // Option type
            "~/Downloads"); // Option default

        // ... and other members here
    }
}

class Option
{
    std::string name;
    std::string description;

    // something like this
    enum {
    OPTION_TYPE_STRING = 0;
    OPTION_TYPE_INT;
    // ...
    } type;
    // I have no fucking idea how unions work
    // or if they are safe in C++
    // or if better alternatives exists
    // (like fucking templates)
       union
    {
        string
        int
        float
        bool
    } values;
    // ... and so on

    // Right function will be called because the caller should know which type they expect
    // For instances of dynamic
    // Could be avoided if I knew how to into templates. I hate C++.
    getValueString();
    getValueBool();
    getValueInt();
    getValueFloat();

    // return a set of lines that would represent it in a config file
    // or maybe some way to update an existing line
    // e.g updateLine(string line)?
    std::string serialize();

    // parse a line into options
    int deserialize(std::string line);
}

If you've read all that you would realize I'm fucking shit at sepples, but let's try to argue about the spirit of this pseudocode.

I think it would be more extensible to have settings done this way? It's easy to add on more functionality as needed to Option members and it addresses dealing with strings directly as we do now.

I believe that doing things this way:

  • will save calling functions the hassle of little boilerplates every time the have to retrieve/set a value.
  • converting values from strings and forth
  • doing their own validation
  • they just need to call Settings.optionName.getValue() or something
  • serialisation and deserialisation is more robust this way?

@benwaffle
Copy link
Member

How is that better than a map? Do you just want to do stuff with types?

@benwaffle
Copy link
Member

If we were only using Qt or gtk this would be easy as each has support for saving user configurations. What if we just stick to gtk instead of planning to use every toolkit but accomplishing nothing?

@IGI-111
Copy link
Contributor

IGI-111 commented Apr 6, 2015

@nyanpasu That's what i thought you described and my main gripe is with

 Option savePath;
    Option maxSeeds;
    Option uploadLimit;
    // etc...

That "etc" and all of its "..." brothers are the main reason this is a bad idea and one that has been widely discussed at that.
Having to write in more than one place to add functionality is bad design, here's something that looks like your idea but only requires one line each time you want to add an option: in this snippet.

@nyanpasu
Copy link
Member Author

nyanpasu commented Apr 6, 2015

That "etc" and all of its "..." brothers are the main reason this is a bad idea and one that has been widely discussed at that.

I see.

Well at this point I need to do more research, I lack the expertise needed to design something that won't turn stale after two or three iterations of implementation. I'll get back to this issue after some.

I like your improvements, but I've got a couple of question(s):

  • Will we still be able to iterate through the options? How would one do it and be able to retrieve the correct values (their types?)?

@benwaffle

If we were only using Qt or gtk this would be easy as each has support for saving user configurations. What if we just stick to gtk instead of planning to use every toolkit but accomplishing nothing?

Yeah, say that again if we ever get rid of core. Core is handling the configuration, not gtk. Toolkit specific config can be handled by the respective toolkit libraries, but that would just cause gtorrent settings to be all over the place.

@benwaffle
Copy link
Member

I'm suggesting we abandon the qt and ncurses repos and just use GTK+. Qt is empty and ncurses has almost nothing. libtorrent is already the cross-platform and cross-client library, so it looks like gtorrent-core is just trying to provide a framework around that. That's great and all but it seems like a lot of effort to build something nobody needs. Why would continuing like this not fail again? As for the toolkit, qbittorrent uses Qt and rtorrent uses ncurses, so why not strive to make a great GTK client?

@IGI-111
Copy link
Contributor

IGI-111 commented Apr 6, 2015

@nyanpasu

Will we still be able to iterate through the options?

Well you will just have to iterate through the map, but the only case where you'd have to do such a thing is when you're reading/writing them and that's handled in plain strings, what i wrote is the abstraction layer to use inside the application that will be generated by that very code.

How would one do it and be able to retrieve the correct values (their types?)?

In this particular instance I'm using static casts (they do compile time checks) so you just have to document what type each option is and specify it when you store or retrieve them, since C++ is statically typed there is no way to store the types, you have to tell the compiler each time you call put() or get() because templates cannot deduce the type on their own (this is not Haskell).

The good thing with this approach (versus dynamic casts) is that the coherence of the put()s and get()s is checked at compile time and not at run time.
Though if you ever find a way to only specify the option type once without resorting to huge ugly type enums and switches i'll be interested.

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

3 participants