Re: [PATCH (GITK) 5/6] gitk: Fixed automatic row selection during load.

From: Alexander Gavrilov <angavrilov@gmail.com>
Date: Jul 31, 2008, 12:41 UTC
Message-ID: <bb6f213e0807310541s383c2938q91543a1c57a4d71f@mail.gmail.com>
In-Reply-To: <18577.41259.758690.635991@cargo.ozlabs.ibm.com>

On Thu, Jul 31, 2008 at 3:25 PM, Paul Mackerras <paulus@samba.org> wrote:

Show 10 quoted lines

> Alexander Gavrilov writes:
>
>> - Switching views now actually preserves the selected commit.
>> - Reloading (also Edit View) preserves the currently selected commit.
>> - Initial selection does not produce weird scrolling.
>>
>> Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
>
> I need a more detailed explanation of the rationale for the specific
> changes you have made in the changelog.

The rationale is that preserving the currently selected commit is more intelligent behavior than always resetting to a preset position, and it makes the UI feel more smooth. Also, although it is possible to restore selection by clicking the 'back' button, it is not immediately obvious; in fact I realized it only after reading the code.

Gitk already tried to preserve the commit in many cases, but failed because of a conflicting change that made it select the head. Such behavior is clearly a bug.

The Reload case is arguable, but I think that Edit View should preserve the selection, and it uses Reload internally. It can be resolved by adding a parameter to the function implementing Reload.

Show 14 quoted lines

> As for the patch, it mostly looks good, but I have a few comments
> below.
>
>> +proc getcommits {selid} {
>>      global canv curview need_redisplay viewactive
>>
>>      initlayout
>>      if {[start_rev_list $curview]} {
>> +     reset_pending_select $selid
>
> Is there any significance to having the call to reset_pending_select
> after the start_rev_list call (other than not setting pending_select
> if start_rev_list fails)?  I couldn't see any.  If there is then it
> should be noted in a comment and/or the patch description.

It simply tries to preserve the original behavior.

Show 10 quoted lines

>> @@ -503,7 +511,7 @@ proc updatecommits {} {
>>      filerun $fd [list getcommitlines $fd $i $view 1]
>>      incr viewactive($view)
>>      set viewcomplete($view) 0
>> -    set pending_select $mainheadid
>> +    reset_pending_select {}
>
> This doesn't actually change anything, right?  If so then I'd prefer
> the simple, direct assignment to calling a procedure that does the
> assignment.

I have a patch WIP that allows specifying a commit on the command line to select instead of the head (I need it to enhance the git gui blame UI). It makes the function somewhat more intelligent. I'll submit it as soon as this series is sorted out.

Show 8 quoted lines

>> @@ -4036,6 +4042,7 @@ proc layoutmore {} {
>>      }
>>      if {[info exists pending_select] &&
>>       [commitinview $pending_select $curview]} {
>> +     update
>>       selectline [rowofcommit $pending_select] 1
>
> What does that update do?  Would update idletasks be better?

That update forces Tk to recompute the widget dimensions. Otherwise selectline sometimes gets all zeroes from yview, which makes it compute really weird scrolling settings. Git-gui always does an update before scrolling computations.

Alexander