[PATCH v2 0/2] curses: fix attribute passing

[PATCH v2 0/2] curses: fix attribute passing

[Matthew Kilgore]

This patch set fixes up ui/curses.c. A previous change to ui/curses.c,
commit 962cf8fd4fae ("ui/curses: manipulate cchar_t with standard curses
functions"), did not correctly pass the attributes from the chtype to
`setcchar()`.

The biggest issue this caused is that colors no longer work when using
the curses display, it instead renders everything in white on black.
This was fixed by correctly passing the color pair number to setcchar().
I also fixed two spots where 0xff was used instead of the bit mask
constants that are part of the curses API.

changes in v2:
- Split into two patches, one dealing with the attribute masks, and one
  dealing with correctly passing the color pair number.

Thanks,
Matthew Kilgore

Matthew Kilgore (2):
  curses: use the bit mask constants provided by curses
  curses: correctly pass the color pair to setcchar()

 ui/curses.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

-- 
2.23.0

[PATCH v2 1/2] curses: use the bit mask constants provided by curses

[Matthew Kilgore]

The curses API provides the A_ATTRIBUTES and A_CHARTEXT bit masks for
getting the attributes and character parts of a chtype, respectively. We
should use provided constants instead of using 0xff.

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

diff --git a/ui/curses.c b/ui/curses.c
index ec281125acbd..84003f56a323 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -75,8 +75,8 @@ 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;
             ret = getcchar(&vga_to_curses[ch], wch, &attrs, &colors, NULL);
             if (ret == ERR || wch[0] == 0) {
                 wch[0] = ch;
-- 
2.23.0

Re: [PATCH v2 1/2] curses: use the bit mask constants provided by curses

[Philippe Mathieu-Daudé]

On 10/4/19 5:53 AM, Matthew Kilgore wrote:
> The curses API provides the A_ATTRIBUTES and A_CHARTEXT bit masks for
> getting the attributes and character parts of a chtype, respectively. We
> should use provided constants instead of using 0xff.
> 
> Signed-off-by: Matthew Kilgore <mattkilgore12@gmail.com>

Thanks for splitting your previous patch!

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   ui/curses.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ui/curses.c b/ui/curses.c
> index ec281125acbd..84003f56a323 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -75,8 +75,8 @@ 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;
>               ret = getcchar(&vga_to_curses[ch], wch, &attrs, &colors, NULL);
>               if (ret == ERR || wch[0] == 0) {
>                   wch[0] = ch;
> 

Re: [PATCH v2 1/2] curses: use the bit mask constants provided by curses

[Samuel Thibault]

Matthew Kilgore, le jeu. 03 oct. 2019 23:53:37 -0400, a ecrit:
> The curses API provides the A_ATTRIBUTES and A_CHARTEXT bit masks for
> getting the attributes and character parts of a chtype, respectively. We
> should use provided constants instead of using 0xff.
> 
> Signed-off-by: Matthew Kilgore <mattkilgore12@gmail.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Tested-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  ui/curses.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ui/curses.c b/ui/curses.c
> index ec281125acbd..84003f56a323 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -75,8 +75,8 @@ 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;
>              ret = getcchar(&vga_to_curses[ch], wch, &attrs, &colors, NULL);
>              if (ret == ERR || wch[0] == 0) {
>                  wch[0] = ch;
> -- 
> 2.23.0
> 

-- 
Samuel
The nice thing about Windows is - It does not just crash, it displays a
dialog box and lets you press 'OK' first.
(Arno Schaefer's .sig)

[PATCH v2 2/2] curses: correctly pass the color pair to setcchar()

[Matthew Kilgore]

The current code does not correctly pass the color pair information to
setcchar(), it instead always passes zero. This results in the curses
output always being in white on black.

This patch fixes this by using PAIR_NUMBER() to retrieve the color pair
number from the chtype value, and then passes that value as an argument
to setcchar().

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

diff --git a/ui/curses.c b/ui/curses.c
index 84003f56a323..3a1b71451c93 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -77,12 +77,14 @@ static void curses_update(DisplayChangeListener *dcl,
         for (x = 0; x < width; x++) {
             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 v2 2/2] curses: correctly pass the color pair to setcchar()

[Samuel Thibault]

Matthew Kilgore, le jeu. 03 oct. 2019 23:53:38 -0400, a ecrit:
> The current code does not correctly pass the color pair information to
> setcchar(), it instead always passes zero. This results in the curses
> output always being in white on black.
> 
> This patch fixes this by using PAIR_NUMBER() to retrieve the color pair
> number from the chtype value, and then passes that value as an argument
> to setcchar().
> 
> Signed-off-by: Matthew Kilgore <mattkilgore12@gmail.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Tested-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Thanks!

> ---
>  ui/curses.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/ui/curses.c b/ui/curses.c
> index 84003f56a323..3a1b71451c93 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -77,12 +77,14 @@ static void curses_update(DisplayChangeListener *dcl,
>          for (x = 0; x < width; x++) {
>              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
> 

-- 
Samuel
<macavity> bash: ls: Computer bought the farm
<macavity> THAT frightens ppl! :P
<macavity> id rather see: "bash: ls: Initialization of googol(AWAX)
        disengaged in HYPER32/64 mode due to faulty page request at
        AX:12A34F84B"
<macavity> at least that would give me the feeling that the
        *programmers* knows what is going on :P
(lovely Hurd...)