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

["Alexander Gavrilov" <angavrilov@gmail.com>]

On Thu, Jul 31, 2008 at 3:25 PM, Paul Mackerras <paulus@samba.org> wrote:
> 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.

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

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

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