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 <address@hidden>
> > ---
> >  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);
> >      }