Re: [PATCH v6 RESEND 4/7] perf config: Use combined {fore,back}ground colors value instead of each two color

[Taeung Song <treeze.taeung@gmail.com>]

Hi, Arnaldo :)

On 08/09/2016 03:58 AM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Aug 02, 2016 at 06:20:46PM +0900, Taeung Song escreveu:
>> To easily set default config values into actual variables for 'colors' config,
>> it would be better that actual variables for each 'colors' config
>> also have only one value like 'default_config_item' type.
>>
>> If we use combined {fore,back}ground colors values in ui_browser_colorset,
>> it smoothly work to initialize default config values for 'colors' config
>> by 'colors_config_items' array that contains default values for it at util/config.c.
>> because both actual variable and config item of 'colors_config_items'
>> are equal in the number of values (as just one).
>>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Masami Hiramatsu <mhiramat@kernel.org>
>> Cc: Wang Nan <wangnan0@huawei.com>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>  tools/perf/ui/browser.c | 53 +++++++++++++++++++++++--------------------------
>>  1 file changed, 25 insertions(+), 28 deletions(-)
>>
>> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
>> index 3eb3edb..31e2028 100644
>> --- a/tools/perf/ui/browser.c
>> +++ b/tools/perf/ui/browser.c
>> @@ -503,61 +503,53 @@ unsigned int ui_browser__list_head_refresh(struct ui_browser *browser)
>>  }
>>
>>  static struct ui_browser_colorset {
>> -	const char *name, *fg, *bg;
>> +	const char *name, *colors;
>>  	int colorset;
>>  } ui_browser__colorsets[] = {
>>  	{
>>  		.colorset = HE_COLORSET_TOP,
>>  		.name	  = "top",
>> -		.fg	  = "red",
>> -		.bg	  = "default",
>> +		.colors	  = "red, default",
>>  	},
>>  	{
>>  		.colorset = HE_COLORSET_MEDIUM,
>>  		.name	  = "medium",
>> -		.fg	  = "green",
>> -		.bg	  = "default",
>> +		.colors	  = "green, default",
>>  	},
>>  	{
>>  		.colorset = HE_COLORSET_NORMAL,
>>  		.name	  = "normal",
>> -		.fg	  = "default",
>> -		.bg	  = "default",
>> +		.colors	  = "default, default",
>>  	},
>>  	{
>>  		.colorset = HE_COLORSET_SELECTED,
>>  		.name	  = "selected",
>> -		.fg	  = "black",
>> -		.bg	  = "yellow",
>> +		.colors	  = "black, yellow",
>>  	},
>>  	{
>>  		.colorset = HE_COLORSET_JUMP_ARROWS,
>>  		.name	  = "jump_arrows",
>> -		.fg	  = "blue",
>> -		.bg	  = "default",
>> +		.colors	  = "blue, default",
>>  	},
>>  	{
>>  		.colorset = HE_COLORSET_ADDR,
>>  		.name	  = "addr",
>> -		.fg	  = "magenta",
>> -		.bg	  = "default",
>> +		.colors	  = "magenta, default",
>>  	},
>>  	{
>>  		.colorset = HE_COLORSET_ROOT,
>>  		.name	  = "root",
>> -		.fg	  = "white",
>> -		.bg	  = "blue",
>> +		.colors	  = "white, blue",
>>  	},
>>  	{
>>  		.name = NULL,
>>  	}
>>  };
>>
>> -
>>  static int ui_browser__color_config(const char *var, const char *value,
>>  				    void *data __maybe_unused)
>>  {
>> -	char *fg = NULL, *bg;
>> +	char *colors;
>>  	int i;
>>
>>  	/* same dir for all commands */
>> @@ -570,22 +562,18 @@ static int ui_browser__color_config(const char *var, const char *value,
>>  		if (strcmp(ui_browser__colorsets[i].name, name) != 0)
>>  			continue;
>>
>> -		fg = strdup(value);
>> -		if (fg == NULL)
>> -			break;
>> +		if (strstr(value, ",") == NULL)
>> +			return -1;
>>
>> -		bg = strchr(fg, ',');
>> -		if (bg == NULL)
>> +		colors = strdup(value);
>> +		if (colors == NULL)
>>  			break;
>> +		ui_browser__colorsets[i].colors = colors;
>>
>> -		*bg = '\0';
>> -		while (isspace(*++bg));
>> -		ui_browser__colorsets[i].bg = bg;
>> -		ui_browser__colorsets[i].fg = fg;
>>  		return 0;
>>  	}
>>
>> -	free(fg);
>> +	free(colors);
>>  	return -1;
>>  }
>>
>> @@ -743,8 +731,17 @@ void ui_browser__init(void)
>>  	perf_config(ui_browser__color_config, NULL);
>>
>>  	while (ui_browser__colorsets[i].name) {
>> +		char *colors, *fg, *bg;
>>  		struct ui_browser_colorset *c = &ui_browser__colorsets[i++];
>> -		sltt_set_color(c->colorset, c->name, c->fg, c->bg);
>> +
>> +		colors = strdup(c->colors);
>> +		if (fg == NULL)
>
> Huh? At this point fb is not even initialized

Sorry for my mistake.
I'll change 'fg' to 'colors' to handle a exception
of strdup().

Thanks,
Taeung


>> +			break;
>> +		fg = strtok(colors, ",");
>> +		bg = strtok(NULL, ",");
>> +		bg = ltrim(bg);
>> +		sltt_set_color(c->colorset, c->name, fg, bg);
>> +		free(colors);
>>  	}
>>
>>  	annotate_browser__init();
>> --
>> 2.5.0