Hi Birger,
Your subject is a bit redundant. A reader of this commit can easily see
the diff and know that you implemented "proc select_path_in_widget".
What's more important is why you implemented it. That is what should go
in the commit message. So for example in this patch, you can say
something like:
git-gui: move last clicked path selection logic to a separate function
This same logic will be used elsewhere in a follow-up commit, so make
it re-useable.
This is what I came up with at first thought. Maybe something even
better and concise can say the same thing.
On 07/10/19 07:11PM, Birger Skogeng Pedersen wrote:
> Signed-off-by: Birger Skogeng Pedersen <birger.sp@xxxxxxxxx>
> ---
> git-gui.sh | 32 +++++++++++++++++++-------------
> 1 file changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index fd476b6..b7f4d1e 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -2669,25 +2669,31 @@ proc show_less_context {} {
> }
>
> proc focus_widget {widget} {
> - global file_lists last_clicked selected_paths
> - global file_lists_last_clicked
> + global file_lists
>
> if {[llength $file_lists($widget)] > 0} {
> - set path $file_lists_last_clicked($widget)
> - set index [lsearch -sorted -exact $file_lists($widget) $path]
> - if {$index < 0} {
> - set index 0
> - set path [lindex $file_lists($widget) $index]
> - }
> -
> + select_path_in_widget $widget
> focus $widget
> - set last_clicked [list $widget [expr $index + 1]]
> - array unset selected_paths
> - set selected_paths($path) 1
> - show_diff $path $widget
There is a change in the order of events here. Earlier, we first
focussed the widget, and then ran `show_diff`. Now we first run
`show_diff` (via `select_path_in_widget`), and then focus the widget.
This won't cause any problems, right?
> }
> }
>
> +proc select_path_in_widget {widget} {
> + global file_lists last_clicked selected_paths
> + global file_lists_last_clicked
> +
> + set path $file_lists_last_clicked($widget)
> + set index [lsearch -sorted -exact $file_lists($widget) $path]
> + if {$index < 0} {
> + set index 0
> + set path [lindex $file_lists($widget) $index]
> + }
> +
> + set last_clicked [list $widget [expr $index + 1]]
> + array unset selected_paths
> + set selected_paths($path) 1
> + show_diff $path $widget
> +}
> +
> proc toggle_commit_type {} {
> global commit_type_is_amend
> set commit_type_is_amend [expr !$commit_type_is_amend]
Other than that, looks good. There isn't much changed here. Just some
code moved around.
--
Regards,
Pratyush Yadav
