threads / patch / 31581

patchgitk: Synchronize highlighting in file view when scrolling diff

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

## tl;dr

10 messages between Sep 18, 2012 and Oct 29, 2012. Diffs are folded; open one to read it.

replies: 9people: 5as markdown or json

Stefan Haller· Sep 18, 2012, 05:57 UTC · lore

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(-)
Show changes to gitk +16 −11
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
Peter Oberndorfer· Sep 18, 2012, 18:23 UTC · re: Stefan Haller · lore

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

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

Show 9 quoted lines
> 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(-)
>
>
Stefan Haller· Sep 19, 2012, 09:57 UTC · re: Peter Oberndorfer · lore

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

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(-)
Show changes to gitk +16 −11
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
Paul Mackerras· Sep 18, 2012, 23:46 UTC · re: Stefan Haller · lore

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

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.
Marc Branchaud· Sep 19, 2012, 14:28 UTC · re: Paul Mackerras · lore

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

On 12-09-18 07:46 PM, Paul Mackerras wrote:
Show 11 quoted lines
> 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.
Stefan Haller· Sep 19, 2012, 18:17 UTC · re: Paul Mackerras · lore

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

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(-)
Show changes to gitk +43 −11
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
Paul Mackerras· Sep 23, 2012, 06:58 UTC · re: Stefan Haller · lore

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

On Wed, Sep 19, 2012 at 08:17:27PM +0200, Stefan Haller wrote:
Show 9 quoted lines
> 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.
Stefan Haller· Sep 24, 2012, 05:51 UTC · re: Paul Mackerras · lore

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

Paul Mackerras <paulus@samba.org> wrote:
Show 13 quoted lines
> 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/
Peter Oberndorfer· Oct 26, 2012, 22:03 UTC · re: Stefan Haller · lore

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

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(-)
Show changes to gitk-git/gitk +12 −7
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
Stefan Haller· Oct 29, 2012, 08:56 UTC · re: Peter Oberndorfer · lore

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

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:
Show 26 quoted lines
>  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/

← back to recent threads