[2/2] git-gui: add hotkey to toggle "Amend Last Commit" check button/menu
diff mbox series

Message ID b82a00441ff1a6a9cea3fd235c1c33729ec31b71.1567713659.git.bert.wesarg@googlemail.com
State New
Headers show
Series
  • [1/2] git-gui: convert new/amend commit radiobutton to checketton
Related show

Commit Message

Bert Wesarg Sept. 5, 2019, 8:09 p.m. UTC
From: Birger Skogeng Pedersen <birger.sp@gmail.com>

Selecting whether to "Amend Last Commit" or not does not have a hotkey.

With this patch, the user may toggle between the two options with
CTRL/CMD+e.

Signed-off-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
Rebased-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
 git-gui.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Pratyush Yadav Sept. 11, 2019, 8:55 p.m. UTC | #1
Birger,

Please ignore the two emails I sent about basing your work on top of 
Bert's. I see that you have already done so (or maybe Bert did it, I 
don't know), and I was reading an older version of the patch.

On 05/09/19 10:09PM, Bert Wesarg wrote:
> From: Birger Skogeng Pedersen <birger.sp@gmail.com>
> 
> Selecting whether to "Amend Last Commit" or not does not have a hotkey.
> 
> With this patch, the user may toggle between the two options with
> CTRL/CMD+e.
> 
> Signed-off-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
> Rebased-by: Bert Wesarg <bert.wesarg@googlemail.com>
> ---
>  git-gui.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 80a07d5..8b776dd 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -2640,6 +2640,12 @@ proc show_less_context {} {
>  	}
>  }
>  
> +proc toggle_commit_type {} {
> +	global commit_type_is_amend
> +	set commit_type_is_amend [expr !$commit_type_is_amend]
> +	do_select_commit_type
> +}

Ah! So we had almost exactly the same thought process. This is pretty 
much what I suggested in my other email ;)

