Update config system about candybar

[Lokaltog]

Ref #41 #40 #27

[Lokaltog]

Started in feature/improved-config.

[ricardomv]

i really like the configuration system as it is we have a simple way to get configs and it's really easy to implement a way to change them and update the config file, or even to reload the config file. this doesn't seem like a big thing but it is a really hard to get and get right if we can pass the complexity of reading and writing from a config file to a library i think we should. It's not worth dropping the dependency of jansson to start depending on libconfuse

[ricardomv]

@Lokaltog can you describe the issues you are having with the current json config?

[Lokaltog]

My main concern with using jansson or JSON as the config file format is that to me it feels increasingly complex and clunky to work with. It also feels very error-prone, both regarding user errors (misplace one comma or use the wrong kind of quotes and wkline won't even launch) and error handling in C (there's no predefined config format, currently we just request data from the config file and hope the values are of the correct type or even there at all). I'd also like the possibility to provide default values in the widgets themselves, so if a value isn't present in the config file we can fall back to a default value instead.

A couple of things I think is appealing with libconfuse:

• much easier to read file format
• supports comments in the config file
• user-friendly, less error prone (handles both kinds of quote marks, isn't strict about commas separating options, etc)
• allows for providing default config values in the config definition, this is particularily useful if the widgets can define default values as you can add any widget without adding an associated config block for it to work
• supports config validation callbacks when the config file is loaded, with optional fallback to defaults
• supports updating/writing config files based on the predefined config format

Some of this stuff is possible to do with jansson, but I think it looks much simpler to do it with libconfuse. If we do stuff like implementing default values/fallback configurations or a locked config format it would be a bit like reinventing the wheel on top of jansson instead of using a config library that provides this functionality out of the box.

Could you explain why you'd rather have the JSON config file? I can understand the simplicity of get_string_value(get_config_value('key')) and not having to deal with predefined config structs and stuff like that, but I'm not sure if this is really a good thing as it's much more prone to errors.

[Lokaltog]

@ricardo-vieira I just wanted to add that I'm not trying to argue or be stubborn here, so I hope I haven't been offending you by initially disagreeing with some of your suggestions (like the other config issue)! I respect your opinion on these issues and I know very well that I could be completely wrong about wanting to exchange JSON for another format, which is the reason I'd really appreciate your feedback. That way I'll understand why it's a bad choice to go for another config format, and it will be much easier for me to accept the current way we're doing configuration of wkline. :)

[ricardomv]

Syntax error handling.(gives out the error)
No need to save configs in structures pre created (as you said)
Easy to reload config file
Easy to write back the config files (are you going to parse all the widget structures to see what changed?)
But you can implement it a see witch one works better. I am sure you will find many other reasons.

[ricardomv]

You wont offend me by disagreeing with me, you are free to have your opinion and you are free to be wrong :D. I am just trying to point you in the direction i think is better.

[Lokaltog]

Allright! Since this is a learning project for me I think I'll do some work implementing libconfuse just to see how it works and learn more C. I'll report back and promise not to merge any changes until we've reached a consensus on what's the best solution.

[Lokaltog]

What are your thoughts on libconfig? It looks a bit bigger/complex than libconfuse, but it also has some pretty neat functions like this one which allows you to fetch a config option based on the path in the config file:

config.lookupValue("path.to.value", ...)

It supports all kinds of stuff that we probably don't need like @include directives and stuff like that, so it may be a bit overkill.

[ricardomv]

one time i tried to look for a config library and i discovered that there is no standard everyone uses it's own implementation. Like weston you might like their system it's pretty simple.

[Lokaltog]

Yeah, that's the impression I've gotten as well, I did a pacman -Qs config and noticed I have a bunch of different config libs installed as dependencies: at least confuse, dconf, dotconf, gconf and libconfig. I'm definitely seeing how it might be just as simple just to just stick to the JSON format since there's no standardized config format for C.

[Lokaltog]

[ricardomv]

yeah i always think of that xkcd pic when i see something about standards :D

[nHurD]

Glib has a built-in config file handling mechanism: https://developer.gnome.org/glib/2.37/glib-Key-value-file-parser.html. If you're already linking to a lot of GTK / Glib / GModule type stuff, it may behoove you to go with that rather than adding another dependency

[Lokaltog]

I agree, since we're already depending on GTK/glib this should probably be the alternative if we're going with another config format.

[nHurD]

Agreed. I don't really feel that JSON is the issue, but rather how it's handled once loaded.

One possible way around this may be to simply store the configs in structures. One for the main config, then have the individual widgets manage their own configs, and have them loaded upon widget initialization.

Another alternative would be to simply create macros to wrap around json_get_string_value(...) calls..

[Lokaltog]

I'm thinking about implementing something like that on top of the current JSON config. I'm thinking that we could do both, with every widget having a struct with valid config values/types with sane default values like this:

struct widget_config {
   int update_interval = 1000;
   char *host = "localhost";
   int port = 6600;
   int timeout = 5000;
};

The widgets are required to provide a widget_config_init function or something like that which will be fed the raw JSON config object, and it's up to the widget to store these properties in the config struct with the correct types and error handling. The widgets will clean up any malloc'ed config values in the widget cleanup handler.

