[PATCH weston 1/2] config-parser: add support for more color formats

[Quentin Glidic]

Hi,

On 20/10/2016 00:08, Eric Engestrom wrote:
> Valid colours start with an optional '0x' or '#', followed by:
> - AARRGGBB
> -   RRGGBB
> -  A R G B
> -    R G B
> -   XYXYXY
> -   XXXXXX

I think this is way too much, even with minimal code effort.

The well-known CSS formats are #RRGGBB and #RGB, and the 
backward-compatible extension are #RRGGBBAA and #RGBA.
The current 0xAARRGGBB are directly derived from the internal usage of a 
colour as a number, because Weston devs are used to this. But *users* 
are not, they are used to CSS notation.

IMO, 0x should be used as-is, with a little clamping, so the old way 
continue to work. OTOH, the # notation should be fully compatible with CSS.

This code would lead to user-unexpected behaviour, and it’s easy to 
avoid that.

Cheers,


> Signed-off-by: Eric Engestrom <eric at engestrom.ch>
> ---
>
> I just stumbled back on this discussion from a few months ago, and
> decided to give it a go.
>
> Once again, I apologise for length of the lines in the switch, but
> I believe they are much less readable when folded.
 >
> I feel like there should be a place where the supported colour formats$
> are documented, but I couldn't find it?
>
> ---
>  shared/config-parser.c | 39 +++++++++++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/shared/config-parser.c b/shared/config-parser.c
> index e2b5fa2..a00b2d6 100644
> --- a/shared/config-parser.c
> +++ b/shared/config-parser.c
> @@ -224,6 +224,9 @@ weston_config_section_get_color(struct weston_config_section *section,
>  	struct weston_config_entry *entry;
>  	int len;
>  	char *end;
> +	const char alpha[] = "FF"; /* Default alpha when unspecified */
> +	const char *val;
> +	const char *color_string;
>
>  	entry = config_section_get_entry(section, key);
>  	if (entry == NULL) {
> @@ -232,19 +235,39 @@ weston_config_section_get_color(struct weston_config_section *section,
>  		return -1;
>  	}
>
> +	val = entry->value;
> -	len = strlen(entry->value);
> +	len = strlen(val);
> +
> -	if (len == 1 && entry->value[0] == '0') {
> +	if (len == 1 && val[0] == '0') {
>  		*color = 0;
>  		return 0;
> +	}
> -	} else if (len != 8 && len != 10) {
> -		*color = default_color;
> -		errno = EINVAL;
> -		return -1;
> +
> +	if (len > 2 && val[0] == '0' && val[1] == 'x')
> +	{
> +		val += 2;
> +		len -= 2;
> +	}
> +	else if (len > 1 && val[0] == '#')
> +	{
> +		val += 1;
> +		len -= 1;
> +	}
> +
> +	switch (len) {
> +	case 8: color_string = (char[]){   val[0],   val[1], val[2], val[3], val[4], val[5], val[6], val[7], '\0' }; break; /* AARRGGBB */
> +	case 6: color_string = (char[]){ alpha[0], alpha[1], val[0], val[1], val[2], val[3], val[4], val[5], '\0' }; break; /*   RRGGBB */
> +	case 4: color_string = (char[]){   val[0],   val[0], val[1], val[1], val[2], val[2], val[3], val[3], '\0' }; break; /*  A R G B */
> +	case 3: color_string = (char[]){ alpha[0], alpha[1], val[0], val[0], val[1], val[1], val[2], val[2], '\0' }; break; /*    R G B */
> +	case 2: color_string = (char[]){ alpha[0], alpha[1], val[0], val[1], val[0], val[1], val[0], val[1], '\0' }; break; /*   XYXYXY */
> +	case 1: color_string = (char[]){ alpha[0], alpha[1], val[0], val[0], val[0], val[0], val[0], val[0], '\0' }; break; /*   XXXXXX */
> +	default: goto invalid;
>  	}
>
>  	errno = 0;
> -	*color = strtoul(entry->value, &end, 16);
> +	*color = strtoul(color_string, &end, 16);
> -	if (errno != 0 || end == entry->value || *end != '\0') {
> +	if (errno != 0 || end == color_string || *end != '\0') {
> +invalid:
>  		*color = default_color;
>  		errno = EINVAL;
>  		return -1;
>


-- 

Quentin “Sardem FF7” Glidic