Re: [PATCH 1/2] git-gui: implement proc select_path_in_widget

From: Pratyush Yadav <me@yadavpratyush.com>
Date: Oct 13, 2019, 20:21 UTC
Message-ID: <20191013202110.z3gyx7eikackvmzb@yadavpratyush.com>
In-Reply-To: <20191007171145.1259-1-birger.sp@gmail.com>

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:

Show 31 quoted lines

> Signed-off-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
> ---
>  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?

Show 23 quoted lines

>  	}
>  }
>  
> +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