This could be done in addition to macros wrapped around the json_get_*_value functions to make the code a bit cleaner. What do you think, maybe the macros aren't necessary? Or am I overthinking the issue? The goal is to have static types in the config object, loading of config values with error handling before widget_init is called, and default fallback values.

[nHurD]

I think both would be appropriate. This way it's easier for other developers to use the configuration system without getting bogged down with multiple calls to fetch data.

Also, by utilizing this method with the structs, default values can be provided in the event that config values are missing, without the calling method really caring about the source of the value..

[Lokaltog]

I've started implementing default values and config structs for the JSON config, along with macros for fetching config values. Could you give me your feedback on these changes?

The goal here is to 1) provide default values in static widget config structs, and 2) make it very easy to pass values from the JSON file to the config struct in widget_init without having to screw around with the json_get_*_value functions (this has been accomplished by using macros).

I've demonstrated the few changes required to implement this in the external IP widget in a1fd743. The changes are also backwards compatible if widget authors for some reason don't want to use this feature (all the other widgets without config structs work fine).

@nHurD @ricardo-vieira could you guys please give me some feedback on this? Do you think we're approaching a decent compromise through these changes, or do you think we should resolve this issue differently?

[Lokaltog]

A side effect of this is that you also can have a very small config.json file that only overrides certain config values, and uses the default values for everything else, which I think is a pretty nice feature.

[nHurD]

Also, once everything has been initialized, there'd be nothing stopping you from cleaning up that object's reference in memory.

The macros look good. Nice work making sure you don't squash the defaults if no values are present.

[Lokaltog]

Which object should I clean up?

[nHurD]

The main json_t config object, unless it's stored else where..

[Lokaltog]

I think it's being freed here.

I tried using malloc for the battery path, how does this look? https://gist.github.com/Lokaltog/9490420

I still need to free the battery path, how would you do that in this case in the widget_cleanup function?

[nHurD]

Opps, you're right, it is getting freed there.

The malloc looks good.

RE the battery path, let me take a look, and I'll get back to you in a few..

[Lokaltog]

Awesome, thanks for the help!

[nHurD]

Here's how I'd do it: https://gist.github.com/nHurD/9490807

I'd just clean it up after it's no longer needed.

[Lokaltog]

It appears to be needed in the for(;;) loop, after the first run (which I don't really understand how even works) it fails with this message:

process 5060: arguments to dbus_message_iter_append_basic() were incorrect, assertion "_dbus_check_is_valid_utf8 (*string_p)" failed in file dbus-message.c line 2675.
This is normally a bug in some application using the D-Bus library.
  D-Bus not built with -rdynamic so unable to print a backtrace

I'd guess this is caused by widget_send_update(widget, properties_proxy, dbus_path); in the for loop which uses this variable?

I think the variable is needed until the function or thread ends since it's passed along to widget_send_update every loop, and to avoid leaking memory I think it has to be cleaned up in the widget_cleanup function. I have no idea how to do this more elegantly (this is the reason I used a static array size to begin with), so I'd really appreciate any help here.

[nHurD]

Ok, let me take another stab at it, sorry about that

[nHurD]

That's odd...I ran it with the free() statements in place and haven't received that error...

[Lokaltog]

I'm not sure what's the issue, but it crashes every time i run wkline on my setup when I have the free calls. Sometimes it appears to take longer, I just did a test that took a couple of minutes before it crashed. It only crashes when I have the free calls from your patch.

[nHurD]

Huh, weird. Anyway, here's an updated patch: https://gist.github.com/nHurD/9490807

[Lokaltog]

Ah, that looks great, I think this is exactly what I was looking for! I thought I had to use a struct to pass multiple arguments as a void pointer if they didn't share the same type, but it kinda makes sense that you can just make an array of void pointers and pass that instead. Great stuff!

[nHurD]

Cool, glad I could help!

[Lokaltog]

I did a couple of changes, first I use void * instead of gpointer, I'm trying to use as little of glib as possible and void * looks just fine to me (and I think "gpointer looks better than void*" is a pretty bad reason to depend on glib)!

I also don't bother casting the pointers back to DBusGProxy before freeing them in widget_cleanup. The cleanup function only frees the pointers and won't use the data for anything else, so is there any reason to cast them to their type before freeing them (other than to be more explicit)?

[Lokaltog]

Here's the updated patch with the changes I mentioned: https://gist.github.com/Lokaltog/9490420

[nHurD]

As long as it doesn't crash, and valgrind doesn't complain about it, then it should be fine...

Should line 34 of that patch be void ** rather than void *? Other than that, the patch looks cool

[Lokaltog]

gpointer is an alias for void* so I just substituted that in your patch. I tried void* now but got a bunch of these:

../src/widgets/battery.c:44:18: error: dereferencing ‘void *’ pointer [-Werror]
  if (cleanup_data[0] != NULL) {
                  ^
../src/widgets/battery.c:44:2: error: void value not ignored as it ought to be
  if (cleanup_data[0] != NULL) {

I'm reading void **x = (void*)y as dereferencing the void pointer y to an array of void pointers x, is that correct? Anyways, it works fine and doesn't crash or anything, so if the rest of the branch looks good I'll merge it into develop. Thanks for the help and feedback!