Re: [PATCH] gdb/tui: Fix unexpected reuse of color pairs

[Andrew Burgess <aburgess@redhat.com>]

Thanks for working on this.  This looks like it could be a good
improvement.

jakob.schaeffeler@tum.de writes:

> From: Jakob Schäffeler <jakob.schaeffeler@tum.de>

Commit message text should be line wrapped at around 72 characters, your
commit message will need to be reformatted so that it is readable in
'git log' output.

>
> TUI translates ANSI styling sequences to curses color pairs.
> Currently in this process uses COLOR_PAIR which only returns values from 0 to 255 which results in unexpected reuse of color pairs.

typo: "Currently, this process uses COLOR_PAIR, which only returns values ..."

> This patch avoids calling COLOR_PAIR(pair) to be able to render more than 256 color pairs. For this the wattron call is replaced with wcolor_set.
> This also results in last_color_pair no longer being needed since we set the color directly with wcolor_set and do not need wattron/off pairs any longer.

> This results in SHRT_MAX different color pairs to be available. To get
> all 65535 color pairs init_pair is replaced with init_extended_pair,
> which returns an int instead of short.

typo: "...which TAKES int instead of short.".  They both return 'int'.

> This patch was tested with make check-gdb TESTS="gdb.tui/*.exp"

It would be good if there was a new test added which checks the new
extended colour range.  Is there a reason why this cannot be done?

I also wonder how widely available this extended colour API is?  Is this
supported on mingw?  Or FreeBSD?  Or Solaris (is this even
used/supported these days)?  I wonder if we should be adding configure
checks for this API, or if it's OK to just put a hard requirement in
place?   At the very least it would be nice to document in the commit
message if nowhere else, what version/package requirements this is now
placing on us.

Also, I suspect this patch is probably more than trivial, so we will
probably need a copyright assignment in agreement before we could accept
it.  Information on this process can be found here:

https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment

If you have any questions, feel free to ask and we can help out.

Thanks,
Andrew


>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34134
> ---
>  gdb/tui/tui-io.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
> index 642b88ead0c..5e239179b5e 100644
> --- a/gdb/tui/tui-io.c
> +++ b/gdb/tui/tui-io.c
> @@ -265,10 +265,6 @@ get_color (const ui_file_style::color &color, int *result)
>    return true;
>  }
>  
> -/* The most recently emitted color pair.  */
> -
> -static int last_color_pair = -1;
> -
>  /* The most recently applied style.  */
>  
>  static ui_file_style last_style;
> @@ -299,7 +295,7 @@ get_color_pair (int fg, int bg)
>  	 back to the default if we've used too many.  */
>        if (next >= COLOR_PAIRS)
>  	return 0;
> -      init_pair (next, fg, bg);
> +      init_extended_pair (next, fg, bg);
>        color_pair_map[c] = next;
>        return next;
>      }
> @@ -320,9 +316,7 @@ tui_apply_style (WINDOW *w, ui_file_style style)
>  #endif
>    wattroff (w, A_UNDERLINE);
>    wattroff (w, A_REVERSE);
> -  if (last_color_pair != -1)
> -    wattroff (w, COLOR_PAIR (last_color_pair));
> -  wattron (w, COLOR_PAIR (0));
> +  wcolor_set (w, 0, nullptr);
>  
>    const ui_file_style::color &fg = style.get_foreground ();
>    const ui_file_style::color &bg = style.get_background ();
> @@ -342,10 +336,7 @@ tui_apply_style (WINDOW *w, ui_file_style style)
>  	    bgi = (ncurses_norm_attr >> 4) & 15;
>  #endif
>  	  int pair = get_color_pair (fgi, bgi);
> -	  if (last_color_pair != -1)
> -	    wattroff (w, COLOR_PAIR (last_color_pair));
> -	  wattron (w, COLOR_PAIR (pair));
> -	  last_color_pair = pair;
> +	  wcolor_set (w, 0, &pair);
>  	}
>      }
>  
> @@ -907,7 +898,6 @@ tui_setup_io (int mode)
>        savetty ();
>  
>        /* Clean up color information.  */
> -      last_color_pair = -1;
>        last_style = ui_file_style ();
>        color_map.clear ();
>        color_pair_map.clear ();
> -- 
> 2.54.0