[PATCH] gitk: Synchronize highlighting in file view when scrolling diff

[PATCH] gitk: Synchronize highlighting in file view when scrolling diff

[Stefan Haller]

Whenever the diff pane scrolls, highlight the corresponding file in the
file list on the right. For a large commit with many files and long
per-file diffs, this makes it easier to keep track of what you're looking
at.

This allows simplifying the prevfile and nextfile functions, because
all they have to do is scroll the diff pane.

Signed-off-by: Stefan Haller <stefan@haller-berlin.de>
---
 gitk | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/gitk b/gitk
index d93bd99..9e3ec71 100755
--- a/gitk
+++ b/gitk
@@ -7947,10 +7947,9 @@ proc changediffdisp {} {
     $ctext tag conf dresult -elide [lindex $diffelide 1]
 }
 
-proc highlightfile {loc cline} {
-    global ctext cflist cflist_top
+proc highlightfile {cline} {
+    global cflist cflist_top
 
-    $ctext yview $loc
     $cflist tag remove highlight $cflist_top.0 "$cflist_top.0 lineend"
     $cflist tag add highlight $cline.0 "$cline.0 lineend"
     $cflist see $cline.0
@@ -7962,17 +7961,15 @@ proc prevfile {} {
 
     if {$cmitmode eq "tree"} return
     set prev 0.0
-    set prevline 1
     set here [$ctext index @0,0]
     foreach loc $difffilestart {
 	if {[$ctext compare $loc >= $here]} {
-	    highlightfile $prev $prevline
+	    $ctext yview $prev
 	    return
 	}
 	set prev $loc
-	incr prevline
     }
-    highlightfile $prev $prevline
+    $ctext yview $prev
 }
 
 proc nextfile {} {
@@ -7980,11 +7977,9 @@ proc nextfile {} {
 
     if {$cmitmode eq "tree"} return
     set here [$ctext index @0,0]
-    set line 1
     foreach loc $difffilestart {
-	incr line
 	if {[$ctext compare $loc > $here]} {
-	    highlightfile $loc $line
+	    $ctext yview $loc
 	    return
 	}
     }
@@ -8138,7 +8133,17 @@ proc searchmarkvisible {doall} {
 }
 
 proc scrolltext {f0 f1} {
-    global searchstring
+    global searchstring cmitmode
+    global ctext cflist cflist_top difffilestart
+
+    if {$cmitmode ne "tree" && [info exists difffilestart]} {
+	set top [lindex [split [$ctext index @0,0] .] 0]
+	if {$top < [lindex $difffilestart 0]} {
+	    highlightfile 0
+	} else {
+	    highlightfile [expr {[bsearch $difffilestart $top] + 2}]
+	}
+    }
 
     .bleft.bottom.sb set $f0 $f1
     if {$searchstring ne {}} {
-- 
1.7.12.376.g8258bbd

Re: [PATCH] gitk: Synchronize highlighting in file view when scrolling diff

[Peter Oberndorfer]

On 2012-09-18 07:57, Stefan Haller wrote:
> Whenever the diff pane scrolls, highlight the corresponding file in the
> file list on the right. For a large commit with many files and long
> per-file diffs, this makes it easier to keep track of what you're looking
> at.

Hi,
i like this function!
I have often lost track which file i am currently looking at.
Especially with long files.

But i noticed following minor thing:
When selecting a new commit, for a split second the first file name is selected
and then the "Comments" entry is selected.
The flickering is more visible when you hold down the mouse on the up/down button
of the "Lines of code" field.

Can this flickering be avoided?

Thanks,
Greetings Peter

> This allows simplifying the prevfile and nextfile functions, because
> all they have to do is scroll the diff pane.
>
> Signed-off-by: Stefan Haller <stefan@haller-berlin.de>
> ---
>   gitk | 27 ++++++++++++++++-----------
>   1 file changed, 16 insertions(+), 11 deletions(-)
>
>

Re: [PATCH] gitk: Synchronize highlighting in file view when scrolling diff

[Paul Mackerras]

On Tue, Sep 18, 2012 at 07:57:54AM +0200, Stefan Haller wrote:
> Whenever the diff pane scrolls, highlight the corresponding file in the
> file list on the right. For a large commit with many files and long
> per-file diffs, this makes it easier to keep track of what you're looking
> at.

I like this as far as it goes, but the one criticism I would have is
that when you find a string (using the "Search" button), the filename
that gets highlighted is often not the file in which the string was
found (because the highlighting is based on the top line visible in
the text window), which could be confusing.

Can you think of a way to solve that too?  Perhaps make the
highlighting based on the currently highlighted instance of the search
string, if there is one, otherwise based on the top line visible?

Paul.

[PATCH v2] gitk: Synchronize highlighting in file view when scrolling diff

[Stefan Haller]

Whenever the diff pane scrolls, highlight the corresponding file in the
file list on the right. For a large commit with many files and long
per-file diffs, this makes it easier to keep track of what you're looking
at.

This allows simplifying the prevfile and nextfile functions, because
all they have to do is scroll the diff pane.

Signed-off-by: Stefan Haller <stefan@haller-berlin.de>
---
The only change from v1 is the addition of the "$difffilestart eq {}" 
condition, this should fix the flickering problem reported by Peter.
I didn't do anything about the search problem yet, will look into this
next. (Personally though, I think this is acceptable the way it is.)

 gitk | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/gitk b/gitk
index d93bd99..8095806 100755
--- a/gitk
+++ b/gitk
@@ -7947,10 +7947,9 @@ proc changediffdisp {} {
     $ctext tag conf dresult -elide [lindex $diffelide 1]
 }
 
-proc highlightfile {loc cline} {
-    global ctext cflist cflist_top
+proc highlightfile {cline} {
+    global cflist cflist_top
 
-    $ctext yview $loc
     $cflist tag remove highlight $cflist_top.0 "$cflist_top.0 lineend"
     $cflist tag add highlight $cline.0 "$cline.0 lineend"
     $cflist see $cline.0
@@ -7962,17 +7961,15 @@ proc prevfile {} {
 
     if {$cmitmode eq "tree"} return
     set prev 0.0
-    set prevline 1
     set here [$ctext index @0,0]
     foreach loc $difffilestart {
 	if {[$ctext compare $loc >= $here]} {
-	    highlightfile $prev $prevline
+	    $ctext yview $prev
 	    return
 	}
 	set prev $loc
-	incr prevline
     }
-    highlightfile $prev $prevline
+    $ctext yview $prev
 }
 
 proc nextfile {} {
@@ -7980,11 +7977,9 @@ proc nextfile {} {
 
     if {$cmitmode eq "tree"} return
     set here [$ctext index @0,0]
-    set line 1
     foreach loc $difffilestart {
-	incr line
 	if {[$ctext compare $loc > $here]} {
-	    highlightfile $loc $line
+	    $ctext yview $loc
 	    return
 	}
     }
@@ -8138,7 +8133,17 @@ proc searchmarkvisible {doall} {
 }
 
 proc scrolltext {f0 f1} {
-    global searchstring
+    global searchstring cmitmode
+    global ctext cflist cflist_top difffilestart
+
+    if {$cmitmode ne "tree" && [info exists difffilestart]} {
+	set top [lindex [split [$ctext index @0,0] .] 0]
+	if {$difffilestart eq {} || $top < [lindex $difffilestart 0]} {
+	    highlightfile 0
+	} else {
+	    highlightfile [expr {[bsearch $difffilestart $top] + 2}]
+	}
+    }
 
     .bleft.bottom.sb set $f0 $f1
     if {$searchstring ne {}} {
-- 
1.7.12.517.g4c1112f

Re: [PATCH] gitk: Synchronize highlighting in file view when scrolling diff

[Marc Branchaud]

On 12-09-18 07:46 PM, Paul Mackerras wrote:
> On Tue, Sep 18, 2012 at 07:57:54AM +0200, Stefan Haller wrote:
>> Whenever the diff pane scrolls, highlight the corresponding file in the
>> file list on the right. For a large commit with many files and long
>> per-file diffs, this makes it easier to keep track of what you're looking
>> at.
> 
> I like this as far as it goes, but the one criticism I would have is
> that when you find a string (using the "Search" button), the filename
> that gets highlighted is often not the file in which the string was
> found (because the highlighting is based on the top line visible in
> the text window), which could be confusing.

Well, gitk currently doesn't highlight the matching file (or files -- there
can be more than one).  Stefan's patch isn't changing anything with how
string matching already works.

> Can you think of a way to solve that too?  Perhaps make the
> highlighting based on the currently highlighted instance of the search
> string, if there is one, otherwise based on the top line visible?

I think you're asking for a new feature.

Highlight-files-with-string-matches is different from Stefan's
consistently-highlight-currently-displayed-file, so it should be done
differently.  Perhaps use a different highlight colour, or overlay the
matching files with an icon.

		M.

[PATCH v3] gitk: Synchronize highlighting in file view when scrolling diff

[Stefan Haller]

Whenever the diff pane scrolls, highlight the corresponding file in the
file list on the right. For a large commit with many files and long
per-file diffs, this makes it easier to keep track of what you're looking
at.

This allows simplifying the prevfile and nextfile functions, because
all they have to do is scroll the diff pane.

In some situations we want to suppress this mechanism, for example when
clicking on a file in the file list to select it, or when searching in the
diff, in which case we want to highlight based on the current search hit
and not the top line visible. In these cases it's not sufficiant to set
a "suppress" flag before scrolling and reset it afterwards, because the
scrolltext notification is sent deferred from a timer or some such; so we
need to remember the scroll position for which we want to suppress the
auto-highlighting until the next call to scrolltext; a bit ugly, but does
the job.

Signed-off-by: Stefan Haller <stefan@haller-berlin.de>
---
Here's one way how to address your concern. When pressing the search button
it will highlight the file that contains the current search hit; if you then
scroll from there though, the normal mechanism kicks in again and might
highlight the previous file. The same happens now if you select the last file
in the list, but it's diff is smaller than a screenful. In the previous
patch versions it would select a different file than you clicked on, which
is probably also confusing.

Is this what you had in mind?

Stefan.

 gitk | 54 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 11 deletions(-)

diff --git a/gitk b/gitk
index d93bd99..16832a9 100755
--- a/gitk
+++ b/gitk
@@ -3309,6 +3309,7 @@ proc sel_flist {w x y} {
     } else {
 	catch {$ctext yview [lindex $difffilestart [expr {$l - 2}]]}
     }
+    suppress_highlighting_file_for_current_scrollpos
 }
 
 proc pop_flist_menu {w X Y x y} {
@@ -7947,32 +7948,42 @@ proc changediffdisp {} {
     $ctext tag conf dresult -elide [lindex $diffelide 1]
 }
 
-proc highlightfile {loc cline} {
-    global ctext cflist cflist_top
+proc highlightfile {cline} {
+    global cflist cflist_top
 
-    $ctext yview $loc
     $cflist tag remove highlight $cflist_top.0 "$cflist_top.0 lineend"
     $cflist tag add highlight $cline.0 "$cline.0 lineend"
     $cflist see $cline.0
     set cflist_top $cline
 }
 
+proc highlightfile_for_scrollpos {topidx} {
+    global difffilestart
+
+    if {![info exists difffilestart]} return
+
+    set top [lindex [split $topidx .] 0]
+    if {$difffilestart eq {} || $top < [lindex $difffilestart 0]} {
+	highlightfile 0
+    } else {
+	highlightfile [expr {[bsearch $difffilestart $top] + 2}]
+    }
+}
+
 proc prevfile {} {
     global difffilestart ctext cmitmode
 
     if {$cmitmode eq "tree"} return
     set prev 0.0
-    set prevline 1
     set here [$ctext index @0,0]
     foreach loc $difffilestart {
 	if {[$ctext compare $loc >= $here]} {
-	    highlightfile $prev $prevline
+	    $ctext yview $prev
 	    return
 	}
 	set prev $loc
-	incr prevline
     }
-    highlightfile $prev $prevline
+    $ctext yview $prev
 }
 
 proc nextfile {} {
@@ -7980,11 +7991,9 @@ proc nextfile {} {
 
     if {$cmitmode eq "tree"} return
     set here [$ctext index @0,0]
-    set line 1
     foreach loc $difffilestart {
-	incr line
 	if {[$ctext compare $loc > $here]} {
-	    highlightfile $loc $line
+	    $ctext yview $loc
 	    return
 	}
     }
@@ -8046,6 +8055,8 @@ proc incrsearch {name ix op} {
 	set here [$ctext search $searchdirn -- $searchstring anchor]
 	if {$here ne {}} {
 	    $ctext see $here
+	    suppress_highlighting_file_for_current_scrollpos
+	    highlightfile_for_scrollpos $here
 	}
 	searchmarkvisible 1
     }
@@ -8071,6 +8082,8 @@ proc dosearch {} {
 	    return
 	}
 	$ctext see $match
+	suppress_highlighting_file_for_current_scrollpos
+	highlightfile_for_scrollpos $match
 	set mend "$match + $mlen c"
 	$ctext tag add sel $match $mend
 	$ctext mark unset anchor
@@ -8097,6 +8110,8 @@ proc dosearchback {} {
 	    return
 	}
 	$ctext see $match
+	suppress_highlighting_file_for_current_scrollpos
+	highlightfile_for_scrollpos $match
 	set mend "$match + $ml c"
 	$ctext tag add sel $match $mend
 	$ctext mark unset anchor
@@ -8137,8 +8152,25 @@ proc searchmarkvisible {doall} {
     }
 }
 
+proc suppress_highlighting_file_for_current_scrollpos {} {
+    global ctext suppress_highlighting_file_for_this_scrollpos
+
+    set suppress_highlighting_file_for_this_scrollpos [$ctext index @0,0]
+}
+
 proc scrolltext {f0 f1} {
-    global searchstring
+    global searchstring cmitmode ctext
+    global suppress_highlighting_file_for_this_scrollpos
+
+    if {$cmitmode ne "tree"} {
+	set topidx [$ctext index @0,0]
+	if {![info exists suppress_highlighting_file_for_this_scrollpos]
+	    || $topidx ne $suppress_highlighting_file_for_this_scrollpos} {
+	    highlightfile_for_scrollpos $topidx
+	}
+    }
+
+    catch {unset suppress_highlighting_file_for_this_scrollpos}
 
     .bleft.bottom.sb set $f0 $f1
     if {$searchstring ne {}} {
-- 
1.7.12.517.g6dec59e

Re: [PATCH v3] gitk: Synchronize highlighting in file view when scrolling diff

[Paul Mackerras]

On Wed, Sep 19, 2012 at 08:17:27PM +0200, Stefan Haller wrote:
> Here's one way how to address your concern. When pressing the search button
> it will highlight the file that contains the current search hit; if you then
> scroll from there though, the normal mechanism kicks in again and might
> highlight the previous file. The same happens now if you select the last file
> in the list, but it's diff is smaller than a screenful. In the previous
> patch versions it would select a different file than you clicked on, which
> is probably also confusing.
> 
> Is this what you had in mind?

Yes, it is, and I applied your patch.  I wonder though if it might
work better to highlight all the files that are visible?

Paul.

Re: [PATCH v3] gitk: Synchronize highlighting in file view when scrolling diff

[Stefan Haller]

Paul Mackerras <paulus@samba.org> wrote:

> On Wed, Sep 19, 2012 at 08:17:27PM +0200, Stefan Haller wrote:
> > Here's one way how to address your concern. When pressing the search button
> > it will highlight the file that contains the current search hit; if you then
> > scroll from there though, the normal mechanism kicks in again and might
> > highlight the previous file. The same happens now if you select the last file
> > in the list, but it's diff is smaller than a screenful. In the previous
> > patch versions it would select a different file than you clicked on, which
> > is probably also confusing.
> > 
> > Is this what you had in mind?
> 
> Yes, it is, and I applied your patch.  I wonder though if it might
> work better to highlight all the files that are visible?

Interesting idea. I tried it, but I don't like it much, it just looks
and feels so odd. I can send a patch if you're interested in trying it
yourself.

But personally, I really only need the synchronization feature in the
case where a file's diff is longer than fits on a screen; as long as a
file header is visible on the left side, it's prominent enough that I
don't need more guidance.

-Stefan


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

[PATCH] gitk: Do not select file list entries during diff loading

[Peter Oberndorfer]

Scrolling notification works by callingscrolltext{}
with with 2 values between 0 and 1
for the beginning and the end of the view relative to the total length.
When a long diff with several files is loaded,
the diff view length is updated several times
and causes executions of scrolltext{} even when
the current view never changed.

Every time scrolltext{} is executed,
a entry in the file list is selected and scrolled to.

This makes it impossible for a user to scroll the file list
while a long diff is still loading.

Signed-off-by: Peter Oberndorfer <kumbayo84@arcor.de>
---
Hi,

i used v3 of the Synchronize patch (+ the 2 fixes on top)
for some time now on mingw,
but i found a slight problem for my usage.

While the diff is loaded, the file list on the right side always scrolls up.
When a single revision touches hundreds of files [1] the loading takes
quite long.
During the diff loading i want to scroll down in the file list to the
relevant file i
am interested in. But the file list jumps up all the time.

Please review/test the patch carefully before applying,
since i do not often work with tcl/tk :-)
(Or suggest better ways to solve this problem)

Greetings Peter

[1] I imported history of a historic project. Each release is represented
by a single commit. Thus one commit contains a lot of files/big amount
of changes.
But most times i am interested in only a single file in the middle of
the file list.

 gitk-git/gitk | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index b294c9e..621db87 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -8004,7 +8004,7 @@ proc nextfile {} {

 proc clear_ctext {{first 1.0}} {
     global ctext smarktop smarkbot
-    global ctext_file_names ctext_file_lines
+    global ctext_file_names ctext_file_lines ctext_last_scroll_pos
     global pendinglinks

     set l [lindex [split $first .] 0]
@@ -8020,6 +8020,7 @@ proc clear_ctext {{first 1.0}} {
     }
     set ctext_file_names {}
     set ctext_file_lines {}
+    set ctext_last_scroll_pos -1
 }

 proc settabs {{firstab {}}} {
@@ -8162,21 +8163,24 @@ proc
suppress_highlighting_file_for_current_scrollpos {} {
 }

 proc scrolltext {f0 f1} {
-    global searchstring cmitmode ctext
+    global searchstring cmitmode ctext ctext_last_scroll_pos
     global suppress_highlighting_file_for_this_scrollpos

+    .bleft.bottom.sb set $f0 $f1
+    if {$searchstring ne {}} {
+	searchmarkvisible 0
+    }
+
     set topidx [$ctext index @0,0]
+    if {$topidx eq $ctext_last_scroll_pos} return
+    set ctext_last_scroll_pos $topidx
+
     if {![info exists suppress_highlighting_file_for_this_scrollpos]
 	|| $topidx ne $suppress_highlighting_file_for_this_scrollpos} {
 	highlightfile_for_scrollpos $topidx
     }

     catch {unset suppress_highlighting_file_for_this_scrollpos}
-
-    .bleft.bottom.sb set $f0 $f1
-    if {$searchstring ne {}} {
-	searchmarkvisible 0
-    }
 }

 proc setcoords {} {
@@ -11643,6 +11647,7 @@ set autoselect 1
 set autosellen 40
 set perfile_attrs 0
 set want_ttk 1
+set ctext_last_scroll_pos -1

 if {[tk windowingsystem] eq "aqua"} {
     set extdifftool "opendiff"
-- 
1.8.0.rc2.251.g3315d86

Re: [PATCH] gitk: Do not select file list entries during diff loading

[Stefan Haller]

Peter Oberndorfer <kumbayo84@arcor.de> wrote:

> Please review/test the patch carefully before applying,
> since i do not often work with tcl/tk :-)

The patch makes perfect sense to me.  (I'm not a great tcl coder either
though, and not very familiar with the gitk code; so another review
would be helpful.)

Just one minor suggestion:

>  proc scrolltext {f0 f1} {
> -    global searchstring cmitmode ctext
> +    global searchstring cmitmode ctext ctext_last_scroll_pos
>      global suppress_highlighting_file_for_this_scrollpos
> 
> +    .bleft.bottom.sb set $f0 $f1
> +    if {$searchstring ne {}} {
> +        searchmarkvisible 0
> +    }
> +
>      set topidx [$ctext index @0,0]
> +    if {$topidx eq $ctext_last_scroll_pos} return
> +    set ctext_last_scroll_pos $topidx
> +
>      if {![info exists suppress_highlighting_file_for_this_scrollpos]
>          || $topidx ne $suppress_highlighting_file_for_this_scrollpos} {
>          highlightfile_for_scrollpos $topidx
>      }
> 
>      catch {unset suppress_highlighting_file_for_this_scrollpos}
> -
> -    .bleft.bottom.sb set $f0 $f1
> -    if {$searchstring ne {}} {
> -        searchmarkvisible 0
> -    }
>  }

I don't like early returns, they can easily become a source of bugs when
someone adds more code to the end of a function without realizing that
there's an early return in the middle.  I'd much rather say something
like

    if {$topidx ne $ctext_last_scroll_pos} {
        if {![info exists suppress_highlighting_file_for_this_scrollpos]
             || $topidx ne $suppress_highlighting_file_for_this_scrollpos} {
             highlightfile_for_scrollpos $topidx
        }

        set ctext_last_scroll_pos $topidx
    }


-Stefan


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/