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

[Bryce Harrington]

On Fri, Oct 21, 2016 at 04:02:15PM +0100, Eric Engestrom wrote:
> On Friday, 2016-10-21 12:30:07 +0200, Quentin Glidic wrote:
> > 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
> > > -   XX
> > 
> > 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.
> 
> You're right, I didn't really think this through :)
> I'll send a v2 with only commonly used formats/orders when
> I have some more time (probably sunday):
> 
> - (0x)(AA)RRGGBB (backward compatibility)
> - #RGB(A)
> - #RRGGBB(AA)
> 
> Bryce, I think you wanted a single repeating char format; do you also
> want the 2-char format?

I agree with Quentin that it's probably most important that color codes
are handled in consistent ways so that they don't encounter odd
unexpected behaviors.  In that light less maybe more as they say.

Bryce
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Eric Engestrom]

On Friday, 2016-10-21 12:30:07 +0200, Quentin Glidic wrote:
> 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
> > -   XX
> 
> 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.

You're right, I didn't really think this through :)
I'll send a v2 with only commonly used formats/orders when
I have some more time (probably sunday):

- (0x)(AA)RRGGBB (backward compatibility)
- #RGB(A)
- #RRGGBB(AA)

Bryce, I think you wanted a single repeating char format; do you also
want the 2-char format?

Cheers,
  Eric
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [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
-   XX


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 
---

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; /*   XX */
+   default: goto invalid;
}

errno = 0;
-   *color = strtoul(entry->value, , 16);
+   *color = strtoul(color_string, , 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
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Eric Engestrom]

Valid colours start with an optional '0x' or '#', followed by:
- AARRGGBB
-   RRGGBB
-  A R G B
-R G B
-   XYXYXY
-   XX

Signed-off-by: Eric Engestrom 
---

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; /*   XX */
+   default: goto invalid;
}
 
errno = 0;
-   *color = strtoul(entry->value, , 16);
+   *color = strtoul(color_string, , 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;
-- 
Cheers,
  Eric

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel