public inbox for insight@sourceware.org
 help / color / mirror / Atom feed
* Re: [PATCH/gdbtk] Make color of changed vars customizable v2
@ 2012-05-25 10:42 Roland Schwingel
  0 siblings, 0 replies; 3+ messages in thread
From: Roland Schwingel @ 2012-05-25 10:42 UTC (permalink / raw)
  To: insight

Hi ...

Thanks for reviewing Keith!

I applied all your wishes regarding formatting.

 > > @@ -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"?
Changed the label to "Highlight". You are right this is the better term.

Committed the patch now.

Roland

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH/gdbtk] Make color of changed vars customizable v2
  2012-04-11 12:27 Roland Schwingel
@ 2012-05-24 21:08 ` Keith Seitz
  0 siblings, 0 replies; 3+ messages in thread
From: Keith Seitz @ 2012-05-24 21:08 UTC (permalink / raw)
  To: Roland Schwingel; +Cc: insight

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH/gdbtk] Make color of changed vars customizable v2
@ 2012-04-11 12:27 Roland Schwingel
  2012-05-24 21:08 ` Keith Seitz
  0 siblings, 1 reply; 3+ messages in thread
From: Roland Schwingel @ 2012-04-11 12:27 UTC (permalink / raw)
  To: insight

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

Hi...

This patch superseeds my previous patch from here:
http://sourceware.org/ml/insight/2012-q1/msg00031.html

As the old patch is yet not reviewed I reworked it a bit.

The new patch additionally takes also care about the register's window 
changed fields color and it does some minor simplifications.

Changelog:

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?

Roland

[-- Attachment #2: custom_color_changed_vars2.patch --]
[-- Type: text/plain, Size: 6494 bytes --]

diff -ruNp gdbtk_orig/library/locals.tcl gdbtk/library/locals.tcl
--- gdbtk_orig/library/locals.tcl	2006-12-01 04:34:39.000000000 +0100
+++ gdbtk/library/locals.tcl	2012-04-11 13:58:01.853490600 +0200
@@ -1,5 +1,5 @@
 # Local Variable Window for Insight.
-# Copyright (C) 2002, 2003, 2006 Red Hat
+# Copyright (C) 2002-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
@@ -45,6 +45,14 @@ itcl::class LocalsWin {
     cursor watch
   }
 
+  # ------------------------------------------------------------------
+  #  PUBLIC METHOD:  reconfig - used when preferences change
+  # ------------------------------------------------------------------
+  method reconfig {} {
+    debug
+    $tree update
+  }
+
   # Re-enable the UI
   method idle {event} {
     debug
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
   }
+  
+  # set color for changes
+  global Colors
+  set Colors(change) [pref get gdb/src/PC_TAG]
 }
 
 # ------------------------------------------------------------------
@@ -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 
   pref define gdb/console/target_fg       blue
   pref define gdb/console/font            global/fixed
 
   # Source window defaults
-  pref define gdb/src/PC_TAG              green
+  pref define gdb/src/PC_TAG              \#00b300
   pref define gdb/src/STACK_TAG           gold
   pref define gdb/src/BROWSE_TAG          \#9595e2
   pref define gdb/src/handlebg            red
@@ -490,6 +494,7 @@ proc pref_set_colors {home} {
   # then Insight won't be able to display sources and highlight things properly.
   # Therefore we will not change the textfg and textbg.
 
+  pref_load_default
   switch [pref get gdb/compat] {
 
     "Windows" {
@@ -508,7 +513,6 @@ proc pref_set_colors {home} {
     "KDE" {
       debug "loading OS colors for KDE"
 
-      pref_load_default
       # try loading "~/.gtkrc-kde"
       if {[pref_load_gnome $home [list .gtkrc-kde]]} {
 	debug "loaded gnome file"
@@ -550,13 +554,11 @@ proc pref_set_colors {home} {
     }
     
     "GNOME" {
-      pref_load_default
       pref_load_gnome $home
       pref_set_option_db 0
     }
 
     "default" {
-      pref_load_default
       pref_set_option_db 1
     }
   }
@@ -719,11 +721,6 @@ proc load_gnome_file {fd} {
 proc pref_set_option_db {makebg} {
   global Colors
 
-  # The color of text that indicates changed items
-  # We standardize on one color here so that changed
-  # items don't blend into any OS color scheme
-  set Colors(change) "green"
-
   option add *background $Colors(bg)
   option add *buttonBackground $Colors(bg)
   option add *Text*background $Colors(textbg)
diff -ruNp gdbtk_orig/library/regwin.itb gdbtk/library/regwin.itb
--- gdbtk_orig/library/regwin.itb	2007-06-27 22:50:50.000000000 +0200
+++ gdbtk/library/regwin.itb	2012-04-11 13:59:49.758880100 +0200
@@ -1,5 +1,5 @@
 # Register display window for Insight.
-# Copyright (C) 1998, 1999, 2001, 2002, 2003, 2004, 2007 Red Hat, Inc.
+# Copyright (C) 1998-2012 Red Hat, Inc.
 #
 # Written by Keith Seitz (keiths@redhat.com)
 #        and Martin Hunt (hunt@redhat.com)
@@ -420,6 +420,7 @@ itcl::body RegWin::_size_column {col dow
 itcl::body RegWin::reconfig {} {
   $itk_component(table) tag configure normal  \
     -state disabled -bg $::Colors(textbg) -fg $::Colors(textfg)
+  $itk_component(table) tag configure highlight -bg $::Colors(change) -fg black
 }
 
 
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]
   
@@ -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]
   }
   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"
 

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2012-05-25 10:42 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-25 10:42 [PATCH/gdbtk] Make color of changed vars customizable v2 Roland Schwingel
  -- strict thread matches above, loose matches on Subject: below --
2012-04-11 12:27 Roland Schwingel
2012-05-24 21:08 ` Keith Seitz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).