Request for help: Provide JSON config data to widgets about candybar

[ricardomv]

let me finish setting the reserved space in the screen then i will look into this

[Lokaltog]

Nice, thanks a lot!

[nHurD]

I actually started looking into configuration handling and dynamic loading of widgets via an API. It's in my plugins branch if you want to take a look there.

[Lokaltog]

I've gotten about as far as you got, I tried almost the same as this solution but I think my file either segfaulted or failed to store the pointer value after returning from the function.

[nHurD]

it looks like your issue is stemming from using uninitialized pointers to memory. Have a look at one of the comments I posted on your commit.

[Lokaltog]

Here's the problem:

diff --git a/src/load_config.c b/src/load_config.c
index 643be91..dc81c46 100644
--- a/src/load_config.c
+++ b/src/load_config.c
@@ -7,17 +7,20 @@ load_config_file (wkline_config_t *config) {
    const char *user_config_file = g_build_filename(g_get_user_config_dir(), "wkline", "config.json", NULL);
    json_error_t err;
    json_t *json_config = json_load_file(user_config_file, 0, &err);
+   json_t *widget_data;

    if (! json_config) {
        fprintf(stderr, "\n***************************\nFailed to load config file!\n***************************\n\n%s\n\nPlease make sure that '%s' exists and contains valid JSON data.\n\n", err.text, user_config_file);
        exit(1);
    }

+   widget_data = json_object_get(json_config, "widgets");
+
    config->height = json_integer_value(json_object_get(json_config, "height"));
    config->position = json_string_value(json_object_get(json_config, "position")) == "top" ? WKLINE_POSITION_TOP : WKLINE_POSITION_BOTTOM;
-   strcpy(config->theme_uri, json_string_value(json_object_get(json_config, "theme_uri")));
-   strcpy(config->background, json_string_value(json_object_get(json_config, "background")));
+   config->widget_data = widget_data;

+   wklog("%i", json_is_array(config->widget_data));
    json_decref(json_config);

    return 0;
diff --git a/src/load_config.h b/src/load_config.h
index 29f2d9d..2450f05 100644
--- a/src/load_config.h
+++ b/src/load_config.h
@@ -11,6 +11,5 @@ enum wkline_position {
 typedef struct wkline_config_t {
    int height;
    enum wkline_position position;
-   char theme_uri[CONFIG_BUF_SIZE];
-   char background[CONFIG_BUF_SIZE];
+   json_t *widget_data;
 } wkline_config_t;
diff --git a/src/wkline.c b/src/wkline.c
index 8da5e29..247dbcb 100644
--- a/src/wkline.c
+++ b/src/wkline.c
@@ -71,6 +71,7 @@ main (int argc, char *argv[]) {

    wkline_config_t config;
    load_config_file(&config);
+   wklog("%i", json_is_array(config.widget_data));

    gtk_init(&argc, &argv);

As you can see, config.widget_data isn't a valid JSON array after being modified by the function. I may be screwing up by using pointers in the wrong way or something, let me know if you see what's wrong.

[Lokaltog]

I've tried doing it by allocating memory with malloc, this doesn't work either:

    wkline_config_t *config = malloc(2000); // just a random large number to test
    load_config_file(config);
    wklog("%i", json_is_array(config->widget_data));

[Lokaltog]

Worked after initializing the struct as a pointer and moving json_decref to the end of main().

[nHurD]

Cool. I started messing around with it too. Here's what I wound up coming up with: nHurD@c4f70f0

I'll try to get it working a la memcpy to see if I can get it working that way..

[Lokaltog]

Neat! Here are the latest updates to config.json and the config code:

cd767f6
0c289fe

If you could find a way to do some malloc/memcpy magic to copy the contents from widgets_enabled into the wkline_config_t struct that would be great! It's getting late here so I won't be able to work on this today. I tried reading about 2D char arrays and malloc but I'm way too tired to try to understand that right now...

[nHurD]

aandd...memcpy: nHurD@f332cea

[nHurD]

Yeah, I'll take a stab at that later tonight :)

[ricardomv]

please see my branch i have some inline comments
https://github.com/ricardo-vieira/wkline/tree/json-config

[Lokaltog]

Looks good, seems like a cleaner solution to the problem!

Regarding failing on invalid configurations, what do you think about loading the user config and the default config separately, then falling back to the default values if the user config is missing a value (but still quitting if the user config is invalid JSON)? If I was a user I'd find it very annoying if wkline stopped working every update because the config file had another key added or something like that - in this case it's particularily annoying since wkline is a WM status bar.

We could warn the user that they should review the config file but still be able to run with default values.

[ricardomv]

yes we can load the default config from /etc/wkline/conf.json and use it as a fallback. and warn the user that there is a new widget.

[nHurD]

I've got the 2-D array setup working for enabled widgets setup in my json-config branch. Even if we don't wind up using it, it can at least serve as an idea of how to do it: https://github.com/nHurD/wkline/tree/feature/json-config

[ricardomv]

we can't have the configs saved anywhere else than the json_t *config in the future i would like to just reload the config file and have the widgets automatically adjust to the new settings

[nHurD]

Sure, that makes absolute sense. There's much less cruft to pass around that way. Have your changes been merged? If not, I'll fetch from your branch before starting on the modules.

[ricardomv]

no it's not finished. but the only thing i need to pass to the widgets is a pointer with the type struct wkline_t. btw @Lokaltog can we start using struct instead of typedef i like to use it to know which structures are created by us.
To define a structure you would do

struct name {
       int var;
};

[Lokaltog]

Sure, we can do structs instead. How are using structs instead of typedefs better, I had the impression typedefs are more convenient as you won't have to add "struct" everywhere you're using it.

BTW, how is the JSON config development going?

[nHurD]

I'm with @Lokaltog on this one. I prefer using typdef struct {} name_t; myself, but am cool with whatever we decide to go with. As an idea for determining what types we create versus what's provided via other APIs, we can do something like this:

typedef struct {
    int var;
} wkline_name_t;

Anyway, it's just a thought.

[Lokaltog]

Prefixing global stuff that we define with wkline_ seems like a good idea in general.

[ricardomv]

i'm good with typedefs but i just remembered another benefit of using just struct is that in heather files you can have for example

struct wkline_t;
int some_function(struct wkline_t *var)

you don't need to define the whole structure

[Lokaltog]

That's a pretty nice feature, but would we use it? Would we have all our struct definitions in one main header and declare them in headers that use them?

[ricardomv]

no no this is used for private structures for example

// this is the global structure of the program
struct wkline_t{
        struct widget_x *x;
}

you dont have to define struct widget_x anywhere else outside x.c

[ricardomv]

i should say global not as a global variable but as used in the whole program we allocate wkline in the main and only free it when we return

[Lokaltog]

I honestly don't have an opinion on the subject as I barely have two weeks of experience with C, so I'll let you guys decide if we're going for structs or typedefs. Either way is totally fine with me.

[ricardomv]

you can see an example here with typedefs this would insane. you can see the structs defined in the .h here

[Lokaltog]

Haha, that's a pretty convincing example. It's fine with me (seems like a good way to avoid potential headaches in the future), if you want to do the refactoring send a PR and I'll merge it.

[ricardomv]

today i'm going to work on the configuration loading i think it will change a lot of structures

[ricardomv]

what if i told you all you need to get a property in a widget is
json_string_value( wkline_widget_get_config(widget, "config_you_want"))
you just have to change json_string_value to json_integer_value if you are asking for an integer

[ricardomv]

oh and widget is a new struct wkline_widget_t

[nHurD]

I as just about to ask that

[ricardomv]

i will push what i have to my json-config branch if anyone wants to help porting widgets to the new configuration type

[nHurD]

Ok. I might have some free time tonight to take a look at it.

[Lokaltog]

That's awesome, great work!

[Lokaltog]

Closing this issue as it's been implemented in the develop branch.