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

[Paul Mackerras]

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 <[hidden email]>

I need a more detailed explanation of the rationale for the specific
changes you have made in the changelog.

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.

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

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

Thanks,
Paul.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Alexander Gavrilov]

On Thu, Jul 31, 2008 at 3:25 PM, Paul Mackerras <[hidden email]> 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 <[hidden email]>
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Alexander Gavrilov]

Date: Sun, 27 Jul 2008 08:18:27 +0400

Other GUI tools may occasionally need to start
gitk and make it automatically select a certain
commit. This patch supports doing it through the
environment or command line.

Using the environment allows graceful degradation of
the tool when used with an old version of gitk:
unsupported command line options cause it to die.

Signed-off-by: Alexander Gavrilov <[hidden email]>
---

        On Thursday 31 July 2008 16:41:20 Alexander Gavrilov wrote:
        > 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.

        I decided to send it now.

        -- Alexander

 gitk |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/gitk b/gitk
index d093a39..2da885d 100755
--- a/gitk
+++ b/gitk
@@ -416,10 +416,12 @@ proc stop_rev_list {view} {
 }
 
 proc reset_pending_select {selid} {
-    global pending_select mainheadid
+    global pending_select mainheadid selectheadid
 
     if {$selid ne {}} {
  set pending_select $selid
+    } elseif {$selectheadid ne {}} {
+ set pending_select $selectheadid
     } else {
  set pending_select $mainheadid
     }
@@ -1607,6 +1609,7 @@ proc getcommit {id} {
 proc readrefs {} {
     global tagids idtags headids idheads tagobjid
     global otherrefids idotherrefs mainhead mainheadid
+    global selecthead selectheadid
 
     foreach v {tagids idtags headids idheads otherrefids idotherrefs} {
  catch {unset $v}
@@ -1653,6 +1656,12 @@ proc readrefs {} {
     set mainhead [string range $thehead 11 end]
  }
     }
+    set selectheadid {}
+    if {$selecthead ne {}} {
+ catch {
+    set selectheadid [exec git rev-parse --verify $selecthead]
+ }
+    }
 }
 
 # skip over fake commits
@@ -9863,6 +9872,13 @@ if {![file isdirectory $gitdir]} {
     exit 1
 }
 
+set selecthead {}
+set selectheadid {}
+
+if {[info exists env(GITK_SELECT_ID)]} {
+    set selecthead $env(GITK_SELECT_ID)
+}
+
 set revtreeargs {}
 set cmdline_files {}
 set i 0
@@ -9874,6 +9890,9 @@ foreach arg $argv {
     set cmdline_files [lrange $argv [expr {$i + 1}] end]
     break
  }
+ "--select-commit=*" {
+    set selecthead [string range $arg 16 end]
+ }
  "--argscmd=*" {
     set revtreeargscmd [string range $arg 10 end]
  }
@@ -9884,6 +9903,10 @@ foreach arg $argv {
     incr i
 }
 
+if {$selecthead eq "HEAD"} {
+    set selecthead {}
+}
+
 if {$i >= [llength $argv] && $revtreeargs ne {}} {
     # no -- on command line, but some arguments (other than --argscmd)
     if {[catch {
--
1.5.6.3.18.gfe82

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html