[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Patch] Adding a reload-config-file uicb.



Hello Nikos,

At 1192083334 time_t, Nikos Ntarmos wrote:
> The following patch adds a ui callback to reload the config file. The
> awesome_config struct has been augmented with a char *configpath member
> holding the path to the configuration file. The code for the new uicb
> was taken from awesome.c:main(...), so please check that I haven't left
> something out by mistake. I've hardcoded the uicb prototype in config.h
> to avoid a dependency recursion, since the UICB_PROTO is defined in
> common.h which in turns includes config.h.

I've tested it and it fails in Xinerama mode, I knew it.
There's a number of problem in this patch. It's not that simple, I'll
explain.

> --- patch-reload-config begins here ---
> diff --git a/awesome.c b/awesome.c
> index 9b5c48b..2c9f0a8 100644
> --- a/awesome.c
> +++ b/awesome.c
> @@ -312,6 +312,7 @@ main(int argc, char *argv[])
>  
>      /* allocate stuff */
>      awesomeconf = p_new(awesome_config, get_screen_count(dpy));
> +    awesomeconf->configpath = (char*)confpath;

using const in awesomeconf would be cleaner. :)

> +void
> +uicb_reloadconfig(Display *disp,
> +               awesome_config *awesomeconf,
> +               const char *arg __attribute__ ((unused)))
> +{
> +    int screen, screen_count = get_screen_count(disp);
> +    for(screen = 0; screen < screen_count; screen++)
> +    {
> +        parse_config(disp, screen, awesomeconf->configpath, &awesomeconf[screen]);
> +        grabkeys(awesomeconf->display, awesomeconf->phys_screen, awesomeconf);
> +
> +        compileregs(awesomeconf->rules, awesomeconf->nrules);
> +
> +        awesomeconf->statusbar.height = awesomeconf->font->height + 2;
> +        initstatusbar(awesomeconf->display, awesomeconf->screen, &awesomeconf->statusbar, awesomeconf->cursor[CurNormal]);
> +        drawstatusbar(disp, &awesomeconf[screen]);
> +    }
> +
> +    for(screen = 0; screen < ScreenCount(disp); screen++)
> +    {
> +        loadawesomeprops(disp, &awesomeconf[screen]);
> +    }
> +
> +    arrange(disp, awesomeconf);
> +}
> +

Here it is: first you assume that the awesomeconf will be
awesomeconf[0]. But if the event came from screen 1, err, we're bad. So
you should use a realconf = &awesomeconf[0 - awesomeconf->screen] to get
the real awesomeconf array pointer.
And then you'll have to reinit arrays, but free them with cleanup
before, or you'll have memory leak.
Currently cleanup() is maybe doing some X stuff you don't want,
so maybe a good way would be to grab out p_delete() stuff outside of
cleanup to use them here.

And there may be a side effect when you will loadawesomeprops or
winprops if the number of tags decrease in conf. Something to think
about.

However, that was a nice try. :)

Cheers,
-- 
Julien Danjou
// ᐰ <julien@xxxxxxxxxxx>   http://julien.danjou.info
// 9A0D 5FD9 EB42 22F6 8974  C95C A462 B51E C2FE E5CD

Attachment: signature.asc
Description: Digital signature