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

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

[Harish K]

---
 git-gui/lib/tools.tcl | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/git-gui/lib/tools.tcl b/git-gui/lib/tools.tcl
index 6ec9411..749bc67 100644
--- a/git-gui/lib/tools.tcl
+++ b/git-gui/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,19 @@ proc tools_populate_one {fullname} {
 		}
 	}
 
-	tools_create_item $parent command \
+	if {[info exists repo_config(guitool.$fullname.accelerator)] && [info exists repo_config(guitool.$fullname.accelerator-label)]} {
+		set accele_key $repo_config(guitool.$fullname.accelerator)
+		set accel_label $repo_config(guitool.$fullname.accelerator-label)
+		tools_create_item $parent command \
 		-label [lindex $names end] \
-		-command [list tools_exec $fullname]
+		-command [list tools_exec $fullname] \
+		-accelerator $accel_label
+		bind . $accele_key [list tools_exec $fullname]
+	} else {
+		tools_create_item $parent command \
+			-label [lindex $names end] \
+			-command [list tools_exec $fullname]
+	}
 }
 
 proc tools_exec {fullname} {

--
https://github.com/git/git/pull/220

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

[David Aguilar]

Hello,

On Tue, Mar 29, 2016 at 11:38:10AM +0000, Harish K wrote:
> ---
>  git-gui/lib/tools.tcl | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/git-gui/lib/tools.tcl b/git-gui/lib/tools.tcl
> index 6ec9411..749bc67 100644
> --- a/git-gui/lib/tools.tcl
> +++ b/git-gui/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,19 @@ proc tools_populate_one {fullname} {
>  		}
>  	}
>  
> -	tools_create_item $parent command \
> +	if {[info exists repo_config(guitool.$fullname.accelerator)] && [info exists repo_config(guitool.$fullname.accelerator-label)]} {
> +		set accele_key $repo_config(guitool.$fullname.accelerator)
> +		set accel_label $repo_config(guitool.$fullname.accelerator-label)
> +		tools_create_item $parent command \
>  		-label [lindex $names end] \
> -		-command [list tools_exec $fullname]
> +		-command [list tools_exec $fullname] \
> +		-accelerator $accel_label
> +		bind . $accele_key [list tools_exec $fullname]
> +	} else {
> +		tools_create_item $parent command \
> +			-label [lindex $names end] \
> +			-command [list tools_exec $fullname]
> +	}
>  }
>  
>  proc tools_exec {fullname} {
> 
> --
> https://github.com/git/git/pull/220

We also support "custom guitools" in git-cola using this same
mechanism.  If this gets accepted then we'll want to make
similar change there.

There's always a small risk that user-defined tools can conflict
with builtin shortcuts, but otherwise this seems like a pretty
nice feature.  Curious, what is the behavior in the event of a
conflict?  Do the builtins win?  IIRC, Qt handles this by
disabling the shortcut and warning that it's ambiguous.

Please documentation guitool.<name>.accellerator[-label] in
Documentation/config.txt otherwise users will not know that it
exists.

It would also be good for the docs to clarify what the
accelerators look like in case we need to munge them when making
it work in cola via Qt, which has its own mechanism for
associating actions with shortcuts.  Documented examples with
one and two modifier keys would be helpful.


cheers,
-- 
David

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

[harish k]

Hi David,

Actually Im a TCL primer.  This is the first time Im dealing with.
That is why I kept it simple ( ie both accel-key and accel-label need
to be defined in config ).

I think, git-cola is using Qt style representation accel-key in the config file.

Is git-cola is an official tool from git ? like git-gui?
if not ,
My suggesion is, it is better to seperate config fields according to
application-domain
like, "git-gui-accel = <Ctrl-l>" etc..
Other vise there is good chance for conflicts. ( Eg: consider the case
 that, <Ctrl-p> was assined to a custom tool by git-cola )

Currently this patch will not handle any conflicting shortcuts. I
think custom shortcuts will overwrite the other.


On Thu, Mar 31, 2016 at 10:11 PM, David Aguilar <davvid@gmail.com> wrote:
> Hello,
>
> On Tue, Mar 29, 2016 at 11:38:10AM +0000, Harish K wrote:
>> ---
>>  git-gui/lib/tools.tcl | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/git-gui/lib/tools.tcl b/git-gui/lib/tools.tcl
>> index 6ec9411..749bc67 100644
>> --- a/git-gui/lib/tools.tcl
>> +++ b/git-gui/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,19 @@ proc tools_populate_one {fullname} {
>>               }
>>       }
>>
>> -     tools_create_item $parent command \
>> +     if {[info exists repo_config(guitool.$fullname.accelerator)] && [info exists repo_config(guitool.$fullname.accelerator-label)]} {
>> +             set accele_key $repo_config(guitool.$fullname.accelerator)
>> +             set accel_label $repo_config(guitool.$fullname.accelerator-label)
>> +             tools_create_item $parent command \
>>               -label [lindex $names end] \
>> -             -command [list tools_exec $fullname]
>> +             -command [list tools_exec $fullname] \
>> +             -accelerator $accel_label
>> +             bind . $accele_key [list tools_exec $fullname]
>> +     } else {
>> +             tools_create_item $parent command \
>> +                     -label [lindex $names end] \
>> +                     -command [list tools_exec $fullname]
>> +     }
>>  }
>>
>>  proc tools_exec {fullname} {
>>
>> --
>> https://github.com/git/git/pull/220
>
> We also support "custom guitools" in git-cola using this same
> mechanism.  If this gets accepted then we'll want to make
> similar change there.
>
> There's always a small risk that user-defined tools can conflict
> with builtin shortcuts, but otherwise this seems like a pretty
> nice feature.  Curious, what is the behavior in the event of a
> conflict?  Do the builtins win?  IIRC, Qt handles this by
> disabling the shortcut and warning that it's ambiguous.
>
> Please documentation guitool.<name>.accellerator[-label] in
> Documentation/config.txt otherwise users will not know that it
> exists.
>
> It would also be good for the docs to clarify what the
> accelerators look like in case we need to munge them when making
> it work in cola via Qt, which has its own mechanism for
> associating actions with shortcuts.  Documented examples with
> one and two modifier keys would be helpful.
>
>
> cheers,
> --
> David



-- 

-Regards
Harish.K

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

[harish k]

Hi All,
I', Just reopening this feature request.
A quick summary of my proposal is given below.

1. This PR will allow an additional configuration option
"guitool.<name>.gitgui-shortcut" which will allow us to specify
keyboard shortcut  for custom commands in git-gui

2. Even there exists a parameter called "guitool.<name>.shortcut"
which is used by git-cola, I suggest to keep this new additional
config parameter as an independent config parameter, which will not
interfere with git-cola in any way, because, both are different
applications and it may have different "built-in" shortcuts already
assigned. So, sharing shortcut scheme between two apps is not a good
idea.

3. New parameter will expect shortcut combinations specified in TCL/TK
's format and we will not be doing any processing on it. Will keep it
simple.

---
 Documentation/config/guitool.txt | 15 +++++++++++++++
 git-gui/lib/tools.tcl            | 15 ++++++++++++---
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/Documentation/config/guitool.txt b/Documentation/config/guitool.txt
index 43fb9466ff..79dac23ca3 100644
--- a/Documentation/config/guitool.txt
+++ b/Documentation/config/guitool.txt
@@ -48,3 +48,18 @@ guitool.<name>.prompt::
  Specifies the general prompt string to display at the top of
  the dialog, before subsections for 'argPrompt' and 'revPrompt'.
  The default value includes the actual command.
+
+guitool.<name>.gitgui-shortcut
+ Specifies a keyboard shortcut for the custom tool in the git-gui
+ application. The value must be a valid string ( without "<" , ">" wrapper )
+ understood by the TCL/TK 's bind command.See
https://www.tcl.tk/man/tcl8.4/TkCmd/bind.htm
+ for more details about the supported values. Avoid creating shortcuts that
+ conflict with existing built-in `git gui` shortcuts.
+ Example:
+ [guitool "Terminal"]
+ cmd = gnome-terminal -e zsh
+ noconsole = yes
+ gitgui-shortcut = "Control-y"
+ [guitool "Sync"]
+ cmd = "git pull; git push"
+ gitgui-shortcut = "Alt-s"
diff --git a/git-gui/lib/tools.tcl b/git-gui/lib/tools.tcl
index 413f1a1700..40db3f6395 100644
--- a/git-gui/lib/tools.tcl
+++ b/git-gui/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,18 @@ proc tools_populate_one {fullname} {
  }
  }

- tools_create_item $parent command \
+ if {[info exists repo_config(guitool.$fullname.gitgui-shortcut)]} {
+ set gitgui_shortcut $repo_config(guitool.$fullname.gitgui-shortcut)
+ tools_create_item $parent command \
  -label [lindex $names end] \
- -command [list tools_exec $fullname]
+ -command [list tools_exec $fullname] \
+ -accelerator $gitgui_shortcut
+ bind . <$gitgui_shortcut> [list tools_exec $fullname]
+ } else {
+ tools_create_item $parent command \
+ -label [lindex $names end] \
+ -command [list tools_exec $fullname]
+ }
 }

 proc tools_exec {fullname} {
--
https://github.com/git/git/pull/220
--


On Fri, Apr 1, 2016 at 12:02 PM harish k <harish2704@gmail.com> wrote:
>
> Hi David,
>
> Actually Im a TCL primer.  This is the first time Im dealing with.
> That is why I kept it simple ( ie both accel-key and accel-label need
> to be defined in config ).
>
> I think, git-cola is using Qt style representation accel-key in the config file.
>
> Is git-cola is an official tool from git ? like git-gui?
> if not ,
> My suggesion is, it is better to seperate config fields according to
> application-domain
> like, "git-gui-accel = <Ctrl-l>" etc..
> Other vise there is good chance for conflicts. ( Eg: consider the case
>  that, <Ctrl-p> was assined to a custom tool by git-cola )
>
> Currently this patch will not handle any conflicting shortcuts. I
> think custom shortcuts will overwrite the other.
>
>
> On Thu, Mar 31, 2016 at 10:11 PM, David Aguilar <davvid@gmail.com> wrote:
> > Hello,
> >
> > On Tue, Mar 29, 2016 at 11:38:10AM +0000, Harish K wrote:
> >> ---
> >>  git-gui/lib/tools.tcl | 16 +++++++++++++---
> >>  1 file changed, 13 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/git-gui/lib/tools.tcl b/git-gui/lib/tools.tcl
> >> index 6ec9411..749bc67 100644
> >> --- a/git-gui/lib/tools.tcl
> >> +++ b/git-gui/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,19 @@ proc tools_populate_one {fullname} {
> >>               }
> >>       }
> >>
> >> -     tools_create_item $parent command \
> >> +     if {[info exists repo_config(guitool.$fullname.accelerator)] && [info exists repo_config(guitool.$fullname.accelerator-label)]} {
> >> +             set accele_key $repo_config(guitool.$fullname.accelerator)
> >> +             set accel_label $repo_config(guitool.$fullname.accelerator-label)
> >> +             tools_create_item $parent command \
> >>               -label [lindex $names end] \
> >> -             -command [list tools_exec $fullname]
> >> +             -command [list tools_exec $fullname] \
> >> +             -accelerator $accel_label
> >> +             bind . $accele_key [list tools_exec $fullname]
> >> +     } else {
> >> +             tools_create_item $parent command \
> >> +                     -label [lindex $names end] \
> >> +                     -command [list tools_exec $fullname]
> >> +     }
> >>  }
> >>
> >>  proc tools_exec {fullname} {
> >>
> >> --
> >> https://github.com/git/git/pull/220
> >
> > We also support "custom guitools" in git-cola using this same
> > mechanism.  If this gets accepted then we'll want to make
> > similar change there.
> >
> > There's always a small risk that user-defined tools can conflict
> > with builtin shortcuts, but otherwise this seems like a pretty
> > nice feature.  Curious, what is the behavior in the event of a
> > conflict?  Do the builtins win?  IIRC, Qt handles this by
> > disabling the shortcut and warning that it's ambiguous.
> >
> > Please documentation guitool.<name>.accellerator[-label] in
> > Documentation/config.txt otherwise users will not know that it
> > exists.
> >
> > It would also be good for the docs to clarify what the
> > accelerators look like in case we need to munge them when making
> > it work in cola via Qt, which has its own mechanism for
> > associating actions with shortcuts.  Documented examples with
> > one and two modifier keys would be helpful.
> >
> >
> > cheers,
> > --
> > David
>
>
>
> --
>
> -Regards
> Harish.K



-- 

-Thanks
Harish.K

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

[Pratyush Yadav]

Hi Harish,

Thanks for the patch. Unfortunately, it seems your mail client messed up 
the formatting, and the patch won't apply. I'm guessing it is because 
your mail client broke long lines into two, messing up the diff.

We use an email-based workflow, so please either configure your mail 
client so it doesn't munge patches, or use `git send-email`. You can 
find a pretty good tutorial on sending patches via email over at [0]. 
The tutorial is for git.git, but works for git-gui.git as well.

If you feel more comfortable with GitHub pull requests, please take a 
look at Gitgitgadget [1]. Johannes (in Cc) has used it recently to send 
patches based on the git-gui repo (AFAIK, it was originally designed 
only for the git.git repo). Maybe ask the folks over there how they do 
it.

One more thing: your patch is based on the main Git repo. That repo is 
not where git-gui development takes place. The current "official" repo 
for git-gui is over at [2]. Please base your patches on top of that 
repo.

[0] https://matheustavares.gitlab.io/posts/first-steps-contributing-to-git#submitting-patches
[1] https://gitgitgadget.github.io/
[2] https://github.com/prati0100/git-gui

Now on to the nitty gritty details.

I like the idea. In fact, there were some discussions recently about 
having configurable key bindings for _all_ shortcuts in git-gui. Nothing 
concrete has been done in that direction yet though. But I feel like 
this is a pretty good first step.

On 03/10/19 08:18PM, harish k wrote:
> Hi All,
> I', Just reopening this feature request.
> A quick summary of my proposal is given below.
> 
> 1. This PR will allow an additional configuration option
> "guitool.<name>.gitgui-shortcut" which will allow us to specify
> keyboard shortcut  for custom commands in git-gui

A pretty nice way of doing it. But I would _really_ like it if there was 
an option in the "create tool" dialog to specify the shortcut. People of 
a gui tool shouldn't have to mess around with config files as much as 
possible.
 
> 2. Even there exists a parameter called "guitool.<name>.shortcut"
> which is used by git-cola, I suggest to keep this new additional
> config parameter as an independent config parameter, which will not
> interfere with git-cola in any way, because, both are different
> applications and it may have different "built-in" shortcuts already
> assigned. So, sharing shortcut scheme between two apps is not a good
> idea.

David has advocated inter-operability between git-gui and git-cola. 
While I personally don't know how many people actually use both the 
tools at the same time, it doesn't sound like a bad idea either.

So, sharing shortcuts with git-cola would be nice. Of course, it would 
then mean that we would have to parse the config parameter before 
feeding them to `bind`. I don't suppose that should be something too 
complicated to do, but I admit I haven't looked too deeply into it.

I'd like to hear what other people think about whether it is worth the 
effort to inter-operate with git-cola.
 
> 3. New parameter will expect shortcut combinations specified in TCL/TK
> 's format and we will not be doing any processing on it. Will keep it
> simple.

Are you sure that is a good idea? I think we should at least make sure 
we are not binding some illegal sequence, and if we are, we should warn 
the user about it. And a much more important case would be when a user 
over-writes a pre-existing shortcut for other commands like "commit", 
"reset", etc. In that case, the menu entires of those commands would 
still be labelled with the shortcut, but it won't actually work.

Yes, your current implementation keeps things simple, but I think some 
light processing would be beneficial. And if we do decide to go the 
inter-operability with git-cola route, then processing would be needed 
anyway, and we can validate there.
 
> ---
>  Documentation/config/guitool.txt | 15 +++++++++++++++
>  git-gui/lib/tools.tcl            | 15 ++++++++++++---
>  2 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/config/guitool.txt b/Documentation/config/guitool.txt
> index 43fb9466ff..79dac23ca3 100644
> --- a/Documentation/config/guitool.txt
> +++ b/Documentation/config/guitool.txt
> @@ -48,3 +48,18 @@ guitool.<name>.prompt::
>   Specifies the general prompt string to display at the top of
>   the dialog, before subsections for 'argPrompt' and 'revPrompt'.
>   The default value includes the actual command.
> +
> +guitool.<name>.gitgui-shortcut
> + Specifies a keyboard shortcut for the custom tool in the git-gui
> + application. The value must be a valid string ( without "<" , ">" wrapper )
> + understood by the TCL/TK 's bind command.See
> https://www.tcl.tk/man/tcl8.4/TkCmd/bind.htm
> + for more details about the supported values. Avoid creating shortcuts that
> + conflict with existing built-in `git gui` shortcuts.
> + Example:
> + [guitool "Terminal"]
> + cmd = gnome-terminal -e zsh
> + noconsole = yes
> + gitgui-shortcut = "Control-y"
> + [guitool "Sync"]
> + cmd = "git pull; git push"
> + gitgui-shortcut = "Alt-s"

The "Documentation/" subdirectory belongs to the Git project, and not to 
git-gui, so if you want to see this change, you'd have to submit a 
separate patch for it.

As far as git-gui's documentation is concerned, unfortunately there is 
none yet. I have been meaning to start working towards it, but just 
haven't found the time or motivation to do it yet.

> diff --git a/git-gui/lib/tools.tcl b/git-gui/lib/tools.tcl

Like I mentioned before, please base your patches on the git-gui.git 
repo, and not git.git. So, this should read "a/lib/tools.tcl" instead of 
"a/git-gui/lib/tools.tcl".

I haven't looked at the contents of the patch because I can't apply it, 
and I'd prefer to tinker around with it before commenting. So please 
re-send the patch in the proper format and we can discuss the 
implementation :).

