| Message ID | b82a00441ff1a6a9cea3fd235c1c33729ec31b71.1567713659.git.bert.wesarg@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] git-gui: convert new/amend commit radiobutton to checketton | expand |
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.
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
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.
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. >
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
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
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 >
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
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 }