Peter Oberndorfer <kumbayo84@xxxxxxxx> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html