git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Pratyush Yadav <me@yadavpratyush.com>
To: Harish Karumuthil <harish2704@gmail.com>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	git@vger.kernel.org, David Aguilar <davvid@gmail.com>
Subject: Re: [PATCH] Feature: custom guitool commands can now have custom keyboard shortcuts
Date: Mon, 14 Oct 2019 01:39:29 +0530
Message-ID: <20191013200929.72giwtxlt6ivitfr@yadavpratyush.com> (raw)
In-Reply-To: <f751705949a7fd23c77cbbf839c081b95b12394b.camel@gmail.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

  parent reply	other threads:[~2019-10-13 20:09 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-29 11:38 Harish K
2016-03-31 16:41 ` David Aguilar
2016-04-01  6:32   ` harish k
2019-10-03 14:48     ` harish k
2019-10-03 21:44       ` Pratyush Yadav
2019-10-04  8:49         ` Johannes Schindelin
2019-10-04 12:01           ` Pratyush Yadav
2019-10-05 20:16             ` Harish Karumuthil
2019-10-05 21:01               ` Pratyush Yadav
2019-10-06  9:49                 ` Johannes Schindelin
2019-10-06 18:39                   ` Pratyush Yadav
2019-10-06 19:37                     ` Philip Oakley
2019-10-06 20:27                     ` Johannes Schindelin
2019-10-06 21:06                       ` Pratyush Yadav
2019-10-07  9:20                         ` GitGUIGadget, was " Johannes Schindelin
2019-10-07 10:43                           ` Birger Skogeng Pedersen
2019-10-07 19:16                             ` Alban Gruin
2019-11-19 22:09                         ` Making GitGitGadget's list -> PR comment mirroring bidirectional, " Johannes Schindelin
2019-11-20 14:22                           ` Pratyush Yadav
2019-10-06 22:40                       ` Philip Oakley
2019-10-07  6:22                   ` Harish Karumuthil
2019-10-07 10:01                     ` Johannes Schindelin
2019-10-08 19:31                       ` Harish Karumuthil
2019-10-09 20:42                         ` Johannes Schindelin
2019-10-13 20:09                         ` Pratyush Yadav [this message]
2019-10-07  6:13                 ` Harish Karumuthil
2019-10-13 19:17                   ` Pratyush Yadav
  -- strict thread matches above, loose matches on Subject: below --
2016-03-29 11:29 Harish K
2016-03-31 16:49 ` David Aguilar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191013200929.72giwtxlt6ivitfr@yadavpratyush.com \
    --to=me@yadavpratyush.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=davvid@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=harish2704@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git