-
Notifications
You must be signed in to change notification settings - Fork 17
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
Update config system #44
Comments
Started in feature/improved-config. |
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 |
@Lokaltog can you describe the issues you are having with the current json config? |
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:
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 |
@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. :) |
Syntax error handling.(gives out the error) |
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. |
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. |
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:
It supports all kinds of stuff that we probably don't need like |
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. |
Yeah, that's the impression I've gotten as well, I did a |
yeah i always think of that xkcd pic when i see something about standards :D |
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 |
I agree, since we're already depending on GTK/glib this should probably be the alternative if we're going with another config format. |
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.. |
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 This could be done in addition to macros wrapped around the |
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.. |
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 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? |
A side effect of this is that you also can have a very small |
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. |
Which object should I clean up? |
The main json_t config object, unless it's stored else where.. |
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 |
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.. |
Awesome, thanks for the help! |
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. |
It appears to be needed in the
I'd guess this is caused by I think the variable is needed until the function or thread ends since it's passed along to |
Ok, let me take another stab at it, sorry about that |
That's odd...I ran it with the |
I'm not sure what's the issue, but it crashes every time i run wkline on my setup when I have the |
Huh, weird. Anyway, here's an updated patch: https://gist.github.com/nHurD/9490807 |
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! |
Cool, glad I could help! |
I did a couple of changes, first I use I also don't bother casting the pointers back to |
Here's the updated patch with the changes I mentioned: https://gist.github.com/Lokaltog/9490420 |
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 |
I'm reading |
I'm starting to think the current way of having a JSON-based config system will cause more and more issues over time. First of all, we don't really need a tree-structured config file, a flat file could work equally well. If we implement JavaScriptCore extensions we won't need to depend on a JSON library at all. Here's a proposal for an improved config format:
Use an existing config library like libconfuse.
Each widget shared object library will provide a predefined config struct with default values, if we use libconfuse it would be something like this.
The config loader will override any default values in the config structs for each widget with new values found in the config file.
Any ideas/suggestions?
The text was updated successfully, but these errors were encountered: