From: Steffen Prohaska <prohaska@zib.de>
Date: Sun, 12 Aug 2007 10:02:53 GMT
Subject: Re: [PATCH v4, ping] gitk: let you easily specify lines of context in diff view
Message-ID: <AC6A824F-E268-41AE-823B-2C01D78FAF30@zib.de>
In-Reply-To: <18110.31705.747583.463206@cargo.ozlabs.ibm.com>


On Aug 12, 2007, at 5:17 AM, Paul Mackerras wrote:

> 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.

Thanks. Didn't understand what pack does and thought it might be
needed to pack all at once into a 'single' pack.


>> +    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.

I fixed more of them. I typically work with a no-tab rule. Hope I
spotted all other places with wrong indent, too.


>> +# 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.

replaced this by a regexp.


>> +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.

Ah, thanks, didn't know that.


>> +            set diffcontext $diffcontextstring
>> +		    reselectline
>
> Inconsistent indentation.

ok.


>> +set diffcontext 3
>> +
>
> It would be nice to save diffcontext in the ~/.gitkrc.

done.

Will send a patch in a minute.

	Steffen