Re: [PATCH/gdbtk] Make color of changed vars customizable v2

[Keith Seitz <keiths at redhat dot com>]

On 04/11/2012 05:26 AM, Roland Schwingel wrote:
> 2012-04-11  Roland Schwingel <roland.schwingel@onevision.com>
>
>      * library/locals.tcl: Updated copyright.
>      (reconfig): New method for getting prefs updates.
>      * library/prefs.tcl: Updated copyright.
>      (pref_read): Moved comment. Take color for changed
>      fields from gdb/src/PC_TAG prefs value.
>      (pref_set_defaults): Change default of
>      PC_TAG color to a somewhat darker green.
>      (pref_set_colors): Call pref_load_default in central place.
>      (pref_set_option_db): Remove hard wired old color for
>      changed values.
>      * library/regwin.itb: Update copyright.
>      (reconfig): Update highlight color of register table.
>      * library/srcpref.itb: Update copyright.
>      (_build_win): Change text of PC color chooser.
>      (_apply): Update global changed fields color from PC_TAG color.
>      * library/vartree.itb (update_var): use changed field color from
>      global colors list.
>      (_init_data): Remove local copy of changed vars color.
>
> Any comments? Is this ok?

This looks pretty good. Some minor nits below.

> diff -ruNp gdbtk_orig/library/prefs.tcl gdbtk/library/prefs.tcl
> --- gdbtk_orig/library/prefs.tcl	2008-03-04 00:25:03.000000000 +0100
> +++ gdbtk/library/prefs.tcl	2012-04-11 13:59:37.809259100 +0200
> @@ -1,5 +1,5 @@
>   # Local preferences functions for Insight.
> -# Copyright (C) 1997, 1998, 1999, 2002, 2003, 2004, 2008 Red Hat
> +# Copyright (C) 1997-2012 Red Hat, Inc.
>   #
>   # This program is free software; you can redistribute it and/or modify it
>   # under the terms of the GNU General Public License (GPL) as published by
> @@ -146,14 +146,18 @@ proc pref_read {} {
>     if {[pref get gdb/use_color_schemes] != "1"} {
>       pref_set_colors $home
>     } else {
> -    global Colors
>       # These colors are the same for all schemes
> +    global Colors
>       set Colors(textfg) black
>       set Colors(fg) black
>       set Colors(sbg) \#4c59a5
>       set Colors(sfg) white
>       set_bg_colors
>     }
> +
^^^ This line appears to have extraneous whitespace on it. Please 
double-check.

> +  # set color for changes
> +  global Colors
> +  set Colors(change) [pref get gdb/src/PC_TAG]
>   }

Please capitalize/attempt to use full sentences for comments. "Set color 
for changes" . Period at the end? Maybe. I don't really care too much 
for imperatives like this.

>   # ------------------------------------------------------------------
> @@ -333,12 +337,12 @@ proc pref_set_defaults {} {
>     pref define gdb/console/wrap            0
>     pref define gdb/console/prompt_fg       DarkGreen
>     pref define gdb/console/error_fg        red
> -  pref define gdb/console/log_fg          green
> +  pref define gdb/console/log_fg          \#00b300

There appear to be extra whitespaces at the end of this line, too. 
Please double-check.

> diff -ruNp gdbtk_orig/library/srcpref.itb gdbtk/library/srcpref.itb
> --- gdbtk_orig/library/srcpref.itb	2008-08-03 00:08:32.000000000 +0200
> +++ gdbtk/library/srcpref.itb	2012-04-11 14:00:33.563757100 +0200
> @@ -1,5 +1,5 @@
>   # Source preferences dialog for Insight.
> -# Copyright (C) 1998, 1999, 2002, 2003, 2008 Red Hat
> +# Copyright (C) 1998-2012 Red Hat, Inc.
>   #
>   # This program is free software; you can redistribute it and/or modify it
>   # under the terms of the GNU General Public License (GPL) as published by
> @@ -54,7 +54,7 @@ itcl::body SrcPref::_build_win {} {
>     set w [$f.colors get_frame]
>
>     set color [pref get gdb/src/PC_TAG]
> -  label $w.pcl -text {PC}
> +  label $w.pcl -text {PC/Var change}
>     button $w.pcb -text {     } -activebackground $color -bg $color \
>       -command [code $this _pick $color $w.pcb PC_TAG]

How about if we just use the more generic term, "Highlight" instead of 
"PC/Var change"?  I think the original choice to use "PC" is poor, since 
it really means *current* PC (as opposed to PC of the non-current 
frame). We highlight all kinds of things, register/variable changes, the 
current PC, etc.

> @@ -212,6 +212,10 @@ itcl::body SrcPref::_apply {} {
>         #debug "$var = $_new($var)"
>         pref set $var $_new($var)
>       }
> +
> +    # update color for changed values
> +    global Colors
> +    set Colors(change) [pref get gdb/src/PC_TAG]

Extra whitespace on first of these four lines. Please double-check. 
Similar comment as before regarding comments.

>     }
>     if {$_new_disassembly_flavor != ""} {
>       gdb_cmd "set disassembly-flavor $_new_disassembly_flavor"
> diff -ruNp gdbtk_orig/library/vartree.itb gdbtk/library/vartree.itb
> --- gdbtk_orig/library/vartree.itb	2010-04-29 23:00:52.000000000 +0200
> +++ gdbtk/library/vartree.itb	2012-04-11 14:01:25.371448100 +0200
> @@ -135,7 +135,7 @@ itcl::body  VarTree::update_var {var ena
>       if {[catch {$var value} value]} {
>         set color $colors(error)
>       } elseif {[$c itemcget $val -text] != $value} {
> -      set color $colors(change)
> +      set color $::Colors(change)
>       } else {
>         set color $colors(value)
>       }
> @@ -404,7 +404,6 @@ itcl::body  VarTree::_init_data {} {
>     set colors(type) "red"
>     set colors(error) "red"
>     set colors(value) "black"
> -  set colors(change) $::Colors(change)
>     set colors(disabled) "gray50"
>     set colors(line) "gray50"

Okay with those changes.

Keith