> index 413f1a1700..40db3f6395 100644
> --- a/git-gui/lib/tools.tcl
> +++ b/git-gui/lib/tools.tcl
[snip]

-- 
Regards,
Pratyush Yadav

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

[Johannes Schindelin]

Hi Pratyush,

please don't top-post on this list (yet another of these things
requiring extra brain cycles in the mailing list workflow).

On Fri, 4 Oct 2019, Pratyush Yadav wrote:

> Hi Harish,
>
> Thanks for the patch. Unfortunately, it seems your mail client messed up
> the formatting, and the patch won't apply. I'm guessing it is because
> your mail client broke long lines into two, messing up the diff.
>
> We use an email-based workflow, so please either configure your mail
> client so it doesn't munge patches, or use `git send-email`. You can
> find a pretty good tutorial on sending patches via email over at [0].
> The tutorial is for git.git, but works for git-gui.git as well.

Ah, well. Mailing list-based workflows are so easy, amirite? They are so
welcoming and inclusive, yes?
</sarcasm>

> If you feel more comfortable with GitHub pull requests, please take a
> look at Gitgitgadget [1]. Johannes (in Cc) has used it recently to send
> patches based on the git-gui repo (AFAIK, it was originally designed
> only for the git.git repo). Maybe ask the folks over there how they do
> it.

Harish, it is actually relatively easy to use GitGitGadget: just add a
remote like this:

	git remote add gitgitgadget https://github.com/gitgitgadget/git
	git fetch gitgitgadget git-gui/master

and then rebase your patch on top of that branch:

	git rebase -i --onto git-gui/master HEAD~1

Then force-push your branch to your GitHub fork of git.git and open a
Pull Request at https://github.com/gitgitgadget/git/pulls, targeting
git-gui/master.

GitGitGadget will welcome you with a (hopefully) helpful message ;-)

Ciao,
Johannes

>
> One more thing: your patch is based on the main Git repo. That repo is
> not where git-gui development takes place. The current "official" repo
> for git-gui is over at [2]. Please base your patches on top of that
> repo.
>
> [0] https://matheustavares.gitlab.io/posts/first-steps-contributing-to-git#submitting-patches
> [1] https://gitgitgadget.github.io/
> [2] https://github.com/prati0100/git-gui
>
> Now on to the nitty gritty details.
>
> I like the idea. In fact, there were some discussions recently about
> having configurable key bindings for _all_ shortcuts in git-gui. Nothing
> concrete has been done in that direction yet though. But I feel like
> this is a pretty good first step.
>
> On 03/10/19 08:18PM, harish k wrote:
> > Hi All,
> > I', Just reopening this feature request.
> > A quick summary of my proposal is given below.
> >
> > 1. This PR will allow an additional configuration option
> > "guitool.<name>.gitgui-shortcut" which will allow us to specify
> > keyboard shortcut  for custom commands in git-gui
>
> A pretty nice way of doing it. But I would _really_ like it if there was
> an option in the "create tool" dialog to specify the shortcut. People of
> a gui tool shouldn't have to mess around with config files as much as
> possible.
>
> > 2. Even there exists a parameter called "guitool.<name>.shortcut"
> > which is used by git-cola, I suggest to keep this new additional
> > config parameter as an independent config parameter, which will not
> > interfere with git-cola in any way, because, both are different
> > applications and it may have different "built-in" shortcuts already
> > assigned. So, sharing shortcut scheme between two apps is not a good
> > idea.
>
> David has advocated inter-operability between git-gui and git-cola.
> While I personally don't know how many people actually use both the
> tools at the same time, it doesn't sound like a bad idea either.
>
> So, sharing shortcuts with git-cola would be nice. Of course, it would
> then mean that we would have to parse the config parameter before
> feeding them to `bind`. I don't suppose that should be something too
> complicated to do, but I admit I haven't looked too deeply into it.
>
> I'd like to hear what other people think about whether it is worth the
> effort to inter-operate with git-cola.
>
> > 3. New parameter will expect shortcut combinations specified in TCL/TK
> > 's format and we will not be doing any processing on it. Will keep it
> > simple.
>
> Are you sure that is a good idea? I think we should at least make sure
> we are not binding some illegal sequence, and if we are, we should warn
> the user about it. And a much more important case would be when a user
> over-writes a pre-existing shortcut for other commands like "commit",
> "reset", etc. In that case, the menu entires of those commands would
> still be labelled with the shortcut, but it won't actually work.
>
> Yes, your current implementation keeps things simple, but I think some
> light processing would be beneficial. And if we do decide to go the
> inter-operability with git-cola route, then processing would be needed
> anyway, and we can validate there.
>
> > ---
> >  Documentation/config/guitool.txt | 15 +++++++++++++++
> >  git-gui/lib/tools.tcl            | 15 ++++++++++++---
> >  2 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/config/guitool.txt b/Documentation/config/guitool.txt
> > index 43fb9466ff..79dac23ca3 100644
> > --- a/Documentation/config/guitool.txt
> > +++ b/Documentation/config/guitool.txt
> > @@ -48,3 +48,18 @@ guitool.<name>.prompt::
> >   Specifies the general prompt string to display at the top of
> >   the dialog, before subsections for 'argPrompt' and 'revPrompt'.
> >   The default value includes the actual command.
> > +
> > +guitool.<name>.gitgui-shortcut
> > + Specifies a keyboard shortcut for the custom tool in the git-gui
> > + application. The value must be a valid string ( without "<" , ">" wrapper )
> > + understood by the TCL/TK 's bind command.See
> > https://www.tcl.tk/man/tcl8.4/TkCmd/bind.htm
> > + for more details about the supported values. Avoid creating shortcuts that
> > + conflict with existing built-in `git gui` shortcuts.
> > + Example:
> > + [guitool "Terminal"]
> > + cmd = gnome-terminal -e zsh
> > + noconsole = yes
> > + gitgui-shortcut = "Control-y"
> > + [guitool "Sync"]
> > + cmd = "git pull; git push"
> > + gitgui-shortcut = "Alt-s"
>
> The "Documentation/" subdirectory belongs to the Git project, and not to
> git-gui, so if you want to see this change, you'd have to submit a
> separate patch for it.
>
> As far as git-gui's documentation is concerned, unfortunately there is
> none yet. I have been meaning to start working towards it, but just
> haven't found the time or motivation to do it yet.
>
> > diff --git a/git-gui/lib/tools.tcl b/git-gui/lib/tools.tcl
>
> Like I mentioned before, please base your patches on the git-gui.git
> repo, and not git.git. So, this should read "a/lib/tools.tcl" instead of
> "a/git-gui/lib/tools.tcl".
>
> I haven't looked at the contents of the patch because I can't apply it,
> and I'd prefer to tinker around with it before commenting. So please
> re-send the patch in the proper format and we can discuss the
> implementation :).
>
> > index 413f1a1700..40db3f6395 100644
> > --- a/git-gui/lib/tools.tcl
> > +++ b/git-gui/lib/tools.tcl
> [snip]
>
> --
> Regards,
> Pratyush Yadav
>

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

[Pratyush Yadav]

On 04/10/19 10:49AM, Johannes Schindelin wrote:
> Hi Pratyush,
> 
> please don't top-post on this list (yet another of these things
> requiring extra brain cycles in the mailing list workflow).

I didn't top-post, or at least it wasn't the intention. The text above 
the quoted text is the "preface" (just like this line I'm replying to is 
a "preface"). It was a comment on the patch formatting, and it wouldn't 
make sense (at least to me) to have it _below_ the quoted part, 
especially since I did have comments in-line (IOW, bottom-posted).

Maybe because I included the references in the middle of the text you 
thought the message was over (understandably so. I probably shouldn't do 
that), and didn't scroll till the end to read the rest of the message.

Or maybe I don't know something about email etiquette that I should.
 
> On Fri, 4 Oct 2019, Pratyush Yadav wrote:
> 
> > Hi Harish,
> >
> > Thanks for the patch. Unfortunately, it seems your mail client messed up
> > the formatting, and the patch won't apply. I'm guessing it is because
> > your mail client broke long lines into two, messing up the diff.
> >
> > We use an email-based workflow, so please either configure your mail
> > client so it doesn't munge patches, or use `git send-email`. You can
> > find a pretty good tutorial on sending patches via email over at [0].
> > The tutorial is for git.git, but works for git-gui.git as well.
> 
> Ah, well. Mailing list-based workflows are so easy, amirite? They are so
> welcoming and inclusive, yes?
> </sarcasm>
> 
> > If you feel more comfortable with GitHub pull requests, please take a
> > look at Gitgitgadget [1]. Johannes (in Cc) has used it recently to send
> > patches based on the git-gui repo (AFAIK, it was originally designed
> > only for the git.git repo). Maybe ask the folks over there how they do
> > it.
> 
> Harish, it is actually relatively easy to use GitGitGadget: just add a
> remote like this:
> 
> 	git remote add gitgitgadget https://github.com/gitgitgadget/git
> 	git fetch gitgitgadget git-gui/master
> 
> and then rebase your patch on top of that branch:
> 
> 	git rebase -i --onto git-gui/master HEAD~1
> 
> Then force-push your branch to your GitHub fork of git.git and open a
> Pull Request at https://github.com/gitgitgadget/git/pulls, targeting
> git-gui/master.
> 
> GitGitGadget will welcome you with a (hopefully) helpful message ;-)

Thanks for these instructions. I will include them in a "Contributing" 
document I'm writing for git-gui.

-- 
Regards,
Pratyush Yadav

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

[Harish Karumuthil]

Hi All,

