Re: [PATCH] Feature: custom guitool commands can now have custom keyboard shortcuts

[Pratyush Yadav <me@yadavpratyush.com>]

On 09/10/19 01:01AM, Harish Karumuthil wrote:
> Hi all, there is an update:
> 
> I added necessary error catching code so that, script will not crash if the
> keybinding code is worng. Instead of crashing it will print error message.
> The final patch will look something like this.

Like I mentioned another reply I wrote just now, this unfortunately is 
not a "proper" patch because it does not contain the subject and commit 
message of your commit. Just the diff is not enough, and needs the 
commit subject and message too.

And even for just "preview" patches, I would still recommend sending a 
proper patch so people can also peek at the commit message too. You can 
pass '-rfc' to `git-format-patch` to have something like '[RFC PATCH]' 
in your email subject so people know it is a preview. "RFC" stands for 
"Request For Comments".

Some comments below.
 
> ---
>  lib/tools.tcl | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/tools.tcl b/lib/tools.tcl
> index 413f1a1700..3135e19131 100644
> --- a/lib/tools.tcl
> +++ b/lib/tools.tcl
> @@ -38,7 +38,7 @@ proc tools_create_item {parent args} {
>  }
>  
>  proc tools_populate_one {fullname} {
> -	global tools_menubar tools_menutbl tools_id
> +	global tools_menubar tools_menutbl tools_id repo_config
>  
>  	if {![info exists tools_id]} {
>  		set tools_id 0
> @@ -61,9 +61,25 @@ proc tools_populate_one {fullname} {
>  		}
>  	}
>  
> -	tools_create_item $parent command \
> -		-label [lindex $names end] \
> -		-command [list tools_exec $fullname]
> +	set accel_key_bound 0
> +	if {[info exists repo_config(guitool.$fullname.gitgui-shortcut)]} {
> +		set accel_key $repo_config(guitool.$fullname.gitgui-shortcut)
> +		if { [ catch { bind . <$accel_key> [list tools_exec $fullname] } msg ] } {

This has inconsistent style. There should not be any spaces between '{' 
and '['. So this line should look something like:

  if {[catch {bind . <$accel_key> [list tools_exec $fullname]} msg]} {

You can look at the code around yours to pick up on the general style. 

> +			puts stderr "Failed to bind keyboard shortcut '$accel_key' for custom tool '$fullname'. Error: $msg"

Putting the error message of a GUI application on stderr is probably not 
a good idea. Firstly, since it is a GUI application, we should show 
error messages in the GUI. And secondly, a lot of the time, people 
probably don't even launch git-gui from a terminal command, and do it 
via some application launcher. In that case, there is no stderr that the 
user can easily read from.

So please use a popup dialog instead. Functions to easily create them 
can be found in lib/error.tcl. I'd recommend either `warn_popup` or 
`error_popup`.

As an example, this is what I got when I added a bad shortcut:

  Failed to bind keyboard shortcut 'Ctrl-Z' for custom tool 'Foo'. Error: bad event type or keysym "Ctrl"

Showing the error message from `bind` is pretty neat! The user can know 
_exactly_ what's wrong. One problem is that this might not make that 
much sense to a non-Tcler. But I still think giving a hint of the why it 
failed is a good idea.

I'd like to hear other people's thoughts about it though.

> +		} else {
> +			tools_create_item $parent command \
> +			-label [lindex $names end] \
> +			-command [list tools_exec $fullname] \
> +			-accelerator $accel_key
> +			set accel_key_bound true

Above you set `accel_key_bound` to '0', and here you set it to 'true'. 
Please use consistent forms of a boolean. Either use 'true' and 'false', 
or use '0' and '1'.

> +		}
> +	}
> +
> +	if { ! $accel_key_bound } {

Same style nitpick about the spaces as above.

> +		tools_create_item $parent command \
> +			-label [lindex $names end] \
> +			-command [list tools_exec $fullname]
> +	}
>  }

Can your whole logic of setting an accelerator in case a shortcut exists 
be simplified a bit? Right now, the tools creation command is executed 
in two places, and it is not obvious at first sight that only one of 
them will ever be executed. So maybe something like:

  ...
  if {[catch {bind . <$accel_key> ...} {
  	puts stderr ...
  	set accel_key_bound false
  } else {
  	set accel_key_bound true
  }
  
  if {accel_key_bound} {
  	# Create tool with accelerator
  	...
  } else {
  	# Create tool without accelerator
  	...
  }

I hope you get what this means, but if you don't, please let me know, 
and I'll clarify.

Overall, I like the idea of the patch. This would move us one step in 
the direction of customizable keybindings for _all_ shortcuts. Thanks.

>  
>  proc tools_exec {fullname} {
> ---
> 
> @Johannes Schindelin: In short, from your previous message I understand point.
> 
> 1. shortcut codes like "<Control-,>" will only in Windows platform. It may not work in Linux / Mac.
> 2. We need do translate shortcut codes somehow ( using one-to-one maping ).
> 
> If this is correct, do you have any example on how to do one-to-one maping of a list of string on TCL ?
 

-- 
Regards,
Pratyush Yadav