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

2012-09-23 Thread Stefan Haller
Paul Mackerras  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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-23 Thread 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-19 Thread 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 
---
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 suppr