From https://www.kernel.org/doc/html/v4.10/process/email-clients.html, I
understood that, my current email client ( that is gmail web ) is not good
for submitting patches. So I was tying to setup a mail client which is
compatible with `git send-mail`. But I was not able to get a satisfactory
result in that.

For now, I followed the instruction of Johannes Schindelin and submitted a
pull request . Please see https://github.com/gitgitgadget/git/pull/376

---------
@ Pratyush: Regarding your comments,


> A pretty nice way of doing it. But I would _really_ like it if there was
> an option in the "create tool" dialog to specify the shortcut. People of
> a gui tool shouldn't have to mess around with config files as much as
> possible.

I agree with this, But that may require some more profficiency in TCL/TK
programming which I don't have. This is the first time I am looking into a
TCL/TK source code.
Any way I will try to integrate the gui gradually in feature. But
unfortunatly, I may not be able to do that now.



> David has advocated inter-operability between git-gui and git-cola.
> While I personally don't know how many people actually use both the
> tools at the same time, it doesn't sound like a bad idea either.
>
> So, sharing shortcuts with git-cola would be nice. Of course, it would
> then mean that we would have to parse the config parameter before
> feeding them to `bind`. I don't suppose that should be something too
> complicated to do, but I admit I haven't looked too deeply into it.

IMHO, Using a uniform shortcut-key code/foramat for both application can be
considered as nice feature.
But, whether we should share common shortcut-scheme with both application is
a different question.
Currently, both apps don't have a common shortcut-scheme. So in this
situation, only sharing custom-tool's shortcut-scheme with both applications
doesn't look like a good  idea to me 


> Are you sure that is a good idea? I think we should at least make sure
> we are not binding some illegal sequence, and if we are, we should warn
> the user about it. And a much more important case would be when a user
> over-writes a pre-existing shortcut for other commands like "commit",
> "reset", etc. In that case, the menu entires of those commands would
> still be labelled with the shortcut, but it won't actually work.

I agree with you. It is an important point. After reading this, I checked
current status of these issues. What I found is given below.

1. When user provides an invalid sequence for the shortcut, it will cuase the
entire gitgui application to crash at the startup

2. When user tries to overwrite existing shortcut, it will not have any
effect. Because, built in shortcuts will overwrite user provided one. But
still, wrong menu accelerator label will persist for custom tools

Since #1 is a serious issue, I tried to find out the function which does the
keycode validation, but I haven't succeded till now. ( I found the C function
name  which is "TkStringToKeysym" from TK source, but I couldn't find its TCL
binding ). It will be helpful if any one can help me on this.

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

[Pratyush Yadav]

On 06/10/19 01:46AM, Harish Karumuthil wrote:
> Hi All,
> 
> From https://www.kernel.org/doc/html/v4.10/process/email-clients.html, I
> understood that, my current email client ( that is gmail web ) is not good
> for submitting patches. So I was tying to setup a mail client which is
> compatible with `git send-mail`. But I was not able to get a satisfactory
> result in that.

You don't need to "set up" an email client with git-send-email. 
git-send-email is an email client itself. Well, one which can only send 
emails.

