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

[harish k <harish2704@gmail.com>]

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