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

[Matthew Kilgore <mattkilgore12@gmail.com>]

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);
>>      }
>>