So what you should do is run `git format-patch -o feature master..HEAD`, 
assuming your feature branch is checked out. This will give you a set of 
'.patch' files depending on how many commits you made in your branch in 
the folder feature/. Then, you can run 

  git send-email --to='Pratyush Yadav <me@yadavpratyush.com>' --cc='<git@vger.kernel.org>' feature/*.patch

This will send all your patch files via email to me with the git list in 
Cc. You can add multiple '--to' and '--cc' options to send it to 
multiple people.

Try sending the patches to yourself to experiment around with it.

A pretty good tutorial to configuring and using git-send-email can be 
found at [0]. And of course, read the man page.

These instructions are for Linux, but you can probably do something 
similar in Windows too (if you're using Windows that is).
 
> For now, I followed the instruction of Johannes Schindelin and submitted a
> pull request . Please see https://github.com/gitgitgadget/git/pull/376

You haven't sent '/submit' over there, so those emails aren't in the 
list (and my inbox) yet. You need to comment with '/submit' (without the 
quotes) to tell GitGitGadget to send your PR as email.

But I see that Dscho has left a comment over there, so you should 
probably address that first. You probably need to amend the commit, 
force push, and then comment with '/submit'. But I'm not a 100% sure 
because I haven't used GitHub PRs a lot.
 
> ---------
> @ Pratyush: Regarding your comments,
> 
> 
> > A pretty nice way of doing it. But I would _really_ like it if there was
> > an option in the "create tool" dialog to specify the shortcut. People of
> > a gui tool shouldn't have to mess around with config files as much as
> > possible.
> 
> I agree with this, But that may require some more profficiency in TCL/TK
> programming which I don't have. This is the first time I am looking into a
> TCL/TK source code.
> Any way I will try to integrate the gui gradually in feature. But
> unfortunatly, I may not be able to do that now.

Please do whatever you can. I will try to add a patch on top of yours to 
add the GUI option.
 
> > David has advocated inter-operability between git-gui and git-cola.
> > While I personally don't know how many people actually use both the
> > tools at the same time, it doesn't sound like a bad idea either.
> >
> > So, sharing shortcuts with git-cola would be nice. Of course, it would
> > then mean that we would have to parse the config parameter before
> > feeding them to `bind`. I don't suppose that should be something too
> > complicated to do, but I admit I haven't looked too deeply into it.
> 
> IMHO, Using a uniform shortcut-key code/foramat for both application can be
> considered as nice feature.
> But, whether we should share common shortcut-scheme with both application is
> a different question.
> Currently, both apps don't have a common shortcut-scheme. So in this
> situation, only sharing custom-tool's shortcut-scheme with both applications
> doesn't look like a good  idea to me 

Makes sense.
 
> > Are you sure that is a good idea? I think we should at least make 
> > sure
> > we are not binding some illegal sequence, and if we are, we should warn
> > the user about it. And a much more important case would be when a user
> > over-writes a pre-existing shortcut for other commands like "commit",
> > "reset", etc. In that case, the menu entires of those commands would
> > still be labelled with the shortcut, but it won't actually work.
> 
> I agree with you. It is an important point. After reading this, I checked
> current status of these issues. What I found is given below.
> 
> 1. When user provides an invalid sequence for the shortcut, it will cuase the
> entire gitgui application to crash at the startup
> 
> 2. When user tries to overwrite existing shortcut, it will not have any
> effect. Because, built in shortcuts will overwrite user provided one. But
> still, wrong menu accelerator label will persist for custom tools

One point I forgot to mention earlier was that I'm honestly not a big 
fan of separating the binding and accelerator label. I understand that 
you might not have the time to do this, but I think it is still worth 
mentioning. Maybe I will implement something like that over your patch. 
But it would certainly be nice if you can figure it out :).

Either ways, detecting an existing shortcut is pretty easy. The `bind` 
man page [1] says:

  If sequence is specified without a script, then the script currently 
  bound to sequence is returned, or an empty string is returned if there 
  is no binding for sequence.

So you can use this to find out if there is a binding conflict, and warn 
the user.

> Since #1 is a serious issue, I tried to find out the function which does the
> keycode validation, but I haven't succeded till now. ( I found the C function
> name  which is "TkStringToKeysym" from TK source, but I couldn't find its TCL
> binding ). It will be helpful if any one can help me on this.

I really think you shouldn't dive around in the C parts of Tcl. I 
haven't looked too deeply into this, but you can probably wrap your bind 
calls in `catch` [2] and handle errors from there. Again, I haven't 
tried actually doing this, so you do need to check first.

You can find examples of how to use `catch` in our codebase. Just search 
for it.

[0] https://www.freedesktop.org/wiki/Software/PulseAudio/HowToUseGitSendEmail/
[1] https://www.tcl.tk/man/tcl8.4/TkCmd/bind.htm
[2] https://www.tcl.tk/man/tcl8.4/TclCmd/catch.htm

-- 
Regards,
Pratyush Yadav

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

[Johannes Schindelin]

Hi Pratyush,

On Sun, 6 Oct 2019, Pratyush Yadav wrote:

> On 06/10/19 01:46AM, Harish Karumuthil wrote:
> >
> > From https://www.kernel.org/doc/html/v4.10/process/email-clients.html, I
> > understood that, my current email client ( that is gmail web ) is not good
> > for submitting patches. So I was tying to setup a mail client which is
> > compatible with `git send-mail`. But I was not able to get a satisfactory
> > result in that.
>
> You don't need to "set up" an email client with git-send-email.
> git-send-email is an email client itself. Well, one which can only send
> emails.

It also cannot reply to mails on the mailing list.

It cannot even notify you when anybody replied to your patch.

Two rather problematic aspects when it comes to patch contributions: how
are you supposed to work with the reviewers when you lack all the tools
to _interact_ with them? All `git send-email` provides is a "fire and
forget" way to send patches, i.e. it encourages a monologue, when you
want to start a dialogue instead.

> So what you should do is run `git format-patch -o feature master..HEAD`,
> assuming your feature branch is checked out. This will give you a set of
> '.patch' files depending on how many commits you made in your branch in
> the folder feature/. Then, you can run
>
>   git send-email --to='Pratyush Yadav <me@yadavpratyush.com>' --cc='<git@vger.kernel.org>' feature/*.patch
>
> This will send all your patch files via email to me with the git list in
> Cc. You can add multiple '--to' and '--cc' options to send it to
> multiple people.
>
> Try sending the patches to yourself to experiment around with it.
>
> A pretty good tutorial to configuring and using git-send-email can be
> found at [0]. And of course, read the man page.
>
> These instructions are for Linux, but you can probably do something
> similar in Windows too (if you're using Windows that is).

Last I checked, `git send-email` worked in Git for Windows.

But of course, it does not only not address the problem it tries to
solve fully (to provide a way to interact with a mailing list when
submitting patches for review), not even close, to add insult to injury,
it now adds an additional burden to contributors (who might already have
struggled to learn themselves enough Tcl/Tk to fix the problem) to
configure `git send-email` correctly.

> > For now, I followed the instruction of Johannes Schindelin and submitted a
> > pull request . Please see https://github.com/gitgitgadget/git/pull/376
>
> You haven't sent '/submit' over there, so those emails aren't in the
> list (and my inbox) yet. You need to comment with '/submit' (without the
> quotes) to tell GitGitGadget to send your PR as email.

They probably did not hit `/submit` because the initial hurdle is to be
`/allow`ed (a very, very simplistic attempt at trying to prevent
spamming the mailing list by jokesters, of which there are unfortunately
quite a number).

This `/allow` command, BTW, can be issued by anybody who has been
`/allow`ed before, it does not always have to be me.

FWIW you should probably be in that list of `/allow`ed people so that
you can `/allow` new contributors to use GitGitGadget, too.

> [...]
>
> > Since #1 is a serious issue, I tried to find out the function which does the
> > keycode validation, but I haven't succeded till now. ( I found the C function
> > name  which is "TkStringToKeysym" from TK source, but I couldn't find its TCL
> > binding ). It will be helpful if any one can help me on this.
>
> I really think you shouldn't dive around in the C parts of Tcl. I
> haven't looked too deeply into this, but you can probably wrap your bind
> calls in `catch` [2] and handle errors from there. Again, I haven't
> tried actually doing this, so you do need to check first.
>
> You can find examples of how to use `catch` in our codebase. Just search
> for it.

FWIW in addition to the `catch` method, I would also recommend looking
into a minimal (not even necessarily complete) way to translate the Qt
way to specify the keyboard shortcuts (as used by `git-cola`) to Tk
ones.

As indicated in
https://github.com/git/git/pull/220#issuecomment-536045075, the Qt style
`CTRL+,` should be translated to `Control-comma`, for example. In
particular, keystrokes specified in the format indicated at
https://doc.qt.io/archives/qt-4.8/qkeysequence.html#QKeySequence-2 to
the format indicated at https://www.tcl.tk/man/tcl8.4/TkCmd/keysyms.htm.

However, it might not even need to put in _such_ a lot of work: in my
tests, `Control-,` worked just as well as `Control-comma`. To test this
for yourself, use this snippet (that is slightly modified from the
example at the bottom of https://www.tcl.tk/man/tcl/TkCmd/bind.htm so
that it reacts _only_ to Control+comma instead of all keys):

-- snip --
set keysym "Press any key"
pack [label .l -textvariable keysym -padx 2m -pady 1m]
#bind . <Key> {
bind . <Control-,> {
    set keysym "You pressed %K"
}
-- snap --

So I could imagine that something like this could serve as an initial
draft for a function that you can turn into a "good enough" version:

-- snip --
proc QKeySequence2keysym {keystroke} {
	regsub -all {(?i)Ctrl\+} $keystroke "Control-" keystroke
	regsub -all {(?i)Alt\+} $keystroke "Alt-" keystroke
	regsub -all {(?i)Shift\+} $keystroke "Shift-" keystroke
	return $keystroke
}
-- snap --

That way, you don't have to introduce settings separate from
`git-cola`'s, and you can reuse the short-and-sweet variable name.

Ciao,
Johannes

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

[Pratyush Yadav]

On 06/10/19 11:49AM, Johannes Schindelin wrote:
> Hi Pratyush,
> 
> On Sun, 6 Oct 2019, Pratyush Yadav wrote:
> 
> > On 06/10/19 01:46AM, Harish Karumuthil wrote:
> > >
> > > From https://www.kernel.org/doc/html/v4.10/process/email-clients.html, I
> > > understood that, my current email client ( that is gmail web ) is not good
> > > for submitting patches. So I was tying to setup a mail client which is
> > > compatible with `git send-mail`. But I was not able to get a satisfactory
> > > result in that.
> >
> > You don't need to "set up" an email client with git-send-email.
> > git-send-email is an email client itself. Well, one which can only send
> > emails.
> 
> It also cannot reply to mails on the mailing list.
> 
> It cannot even notify you when anybody replied to your patch.
> 
> Two rather problematic aspects when it comes to patch contributions: how
> are you supposed to work with the reviewers when you lack all the tools
> to _interact_ with them? All `git send-email` provides is a "fire and
> forget" way to send patches, i.e. it encourages a monologue, when you
> want to start a dialogue instead.

Well, I started with email based patch contribution when I was first got 
started with open source, so I might be a bit biased, but in my 
experience, it is not that difficult to set all these things up. Most of 
the time, all you need to tell git-send-email is your SMTP server, 
username, and password. All pretty easy things to do.

And you add in your email client (which pretty much everyone should 
have), and it is a complete setup. I personally use neomutt as my email 
client, but I have used Thunderbird before, and it is really easy to set 
it up to send plain text emails. All you need to do is hold Shift before 
hitting reply, and you're in plain text mode. And you can even make it 
use plain text by default by flipping a switch in the settings.

So while I agree with you that there is certainly a learning curve 
involved, I don't think it is all too bad. But again, that is all my 
personal opinion, and nothing based on facts or data.
 
> > So what you should do is run `git format-patch -o feature master..HEAD`,
> > assuming your feature branch is checked out. This will give you a set of
> > '.patch' files depending on how many commits you made in your branch in
> > the folder feature/. Then, you can run
> >
> >   git send-email --to='Pratyush Yadav <me@yadavpratyush.com>' --cc='<git@vger.kernel.org>' feature/*.patch
> >
> > This will send all your patch files via email to me with the git list in
> > Cc. You can add multiple '--to' and '--cc' options to send it to
> > multiple people.
> >
> > Try sending the patches to yourself to experiment around with it.
> >
> > A pretty good tutorial to configuring and using git-send-email can be
> > found at [0]. And of course, read the man page.
> >
> > These instructions are for Linux, but you can probably do something
> > similar in Windows too (if you're using Windows that is).
> 
> Last I checked, `git send-email` worked in Git for Windows.
> 
> But of course, it does not only not address the problem it tries to
> solve fully (to provide a way to interact with a mailing list when
> submitting patches for review), not even close, to add insult to injury,
> it now adds an additional burden to contributors (who might already have
> struggled to learn themselves enough Tcl/Tk to fix the problem) to
> configure `git send-email` correctly.

The way I see it, git-send-email does not need to solve the problem of 
interacting with a mailing list. That problem is already solved by a 
hoard of MUAs. All git-send-email should do is, you guessed it, send 
emails (or patches to be specific).

Anyway, GitGitGadget solves a large part of the problem. It eliminates 
the need for using git-send-email, and it even shows you the replies 
received on the list. I honestly think it is a great tool, and it gives 
people a very good alternative to using git-send-email.

One feature that would make it complete would be the ability to reply to 
review comments. This would remove the need for an email client (almost) 
completely. I have never written Typescript or used Azure pipelines 
ever, but I can try tinkering around to see if I can figure out how to 
do something like that. Unless, of course, you or someone else is 
already doing it. If not, some pointers would be appreciated.
 
> > > For now, I followed the instruction of Johannes Schindelin and submitted a
> > > pull request . Please see https://github.com/gitgitgadget/git/pull/376
> >
> > You haven't sent '/submit' over there, so those emails aren't in the
> > list (and my inbox) yet. You need to comment with '/submit' (without the
> > quotes) to tell GitGitGadget to send your PR as email.
> 
> They probably did not hit `/submit` because the initial hurdle is to be
> `/allow`ed (a very, very simplistic attempt at trying to prevent
> spamming the mailing list by jokesters, of which there are unfortunately
> quite a number).
> 
> This `/allow` command, BTW, can be issued by anybody who has been
> `/allow`ed before, it does not always have to be me.
> 
> FWIW you should probably be in that list of `/allow`ed people so that
> you can `/allow` new contributors to use GitGitGadget, too.

That would be great! How do I get '/allow'ed? Do I have to open a PR 
there for you to '/allow' me?
 
> > [...]
> >
> > > Since #1 is a serious issue, I tried to find out the function which does the
> > > keycode validation, but I haven't succeded till now. ( I found the C function
> > > name  which is "TkStringToKeysym" from TK source, but I couldn't find its TCL
> > > binding ). It will be helpful if any one can help me on this.
> >
> > I really think you shouldn't dive around in the C parts of Tcl. I
> > haven't looked too deeply into this, but you can probably wrap your bind
> > calls in `catch` [2] and handle errors from there. Again, I haven't
> > tried actually doing this, so you do need to check first.
> >
> > You can find examples of how to use `catch` in our codebase. Just search
> > for it.
> 
> FWIW in addition to the `catch` method, I would also recommend looking
> into a minimal (not even necessarily complete) way to translate the Qt
> way to specify the keyboard shortcuts (as used by `git-cola`) to Tk
> ones.
> 
> As indicated in
> https://github.com/git/git/pull/220#issuecomment-536045075, the Qt style
> `CTRL+,` should be translated to `Control-comma`, for example. In
> particular, keystrokes specified in the format indicated at
> https://doc.qt.io/archives/qt-4.8/qkeysequence.html#QKeySequence-2 to
> the format indicated at https://www.tcl.tk/man/tcl8.4/TkCmd/keysyms.htm.
> 
> However, it might not even need to put in _such_ a lot of work: in my
> tests, `Control-,` worked just as well as `Control-comma`. To test this
> for yourself, use this snippet (that is slightly modified from the
> example at the bottom of https://www.tcl.tk/man/tcl/TkCmd/bind.htm so
> that it reacts _only_ to Control+comma instead of all keys):

Another benefit to the translation framework would be that we could also 
generate the menu labels (aka "accelerator") for the tools, instead of 
making the user specify both the shortcut and the label.
 
> -- snip --
> set keysym "Press any key"
> pack [label .l -textvariable keysym -padx 2m -pady 1m]
> #bind . <Key> {
> bind . <Control-,> {
>     set keysym "You pressed %K"
> }
> -- snap --
> 
> So I could imagine that something like this could serve as an initial
> draft for a function that you can turn into a "good enough" version:
> 
> -- snip --
> proc QKeySequence2keysym {keystroke} {
> 	regsub -all {(?i)Ctrl\+} $keystroke "Control-" keystroke
> 	regsub -all {(?i)Alt\+} $keystroke "Alt-" keystroke
> 	regsub -all {(?i)Shift\+} $keystroke "Shift-" keystroke
> 	return $keystroke
> }
> -- snap --
> 
> That way, you don't have to introduce settings separate from
> `git-cola`'s, and you can reuse the short-and-sweet variable name.

I think a more important question is whether we _really_ need to have 
compatibility with git-cola. Most of our shortcuts don't match with 
them, so is it really worth the effort to try to keep compatibility?

I'm not against something like this, but just want to be sure we 
evaluate whether the effort is worth it.

-- 
Regards,
Pratyush Yadav

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

[Philip Oakley]

On 06/10/2019 19:39, Pratyush Yadav wrote:
>> That way, you don't have to introduce settings separate from
>> `git-cola`'s, and you can reuse the short-and-sweet variable name.
> I think a more important question is whether we_really_  need to have
> compatibility with git-cola. Most of our shortcuts don't match with
> them, so is it really worth the effort to try to keep compatibility?
>
> I'm not against something like this, but just want to be sure we
> evaluate whether the effort is worth it.
I just wondered if the custom list would/could be split between a 
"Common-shortcuts" list, and "GUI-local" list (and hence, elsewhere, a 
"Cola-local" list)

I don't use Cola at all, so this is just a bikeshed comment...
-- 
Philip

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

[Johannes Schindelin]

Hi Pratyush,

On Mon, 7 Oct 2019, Pratyush Yadav wrote:

> On 06/10/19 11:49AM, Johannes Schindelin wrote:
> > Hi Pratyush,
> >
> > On Sun, 6 Oct 2019, Pratyush Yadav wrote:
> >
> > > On 06/10/19 01:46AM, Harish Karumuthil wrote:
> > > >
> > > > From https://www.kernel.org/doc/html/v4.10/process/email-clients.html, I
> > > > understood that, my current email client ( that is gmail web ) is not good
> > > > for submitting patches. So I was tying to setup a mail client which is
> > > > compatible with `git send-mail`. But I was not able to get a satisfactory
> > > > result in that.
> > >
> > > You don't need to "set up" an email client with git-send-email.
> > > git-send-email is an email client itself. Well, one which can only send
> > > emails.
> >
> > It also cannot reply to mails on the mailing list.
> >
> > It cannot even notify you when anybody replied to your patch.
> >
> > Two rather problematic aspects when it comes to patch contributions: how
> > are you supposed to work with the reviewers when you lack all the tools
> > to _interact_ with them? All `git send-email` provides is a "fire and
> > forget" way to send patches, i.e. it encourages a monologue, when you
> > want to start a dialogue instead.
>
> Well, I started with email based patch contribution when I was first got
> started with open source, so I might be a bit biased, but in my
> experience, it is not that difficult to set all these things up. Most of
> the time, all you need to tell git-send-email is your SMTP server,
> username, and password. All pretty easy things to do.

Okay, set it up with a corporate Exchange server.

I'll be waiting right here.

> And you add in your email client (which pretty much everyone should
> have), and it is a complete setup. I personally use neomutt as my email
> client, but I have used Thunderbird before, and it is really easy to set
> it up to send plain text emails. All you need to do is hold Shift before
> hitting reply, and you're in plain text mode. And you can even make it
> use plain text by default by flipping a switch in the settings.

How intuitive. And of course Thunderbird still messes up the patches so
that they won't apply, unless you *checks notes* do things that are
quite involved or *checks notes* do other things that are quite
involved.

But then, everybody reads their mails on their command-line anyway.
Right?

> So while I agree with you that there is certainly a learning curve
> involved, I don't think it is all too bad. But again, that is all my
> personal opinion, and nothing based on facts or data.

Let me provide you with some data, then. Granted, it's not necessarily
all Git GUI, but it includes Git GUI patches, too: Git for Windows'
contributions.

As should be well-known, I try to follow Postel's Law when it comes to
Git for Windows' patches: be lenient in the input, strict in the output.
As such, I don't force contributors to use GitHub PRs (although that is
certainly encouraged by virtue of Git for Windows' source code being
hosted on GitHub), or send patches, or send pull requests to their own
public repositories or bundles sent to the mailing list. I accept them
all. At least that is the idea.

I cannot tell you how many contributions came in via GitHub PRs. I can
tell precisely you how many contributions were made _not_ using GitHub
PRs. One one hand. Actually, on zero hands.

So clearly, at least Git for Windows' contributors (including some who
provided Git GUI patches) are much more comfortable with the PR workflow
than with the mailing list-based workflow.

Just so you can't say you don't have data.

> > > So what you should do is run `git format-patch -o feature master..HEAD`,
> > > assuming your feature branch is checked out. This will give you a set of
> > > '.patch' files depending on how many commits you made in your branch in
> > > the folder feature/. Then, you can run
> > >
> > >   git send-email --to='Pratyush Yadav <me@yadavpratyush.com>' --cc='<git@vger.kernel.org>' feature/*.patch
> > >
> > > This will send all your patch files via email to me with the git list in
> > > Cc. You can add multiple '--to' and '--cc' options to send it to
> > > multiple people.
> > >
> > > Try sending the patches to yourself to experiment around with it.
> > >
> > > A pretty good tutorial to configuring and using git-send-email can be
> > > found at [0]. And of course, read the man page.
> > >
> > > These instructions are for Linux, but you can probably do something
> > > similar in Windows too (if you're using Windows that is).
> >
> > Last I checked, `git send-email` worked in Git for Windows.
> >
> > But of course, it does not only not address the problem it tries to
> > solve fully (to provide a way to interact with a mailing list when
> > submitting patches for review), not even close, to add insult to injury,
> > it now adds an additional burden to contributors (who might already have
> > struggled to learn themselves enough Tcl/Tk to fix the problem) to
> > configure `git send-email` correctly.
>
> The way I see it, git-send-email does not need to solve the problem of
> interacting with a mailing list. That problem is already solved by a
> hoard of MUAs. All git-send-email should do is, you guessed it, send
> emails (or patches to be specific).

But of course! And it is natural that you should use two separate MUAs.

> Anyway, GitGitGadget solves a large part of the problem. It eliminates
> the need for using git-send-email, and it even shows you the replies
> received on the list. I honestly think it is a great tool, and it gives
> people a very good alternative to using git-send-email.

GitGitGadget is just a workaround. Not even complete. Can't be complete,
really. Because problems. It has much of the same problems of `git
send-email`: it's a one-way conversation. Code is not discussed in the
right context (which would be a worktree with the correct commit checked
out). The transfer is lossy (email is designed for human-readable
messages, not for transferring machine-readable serialized objects).
Matching original commits and/or branches to the ones on the other side
is tedious. Any interaction requires switching between many tools. Etc

> One feature that would make it complete would be the ability to reply to
> review comments.

And how would that work, exactly? How to determine *which* email to
respond to? *Which* person to reply to? *What* to quote?

> This would remove the need for an email client (almost) completely. I
> have never written Typescript or used Azure pipelines ever, but I can
> try tinkering around to see if I can figure out how to do something
> like that. Unless, of course, you or someone else is already doing it.
> If not, some pointers would be appreciated.

Feel free to give this challenge a try.

> > > > For now, I followed the instruction of Johannes Schindelin and submitted a
> > > > pull request . Please see https://github.com/gitgitgadget/git/pull/376
> > >
> > > You haven't sent '/submit' over there, so those emails aren't in the
> > > list (and my inbox) yet. You need to comment with '/submit' (without the
> > > quotes) to tell GitGitGadget to send your PR as email.
> >
> > They probably did not hit `/submit` because the initial hurdle is to be
> > `/allow`ed (a very, very simplistic attempt at trying to prevent
> > spamming the mailing list by jokesters, of which there are unfortunately
> > quite a number).
> >
> > This `/allow` command, BTW, can be issued by anybody who has been
> > `/allow`ed before, it does not always have to be me.
> >
> > FWIW you should probably be in that list of `/allow`ed people so that
> > you can `/allow` new contributors to use GitGitGadget, too.
>
> That would be great! How do I get '/allow'ed? Do I have to open a PR
> there for you to '/allow' me?

https://github.com/gitgitgadget/git/pull/376#issuecomment-538784646

> > > [...]
> > >
> > > > Since #1 is a serious issue, I tried to find out the function which does the
> > > > keycode validation, but I haven't succeded till now. ( I found the C function
> > > > name  which is "TkStringToKeysym" from TK source, but I couldn't find its TCL
> > > > binding ). It will be helpful if any one can help me on this.
> > >
> > > I really think you shouldn't dive around in the C parts of Tcl. I
> > > haven't looked too deeply into this, but you can probably wrap your bind
> > > calls in `catch` [2] and handle errors from there. Again, I haven't
> > > tried actually doing this, so you do need to check first.
> > >
> > > You can find examples of how to use `catch` in our codebase. Just search
> > > for it.
> >
> > FWIW in addition to the `catch` method, I would also recommend looking
> > into a minimal (not even necessarily complete) way to translate the Qt
> > way to specify the keyboard shortcuts (as used by `git-cola`) to Tk
> > ones.
> >
> > As indicated in
> > https://github.com/git/git/pull/220#issuecomment-536045075, the Qt style
> > `CTRL+,` should be translated to `Control-comma`, for example. In
> > particular, keystrokes specified in the format indicated at
> > https://doc.qt.io/archives/qt-4.8/qkeysequence.html#QKeySequence-2 to
> > the format indicated at https://www.tcl.tk/man/tcl8.4/TkCmd/keysyms.htm.
> >
> > However, it might not even need to put in _such_ a lot of work: in my
> > tests, `Control-,` worked just as well as `Control-comma`. To test this
> > for yourself, use this snippet (that is slightly modified from the
> > example at the bottom of https://www.tcl.tk/man/tcl/TkCmd/bind.htm so
> > that it reacts _only_ to Control+comma instead of all keys):
>
> Another benefit to the translation framework would be that we could also
> generate the menu labels (aka "accelerator") for the tools, instead of
> making the user specify both the shortcut and the label.
>
> > -- snip --
> > set keysym "Press any key"
> > pack [label .l -textvariable keysym -padx 2m -pady 1m]
> > #bind . <Key> {
> > bind . <Control-,> {
> >     set keysym "You pressed %K"
> > }
> > -- snap --
> >
> > So I could imagine that something like this could serve as an initial
> > draft for a function that you can turn into a "good enough" version:
> >
> > -- snip --
> > proc QKeySequence2keysym {keystroke} {
> > 	regsub -all {(?i)Ctrl\+} $keystroke "Control-" keystroke
> > 	regsub -all {(?i)Alt\+} $keystroke "Alt-" keystroke
> > 	regsub -all {(?i)Shift\+} $keystroke "Shift-" keystroke
> > 	return $keystroke
> > }
> > -- snap --
> >
> > That way, you don't have to introduce settings separate from
> > `git-cola`'s, and you can reuse the short-and-sweet variable name.
>
> I think a more important question is whether we _really_ need to have
> compatibility with git-cola. Most of our shortcuts don't match with
> them, so is it really worth the effort to try to keep compatibility?
>
> I'm not against something like this, but just want to be sure we
> evaluate whether the effort is worth it.

`git-cola`, by virtue of being there first, squats on the neat config
setting name `shortcut`.

I expect users to be utterly surprised when that name does not work for
them.

Please note that I intended `QKeySequence2keysym` to leave parameters
unchanged that already refer to keysyms. So this is purely a way to
reuse the same name as `git-cola` while still playing nice with existing
configurations targeting `git-cola`.

Ciao,
Johannes

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

[Pratyush Yadav]

On 06/10/19 10:27PM, Johannes Schindelin wrote:
> Hi Pratyush,
> 
> On Mon, 7 Oct 2019, Pratyush Yadav wrote:
> 
> > On 06/10/19 11:49AM, Johannes Schindelin wrote:
> > > Hi Pratyush,
> > >
> > > On Sun, 6 Oct 2019, Pratyush Yadav wrote:
> > >
> > > > On 06/10/19 01:46AM, Harish Karumuthil wrote:
> > > > >
> > > > > From https://www.kernel.org/doc/html/v4.10/process/email-clients.html, I
> > > > > understood that, my current email client ( that is gmail web ) is not good
> > > > > for submitting patches. So I was tying to setup a mail client which is
> > > > > compatible with `git send-mail`. But I was not able to get a satisfactory
> > > > > result in that.
> > > >
> > > > You don't need to "set up" an email client with git-send-email.
> > > > git-send-email is an email client itself. Well, one which can only send
> > > > emails.
> > >
> > > It also cannot reply to mails on the mailing list.
> > >
> > > It cannot even notify you when anybody replied to your patch.
> > >
> > > Two rather problematic aspects when it comes to patch contributions: how
> > > are you supposed to work with the reviewers when you lack all the tools
> > > to _interact_ with them? All `git send-email` provides is a "fire and
> > > forget" way to send patches, i.e. it encourages a monologue, when you
> > > want to start a dialogue instead.
> >
> > Well, I started with email based patch contribution when I was first got
> > started with open source, so I might be a bit biased, but in my
> > experience, it is not that difficult to set all these things up. Most of
> > the time, all you need to tell git-send-email is your SMTP server,
> > username, and password. All pretty easy things to do.
> 
> Okay, set it up with a corporate Exchange server.
> 
> I'll be waiting right here.

I admit, I've never had to do that. And by how you word it, hope I never 
have to do it in the future either.
 
> > And you add in your email client (which pretty much everyone should
> > have), and it is a complete setup. I personally use neomutt as my email
> > client, but I have used Thunderbird before, and it is really easy to set
> > it up to send plain text emails. All you need to do is hold Shift before
> > hitting reply, and you're in plain text mode. And you can even make it
> > use plain text by default by flipping a switch in the settings.
> 
> How intuitive. And of course Thunderbird still messes up the patches so
> that they won't apply, unless you *checks notes* do things that are
> quite involved or *checks notes* do other things that are quite
> involved.

Ha! Made me chuckle. You got me there :). I suppose it isn't _that_ 
simple and I'm just biased because I am so used to it.
 
> But then, everybody reads their mails on their command-line anyway.
> Right?

Well, they should. It is objectively superior ;) </sarcasm>
 
> > So while I agree with you that there is certainly a learning curve
> > involved, I don't think it is all too bad. But again, that is all my
> > personal opinion, and nothing based on facts or data.
> 
> Let me provide you with some data, then. Granted, it's not necessarily
> all Git GUI, but it includes Git GUI patches, too: Git for Windows'
> contributions.
> 
> As should be well-known, I try to follow Postel's Law when it comes to
> Git for Windows' patches: be lenient in the input, strict in the output.
> As such, I don't force contributors to use GitHub PRs (although that is
> certainly encouraged by virtue of Git for Windows' source code being
> hosted on GitHub), or send patches, or send pull requests to their own
> public repositories or bundles sent to the mailing list. I accept them
> all. At least that is the idea.
> 
> I cannot tell you how many contributions came in via GitHub PRs. I can
> tell precisely you how many contributions were made _not_ using GitHub
> PRs. One one hand. Actually, on zero hands.
> 
> So clearly, at least Git for Windows' contributors (including some who
> provided Git GUI patches) are much more comfortable with the PR workflow
> than with the mailing list-based workflow.

I never said email is better that GitHub PRs. It isn't. My point was 
that using email isn't _that_ hard. When I first did it, it maybe took 
me 3-4 hours to figure everything out, and then I was set forever. I 
carry around the same '.gitconfig' file to all my setups, and everything 
"just works".

So yes, GitHub PRs are certainly easier, but email wasn't too difficult 
in my experience. But then I'm a kernel developer, so I'm a minority to 
begin with.

I suspect you've had this debate more than once, because you come in 
guns blazing ;)
 
> Just so you can't say you don't have data.
> 
> > > > So what you should do is run `git format-patch -o feature master..HEAD`,
> > > > assuming your feature branch is checked out. This will give you a set of
> > > > '.patch' files depending on how many commits you made in your branch in
> > > > the folder feature/. Then, you can run
> > > >
> > > >   git send-email --to='Pratyush Yadav <me@yadavpratyush.com>' --cc='<git@vger.kernel.org>' feature/*.patch
> > > >
> > > > This will send all your patch files via email to me with the git list in
> > > > Cc. You can add multiple '--to' and '--cc' options to send it to
> > > > multiple people.
> > > >
> > > > Try sending the patches to yourself to experiment around with it.
> > > >
> > > > A pretty good tutorial to configuring and using git-send-email can be
> > > > found at [0]. And of course, read the man page.
> > > >
> > > > These instructions are for Linux, but you can probably do something
> > > > similar in Windows too (if you're using Windows that is).
> > >
> > > Last I checked, `git send-email` worked in Git for Windows.
> > >
> > > But of course, it does not only not address the problem it tries to
> > > solve fully (to provide a way to interact with a mailing list when
> > > submitting patches for review), not even close, to add insult to injury,
> > > it now adds an additional burden to contributors (who might already have
> > > struggled to learn themselves enough Tcl/Tk to fix the problem) to
> > > configure `git send-email` correctly.
> >
> > The way I see it, git-send-email does not need to solve the problem of
> > interacting with a mailing list. That problem is already solved by a
> > hoard of MUAs. All git-send-email should do is, you guessed it, send
> > emails (or patches to be specific).
> 
> But of course! And it is natural that you should use two separate MUAs.
> 
> > Anyway, GitGitGadget solves a large part of the problem. It eliminates
> > the need for using git-send-email, and it even shows you the replies
> > received on the list. I honestly think it is a great tool, and it gives
> > people a very good alternative to using git-send-email.
> 
> GitGitGadget is just a workaround. Not even complete. Can't be complete,
> really. Because problems. It has much of the same problems of `git
> send-email`: it's a one-way conversation. Code is not discussed in the
> right context (which would be a worktree with the correct commit checked
> out). The transfer is lossy (email is designed for human-readable
> messages, not for transferring machine-readable serialized objects).
> Matching original commits and/or branches to the ones on the other side
> is tedious. Any interaction requires switching between many tools. Etc
> 
> > One feature that would make it complete would be the ability to reply to
> > review comments.
> 
> And how would that work, exactly? How to determine *which* email to
> respond to? *Which* person to reply to? *What* to quote?

GGG already shows replies to the patches as a comment. On GitHub you can 
"Quote reply" a comment, which quotes the entire comment just like your 
MUA would. The option can be found by clicking the 3 dots on the top 
right of a comment.

Then you can write your reply there, and the last line would be 
'/reply', which would make GGG send that email as a reply. You would 
need to strip the first line from the reply because GGG starts the reply 
with something like:

  > [On the Git mailing list](https://public-inbox.org/git/xmqq7e5l9zb1.fsf@gitster-ct.c.googlers.com), Junio C Hamano wrote ([reply to this](https://github.com/gitgitgadget/gitgitgadget/wiki/ReplyToThis)):
 
GGG also adds 3 backticks before and after the reply content, so those 
would need to be removed too.

Does this sound like a sane solution?

> > This would remove the need for an email client (almost) completely. I
> > have never written Typescript or used Azure pipelines ever, but I can
> > try tinkering around to see if I can figure out how to do something
> > like that. Unless, of course, you or someone else is already doing it.
> > If not, some pointers would be appreciated.
> 
> Feel free to give this challenge a try.

The first challenge is learning Typescript :)
 
> > > > > For now, I followed the instruction of Johannes Schindelin and submitted a
> > > > > pull request . Please see https://github.com/gitgitgadget/git/pull/376
> > > >
> > > > You haven't sent '/submit' over there, so those emails aren't in the
> > > > list (and my inbox) yet. You need to comment with '/submit' (without the
> > > > quotes) to tell GitGitGadget to send your PR as email.
> > >
> > > They probably did not hit `/submit` because the initial hurdle is to be
> > > `/allow`ed (a very, very simplistic attempt at trying to prevent
> > > spamming the mailing list by jokesters, of which there are unfortunately
> > > quite a number).
> > >
> > > This `/allow` command, BTW, can be issued by anybody who has been
> > > `/allow`ed before, it does not always have to be me.
> > >
> > > FWIW you should probably be in that list of `/allow`ed people so that
> > > you can `/allow` new contributors to use GitGitGadget, too.
> >
> > That would be great! How do I get '/allow'ed? Do I have to open a PR
> > there for you to '/allow' me?
> 
> https://github.com/gitgitgadget/git/pull/376#issuecomment-538784646
> 
> > > > [...]
> > > >
> > > > > Since #1 is a serious issue, I tried to find out the function which does the
> > > > > keycode validation, but I haven't succeded till now. ( I found the C function
> > > > > name  which is "TkStringToKeysym" from TK source, but I couldn't find its TCL
> > > > > binding ). It will be helpful if any one can help me on this.
> > > >
> > > > I really think you shouldn't dive around in the C parts of Tcl. I
> > > > haven't looked too deeply into this, but you can probably wrap your bind
> > > > calls in `catch` [2] and handle errors from there. Again, I haven't
> > > > tried actually doing this, so you do need to check first.
> > > >
> > > > You can find examples of how to use `catch` in our codebase. Just search
> > > > for it.
> > >
> > > FWIW in addition to the `catch` method, I would also recommend looking
> > > into a minimal (not even necessarily complete) way to translate the Qt
> > > way to specify the keyboard shortcuts (as used by `git-cola`) to Tk
> > > ones.
> > >
> > > As indicated in
> > > https://github.com/git/git/pull/220#issuecomment-536045075, the Qt style
> > > `CTRL+,` should be translated to `Control-comma`, for example. In
> > > particular, keystrokes specified in the format indicated at
> > > https://doc.qt.io/archives/qt-4.8/qkeysequence.html#QKeySequence-2 to
> > > the format indicated at https://www.tcl.tk/man/tcl8.4/TkCmd/keysyms.htm.
> > >
> > > However, it might not even need to put in _such_ a lot of work: in my
> > > tests, `Control-,` worked just as well as `Control-comma`. To test this
> > > for yourself, use this snippet (that is slightly modified from the
> > > example at the bottom of https://www.tcl.tk/man/tcl/TkCmd/bind.htm so
> > > that it reacts _only_ to Control+comma instead of all keys):
> >
> > Another benefit to the translation framework would be that we could also
> > generate the menu labels (aka "accelerator") for the tools, instead of
> > making the user specify both the shortcut and the label.
> >
> > > -- snip --
> > > set keysym "Press any key"
> > > pack [label .l -textvariable keysym -padx 2m -pady 1m]
> > > #bind . <Key> {
> > > bind . <Control-,> {
> > >     set keysym "You pressed %K"
> > > }
> > > -- snap --
> > >
> > > So I could imagine that something like this could serve as an initial
> > > draft for a function that you can turn into a "good enough" version:
> > >
> > > -- snip --
> > > proc QKeySequence2keysym {keystroke} {
> > > 	regsub -all {(?i)Ctrl\+} $keystroke "Control-" keystroke
> > > 	regsub -all {(?i)Alt\+} $keystroke "Alt-" keystroke
> > > 	regsub -all {(?i)Shift\+} $keystroke "Shift-" keystroke
> > > 	return $keystroke
> > > }
> > > -- snap --
> > >
> > > That way, you don't have to introduce settings separate from
> > > `git-cola`'s, and you can reuse the short-and-sweet variable name.
> >
> > I think a more important question is whether we _really_ need to have
> > compatibility with git-cola. Most of our shortcuts don't match with
> > them, so is it really worth the effort to try to keep compatibility?
> >
> > I'm not against something like this, but just want to be sure we
> > evaluate whether the effort is worth it.
> 
> `git-cola`, by virtue of being there first, squats on the neat config
> setting name `shortcut`.
> 
> I expect users to be utterly surprised when that name does not work for
> them.

Correct. Makes sense.
 
> Please note that I intended `QKeySequence2keysym` to leave parameters
> unchanged that already refer to keysyms. So this is purely a way to
> reuse the same name as `git-cola` while still playing nice with existing
> configurations targeting `git-cola`.

Since you already gave a draft of the function, it shouldn't be too 
difficult to pick up from there. I'm just waiting for Harish to send in 
his first iteration so we can first discuss that.

-- 
Regards,
Pratyush Yadav

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

[Johannes Schindelin]

Hi Pratyush,

On Mon, 7 Oct 2019, Pratyush Yadav wrote:

> On 06/10/19 10:27PM, Johannes Schindelin wrote:
> >
> > On Mon, 7 Oct 2019, Pratyush Yadav wrote:
> >
> > > On 06/10/19 11:49AM, Johannes Schindelin wrote:
> > > >
> > > > On Sun, 6 Oct 2019, Pratyush Yadav wrote:
> > > >
> > > > > On 06/10/19 01:46AM, Harish Karumuthil wrote:
> > > > > >
> > > > > > From
> > > > > > https://www.kernel.org/doc/html/v4.10/process/email-clients.html,
> > > > > > I understood that, my current email client ( that is gmail
> > > > > > web ) is not good for submitting patches. So I was tying to
> > > > > > setup a mail client which is compatible with `git
> > > > > > send-mail`. But I was not able to get a satisfactory result
> > > > > > in that.
> > > > >
> > > > > You don't need to "set up" an email client with
> > > > > git-send-email.  git-send-email is an email client itself.
> > > > > Well, one which can only send emails.
> > > >
> > > > It also cannot reply to mails on the mailing list.
> > > >
> > > > It cannot even notify you when anybody replied to your patch.
> > > >
> > > > Two rather problematic aspects when it comes to patch
> > > > contributions: how are you supposed to work with the reviewers
> > > > when you lack all the tools to _interact_ with them? All `git
> > > > send-email` provides is a "fire and forget" way to send patches,
> > > > i.e. it encourages a monologue, when you want to start a
> > > > dialogue instead.
> > >
> > > Well, I started with email based patch contribution when I was
> > > first got started with open source, so I might be a bit biased,
> > > but in my experience, it is not that difficult to set all these
> > > things up. Most of the time, all you need to tell git-send-email
> > > is your SMTP server, username, and password. All pretty easy
> > > things to do.
> >
> > Okay, set it up with a corporate Exchange server.
> >
> > I'll be waiting right here.
>
> I admit, I've never had to do that. And by how you word it, hope I
> never have to do it in the future either.

And I hope that you make peace with the fact that you prevent any
corporate developer from contributing easily.

> > > And you add in your email client (which pretty much everyone
> > > should have), and it is a complete setup. I personally use neomutt
> > > as my email client, but I have used Thunderbird before, and it is
> > > really easy to set it up to send plain text emails. All you need
> > > to do is hold Shift before hitting reply, and you're in plain text
> > > mode. And you can even make it use plain text by default by
> > > flipping a switch in the settings.
> >
> > How intuitive. And of course Thunderbird still messes up the patches
> > so that they won't apply, unless you *checks notes* do things that
> > are quite involved or *checks notes* do other things that are quite
> > involved.
>
> Ha! Made me chuckle. You got me there :). I suppose it isn't _that_
> simple and I'm just biased because I am so used to it.

I assumed that you were comfortable with it, and a bit oblivious about
the hurdle that this represents to the majority of potential
contributors.

Remember, one of the beautiful things GitHub has given the world _on
top_ of Git is how easy one-off contributions are.

> > > So while I agree with you that there is certainly a learning curve
> > > involved, I don't think it is all too bad. But again, that is all my
> > > personal opinion, and nothing based on facts or data.
> >
> > Let me provide you with some data, then. Granted, it's not necessarily
> > all Git GUI, but it includes Git GUI patches, too: Git for Windows'
> > contributions.
> >
> > As should be well-known, I try to follow Postel's Law when it comes to
> > Git for Windows' patches: be lenient in the input, strict in the output.
> > As such, I don't force contributors to use GitHub PRs (although that is
> > certainly encouraged by virtue of Git for Windows' source code being
> > hosted on GitHub), or send patches, or send pull requests to their own
> > public repositories or bundles sent to the mailing list. I accept them
> > all. At least that is the idea.
> >
> > I cannot tell you how many contributions came in via GitHub PRs. I can
> > tell precisely you how many contributions were made _not_ using GitHub
> > PRs. One one hand. Actually, on zero hands.
> >
> > So clearly, at least Git for Windows' contributors (including some who
> > provided Git GUI patches) are much more comfortable with the PR workflow
> > than with the mailing list-based workflow.
>
> I never said email is better that GitHub PRs. It isn't. My point was
> that using email isn't _that_ hard. When I first did it, it maybe took
> me 3-4 hours to figure everything out, and then I was set forever. I
> carry around the same '.gitconfig' file to all my setups, and everything
> "just works".
>
> So yes, GitHub PRs are certainly easier, but email wasn't too difficult
> in my experience. But then I'm a kernel developer, so I'm a minority to
> begin with.
>
> I suspect you've had this debate more than once, because you come in
> guns blazing ;)

I come in guns blazing because these obstacles that are put in the way
of contributors are the opposite of what I consider inclusive and
welcoming.

The fact that I am blessed with a lot of privilege (which, let's face
it, I did nothing to earn) does not mean that I want to discount those
who do not have that privilege. I have the time to contribute to Open
Source, which is a privilege. I have the education to do so, with is a
privilege. I even have the time to struggle with a mailing list-based
code contribution process, which is a privilege I imagine only
preciously few people enjoy.

So I work as hard as I can against obstacles that are essentially big
"Keep Out" signs (or, if you will, a big middle finger) to contributors
without these privileges.

> > > [... talking about GitGitGadget...]
> > >
> > > One  feature that would make it complete would be the ability to
> > > reply to review comments.
> >
> > And how would that work, exactly? How to determine *which* email to
> > respond to? *Which* person to reply to? *What* to quote?
>
> GGG already shows replies to the patches as a comment.

Yes.

(I know, I implemented this.)

> On GitHub you can "Quote reply" a comment, which quotes the entire
> comment just like your MUA would.

On GitHub, you can also select part of the comment and press the `r`
key, which results in the equivalent of what I am doing right here:
quoting part of your mail and responding to just that part.

You can also just reply without quoting.

These are three ways to reply to comments on GitHub, and in my
experience the rarest form is the full quote, the most common form is
the "no quote" form. (Which makes sense because you already have
everything in that UI, you don't need to quote unless you need to make a
point about only a certain part of what you are replying to, and only if
that point might be otherwise missed.)

Something I also saw often enough is to accumulate quotes from multiple
comments in the same reply.

> Then you can write your reply there, and the last line would be
> '/reply', which would make GGG send that email as a reply. You would
> need to strip the first line from the reply because GGG starts the
> reply with something like:
>
>   > [On the Git mailing
>   > list](https://public-inbox.org/git/xmqq7e5l9zb1.fsf@gitster-ct.c.googlers.com),
>   > Junio C Hamano wrote ([reply to
>   > this](https://github.com/gitgitgadget/gitgitgadget/wiki/ReplyToThis)):
>
> GGG also adds 3 backticks before and after the reply content, so those
> would need to be removed too.

Apart from the problems to identify the correct mail to reply to
(unless, as you suggested, the Message-ID is part of the quoted part by
virtue of including that public-inbox URL), I think it would make it
cumbersome to require the `/reply` command. Quite honestly, I would
prefer it if GitGitGadget would simply send replies whenever it can
figure out to who to send, and which Message-ID to reply to.

> > > This would remove the need for an email client (almost)
> > > completely. I have never written Typescript or used Azure
> > > pipelines ever, but I can try tinkering around to see if I can
> > > figure out how to do something like that. Unless, of course, you
> > > or someone else is already doing it.  If not, some pointers would
> > > be appreciated.
> >
> > Feel free to give this challenge a try.
>
> The first challenge is learning Typescript :)

I learned Typescript to implement GitGitGadget. It maybe took me 3-4
hours to figure everything out, and then I was set forever. :-P

Ciao,
Johannes

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

[Birger Skogeng Pedersen]

It seems this topic has kindof derailed(?). But I feel like voicing my
opinion nonetheless.

I have to say I absolutely agree with Johannes Schindelin here. I
would really prefer a more modern way of contributing to this project
(any software project, really) than using emails. Be that Github,
Gitlab, bitbucket or whatever else.

But no it's not *impossible* to learn how to send patches by email.
But IMO it is a less efficient way to develop and maintain an open
source project. If getting more people interested in contributing,
there is no doubt in my mind that using Github would be better.

My biggest gripe with not using Github is how to keep track of replies
(comments) to a topic. I have to navigate through multiple emails to
get and overview of what everyone has been saying, when it should just
be a single collection of replies from everyone (like in a Github
issue). Where each issue has their own thread, and they can be linked
to each other or to PRs.


Just felt like chiming in,
Birger

On Mon, Oct 7, 2019 at 11:21 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Pratyush,
>
> On Mon, 7 Oct 2019, Pratyush Yadav wrote:
>
> > On 06/10/19 10:27PM, Johannes Schindelin wrote:
> > >
> > > On Mon, 7 Oct 2019, Pratyush Yadav wrote:
> > >
> > > > On 06/10/19 11:49AM, Johannes Schindelin wrote:
> > > > >
> > > > > On Sun, 6 Oct 2019, Pratyush Yadav wrote:
> > > > >
> > > > > > On 06/10/19 01:46AM, Harish Karumuthil wrote:
> > > > > > >
> > > > > > > From
> > > > > > > https://www.kernel.org/doc/html/v4.10/process/email-clients.html,
> > > > > > > I understood that, my current email client ( that is gmail
> > > > > > > web ) is not good for submitting patches. So I was tying to
> > > > > > > setup a mail client which is compatible with `git
> > > > > > > send-mail`. But I was not able to get a satisfactory result
> > > > > > > in that.
> > > > > >
> > > > > > You don't need to "set up" an email client with
> > > > > > git-send-email.  git-send-email is an email client itself.
> > > > > > Well, one which can only send emails.
> > > > >
> > > > > It also cannot reply to mails on the mailing list.
> > > > >
> > > > > It cannot even notify you when anybody replied to your patch.
> > > > >
> > > > > Two rather problematic aspects when it comes to patch
> > > > > contributions: how are you supposed to work with the reviewers
> > > > > when you lack all the tools to _interact_ with them? All `git
> > > > > send-email` provides is a "fire and forget" way to send patches,
> > > > > i.e. it encourages a monologue, when you want to start a
> > > > > dialogue instead.
> > > >
> > > > Well, I started with email based patch contribution when I was
> > > > first got started with open source, so I might be a bit biased,
> > > > but in my experience, it is not that difficult to set all these
> > > > things up. Most of the time, all you need to tell git-send-email
> > > > is your SMTP server, username, and password. All pretty easy
> > > > things to do.
> > >
> > > Okay, set it up with a corporate Exchange server.
> > >
> > > I'll be waiting right here.
> >
> > I admit, I've never had to do that. And by how you word it, hope I
> > never have to do it in the future either.
>
> And I hope that you make peace with the fact that you prevent any
> corporate developer from contributing easily.
>
> > > > And you add in your email client (which pretty much everyone
> > > > should have), and it is a complete setup. I personally use neomutt
> > > > as my email client, but I have used Thunderbird before, and it is
> > > > really easy to set it up to send plain text emails. All you need
> > > > to do is hold Shift before hitting reply, and you're in plain text
> > > > mode. And you can even make it use plain text by default by
> > > > flipping a switch in the settings.
> > >
> > > How intuitive. And of course Thunderbird still messes up the patches
> > > so that they won't apply, unless you *checks notes* do things that
> > > are quite involved or *checks notes* do other things that are quite
> > > involved.
> >
> > Ha! Made me chuckle. You got me there :). I suppose it isn't _that_
> > simple and I'm just biased because I am so used to it.
>
> I assumed that you were comfortable with it, and a bit oblivious about
> the hurdle that this represents to the majority of potential
> contributors.
>
> Remember, one of the beautiful things GitHub has given the world _on
> top_ of Git is how easy one-off contributions are.
>
> > > > So while I agree with you that there is certainly a learning curve
> > > > involved, I don't think it is all too bad. But again, that is all my
> > > > personal opinion, and nothing based on facts or data.
> > >
> > > Let me provide you with some data, then. Granted, it's not necessarily
> > > all Git GUI, but it includes Git GUI patches, too: Git for Windows'
> > > contributions.
> > >
> > > As should be well-known, I try to follow Postel's Law when it comes to
> > > Git for Windows' patches: be lenient in the input, strict in the output.
> > > As such, I don't force contributors to use GitHub PRs (although that is
> > > certainly encouraged by virtue of Git for Windows' source code being
> > > hosted on GitHub), or send patches, or send pull requests to their own
> > > public repositories or bundles sent to the mailing list. I accept them
> > > all. At least that is the idea.
> > >
> > > I cannot tell you how many contributions came in via GitHub PRs. I can
> > > tell precisely you how many contributions were made _not_ using GitHub
> > > PRs. One one hand. Actually, on zero hands.
> > >
> > > So clearly, at least Git for Windows' contributors (including some who
> > > provided Git GUI patches) are much more comfortable with the PR workflow
> > > than with the mailing list-based workflow.
> >
> > I never said email is better that GitHub PRs. It isn't. My point was
> > that using email isn't _that_ hard. When I first did it, it maybe took
> > me 3-4 hours to figure everything out, and then I was set forever. I
> > carry around the same '.gitconfig' file to all my setups, and everything
> > "just works".
> >
> > So yes, GitHub PRs are certainly easier, but email wasn't too difficult
> > in my experience. But then I'm a kernel developer, so I'm a minority to
> > begin with.
> >
> > I suspect you've had this debate more than once, because you come in
> > guns blazing ;)
>
> I come in guns blazing because these obstacles that are put in the way
> of contributors are the opposite of what I consider inclusive and
> welcoming.
>
> The fact that I am blessed with a lot of privilege (which, let's face
> it, I did nothing to earn) does not mean that I want to discount those
> who do not have that privilege. I have the time to contribute to Open
> Source, which is a privilege. I have the education to do so, with is a
> privilege. I even have the time to struggle with a mailing list-based
> code contribution process, which is a privilege I imagine only
> preciously few people enjoy.
>
> So I work as hard as I can against obstacles that are essentially big
> "Keep Out" signs (or, if you will, a big middle finger) to contributors
> without these privileges.
>
> > > > [... talking about GitGitGadget...]
> > > >
> > > > One  feature that would make it complete would be the ability to
> > > > reply to review comments.
> > >
> > > And how would that work, exactly? How to determine *which* email to
> > > respond to? *Which* person to reply to? *What* to quote?
> >
> > GGG already shows replies to the patches as a comment.
>
> Yes.
>
> (I know, I implemented this.)
>
> > On GitHub you can "Quote reply" a comment, which quotes the entire
> > comment just like your MUA would.
>
> On GitHub, you can also select part of the comment and press the `r`
> key, which results in the equivalent of what I am doing right here:
> quoting part of your mail and responding to just that part.
>
> You can also just reply without quoting.
>
> These are three ways to reply to comments on GitHub, and in my
> experience the rarest form is the full quote, the most common form is
> the "no quote" form. (Which makes sense because you already have
> everything in that UI, you don't need to quote unless you need to make a
> point about only a certain part of what you are replying to, and only if
> that point might be otherwise missed.)
>
> Something I also saw often enough is to accumulate quotes from multiple
> comments in the same reply.
>
> > Then you can write your reply there, and the last line would be
> > '/reply', which would make GGG send that email as a reply. You would
> > need to strip the first line from the reply because GGG starts the
> > reply with something like:
> >
> >   > [On the Git mailing
> >   > list](https://public-inbox.org/git/xmqq7e5l9zb1.fsf@gitster-ct.c.googlers.com),
> >   > Junio C Hamano wrote ([reply to
> >   > this](https://github.com/gitgitgadget/gitgitgadget/wiki/ReplyToThis)):
> >
> > GGG also adds 3 backticks before and after the reply content, so those
> > would need to be removed too.
>
> Apart from the problems to identify the correct mail to reply to
> (unless, as you suggested, the Message-ID is part of the quoted part by
> virtue of including that public-inbox URL), I think it would make it
> cumbersome to require the `/reply` command. Quite honestly, I would
> prefer it if GitGitGadget would simply send replies whenever it can
> figure out to who to send, and which Message-ID to reply to.
>
> > > > This would remove the need for an email client (almost)
> > > > completely. I have never written Typescript or used Azure
> > > > pipelines ever, but I can try tinkering around to see if I can
> > > > figure out how to do something like that. Unless, of course, you
> > > > or someone else is already doing it.  If not, some pointers would
> > > > be appreciated.
> > >
> > > Feel free to give this challenge a try.
> >
> > The first challenge is learning Typescript :)
>
> I learned Typescript to implement GitGitGadget. It maybe took me 3-4
> hours to figure everything out, and then I was set forever. :-P
>
> Ciao,
> Johannes

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

[Alban Gruin]

Hi Birger,

Le 07/10/2019 à 12:43, Birger Skogeng Pedersen a écrit :
> It seems this topic has kindof derailed(?). But I feel like voicing my
> opinion nonetheless.
> 
> -%<-
> 
> My biggest gripe with not using Github is how to keep track of replies
> (comments) to a topic. I have to navigate through multiple emails to
> get and overview of what everyone has been saying, when it should just
> be a single collection of replies from everyone (like in a Github
> issue). Where each issue has their own thread, and they can be linked
> to each other or to PRs.
> 

I don’t know which email client you use, but they usually have an option
to sort messages by thread -- this is how I handle my git folder with
Thunderbird; it takes one click to switch from plain view to threaded
view, and two to isolate a specific thread.  public-inbox.org also does
this.  Some don’t though, and gmail specifically has weird opinions on
which message is part of which thread.

On the other hand, I don’t really like non-threaded means of
communication, and I am very confused as to why modern services neglect
this feature -- there may be a good reason, I’m not an expert in
human-computer interaction :).  But personally, I find it very hard to
follow issues on github (or even IRC logs, for that matter) when there
is more than 2 people involved.

> 
> Just felt like chiming in,
> Birger
> 
Cheers,
Alban

Making GitGitGadget's list -> PR comment mirroring bidirectional, was Re: [PATCH] Feature: custom guitool commands can now have custom keyboard shortcuts

[Johannes Schindelin]

Hi Pratyush,

On Mon, 7 Oct 2019, Pratyush Yadav wrote:

> On 06/10/19 10:27PM, Johannes Schindelin wrote:
> > Hi Pratyush,
> >
> > On Mon, 7 Oct 2019, Pratyush Yadav wrote:
> >
> > > Anyway, GitGitGadget solves a large part of the problem. It
> > > eliminates the need for using git-send-email, and it even shows you
> > > the replies received on the list. I honestly think it is a great
> > > tool, and it gives people a very good alternative to using
> > > git-send-email.
> >
> > GitGitGadget is just a workaround. Not even complete. Can't be
> > complete, really. Because problems. It has much of the same problems
> > of `git send-email`: it's a one-way conversation. Code is not
> > discussed in the right context (which would be a worktree with the
> > correct commit checked out). The transfer is lossy (email is designed
> > for human-readable messages, not for transferring machine-readable
> > serialized objects). Matching original commits and/or branches to the
> > ones on the other side is tedious. Any interaction requires switching
> > between many tools. Etc
> >
> > > One feature that would make it complete would be the ability to
> > > reply to review comments.
> >
> > And how would that work, exactly? How to determine *which* email to
> > respond to? *Which* person to reply to? *What* to quote?
>
> GGG already shows replies to the patches as a comment. On GitHub you can
> "Quote reply" a comment, which quotes the entire comment just like your
> MUA would. The option can be found by clicking the 3 dots on the top
> right of a comment.
>
> Then you can write your reply there, and the last line would be
> '/reply', which would make GGG send that email as a reply. You would
> need to strip the first line from the reply because GGG starts the reply
> with something like:
>
>   > [On the Git mailing list](https://public-inbox.org/git/xmqq7e5l9zb1.fsf@gitster-ct.c.googlers.com), Junio C Hamano wrote ([reply to this](https://github.com/gitgitgadget/gitgitgadget/wiki/ReplyToThis)):
>
> GGG also adds 3 backticks before and after the reply content, so those
> would need to be removed too.
>
> Does this sound like a sane solution?

Here are two real life examples where an unsuspecting GitGitGadget user
expected GitGitGadget to mirror replies _to_ the Git mailing list:

https://github.com/gitgitgadget/git/pull/451#issuecomment-555044068 and
https://github.com/gitgitgadget/git/pull/451#issuecomment-555077933

Neither of them include the line with the link.

Just to throw a bit of real life into the discussion...

Ciao,
Dscho

Re: Making GitGitGadget's list -> PR comment mirroring bidirectional, was Re: [PATCH] Feature: custom guitool commands can now have custom keyboard shortcuts

[Pratyush Yadav]

On 19/11/19 11:09PM, Johannes Schindelin wrote:
> Hi Pratyush,
> 
> On Mon, 7 Oct 2019, Pratyush Yadav wrote:
> 
> > On 06/10/19 10:27PM, Johannes Schindelin wrote:
> > > Hi Pratyush,
> > >
> > > On Mon, 7 Oct 2019, Pratyush Yadav wrote:
> > >
> > > > Anyway, GitGitGadget solves a large part of the problem. It
> > > > eliminates the need for using git-send-email, and it even shows you
> > > > the replies received on the list. I honestly think it is a great
> > > > tool, and it gives people a very good alternative to using
> > > > git-send-email.
> > >
> > > GitGitGadget is just a workaround. Not even complete. Can't be
> > > complete, really. Because problems. It has much of the same problems
> > > of `git send-email`: it's a one-way conversation. Code is not
> > > discussed in the right context (which would be a worktree with the
> > > correct commit checked out). The transfer is lossy (email is designed
> > > for human-readable messages, not for transferring machine-readable
> > > serialized objects). Matching original commits and/or branches to the
> > > ones on the other side is tedious. Any interaction requires switching
> > > between many tools. Etc
> > >
> > > > One feature that would make it complete would be the ability to
> > > > reply to review comments.
> > >
> > > And how would that work, exactly? How to determine *which* email to
> > > respond to? *Which* person to reply to? *What* to quote?
> >
> > GGG already shows replies to the patches as a comment. On GitHub you can
> > "Quote reply" a comment, which quotes the entire comment just like your
> > MUA would. The option can be found by clicking the 3 dots on the top
> > right of a comment.
> >
> > Then you can write your reply there, and the last line would be
> > '/reply', which would make GGG send that email as a reply. You would
> > need to strip the first line from the reply because GGG starts the reply
> > with something like:
> >
> >   > [On the Git mailing list](https://public-inbox.org/git/xmqq7e5l9zb1.fsf@gitster-ct.c.googlers.com), Junio C Hamano wrote ([reply to this](https://github.com/gitgitgadget/gitgitgadget/wiki/ReplyToThis)):
> >
> > GGG also adds 3 backticks before and after the reply content, so those
> > would need to be removed too.
> >
> > Does this sound like a sane solution?
> 
> Here are two real life examples where an unsuspecting GitGitGadget user
> expected GitGitGadget to mirror replies _to_ the Git mailing list:
> 
> https://github.com/gitgitgadget/git/pull/451#issuecomment-555044068 and
> https://github.com/gitgitgadget/git/pull/451#issuecomment-555077933
> 
> Neither of them include the line with the link.

Correct.

The fundamental problem we have is that GitHub's threads are 
"shallow"/"linear". You don't reply to a reply, you reply to the main 
thread, and your comment gets appended to the end of that list. In 
contrast, email based threads are "deep"/"tree-like". Here you can reply 
to a reply.

So the comment model of GitHub is less information-rich than the email 
model. The piece of information missing is "which comment does this 
comment reply to". That information has to be obtained somehow, and the 
best bet are the users themselves (by not deleting the line with the 
link in their replies). We can give the instructions in the GGG welcome 
message, and hope the users read it.

Frequent users and anyone who properly reads the instructions will 
probably manage to use this just fine. Those who don't, well they 
weren't sending replies to the list to begin with. So this will help 
people who don't want to open their email clients to send replies to the 
list, but won't help the uninformed ones. Certainly not ideal, but I 
think this might be the best we can do given the constraints.

In the meantime, I notice that GGG does not advertise the fact that 
replies don't go on the list directly very well. Yes, its mentioned in 
the welcome message, but its not instantly obvious. So maybe making it 
clearer/more noticeable will help the issue.

Another alternative might be to rely on heuristics like seeing how 
similar the quoted text in a reply is to the replies in the list. But I 
think this will cause more problems than help because users can cut 
un-necessary quoting and even edit them sometimes.

If you can think of something clever that I can't, suggestions are 
welcome :)

Anyway, I probably won't have much time to work on this feature for at 
least a couple more weeks. Maybe we'll learn more after the feature goes 
live.

> Just to throw a bit of real life into the discussion...
> 
> Ciao,
> Dscho

-- 
Regards,
Pratyush Yadav

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

[Philip Oakley]

Hi Dscho,

On 06/10/2019 21:27, Johannes Schindelin wrote:
> Let me provide you with some data, then. Granted, it's not necessarily
> all Git GUI, but it includes Git GUI patches, too: Git for Windows'
> contributions.
>
> As should be well-known, I try to follow Postel's Law when it comes to
> Git for Windows' patches: be lenient in the input, strict in the output.
> As such, I don't force contributors to use GitHub PRs (although that is
> certainly encouraged by virtue of Git for Windows' source code being
> hosted on GitHub), or send patches, or send pull requests to their own
> public repositories or bundles sent to the mailing list. I accept them
> all. At least that is the idea.
>
> I cannot tell you how many contributions came in via GitHub PRs. I can
> tell precisely you how many contributions were made_not_  using GitHub
> PRs. One one hand. Actually, on zero hands.
>
> So clearly, at least Git for Windows' contributors (including some who
> provided Git GUI patches) are much more comfortable with the PR workflow
> than with the mailing list-based workflow.
Just to say that most of the numbers are governed by the strength and 
experience with the particular infrastructures.

Tonight I had wanted to send in patches for G-f-W because of branch 
placement confusion. Eventually I had to throw in an extra rebase to a 
fresh branch, just so I could create a PR, all because of the zero 
experience you mentioned with using a G-f-W 2-patch series.

The Git list is strongly patch based and it's infrastructure works 
adequately, even if it is 'antiquated' by millennial standards.

The G-f-W interaction is almost totally via Github, and a few 
notification emails and occasional google-groups interactions. It's 
still imperfect, just like the Git list emails, but with it's own, 
different issues for trying to build its community.

Most community bondings actually build through their common adversity, 
rather than the apparent ease of joining/leaving.

The main bit I wanted to say (I think), was that having a maintainer who 
accepts input is probably the most important aspect, no matter the 
particular route used for the input. So thanks to both of you (Dscho, 
Pratyush) for *facilitating* the contribution flow.
-- 
Philip

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

[Harish Karumuthil]

Hi Johannes Schindelin, Regarding your messages,


> However, it might not even need to put in _such_ a lot of work: in my
> tests, `Control-,` worked just as well as `Control-comma`. To test this
> for yourself, use this snippet (that is slightly modified from the
> example at the bottom of https://www.tcl.tk/man/tcl/TkCmd/bind.htm so
> that it reacts _only_ to Control+comma instead of all keys):
> 
> -- snip --
> set keysym "Press any key"
> pack [label .l -textvariable keysym -padx 2m -pady 1m]
> #bind . <Key> {
> bind . <Control-,> {
>     set keysym "You pressed %K"
> }
> -- snap --

I tried this, but unfortunatly, it didn't worked for me. My tclsh version is
"8.6".  The script crashed with following error message

---
bad event type or keysym ","
    while executing
"bind . <Control-,> {
    set keysym "You pressed %K"
}"
    (file "./test.tcl" line 6)
---


The complete ( or modified ) script which I used is given below

---
package require Tk

set keysym "Press any key"
pack [label .l -textvariable keysym -padx 2m -pady 1m]
#bind . <Key> {
bind . <Control-,> {
    set keysym "You pressed %K"
}

---

From the error messages, I understand that, "<Control-,>" will not work
instead of "<Control-comma>" .

> 
> So I could imagine that something like this could serve as an initial
> draft for a function that you can turn into a "good enough" version:
> 
> -- snip --
> proc QKeySequence2keysym {keystroke} {
> 	regsub -all {(?i)Ctrl\+} $keystroke "Control-" keystroke
> 	regsub -all {(?i)Alt\+} $keystroke "Alt-" keystroke
> 	regsub -all {(?i)Shift\+} $keystroke "Shift-" keystroke
> 	return $keystroke
> }
> -- snap --
> 
> That way, you don't have to introduce settings separate from
> `git-cola`'s, and you can reuse the short-and-sweet variable name.

If my previous observation is correct, then we may have to translate a list
of key names ( in addition to atl,ctrl & shirt ) to get it working .


> Ciao,
> Johannes

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

[Johannes Schindelin]

Hi Harish,

On Mon, 7 Oct 2019, Harish Karumuthil wrote:

> > However, it might not even need to put in _such_ a lot of work: in my
> > tests, `Control-,` worked just as well as `Control-comma`. To test this
> > for yourself, use this snippet (that is slightly modified from the
> > example at the bottom of https://www.tcl.tk/man/tcl/TkCmd/bind.htm so
> > that it reacts _only_ to Control+comma instead of all keys):
> >
> > -- snip --
> > set keysym "Press any key"
> > pack [label .l -textvariable keysym -padx 2m -pady 1m]
> > #bind . <Key> {
> > bind . <Control-,> {
> >     set keysym "You pressed %K"
> > }
> > -- snap --
>
> I tried this, but unfortunatly, it didn't worked for me. My tclsh version is
> "8.6".  The script crashed with following error message
>
> ---
> bad event type or keysym ","
>     while executing
> "bind . <Control-,> {
>     set keysym "You pressed %K"
> }"
>     (file "./test.tcl" line 6)
> ---

That's too bad! I tested this on Windows, and I imagine it just does not
work on Linux/macOS...

> The complete ( or modified ) script which I used is given below
>
> ---
> package require Tk
>
> set keysym "Press any key"
> pack [label .l -textvariable keysym -padx 2m -pady 1m]
> #bind . <Key> {
> bind . <Control-,> {
>     set keysym "You pressed %K"
> }
>
> ---
>
> From the error messages, I understand that, "<Control-,>" will not work
> instead of "<Control-comma>" .
>
> >
> > So I could imagine that something like this could serve as an initial
> > draft for a function that you can turn into a "good enough" version:
> >
> > -- snip --
> > proc QKeySequence2keysym {keystroke} {
> > 	regsub -all {(?i)Ctrl\+} $keystroke "Control-" keystroke
> > 	regsub -all {(?i)Alt\+} $keystroke "Alt-" keystroke
> > 	regsub -all {(?i)Shift\+} $keystroke "Shift-" keystroke
> > 	return $keystroke
> > }
> > -- snap --
> >
> > That way, you don't have to introduce settings separate from
> > `git-cola`'s, and you can reuse the short-and-sweet variable name.
>
> If my previous observation is correct, then we may have to translate a list
> of key names ( in addition to atl,ctrl & shirt ) to get it working .

I fear you're right. Hopefully we can get away with a relatively short
list...

Ciao,
Johannes

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

[Harish Karumuthil]

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.

---
 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 ] } {
+			puts stderr "Failed to bind keyboard shortcut '$accel_key' for custom tool '$fullname'. Error: $msg"
+		} else {
+			tools_create_item $parent command \
+			-label [lindex $names end] \
+			-command [list tools_exec $fullname] \
+			-accelerator $accel_key
+			set accel_key_bound true
+		}
+	}
+
+	if { ! $accel_key_bound } {
+		tools_create_item $parent command \
+			-label [lindex $names end] \
+			-command [list tools_exec $fullname]
+	}
 }
 
 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 ?

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

[Johannes Schindelin]

Hi,

On Wed, 9 Oct 2019, Harish Karumuthil wrote:

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

This took much longer to find than I expected, probably my web search fu
is deserting me. But I found something: `string map`, see
https://tcl.tk/man/tcl8.6/TclCmd/string.htm#M34 for details.

Ciao,
Johannes

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

[Pratyush Yadav]

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

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

[Harish Karumuthil]

Hi Pratyush, Regarding your messages,

>On Sun, 2019-10-06 at 02:31 +0530, Pratyush Yadav wrote:
> You don't need to "set up" an email client with git-send-email. 
> git-send-email is an email client itself. Well, one which can only send 
> emails.

For now, I am sticking with a mail client ( evolution ) which does minimal
( or atleast transparent ) preprocessing  ( Tab => space conversion , line
wrapping  etc ).
Now I can send patches using the output of `git diff --patch-with-stat`
command and I hope is it enough for now.
Personaly I dont' like any solution which requires storing our mail password
as a plain text file.


> You haven't sent '/submit' over there, so those emails aren't in the 
> list (and my inbox) yet. You need to comment with '/submit' (without the 
> quotes) to tell GitGitGadget to send your PR as email.

I thought, lets finalize discussion about all the changes here in mail
thread   it self before submitting the patch. Otherwise, That is why I didn't
submitted the patch.


> One point I forgot to mention earlier was that I'm honestly not a big 
> fan of separating the binding and accelerator label. I understand that 
> you might not have the time to do this, but I think it is still worth 
> mentioning. Maybe I will implement something like that over your patch. 
> But it would certainly be nice if you can figure it out :).

I think there is a small missunderstanind in that point.

I agree that, in the initial implementation ( which I did @ 2016 ) menu
labels were separated from binding keys. But in the last update, it is not
like that.

Currently, user only need to specify single config value which is
`guitool.<name>.gitgui-shortcut` and don't have to specify accel-lable
separatly.
Label is generated from the shortcut.


> Either ways, detecting an existing shortcut is pretty easy. The `bind` 
> man page [1] says:
> 
>   If sequence is specified without a script, then the script currently 
>   bound to sequence is returned, or an empty string is returned if there 
>   is no binding for sequence.
> 
> So you can use this to find out if there is a binding conflict, and warn 
> the user.

Will try this. Thanks!

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

[Pratyush Yadav]

Hi Harish,

Sorry for the late reply. Couldn't find much time last few days.

On 07/10/19 11:43AM, Harish Karumuthil wrote:
> Hi Pratyush, Regarding your messages,
> 
> >On Sun, 2019-10-06 at 02:31 +0530, Pratyush Yadav wrote:
> > You don't need to "set up" an email client with git-send-email. 
> > git-send-email is an email client itself. Well, one which can only send 
> > emails.
> 
> For now, I am sticking with a mail client ( evolution ) which does minimal
> ( or atleast transparent ) preprocessing  ( Tab => space conversion , line
> wrapping  etc ).
> Now I can send patches using the output of `git diff --patch-with-stat`
> command and I hope is it enough for now.
> Personaly I dont' like any solution which requires storing our mail password
> as a plain text file.

I'm afraid this won't work. The '.patch' file that `git-format-patch` 
generates also contains your commit message and the author information. 
All those are needed to properly convert your patch to a commit in my 
repo. The output of `git diff --patch-with-stat` won't be enough.

As for not wanting to store your mail password in a plain text file, 
check out [0].

And then there is GitGitGadget too, which I'd recommend since you seem 
to be having trouble sending patches directly :).
 
> > You haven't sent '/submit' over there, so those emails aren't in the 
> > list (and my inbox) yet. You need to comment with '/submit' (without 
> > the quotes) to tell GitGitGadget to send your PR as email.
> 
> I thought, lets finalize discussion about all the changes here in mail
> thread   it self before submitting the patch. Otherwise, That is why I didn't
> submitted the patch.

Makes sense.
 
> > One point I forgot to mention earlier was that I'm honestly not a big 
> > fan of separating the binding and accelerator label. I understand that 
> > you might not have the time to do this, but I think it is still worth 
> > mentioning. Maybe I will implement something like that over your patch. 
> > But it would certainly be nice if you can figure it out :).
> 
> I think there is a small missunderstanind in that point.
> 
> I agree that, in the initial implementation ( which I did @ 2016 ) menu
> labels were separated from binding keys. But in the last update, it is not
> like that.
> 
> Currently, user only need to specify single config value which is
> `guitool.<name>.gitgui-shortcut` and don't have to specify accel-lable
> separatly.
> Label is generated from the shortcut.

Thanks for clarifying. It indeed was a misunderstanding.
 
> > Either ways, detecting an existing shortcut is pretty easy. The `bind` 
> > man page [1] says:
> > 
> >   If sequence is specified without a script, then the script currently 
> >   bound to sequence is returned, or an empty string is returned if there 
> >   is no binding for sequence.
> > 
> > So you can use this to find out if there is a binding conflict, and warn 
> > the user.
> 
> Will try this. Thanks!

[0] https://www.softwaredeveloper.blog/git-credential-storage-libsecret

-- 
Regards,
Pratyush Yadav

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

[Harish K]

---
 git-gui/lib/tools.tcl | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/git-gui/lib/tools.tcl b/git-gui/lib/tools.tcl
index 6ec9411..749bc67 100644
--- a/git-gui/lib/tools.tcl
+++ b/git-gui/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,19 @@ proc tools_populate_one {fullname} {
 		}
 	}
 
-	tools_create_item $parent command \
+	if {[info exists repo_config(guitool.$fullname.accelerator)] && [info exists repo_config(guitool.$fullname.accelerator-label)]} {
+		set accele_key $repo_config(guitool.$fullname.accelerator)
+		set accel_label $repo_config(guitool.$fullname.accelerator-label)
+		tools_create_item $parent command \
 		-label [lindex $names end] \
-		-command [list tools_exec $fullname]
+		-command [list tools_exec $fullname] \
+		-accelerator $accel_label
+		bind . $accele_key [list tools_exec $fullname]
+	} else {
+		tools_create_item $parent command \
+			-label [lindex $names end] \
+			-command [list tools_exec $fullname]
+	}
 }
 
 proc tools_exec {fullname} {

--
https://github.com/git/git/pull/220

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

[David Aguilar]

On Tue, Mar 29, 2016 at 11:29:41AM +0000, Harish K wrote:
> ---
>  git-gui/lib/tools.tcl | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)


I forgot to mention that git-gui has its own repository.
The git project merges the upstream repo as a subtree into its
git-gui directory.

You should probably clone the git-gui repository and create a
patch there so that it can be applied to the upstream repo:

http://repo.or.cz/w/git-gui.git
-- 
David