{"thread":{"id":"31581","subject":"[PATCH] gitk: Synchronize highlighting in file view when scrolling diff","startedAt":"2012-09-18T05:57:54Z","lastAt":"2012-10-29T08:56:50Z","messageCount":10,"participants":["Stefan Haller","Peter Oberndorfer","Paul Mackerras","Marc Branchaud"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"199331","messageId":"1347947874-38597-1-git-send-email-stefan@haller-berlin.de","threadId":"31581","inReplyTo":null,"subject":"[PATCH] gitk: Synchronize highlighting in file view when scrolling diff","fromName":"Stefan Haller","fromEmail":"stefan@haller-berlin.de","sentAt":"2012-09-18T05:57:54Z","receivedAt":"2012-09-18T05:57:54Z","isPatch":true,"sender":{"key":"stefan@haller-berlin.de","avatar":"https://avatars.githubusercontent.com/u/1225667?v=4"},"body":"Whenever the diff pane scrolls, highlight the corresponding file in the\nfile list on the right. For a large commit with many files and long\nper-file diffs, this makes it easier to keep track of what you're looking\nat.\n\nThis allows simplifying the prevfile and nextfile functions, because\nall they have to do is scroll the diff pane.\n\nSigned-off-by: Stefan Haller \n---\n gitk | 27 ++++++++++++++++-----------\n 1 file changed, 16 insertions(+), 11 deletions(-)\n\ndiff --git a/gitk b/gitk\nindex d93bd99..9e3ec71 100755\n--- a/gitk\n+++ b/gitk\n@@ -7947,10 +7947,9 @@ proc changediffdisp {} {\n $ctext tag conf dresult -elide [lindex $diffelide 1]\n }\n \n-proc highlightfile {loc cline} {\n- global ctext cflist cflist_top\n+proc highlightfile {cline} {\n+ global cflist cflist_top\n \n- $ctext yview $loc\n $cflist tag remove highlight $cflist_top.0 \"$cflist_top.0 lineend\"\n $cflist tag add highlight $cline.0 \"$cline.0 lineend\"\n $cflist see $cline.0\n@@ -7962,17 +7961,15 @@ proc prevfile {} {\n \n if {$cmitmode eq \"tree\"} return\n set prev 0.0\n- set prevline 1\n set here [$ctext index @0,0]\n foreach loc $difffilestart {\n \tif {[$ctext compare $loc >= $here]} {\n-\t highlightfile $prev $prevline\n+\t $ctext yview $prev\n \t return\n \t}\n \tset prev $loc\n-\tincr prevline\n }\n- highlightfile $prev $prevline\n+ $ctext yview $prev\n }\n \n proc nextfile {} {\n@@ -7980,11 +7977,9 @@ proc nextfile {} {\n \n if {$cmitmode eq \"tree\"} return\n set here [$ctext index @0,0]\n- set line 1\n foreach loc $difffilestart {\n-\tincr line\n \tif {[$ctext compare $loc > $here]} {\n-\t highlightfile $loc $line\n+\t $ctext yview $loc\n \t return\n \t}\n }\n@@ -8138,7 +8133,17 @@ proc searchmarkvisible {doall} {\n }\n \n proc scrolltext {f0 f1} {\n- global searchstring\n+ global searchstring cmitmode\n+ global ctext cflist cflist_top difffilestart\n+\n+ if {$cmitmode ne \"tree\" && [info exists difffilestart]} {\n+\tset top [lindex [split [$ctext index @0,0] .] 0]\n+\tif {$top < [lindex $difffilestart 0]} {\n+\t highlightfile 0\n+\t} else {\n+\t highlightfile [expr {[bsearch $difffilestart $top] + 2}]\n+\t}\n+ }\n \n .bleft.bottom.sb set $f0 $f1\n if {$searchstring ne {}} {\n-- \n1.7.12.376.g8258bbd\n"},{"id":"199383","messageId":"5058BC35.6050007@arcor.de","threadId":"31581","inReplyTo":"1347947874-38597-1-git-send-email-stefan@haller-berlin.de","subject":"Re: [PATCH] gitk: Synchronize highlighting in file view when scrolling diff","fromName":"Peter Oberndorfer","fromEmail":"kumbayo84@arcor.de","sentAt":"2012-09-18T18:23:49Z","receivedAt":"2012-09-18T18:23:49Z","isPatch":true,"sender":{"key":"kumbayo84@arcor.de","avatar":"https://avatars.githubusercontent.com/u/1041267?v=4"},"body":"On 2012-09-18 07:57, Stefan Haller wrote:\n> Whenever the diff pane scrolls, highlight the corresponding file in the\n> file list on the right. For a large commit with many files and long\n> per-file diffs, this makes it easier to keep track of what you're looking\n> at.\n\nHi,\ni like this function!\nI have often lost track which file i am currently looking at.\nEspecially with long files.\n\nBut i noticed following minor thing:\nWhen selecting a new commit, for a split second the first file name is selected\nand then the \"Comments\" entry is selected.\nThe flickering is more visible when you hold down the mouse on the up/down button\nof the \"Lines of code\" field.\n\nCan this flickering be avoided?\n\nThanks,\nGreetings Peter\n\n> This allows simplifying the prevfile and nextfile functions, because\n> all they have to do is scroll the diff pane.\n>\n> Signed-off-by: Stefan Haller \n> ---\n> gitk | 27 ++++++++++++++++-----------\n> 1 file changed, 16 insertions(+), 11 deletions(-)\n>\n>\n"},{"id":"199435","messageId":"20120918234611.GA5544@bloggs.ozlabs.ibm.com","threadId":"31581","inReplyTo":"1347947874-38597-1-git-send-email-stefan@haller-berlin.de","subject":"Re: [PATCH] gitk: Synchronize highlighting in file view when scrolling diff","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2012-09-18T23:46:11Z","receivedAt":"2012-09-18T23:46:11Z","isPatch":true,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"On Tue, Sep 18, 2012 at 07:57:54AM +0200, Stefan Haller wrote:\n> Whenever the diff pane scrolls, highlight the corresponding file in the\n> file list on the right. For a large commit with many files and long\n> per-file diffs, this makes it easier to keep track of what you're looking\n> at.\n\nI like this as far as it goes, but the one criticism I would have is\nthat when you find a string (using the \"Search\" button), the filename\nthat gets highlighted is often not the file in which the string was\nfound (because the highlighting is based on the top line visible in\nthe text window), which could be confusing.\n\nCan you think of a way to solve that too? Perhaps make the\nhighlighting based on the currently highlighted instance of the search\nstring, if there is one, otherwise based on the top line visible?\n\nPaul.\n"},{"id":"199455","messageId":"1348048641-5765-1-git-send-email-stefan@haller-berlin.de","threadId":"31581","inReplyTo":"5058BC35.6050007@arcor.de","subject":"[PATCH v2] gitk: Synchronize highlighting in file view when scrolling diff","fromName":"Stefan Haller","fromEmail":"stefan@haller-berlin.de","sentAt":"2012-09-19T09:57:21Z","receivedAt":"2012-09-19T09:57:21Z","isPatch":true,"sender":{"key":"stefan@haller-berlin.de","avatar":"https://avatars.githubusercontent.com/u/1225667?v=4"},"body":"Whenever the diff pane scrolls, highlight the corresponding file in the\nfile list on the right. For a large commit with many files and long\nper-file diffs, this makes it easier to keep track of what you're looking\nat.\n\nThis allows simplifying the prevfile and nextfile functions, because\nall they have to do is scroll the diff pane.\n\nSigned-off-by: Stefan Haller \n---\nThe only change from v1 is the addition of the \"$difffilestart eq {}\" \ncondition, this should fix the flickering problem reported by Peter.\nI didn't do anything about the search problem yet, will look into this\nnext. (Personally though, I think this is acceptable the way it is.)\n\n gitk | 27 ++++++++++++++++-----------\n 1 file changed, 16 insertions(+), 11 deletions(-)\n\ndiff --git a/gitk b/gitk\nindex d93bd99..8095806 100755\n--- a/gitk\n+++ b/gitk\n@@ -7947,10 +7947,9 @@ proc changediffdisp {} {\n $ctext tag conf dresult -elide [lindex $diffelide 1]\n }\n \n-proc highlightfile {loc cline} {\n- global ctext cflist cflist_top\n+proc highlightfile {cline} {\n+ global cflist cflist_top\n \n- $ctext yview $loc\n $cflist tag remove highlight $cflist_top.0 \"$cflist_top.0 lineend\"\n $cflist tag add highlight $cline.0 \"$cline.0 lineend\"\n $cflist see $cline.0\n@@ -7962,17 +7961,15 @@ proc prevfile {} {\n \n if {$cmitmode eq \"tree\"} return\n set prev 0.0\n- set prevline 1\n set here [$ctext index @0,0]\n foreach loc $difffilestart {\n \tif {[$ctext compare $loc >= $here]} {\n-\t highlightfile $prev $prevline\n+\t $ctext yview $prev\n \t return\n \t}\n \tset prev $loc\n-\tincr prevline\n }\n- highlightfile $prev $prevline\n+ $ctext yview $prev\n }\n \n proc nextfile {} {\n@@ -7980,11 +7977,9 @@ proc nextfile {} {\n \n if {$cmitmode eq \"tree\"} return\n set here [$ctext index @0,0]\n- set line 1\n foreach loc $difffilestart {\n-\tincr line\n \tif {[$ctext compare $loc > $here]} {\n-\t highlightfile $loc $line\n+\t $ctext yview $loc\n \t return\n \t}\n }\n@@ -8138,7 +8133,17 @@ proc searchmarkvisible {doall} {\n }\n \n proc scrolltext {f0 f1} {\n- global searchstring\n+ global searchstring cmitmode\n+ global ctext cflist cflist_top difffilestart\n+\n+ if {$cmitmode ne \"tree\" && [info exists difffilestart]} {\n+\tset top [lindex [split [$ctext index @0,0] .] 0]\n+\tif {$difffilestart eq {} || $top < [lindex $difffilestart 0]} {\n+\t highlightfile 0\n+\t} else {\n+\t highlightfile [expr {[bsearch $difffilestart $top] + 2}]\n+\t}\n+ }\n \n .bleft.bottom.sb set $f0 $f1\n if {$searchstring ne {}} {\n-- \n1.7.12.517.g4c1112f\n"},{"id":"199462","messageId":"5059D6A2.3000401@xiplink.com","threadId":"31581","inReplyTo":"20120918234611.GA5544@bloggs.ozlabs.ibm.com","subject":"Re: [PATCH] gitk: Synchronize highlighting in file view when scrolling diff","fromName":"Marc Branchaud","fromEmail":"mbranchaud@xiplink.com","sentAt":"2012-09-19T14:28:50Z","receivedAt":"2012-09-19T14:28:50Z","isPatch":true,"sender":{"key":"mbranchaud@xiplink.com","avatar":null},"body":"On 12-09-18 07:46 PM, Paul Mackerras wrote:\n> On Tue, Sep 18, 2012 at 07:57:54AM +0200, Stefan Haller wrote:\n>> Whenever the diff pane scrolls, highlight the corresponding file in the\n>> file list on the right. For a large commit with many files and long\n>> per-file diffs, this makes it easier to keep track of what you're looking\n>> at.\n> \n> I like this as far as it goes, but the one criticism I would have is\n> that when you find a string (using the \"Search\" button), the filename\n> that gets highlighted is often not the file in which the string was\n> found (because the highlighting is based on the top line visible in\n> the text window), which could be confusing.\n\nWell, gitk currently doesn't highlight the matching file (or files -- there\ncan be more than one). Stefan's patch isn't changing anything with how\nstring matching already works.\n\n> Can you think of a way to solve that too? Perhaps make the\n> highlighting based on the currently highlighted instance of the search\n> string, if there is one, otherwise based on the top line visible?\n\nI think you're asking for a new feature.\n\nHighlight-files-with-string-matches is different from Stefan's\nconsistently-highlight-currently-displayed-file, so it should be done\ndifferently. Perhaps use a different highlight colour, or overlay the\nmatching files with an icon.\n\n\t\tM.\n"},{"id":"199504","messageId":"1348078647-22516-1-git-send-email-stefan@haller-berlin.de","threadId":"31581","inReplyTo":"20120918234611.GA5544@bloggs.ozlabs.ibm.com","subject":"[PATCH v3] gitk: Synchronize highlighting in file view when scrolling diff","fromName":"Stefan Haller","fromEmail":"stefan@haller-berlin.de","sentAt":"2012-09-19T18:17:27Z","receivedAt":"2012-09-19T18:17:27Z","isPatch":true,"sender":{"key":"stefan@haller-berlin.de","avatar":"https://avatars.githubusercontent.com/u/1225667?v=4"},"body":"Whenever the diff pane scrolls, highlight the corresponding file in the\nfile list on the right. For a large commit with many files and long\nper-file diffs, this makes it easier to keep track of what you're looking\nat.\n\nThis allows simplifying the prevfile and nextfile functions, because\nall they have to do is scroll the diff pane.\n\nIn some situations we want to suppress this mechanism, for example when\nclicking on a file in the file list to select it, or when searching in the\ndiff, in which case we want to highlight based on the current search hit\nand not the top line visible. In these cases it's not sufficiant to set\na \"suppress\" flag before scrolling and reset it afterwards, because the\nscrolltext notification is sent deferred from a timer or some such; so we\nneed to remember the scroll position for which we want to suppress the\nauto-highlighting until the next call to scrolltext; a bit ugly, but does\nthe job.\n\nSigned-off-by: Stefan Haller \n---\nHere's one way how to address your concern. When pressing the search button\nit will highlight the file that contains the current search hit; if you then\nscroll from there though, the normal mechanism kicks in again and might\nhighlight the previous file. The same happens now if you select the last file\nin the list, but it's diff is smaller than a screenful. In the previous\npatch versions it would select a different file than you clicked on, which\nis probably also confusing.\n\nIs this what you had in mind?\n\nStefan.\n\n gitk | 54 +++++++++++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 43 insertions(+), 11 deletions(-)\n\ndiff --git a/gitk b/gitk\nindex d93bd99..16832a9 100755\n--- a/gitk\n+++ b/gitk\n@@ -3309,6 +3309,7 @@ proc sel_flist {w x y} {\n } else {\n \tcatch {$ctext yview [lindex $difffilestart [expr {$l - 2}]]}\n }\n+ suppress_highlighting_file_for_current_scrollpos\n }\n \n proc pop_flist_menu {w X Y x y} {\n@@ -7947,32 +7948,42 @@ proc changediffdisp {} {\n $ctext tag conf dresult -elide [lindex $diffelide 1]\n }\n \n-proc highlightfile {loc cline} {\n- global ctext cflist cflist_top\n+proc highlightfile {cline} {\n+ global cflist cflist_top\n \n- $ctext yview $loc\n $cflist tag remove highlight $cflist_top.0 \"$cflist_top.0 lineend\"\n $cflist tag add highlight $cline.0 \"$cline.0 lineend\"\n $cflist see $cline.0\n set cflist_top $cline\n }\n \n+proc highlightfile_for_scrollpos {topidx} {\n+ global difffilestart\n+\n+ if {![info exists difffilestart]} return\n+\n+ set top [lindex [split $topidx .] 0]\n+ if {$difffilestart eq {} || $top < [lindex $difffilestart 0]} {\n+\thighlightfile 0\n+ } else {\n+\thighlightfile [expr {[bsearch $difffilestart $top] + 2}]\n+ }\n+}\n+\n proc prevfile {} {\n global difffilestart ctext cmitmode\n \n if {$cmitmode eq \"tree\"} return\n set prev 0.0\n- set prevline 1\n set here [$ctext index @0,0]\n foreach loc $difffilestart {\n \tif {[$ctext compare $loc >= $here]} {\n-\t highlightfile $prev $prevline\n+\t $ctext yview $prev\n \t return\n \t}\n \tset prev $loc\n-\tincr prevline\n }\n- highlightfile $prev $prevline\n+ $ctext yview $prev\n }\n \n proc nextfile {} {\n@@ -7980,11 +7991,9 @@ proc nextfile {} {\n \n if {$cmitmode eq \"tree\"} return\n set here [$ctext index @0,0]\n- set line 1\n foreach loc $difffilestart {\n-\tincr line\n \tif {[$ctext compare $loc > $here]} {\n-\t highlightfile $loc $line\n+\t $ctext yview $loc\n \t return\n \t}\n }\n@@ -8046,6 +8055,8 @@ proc incrsearch {name ix op} {\n \tset here [$ctext search $searchdirn -- $searchstring anchor]\n \tif {$here ne {}} {\n \t $ctext see $here\n+\t suppress_highlighting_file_for_current_scrollpos\n+\t highlightfile_for_scrollpos $here\n \t}\n \tsearchmarkvisible 1\n }\n@@ -8071,6 +8082,8 @@ proc dosearch {} {\n \t return\n \t}\n \t$ctext see $match\n+\tsuppress_highlighting_file_for_current_scrollpos\n+\thighlightfile_for_scrollpos $match\n \tset mend \"$match + $mlen c\"\n \t$ctext tag add sel $match $mend\n \t$ctext mark unset anchor\n@@ -8097,6 +8110,8 @@ proc dosearchback {} {\n \t return\n \t}\n \t$ctext see $match\n+\tsuppress_highlighting_file_for_current_scrollpos\n+\thighlightfile_for_scrollpos $match\n \tset mend \"$match + $ml c\"\n \t$ctext tag add sel $match $mend\n \t$ctext mark unset anchor\n@@ -8137,8 +8152,25 @@ proc searchmarkvisible {doall} {\n }\n }\n \n+proc suppress_highlighting_file_for_current_scrollpos {} {\n+ global ctext suppress_highlighting_file_for_this_scrollpos\n+\n+ set suppress_highlighting_file_for_this_scrollpos [$ctext index @0,0]\n+}\n+\n proc scrolltext {f0 f1} {\n- global searchstring\n+ global searchstring cmitmode ctext\n+ global suppress_highlighting_file_for_this_scrollpos\n+\n+ if {$cmitmode ne \"tree\"} {\n+\tset topidx [$ctext index @0,0]\n+\tif {![info exists suppress_highlighting_file_for_this_scrollpos]\n+\t || $topidx ne $suppress_highlighting_file_for_this_scrollpos} {\n+\t highlightfile_for_scrollpos $topidx\n+\t}\n+ }\n+\n+ catch {unset suppress_highlighting_file_for_this_scrollpos}\n \n .bleft.bottom.sb set $f0 $f1\n if {$searchstring ne {}} {\n-- \n1.7.12.517.g6dec59e\n"},{"id":"199753","messageId":"20120923065825.GA15889@bloggs.ozlabs.ibm.com","threadId":"31581","inReplyTo":"1348078647-22516-1-git-send-email-stefan@haller-berlin.de","subject":"Re: [PATCH v3] gitk: Synchronize highlighting in file view when scrolling diff","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2012-09-23T06:58:25Z","receivedAt":"2012-09-23T06:58:25Z","isPatch":true,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"On Wed, Sep 19, 2012 at 08:17:27PM +0200, Stefan Haller wrote:\n> Here's one way how to address your concern. When pressing the search button\n> it will highlight the file that contains the current search hit; if you then\n> scroll from there though, the normal mechanism kicks in again and might\n> highlight the previous file. The same happens now if you select the last file\n> in the list, but it's diff is smaller than a screenful. In the previous\n> patch versions it would select a different file than you clicked on, which\n> is probably also confusing.\n> \n> Is this what you had in mind?\n\nYes, it is, and I applied your patch. I wonder though if it might\nwork better to highlight all the files that are visible?\n\nPaul.\n"},{"id":"199786","messageId":"1kqvbob.1e7rv3c1cyer9qM%lists@haller-berlin.de","threadId":"31581","inReplyTo":"20120923065825.GA15889@bloggs.ozlabs.ibm.com","subject":"Re: [PATCH v3] gitk: Synchronize highlighting in file view when scrolling diff","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2012-09-24T05:51:56Z","receivedAt":"2012-09-24T05:51:56Z","isPatch":true,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"Paul Mackerras wrote:\n\n> On Wed, Sep 19, 2012 at 08:17:27PM +0200, Stefan Haller wrote:\n> > Here's one way how to address your concern. When pressing the search button\n> > it will highlight the file that contains the current search hit; if you then\n> > scroll from there though, the normal mechanism kicks in again and might\n> > highlight the previous file. The same happens now if you select the last file\n> > in the list, but it's diff is smaller than a screenful. In the previous\n> > patch versions it would select a different file than you clicked on, which\n> > is probably also confusing.\n> > \n> > Is this what you had in mind?\n> \n> Yes, it is, and I applied your patch. I wonder though if it might\n> work better to highlight all the files that are visible?\n\nInteresting idea. I tried it, but I don't like it much, it just looks\nand feels so odd. I can send a patch if you're interested in trying it\nyourself.\n\nBut personally, I really only need the synchronization feature in the\ncase where a file's diff is longer than fits on a screen; as long as a\nfile header is visible on the left side, it's prominent enough that I\ndon't need more guidance.\n\n-Stefan\n\n\n-- \nStefan Haller\nBerlin, Germany\nhttp://www.haller-berlin.de/\n"},{"id":"201994","messageId":"508B08CB.5060702@arcor.de","threadId":"31581","inReplyTo":"1348078647-22516-1-git-send-email-stefan@haller-berlin.de","subject":"[PATCH] gitk: Do not select file list entries during diff loading","fromName":"Peter Oberndorfer","fromEmail":"kumbayo84@arcor.de","sentAt":"2012-10-26T22:03:55Z","receivedAt":"2012-10-26T22:03:55Z","isPatch":true,"sender":{"key":"kumbayo84@arcor.de","avatar":"https://avatars.githubusercontent.com/u/1041267?v=4"},"body":"Scrolling notification works by callingscrolltext{}\nwith with 2 values between 0 and 1\nfor the beginning and the end of the view relative to the total length.\nWhen a long diff with several files is loaded,\nthe diff view length is updated several times\nand causes executions of scrolltext{} even when\nthe current view never changed.\n\nEvery time scrolltext{} is executed,\na entry in the file list is selected and scrolled to.\n\nThis makes it impossible for a user to scroll the file list\nwhile a long diff is still loading.\n\nSigned-off-by: Peter Oberndorfer \n---\nHi,\n\ni used v3 of the Synchronize patch (+ the 2 fixes on top)\nfor some time now on mingw,\nbut i found a slight problem for my usage.\n\nWhile the diff is loaded, the file list on the right side always scrolls up.\nWhen a single revision touches hundreds of files [1] the loading takes\nquite long.\nDuring the diff loading i want to scroll down in the file list to the\nrelevant file i\nam interested in. But the file list jumps up all the time.\n\nPlease review/test the patch carefully before applying,\nsince i do not often work with tcl/tk :-)\n(Or suggest better ways to solve this problem)\n\nGreetings Peter\n\n[1] I imported history of a historic project. Each release is represented\nby a single commit. Thus one commit contains a lot of files/big amount\nof changes.\nBut most times i am interested in only a single file in the middle of\nthe file list.\n\n gitk-git/gitk | 19 ++++++++++++-------\n 1 file changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/gitk-git/gitk b/gitk-git/gitk\nindex b294c9e..621db87 100755\n--- a/gitk-git/gitk\n+++ b/gitk-git/gitk\n@@ -8004,7 +8004,7 @@ proc nextfile {} {\n\n proc clear_ctext {{first 1.0}} {\n global ctext smarktop smarkbot\n- global ctext_file_names ctext_file_lines\n+ global ctext_file_names ctext_file_lines ctext_last_scroll_pos\n global pendinglinks\n\n set l [lindex [split $first .] 0]\n@@ -8020,6 +8020,7 @@ proc clear_ctext {{first 1.0}} {\n }\n set ctext_file_names {}\n set ctext_file_lines {}\n+ set ctext_last_scroll_pos -1\n }\n\n proc settabs {{firstab {}}} {\n@@ -8162,21 +8163,24 @@ proc\nsuppress_highlighting_file_for_current_scrollpos {} {\n }\n\n proc scrolltext {f0 f1} {\n- global searchstring cmitmode ctext\n+ global searchstring cmitmode ctext ctext_last_scroll_pos\n global suppress_highlighting_file_for_this_scrollpos\n\n+ .bleft.bottom.sb set $f0 $f1\n+ if {$searchstring ne {}} {\n+\tsearchmarkvisible 0\n+ }\n+\n set topidx [$ctext index @0,0]\n+ if {$topidx eq $ctext_last_scroll_pos} return\n+ set ctext_last_scroll_pos $topidx\n+\n if {![info exists suppress_highlighting_file_for_this_scrollpos]\n \t|| $topidx ne $suppress_highlighting_file_for_this_scrollpos} {\n \thighlightfile_for_scrollpos $topidx\n }\n\n catch {unset suppress_highlighting_file_for_this_scrollpos}\n-\n- .bleft.bottom.sb set $f0 $f1\n- if {$searchstring ne {}} {\n-\tsearchmarkvisible 0\n- }\n }\n\n proc setcoords {} {\n@@ -11643,6 +11647,7 @@ set autoselect 1\n set autosellen 40\n set perfile_attrs 0\n set want_ttk 1\n+set ctext_last_scroll_pos -1\n\n if {[tk windowingsystem] eq \"aqua\"} {\n set extdifftool \"opendiff\"\n-- \n1.8.0.rc2.251.g3315d86\n"},{"id":"202106","messageId":"1ksq2y4.1mzh8yzydp87lM%lists@haller-berlin.de","threadId":"31581","inReplyTo":"508B08CB.5060702@arcor.de","subject":"Re: [PATCH] gitk: Do not select file list entries during diff loading","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2012-10-29T08:56:50Z","receivedAt":"2012-10-29T08:56:50Z","isPatch":true,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"Peter Oberndorfer wrote:\n\n> Please review/test the patch carefully before applying,\n> since i do not often work with tcl/tk :-)\n\nThe patch makes perfect sense to me. (I'm not a great tcl coder either\nthough, and not very familiar with the gitk code; so another review\nwould be helpful.)\n\nJust one minor suggestion:\n\n> proc scrolltext {f0 f1} {\n> - global searchstring cmitmode ctext\n> + global searchstring cmitmode ctext ctext_last_scroll_pos\n> global suppress_highlighting_file_for_this_scrollpos\n> \n> + .bleft.bottom.sb set $f0 $f1\n> + if {$searchstring ne {}} {\n> + searchmarkvisible 0\n> + }\n> +\n> set topidx [$ctext index @0,0]\n> + if {$topidx eq $ctext_last_scroll_pos} return\n> + set ctext_last_scroll_pos $topidx\n> +\n> if {![info exists suppress_highlighting_file_for_this_scrollpos]\n> || $topidx ne $suppress_highlighting_file_for_this_scrollpos} {\n> highlightfile_for_scrollpos $topidx\n> }\n> \n> catch {unset suppress_highlighting_file_for_this_scrollpos}\n> -\n> - .bleft.bottom.sb set $f0 $f1\n> - if {$searchstring ne {}} {\n> - searchmarkvisible 0\n> - }\n> }\n\nI don't like early returns, they can easily become a source of bugs when\nsomeone adds more code to the end of a function without realizing that\nthere's an early return in the middle. I'd much rather say something\nlike\n\n if {$topidx ne $ctext_last_scroll_pos} {\n if {![info exists suppress_highlighting_file_for_this_scrollpos]\n || $topidx ne $suppress_highlighting_file_for_this_scrollpos} {\n highlightfile_for_scrollpos $topidx\n }\n\n set ctext_last_scroll_pos $topidx\n }\n\n\n-Stefan\n\n\n-- \nStefan Haller\nBerlin, Germany\nhttp://www.haller-berlin.de/\n"}]}