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

From: Pratyush Yadav <me@yadavpratyush.com>
Date: Oct 13, 2019, 20:09 UTC
Message-ID: <20191013200929.72giwtxlt6ivitfr@yadavpratyush.com>
In-Reply-To: <f751705949a7fd23c77cbbf839c081b95b12394b.camel@gmail.com>

On 09/10/19 01:01AM, Harish Karumuthil wrote:

Show 5 quoted lines

> 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.

Show 28 quoted lines

> ---
>  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.

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 10 quoted lines

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