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 = " etc..
Other vise there is good chance for conflicts. ( Eg: consider the case
 that,  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  wrote:
> Hello,
>
> On Tue, Mar 29, 2016 at 11:38:10AM +, 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..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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Aguilar]

On Tue, Mar 29, 2016 at 11:29:41AM +, 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Aguilar]

Hello,

On Tue, Mar 29, 2016 at 11:38:10AM +, 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..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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html