> +
>  ######################################################################
>  ##
>  ## ui construction
> @@ -2830,6 +2836,7 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} {
>  	if {![is_enabled nocommit]} {
>  		.mbar.commit add checkbutton \
>  			-label [mc "Amend Last Commit"] \
> +			-accelerator $M1T-E \
>  			-variable commit_type_is_amend \
>  			-command do_select_commit_type
>  		lappend disable_on_lock \
> @@ -3825,6 +3832,7 @@ bind .   <$M1B-Key-equal> {show_more_context;break}
>  bind .   <$M1B-Key-plus> {show_more_context;break}
>  bind .   <$M1B-Key-KP_Add> {show_more_context;break}
>  bind .   <$M1B-Key-Return> do_commit
> +bind .   <$M1B-Key-e> toggle_commit_type

Nitpick: Please move this up with the binds for other letters (u, j, 
etc)

Also, I notice that the bindings for other letters have the same 
function bound for both small and capital letters (IOW, same behavior 
with shift held and released).

I don't necessarily think that is a great idea. It is a pretty common 
pattern to have, say Ctrl+a, do something, and Ctrl+Shift+a, do 
something else. Just want to pick your brain on whether you think we 
should do the same thing for both Ctrl+e and for Ctrl+E (aka 
Ctrl+Shift+e), or just bind it to Ctrl+e, and leave Ctrl+E for something 
else.

>  foreach i [list $ui_index $ui_workdir] {
>  	bind $i <Button-1>       { toggle_or_diff click %W %x %y; break }
>  	bind $i <$M1B-Button-1>  { add_one_to_selection %W %x %y; break }

Overall, the patch LGTM apart from a couple of nitpicks above. Thanks.
Birger Skogeng Pedersen Sept. 12, 2019, 6:05 a.m. UTC | #2
Hi Pratyush,

On Wed, Sep 11, 2019 at 10:55 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> Also, I notice that the bindings for other letters have the same
> function bound for both small and capital letters (IOW, same behavior
> with shift held and released).
>
> I don't necessarily think that is a great idea. It is a pretty common
> pattern to have, say Ctrl+a, do something, and Ctrl+Shift+a, do
> something else. Just want to pick your brain on whether you think we
> should do the same thing for both Ctrl+e and for Ctrl+E (aka
> Ctrl+Shift+e), or just bind it to Ctrl+e, and leave Ctrl+E for something
> else.

I just tested what happens when you press Ctrl+e while Caps Lock is
enabled; the Ctrl+e binding is not invoked. That's probably why other
key bindings have the same function bound for both lower- and
upper-case letters, to have the same behaviour with/without Caps Lock
enabled. With that in mind, we should probably bind Ctrl+E aswell.

Should I create and send a new patch?

Birger
Pratyush Yadav Sept. 12, 2019, 4:29 p.m. UTC | #3
On 12/09/19 08:05AM, Birger Skogeng Pedersen wrote:
> Hi Pratyush,
> 
> On Wed, Sep 11, 2019 at 10:55 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > Also, I notice that the bindings for other letters have the same
> > function bound for both small and capital letters (IOW, same behavior
> > with shift held and released).
> >
> > I don't necessarily think that is a great idea. It is a pretty common
> > pattern to have, say Ctrl+a, do something, and Ctrl+Shift+a, do
> > something else. Just want to pick your brain on whether you think we
> > should do the same thing for both Ctrl+e and for Ctrl+E (aka
> > Ctrl+Shift+e), or just bind it to Ctrl+e, and leave Ctrl+E for something
> > else.
> 
> I just tested what happens when you press Ctrl+e while Caps Lock is
> enabled; the Ctrl+e binding is not invoked. That's probably why other
> key bindings have the same function bound for both lower- and
> upper-case letters, to have the same behaviour with/without Caps Lock
> enabled. With that in mind, we should probably bind Ctrl+E aswell.

Nice catch! Makes sense to have the same behaviour for both caps lock 
enabled and disabled.

> 
> Should I create and send a new patch?

Yes, please do.
Marc Branchaud Sept. 12, 2019, 9:34 p.m. UTC | #4
On 2019-09-12 12:29 p.m., Pratyush Yadav wrote:
> On 12/09/19 08:05AM, Birger Skogeng Pedersen wrote:
>> Hi Pratyush,
>>
>> On Wed, Sep 11, 2019 at 10:55 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
>>> Also, I notice that the bindings for other letters have the same
>>> function bound for both small and capital letters (IOW, same behavior
>>> with shift held and released).
>>>
>>> I don't necessarily think that is a great idea. It is a pretty common
>>> pattern to have, say Ctrl+a, do something, and Ctrl+Shift+a, do
>>> something else. Just want to pick your brain on whether you think we
>>> should do the same thing for both Ctrl+e and for Ctrl+E (aka
>>> Ctrl+Shift+e), or just bind it to Ctrl+e, and leave Ctrl+E for something
>>> else.
>>
>> I just tested what happens when you press Ctrl+e while Caps Lock is
>> enabled; the Ctrl+e binding is not invoked. That's probably why other
>> key bindings have the same function bound for both lower- and
>> upper-case letters, to have the same behaviour with/without Caps Lock
>> enabled. With that in mind, we should probably bind Ctrl+E aswell.
> 
> Nice catch! Makes sense to have the same behaviour for both caps lock
> enabled and disabled.

(I've been a git-gui user for many years...)

I disagree!  Who expects anything to work properly when capslock is on?

		M.


>>
>> Should I create and send a new patch?
> 
> Yes, please do.
>
Philip Oakley Sept. 12, 2019, 10:23 p.m. UTC | #5
On 12/09/2019 22:34, Marc Branchaud wrote:
>>> I just tested what happens when you press Ctrl+e while Caps Lock is
>>> enabled; the Ctrl+e binding is not invoked. That's probably why other
>>> key bindings have the same function bound for both lower- and
>>> upper-case letters, to have the same behaviour with/without Caps Lock
>>> enabled. With that in mind, we should probably bind Ctrl+E aswell.
>>
>> Nice catch! Makes sense to have the same behaviour for both caps lock
>> enabled and disabled.
>
> (I've been a git-gui user for many years...)
>
> I disagree!  Who expects anything to work properly when capslock is on?
>
>         M.
>
I'd tend to agree. In other areas the use of shift is often used as the 
complement of the unshifted action, so it does feel 'odd'. Thus it could 
be used directly as the bool for amend or direct commit.

This all assumes that Caps Lock is equivalent to having the shift on, 
rather than being a special extra key.

Philip
Birger Skogeng Pedersen Sept. 13, 2019, 7:50 a.m. UTC | #6
Hi Marc and Philip,


On 12/09/2019 22:34, Marc Branchaud wrote:
> I disagree!  Who expects anything to work properly when capslock is on?

Me :-)


On Fri, Sep 13, 2019 at 12:23 AM Philip Oakley <philipoakley@iee.email> wrote:
> I'd tend to agree. In other areas the use of shift is often used as the
> complement of the unshifted action, so it does feel 'odd'. Thus it could
> be used directly as the bool for amend or direct commit.
>
> This all assumes that Caps Lock is equivalent to having the shift on,
> rather than being a special extra key.

It seems all the Ctrl+(lowercase character) hotkeys in git-gui have an
equivalent Ctrl+(uppercase character).
So for this feature, we should keep the Ctrl+E bind aswell as the
Ctrl+e bind. If nothing else, to keep it consistent with the rest of
the hotkey bindings.
But honestly, (as Marc pointed out) it is a quite weird that
Ctrl+Shift+(character) has the excact same function as
Ctrl+(character). Perhaps we should find another way to bind the
hotkeys, where the state of Caps Lock doesn't matter? If possible.


Birger
Marc Branchaud Sept. 13, 2019, 1:55 p.m. UTC | #7
On 2019-09-13 3:50 a.m., Birger Skogeng Pedersen wrote:
> Hi Marc and Philip,
> 
> 
> On 12/09/2019 22:34, Marc Branchaud wrote:
>> I disagree!  Who expects anything to work properly when capslock is on?
> 
> Me :-)

Fair enough, though I imagine you have a pretty narrow definition of 
"anything".  :)

> On Fri, Sep 13, 2019 at 12:23 AM Philip Oakley <philipoakley@iee.email> wrote:
>> I'd tend to agree. In other areas the use of shift is often used as the
>> complement of the unshifted action, so it does feel 'odd'. Thus it could
>> be used directly as the bool for amend or direct commit.
>>
>> This all assumes that Caps Lock is equivalent to having the shift on,
>> rather than being a special extra key.
> 
> It seems all the Ctrl+(lowercase character) hotkeys in git-gui have an
> equivalent Ctrl+(uppercase character).
> So for this feature, we should keep the Ctrl+E bind aswell as the
> Ctrl+e bind. If nothing else, to keep it consistent with the rest of
> the hotkey bindings.

Ah, OK.  I agree that keeping git-gui internally consistent trumps the 
other considerations.

		M.


> But honestly, (as Marc pointed out) it is a quite weird that
> Ctrl+Shift+(character) has the excact same function as
> Ctrl+(character). Perhaps we should find another way to bind the
> hotkeys, where the state of Caps Lock doesn't matter? If possible.
> 
> 
> Birger
>
Pratyush Yadav Sept. 13, 2019, 5:47 p.m. UTC | #8
On 13/09/19 09:50AM, Birger Skogeng Pedersen wrote:
> Hi Marc and Philip,
> 
> 
> On 12/09/2019 22:34, Marc Branchaud wrote:
> > I disagree!  Who expects anything to work properly when capslock is on?
> 
> Me :-)
> 
> 
> On Fri, Sep 13, 2019 at 12:23 AM Philip Oakley <philipoakley@iee.email> wrote:
> > I'd tend to agree. In other areas the use of shift is often used as the
> > complement of the unshifted action, so it does feel 'odd'. Thus it could
> > be used directly as the bool for amend or direct commit.
> >
> > This all assumes that Caps Lock is equivalent to having the shift on,
> > rather than being a special extra key.
> 
> It seems all the Ctrl+(lowercase character) hotkeys in git-gui have an
> equivalent Ctrl+(uppercase character).
> So for this feature, we should keep the Ctrl+E bind aswell as the
> Ctrl+e bind. If nothing else, to keep it consistent with the rest of
> the hotkey bindings.

I agree with this that we should keep it consistent with the rest of the 
bindings for now...

> But honestly, (as Marc pointed out) it is a quite weird that
> Ctrl+Shift+(character) has the excact same function as
> Ctrl+(character). Perhaps we should find another way to bind the
> hotkeys, where the state of Caps Lock doesn't matter? If possible.

...but I'd love to see this happen. To me shift is a modifier. No matter 
whether Caps Lock is pressed or not, it should not do the shift-modified 
behavior (that's just me, maybe other people think differently).

AFAIK, Tk does not provide any direct way to find out whether shift is 
pressed (correct me if I'm wrong). What you instead have to do is some 
bit arithmetic on the number passed to the "Key" event via the "%s" 
substitution. Source: [0]. We can probably have a bind_alpha procedure 
that takes two arguments: what to run when shift is pressed and what to 
run when it isn't.

This, of course, would be incompatible with the current behavior, but do 
people even keep the Caps Lock on? I personally use it so rarely I have 
my Caps Lock bound to Escape because I might as well use that key for 
something I use more often.

[0] https://blog.tcl.tk/4238

Patch
diff mbox series

diff --git a/git-gui.sh b/git-gui.sh
index 80a07d5..8b776dd 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -2640,6 +2640,12 @@  proc show_less_context {} {
 	}
 }
 
+proc toggle_commit_type {} {
+	global commit_type_is_amend
+	set commit_type_is_amend [expr !$commit_type_is_amend]
+	do_select_commit_type
+}
+
 ######################################################################
 ##
 ## ui construction
@@ -2830,6 +2836,7 @@  if {[is_enabled multicommit] || [is_enabled singlecommit]} {
 	if {![is_enabled nocommit]} {
 		.mbar.commit add checkbutton \
 			-label [mc "Amend Last Commit"] \
+			-accelerator $M1T-E \
 			-variable commit_type_is_amend \
 			-command do_select_commit_type
 		lappend disable_on_lock \
@@ -3825,6 +3832,7 @@  bind .   <$M1B-Key-equal> {show_more_context;break}
 bind .   <$M1B-Key-plus> {show_more_context;break}
 bind .   <$M1B-Key-KP_Add> {show_more_context;break}
 bind .   <$M1B-Key-Return> do_commit
+bind .   <$M1B-Key-e> toggle_commit_type
 foreach i [list $ui_index $ui_workdir] {
 	bind $i <Button-1>       { toggle_or_diff click %W %x %y; break }
 	bind $i <$M1B-Button-1>  { add_one_to_selection %W %x %y; break }