From: Paul Mackerras <paulus@samba.org>
Date: Sun, 12 Aug 2007 03:17:45 GMT
Subject: Re: [PATCH v4, ping] gitk: let you easily specify lines of context in diff view
Message-ID: <18110.31705.747583.463206@cargo.ozlabs.ibm.com>
In-Reply-To: <11868462503722-git-send-email-prohaska@zib.de>

Steffen Prohaska writes:

> Any chance to get this patch applied? It works for me.

Some comments:

> @@ -731,7 +732,16 @@ proc makewindow {} {
>  	-command changediffdisp -variable diffelide -value {0 1}
>      radiobutton .bleft.mid.new -text "New version" \
>  	-command changediffdisp -variable diffelide -value {1 0}
> -    pack .bleft.mid.diff .bleft.mid.old .bleft.mid.new -side left

Just add another pack command rather than extending this one.

> +    label .bleft.mid.labeldiffcontext -text "      Lines of context: " \
> +    -font $uifont

This is hard to read because the continuation line isn't indented
further that the first line.  Please indent continuation lines by an
extra 4 spaces.

> +# empty strings or integers accepted
> +proc diffcontextvalidate {v} {
> +    if {[string length $v] == 0} {
> +	return 1
> +    }
> +    if {[string is integer $v]} {
> +	if {$v > 0} {
> +	    return 1
> +	}
> +    }
> +    return 0
> +}

"string is integer" will already accept the null string and return 1.

> +proc diffcontextchange {n1 n2 op} {
> +    global diffcontextstring diffcontext
> +
> +    if {[string is integer $diffcontextstring]} {
> +        if {$diffcontextstring > 0} {

Once again, "string is integer" returning 1 doesn't guarantee the
string is non-empty.  Use "string is integer -strict" if you want
that.

> +            set diffcontext $diffcontextstring
> +		    reselectline

Inconsistent indentation.

> +set diffcontext 3
> +

It would be nice to save diffcontext in the ~/.gitkrc.

Paul.