[PATCH] curses: correctly pass color and attributes to setcchar()

[PATCH] curses: correctly pass color and attributes to setcchar()

[Matthew Kilgore]

The current code uses getcchar() and setcchar() to handle the cchar_t
values, which is correct, however it incorrectly deconstructs the chtype
value that it then passes to setcchar():

    1. The bit mask 0xff against the chtype is not guaranteed to be
       correct. curses provides the 'A_ATTRIBUTES' and 'A_CHARTEXT' masks
       to do this.

    2. The color pair has to be passed to setcchar() separately, any color
       pair provided in the attributes is ignored.

The first point is not that big of a deal, the 0xff mask is very likely
to be correct. The second issue however results in colors no longer
working when using the curses display, instead the text will always be
white on black.

This patch fixes the color issue by using PAIR_NUMBER() to retrieve the
color pair number from the chtype value, and then passes that number to
setcchar. Along with that, the hardcoded 0xff masks are replaced with
A_ATTRIBUTES and A_CHARTEXT.

Signed-off-by: Matthew Kilgore <mattkilgore12@gmail.com>
---
 ui/curses.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/ui/curses.c b/ui/curses.c
index ec281125acbd..3a1b71451c93 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -75,14 +75,16 @@ static void curses_update(DisplayChangeListener *dcl,
     line = screen + y * width;
     for (h += y; y < h; y ++, line += width) {
         for (x = 0; x < width; x++) {
-            chtype ch = line[x] & 0xff;
-            chtype at = line[x] & ~0xff;
+            chtype ch = line[x] & A_CHARTEXT;
+            chtype at = line[x] & A_ATTRIBUTES;
+            short color_pair = PAIR_NUMBER(line[x]);
+
             ret = getcchar(&vga_to_curses[ch], wch, &attrs, &colors, NULL);
             if (ret == ERR || wch[0] == 0) {
                 wch[0] = ch;
                 wch[1] = 0;
             }
-            setcchar(&curses_line[x], wch, at, 0, NULL);
+            setcchar(&curses_line[x], wch, at, color_pair, NULL);
         }
         mvwadd_wchnstr(screenpad, y, 0, curses_line, width);
     }
-- 
2.23.0

Re: [PATCH] curses: correctly pass color and attributes to setcchar()

[Philippe Mathieu-Daudé]

Hi Matthew,

On 10/3/19 2:18 AM, Matthew Kilgore wrote:
> The current code uses getcchar() and setcchar() to handle the cchar_t
> values, which is correct, however it incorrectly deconstructs the chtype
> value that it then passes to setcchar():
> 
>      1. The bit mask 0xff against the chtype is not guaranteed to be
>         correct. curses provides the 'A_ATTRIBUTES' and 'A_CHARTEXT' masks
>         to do this.
> 
>      2. The color pair has to be passed to setcchar() separately, any color
>         pair provided in the attributes is ignored.

You clearly describe 2 different changes in the same patch.
Do you mind splitting in 2 atomic patches?

> 
> The first point is not that big of a deal, the 0xff mask is very likely
> to be correct. The second issue however results in colors no longer
> working when using the curses display, instead the text will always be
> white on black.
> 
> This patch fixes the color issue by using PAIR_NUMBER() to retrieve the
> color pair number from the chtype value, and then passes that number to
> setcchar.

> Along with that, the hardcoded 0xff masks are replaced with
> A_ATTRIBUTES and A_CHARTEXT.

This comment would go in the 1st patch.

> 
> Signed-off-by: Matthew Kilgore <mattkilgore12@gmail.com>
> ---
>   ui/curses.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/ui/curses.c b/ui/curses.c
> index ec281125acbd..3a1b71451c93 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -75,14 +75,16 @@ static void curses_update(DisplayChangeListener *dcl,
>       line = screen + y * width;
>       for (h += y; y < h; y ++, line += width) {
>           for (x = 0; x < width; x++) {
> -            chtype ch = line[x] & 0xff;
> -            chtype at = line[x] & ~0xff;
> +            chtype ch = line[x] & A_CHARTEXT;
> +            chtype at = line[x] & A_ATTRIBUTES;
> +            short color_pair = PAIR_NUMBER(line[x]);
> +
>               ret = getcchar(&vga_to_curses[ch], wch, &attrs, &colors, NULL);
>               if (ret == ERR || wch[0] == 0) {
>                   wch[0] = ch;
>                   wch[1] = 0;
>               }
> -            setcchar(&curses_line[x], wch, at, 0, NULL);
> +            setcchar(&curses_line[x], wch, at, color_pair, NULL);
>           }
>           mvwadd_wchnstr(screenpad, y, 0, curses_line, width);
>       }
> 

Re: [PATCH] curses: correctly pass color and attributes to setcchar()

[Matthew Kilgore]

Hi Philippe,

On Thu, Oct 03, 2019 at 09:36:56AM +0200, Philippe Mathieu-Daudé wrote:
>Hi Matthew,
>
>On 10/3/19 2:18 AM, Matthew Kilgore wrote:
>>The current code uses getcchar() and setcchar() to handle the cchar_t
>>values, which is correct, however it incorrectly deconstructs the chtype
>>value that it then passes to setcchar():
>>
>>     1. The bit mask 0xff against the chtype is not guaranteed to be
>>        correct. curses provides the 'A_ATTRIBUTES' and 'A_CHARTEXT' masks
>>        to do this.
>>
>>     2. The color pair has to be passed to setcchar() separately, any color
>>        pair provided in the attributes is ignored.
>
>You clearly describe 2 different changes in the same patch.
>Do you mind splitting in 2 atomic patches?

Done, I've sent a new patch set with these changes split into two
patches.

Thanks,
Matthew Kilgore

>
>>
>>The first point is not that big of a deal, the 0xff mask is very likely
>>to be correct. The second issue however results in colors no longer
>>working when using the curses display, instead the text will always be
>>white on black.
>>
>>This patch fixes the color issue by using PAIR_NUMBER() to retrieve the
>>color pair number from the chtype value, and then passes that number to
>>setcchar.
>
>>Along with that, the hardcoded 0xff masks are replaced with
>>A_ATTRIBUTES and A_CHARTEXT.
>
>This comment would go in the 1st patch.
>
>>
>>Signed-off-by: Matthew Kilgore <mattkilgore12@gmail.com>
>>---
>>  ui/curses.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>>diff --git a/ui/curses.c b/ui/curses.c
>>index ec281125acbd..3a1b71451c93 100644
>>--- a/ui/curses.c
>>+++ b/ui/curses.c
>>@@ -75,14 +75,16 @@ static void curses_update(DisplayChangeListener *dcl,
>>      line = screen + y * width;
>>      for (h += y; y < h; y ++, line += width) {
>>          for (x = 0; x < width; x++) {
>>-            chtype ch = line[x] & 0xff;
>>-            chtype at = line[x] & ~0xff;
>>+            chtype ch = line[x] & A_CHARTEXT;
>>+            chtype at = line[x] & A_ATTRIBUTES;
>>+            short color_pair = PAIR_NUMBER(line[x]);
>>+
>>              ret = getcchar(&vga_to_curses[ch], wch, &attrs, &colors, NULL);
>>              if (ret == ERR || wch[0] == 0) {
>>                  wch[0] = ch;
>>                  wch[1] = 0;
>>              }
>>-            setcchar(&curses_line[x], wch, at, 0, NULL);
>>+            setcchar(&curses_line[x], wch, at, color_pair, NULL);
>>          }
>>          mvwadd_wchnstr(screenpad, y, 0, curses_line, width);
>>      }
>>