From: Keith Seitz <keiths@redhat.com>
To: Roland Schwingel <roland@onevision.com>
Cc: insight@sourceware.org
Subject: Re: [PATCH/gdbtk] Make color of changed vars customizable v2
Date: Thu, 24 May 2012 21:08:00 -0000 [thread overview]
Message-ID: <4FBEA349.8050505@redhat.com> (raw)
In-Reply-To: <4F85787A.7030700@onevision.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
next prev parent reply other threads:[~2012-05-24 21:08 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-04-11 12:27 Roland Schwingel
2012-05-24 21:08 ` Keith Seitz [this message]
2012-05-25 10:42 Roland Schwingel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4FBEA349.8050505@redhat.com \
--to=keiths@redhat.com \
--cc=insight@sourceware.org \
--cc=roland@onevision.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).