From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29397 invoked by alias); 24 May 2012 21:08:56 -0000 Received: (qmail 29384 invoked by uid 22791); 24 May 2012 21:08:54 -0000 X-SWARE-Spam-Status: No, hits=-6.8 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 24 May 2012 21:08:40 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q4OL8Sik030633 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 24 May 2012 17:08:28 -0400 Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q4OL8PFX020160 (version=TLSv1/SSLv3 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Thu, 24 May 2012 17:08:27 -0400 Message-ID: <4FBEA349.8050505@redhat.com> Date: Thu, 24 May 2012 21:08:00 -0000 From: Keith Seitz User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120329 Thunderbird/11.0.1 MIME-Version: 1.0 To: Roland Schwingel CC: insight@sourceware.org Subject: Re: [PATCH/gdbtk] Make color of changed vars customizable v2 References: <4F85787A.7030700@onevision.com> In-Reply-To: <4F85787A.7030700@onevision.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact insight-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: insight-owner@sourceware.org X-SW-Source: 2012-q2/txt/msg00027.txt.bz2 On 04/11/2012 05:26 AM, Roland Schwingel wrote: > 2012-04-11 Roland Schwingel > > * 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