[PATCH 1/2] git-gui: convert new/amend commit radiobutton to checketton

[PATCH 1/2] git-gui: convert new/amend commit radiobutton to checketton

[Bert Wesarg]

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
 git-gui.sh          | 36 +++++++++---------------------------
 lib/checkout_op.tcl |  6 +++---
 lib/commit.tcl      |  4 ++--
 lib/index.tcl       |  8 ++++----
 4 files changed, 18 insertions(+), 36 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index 5bc21b8..80a07d5 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -1341,6 +1341,7 @@ set HEAD {}
 set PARENT {}
 set MERGE_HEAD [list]
 set commit_type {}
+set commit_type_is_amend 0
 set empty_tree {}
 set current_branch {}
 set is_detached 0
@@ -1348,7 +1349,6 @@ set current_diff_path {}
 set is_3way_diff 0
 set is_submodule_diff 0
 set is_conflict_diff 0
-set selected_commit_type new
 set diff_empty_count 0
 
 set nullid "0000000000000000000000000000000000000000"
@@ -1435,7 +1435,7 @@ proc PARENT {} {
 }
 
 proc force_amend {} {
-	global selected_commit_type
+	global commit_type_is_amend
 	global HEAD PARENT MERGE_HEAD commit_type
 
 	repository_state newType newHEAD newMERGE_HEAD
@@ -1444,7 +1444,7 @@ proc force_amend {} {
 	set MERGE_HEAD $newMERGE_HEAD
 	set commit_type $newType
 
-	set selected_commit_type amend
+	set commit_type_is_amend 1
 	do_select_commit_type
 }
 
@@ -2828,19 +2828,10 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} {
 	menu .mbar.commit
 
 	if {![is_enabled nocommit]} {
-		.mbar.commit add radiobutton \
-			-label [mc "New Commit"] \
-			-command do_select_commit_type \
-			-variable selected_commit_type \
-			-value new
-		lappend disable_on_lock \
-			[list .mbar.commit entryconf [.mbar.commit index last] -state]
-
-		.mbar.commit add radiobutton \
+		.mbar.commit add checkbutton \
 			-label [mc "Amend Last Commit"] \
-			-command do_select_commit_type \
-			-variable selected_commit_type \
-			-value amend
+			-variable commit_type_is_amend \
+			-command do_select_commit_type
 		lappend disable_on_lock \
 			[list .mbar.commit entryconf [.mbar.commit index last] -state]
 
@@ -3313,18 +3304,10 @@ set ui_comm .vpane.lower.commarea.buffer.frame.t
 set ui_coml .vpane.lower.commarea.buffer.header.l
 
 if {![is_enabled nocommit]} {
-	${NS}::radiobutton .vpane.lower.commarea.buffer.header.new \
-		-text [mc "New Commit"] \
-		-command do_select_commit_type \
-		-variable selected_commit_type \
-		-value new
-	lappend disable_on_lock \
-		[list .vpane.lower.commarea.buffer.header.new conf -state]
-	${NS}::radiobutton .vpane.lower.commarea.buffer.header.amend \
+	${NS}::checkbutton .vpane.lower.commarea.buffer.header.amend \
 		-text [mc "Amend Last Commit"] \
-		-command do_select_commit_type \
-		-variable selected_commit_type \
-		-value amend
+		-variable commit_type_is_amend \
+		-command do_select_commit_type
 	lappend disable_on_lock \
 		[list .vpane.lower.commarea.buffer.header.amend conf -state]
 }
@@ -3349,7 +3332,6 @@ pack $ui_coml -side left -fill x
 
 if {![is_enabled nocommit]} {
 	pack .vpane.lower.commarea.buffer.header.amend -side right
-	pack .vpane.lower.commarea.buffer.header.new -side right
 }
 
 textframe .vpane.lower.commarea.buffer.frame
diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl
index 9e7412c..a522829 100644
--- a/lib/checkout_op.tcl
+++ b/lib/checkout_op.tcl
@@ -389,7 +389,7 @@ $err
 }
 
 method _after_readtree {} {
-	global selected_commit_type commit_type HEAD MERGE_HEAD PARENT
+	global commit_type HEAD MERGE_HEAD PARENT
 	global current_branch is_detached
 	global ui_comm
 
@@ -490,12 +490,12 @@ method _update_repo_state {} {
 	#    amend mode our file lists are accurate and we can avoid
 	#    the rescan.
 	#
-	global selected_commit_type commit_type HEAD MERGE_HEAD PARENT
+	global commit_type_is_amend commit_type HEAD MERGE_HEAD PARENT
 	global ui_comm
 
 	unlock_index
 	set name [_name $this]
-	set selected_commit_type new
+	set commit_type_is_amend 0
 	if {[string match amend* $commit_type]} {
 		$ui_comm delete 0.0 end
 		$ui_comm edit reset
diff --git a/lib/commit.tcl b/lib/commit.tcl
index 83620b7..384f18f 100644
--- a/lib/commit.tcl
+++ b/lib/commit.tcl
@@ -327,7 +327,7 @@ proc commit_writetree {curHEAD msg_p} {
 proc commit_committree {fd_wt curHEAD msg_p} {
 	global HEAD PARENT MERGE_HEAD commit_type commit_author
 	global current_branch
-	global ui_comm selected_commit_type
+	global ui_comm commit_type_is_amend
 	global file_states selected_paths rescan_active
 	global repo_config
 	global env
@@ -461,8 +461,8 @@ A rescan will be automatically started now.
 
 	# -- Update in memory status
 	#
-	set selected_commit_type new
 	set commit_type normal
+	set commit_type_is_amend 0
 	set HEAD $cmt_id
 	set PARENT $cmt_id
 	set MERGE_HEAD [list]
diff --git a/lib/index.tcl b/lib/index.tcl
index b588db1..e07b7a3 100644
--- a/lib/index.tcl
+++ b/lib/index.tcl
@@ -466,19 +466,19 @@ proc do_revert_selection {} {
 }
 
 proc do_select_commit_type {} {
-	global commit_type selected_commit_type
+	global commit_type commit_type_is_amend
 
-	if {$selected_commit_type eq {new}
+	if {$commit_type_is_amend == 0
 		&& [string match amend* $commit_type]} {
 		create_new_commit
-	} elseif {$selected_commit_type eq {amend}
+	} elseif {$commit_type_is_amend == 1
 		&& ![string match amend* $commit_type]} {
 		load_last_commit
 
 		# The amend request was rejected...
 		#
 		if {![string match amend* $commit_type]} {
-			set selected_commit_type new
+			set commit_type_is_amend 0
 		}
 	}
 }
-- 
2.21.0.789.ga095d9d866

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

[Bert Wesarg]

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
+}
+
 ######################################################################
 ##
 ## 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 }
-- 
2.21.0.789.ga095d9d866

Re: [PATCH 1/2] git-gui: convert new/amend commit radiobutton to checketton

[Pratyush Yadav]

Hi Bert,

I have some _very_ busy few days coming up, so I won't be able to devote 
much time to this right now. I will take a look at the series and your 
other patches by the coming Wednesday. Thanks.

[snip]

-- 
Regards,
Pratyush Yadav

Re: [PATCH 1/2] git-gui: convert new/amend commit radiobutton to checketton

[Pratyush Yadav]

Typo in the subject. s/checketton/checkbutton/

On 05/09/19 10:09PM, Bert Wesarg wrote:
> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> ---
>  git-gui.sh          | 36 +++++++++---------------------------
>  lib/checkout_op.tcl |  6 +++---
>  lib/commit.tcl      |  4 ++--
>  lib/index.tcl       |  8 ++++----
>  4 files changed, 18 insertions(+), 36 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 5bc21b8..80a07d5 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -1341,6 +1341,7 @@ set HEAD {}
>  set PARENT {}
>  set MERGE_HEAD [list]
>  set commit_type {}
> +set commit_type_is_amend 0
>  set empty_tree {}
>  set current_branch {}
>  set is_detached 0
> @@ -1348,7 +1349,6 @@ set current_diff_path {}
>  set is_3way_diff 0
>  set is_submodule_diff 0
>  set is_conflict_diff 0
> -set selected_commit_type new
>  set diff_empty_count 0
>  
>  set nullid "0000000000000000000000000000000000000000"
> @@ -1435,7 +1435,7 @@ proc PARENT {} {
>  }
>  
>  proc force_amend {} {
> -	global selected_commit_type
> +	global commit_type_is_amend
>  	global HEAD PARENT MERGE_HEAD commit_type
>  
>  	repository_state newType newHEAD newMERGE_HEAD
> @@ -1444,7 +1444,7 @@ proc force_amend {} {
>  	set MERGE_HEAD $newMERGE_HEAD
>  	set commit_type $newType
>  
> -	set selected_commit_type amend
> +	set commit_type_is_amend 1

Why do we need a separate variable for this? Why not just check 
commit_type to know whether it is an amend or not? My guess after 
reading the patch is that we need to associate a variable with the 
checkbutton to decide its state, and since there are multiple types of 
amend that commit_type can be (amend, amend-initial, amend-merge), it is 
not easy to use it directly. Am I guessing correctly?

>  	do_select_commit_type
>  }
>  
> @@ -2828,19 +2828,10 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} {
>  	menu .mbar.commit
>  
>  	if {![is_enabled nocommit]} {
> -		.mbar.commit add radiobutton \
> -			-label [mc "New Commit"] \
> -			-command do_select_commit_type \
> -			-variable selected_commit_type \
> -			-value new
> -		lappend disable_on_lock \
> -			[list .mbar.commit entryconf [.mbar.commit index last] -state]
> -
> -		.mbar.commit add radiobutton \
> +		.mbar.commit add checkbutton \
>  			-label [mc "Amend Last Commit"] \
> -			-command do_select_commit_type \
> -			-variable selected_commit_type \
> -			-value amend
> +			-variable commit_type_is_amend \
> +			-command do_select_commit_type
>  		lappend disable_on_lock \
>  			[list .mbar.commit entryconf [.mbar.commit index last] -state]
>  
> @@ -3313,18 +3304,10 @@ set ui_comm .vpane.lower.commarea.buffer.frame.t
>  set ui_coml .vpane.lower.commarea.buffer.header.l
>  
>  if {![is_enabled nocommit]} {
> -	${NS}::radiobutton .vpane.lower.commarea.buffer.header.new \
> -		-text [mc "New Commit"] \
> -		-command do_select_commit_type \
> -		-variable selected_commit_type \
> -		-value new
> -	lappend disable_on_lock \
> -		[list .vpane.lower.commarea.buffer.header.new conf -state]
> -	${NS}::radiobutton .vpane.lower.commarea.buffer.header.amend \
> +	${NS}::checkbutton .vpane.lower.commarea.buffer.header.amend \
>  		-text [mc "Amend Last Commit"] \
> -		-command do_select_commit_type \
> -		-variable selected_commit_type \
> -		-value amend
> +		-variable commit_type_is_amend \
> +		-command do_select_commit_type
>  	lappend disable_on_lock \
>  		[list .vpane.lower.commarea.buffer.header.amend conf -state]
>  }
> @@ -3349,7 +3332,6 @@ pack $ui_coml -side left -fill x
>  
>  if {![is_enabled nocommit]} {
>  	pack .vpane.lower.commarea.buffer.header.amend -side right
> -	pack .vpane.lower.commarea.buffer.header.new -side right
>  }
>  
>  textframe .vpane.lower.commarea.buffer.frame
> diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl
> index 9e7412c..a522829 100644
> --- a/lib/checkout_op.tcl
> +++ b/lib/checkout_op.tcl
> @@ -389,7 +389,7 @@ $err
>  }
>  
>  method _after_readtree {} {
> -	global selected_commit_type commit_type HEAD MERGE_HEAD PARENT
> +	global commit_type HEAD MERGE_HEAD PARENT

This cleans up an unused variable declaration. Nice.

>  	global current_branch is_detached
>  	global ui_comm
>  
> @@ -490,12 +490,12 @@ method _update_repo_state {} {
>  	#    amend mode our file lists are accurate and we can avoid
>  	#    the rescan.
>  	#
> -	global selected_commit_type commit_type HEAD MERGE_HEAD PARENT
> +	global commit_type_is_amend commit_type HEAD MERGE_HEAD PARENT
>  	global ui_comm
>  
>  	unlock_index
>  	set name [_name $this]
> -	set selected_commit_type new
> +	set commit_type_is_amend 0
>  	if {[string match amend* $commit_type]} {
>  		$ui_comm delete 0.0 end
>  		$ui_comm edit reset
> diff --git a/lib/commit.tcl b/lib/commit.tcl
> index 83620b7..384f18f 100644
> --- a/lib/commit.tcl
> +++ b/lib/commit.tcl
> @@ -327,7 +327,7 @@ proc commit_writetree {curHEAD msg_p} {
>  proc commit_committree {fd_wt curHEAD msg_p} {
>  	global HEAD PARENT MERGE_HEAD commit_type commit_author
>  	global current_branch
> -	global ui_comm selected_commit_type
> +	global ui_comm commit_type_is_amend
>  	global file_states selected_paths rescan_active
>  	global repo_config
>  	global env
> @@ -461,8 +461,8 @@ A rescan will be automatically started now.
>  
>  	# -- Update in memory status
>  	#
> -	set selected_commit_type new
>  	set commit_type normal
> +	set commit_type_is_amend 0
>  	set HEAD $cmt_id
>  	set PARENT $cmt_id
>  	set MERGE_HEAD [list]
> diff --git a/lib/index.tcl b/lib/index.tcl
> index b588db1..e07b7a3 100644
> --- a/lib/index.tcl
> +++ b/lib/index.tcl
> @@ -466,19 +466,19 @@ proc do_revert_selection {} {
>  }
>  
>  proc do_select_commit_type {} {
> -	global commit_type selected_commit_type
> +	global commit_type commit_type_is_amend
>  
> -	if {$selected_commit_type eq {new}
> +	if {$commit_type_is_amend == 0
>  		&& [string match amend* $commit_type]} {
>  		create_new_commit
> -	} elseif {$selected_commit_type eq {amend}
> +	} elseif {$commit_type_is_amend == 1
>  		&& ![string match amend* $commit_type]} {

Not exactly related to your change, but shouldn't these "string match 
amend*" in the two ifs be assertions instead of checks? If 
$commit_type_is_amend == 0, then $commit_type should _always_ be amend*, 
and if $commit_type_is_amend == 1, then $commit_type should _never_ be 
amend*. 

I don't see assertions being used anywhere, but I suppose we should look 
into them in the future. It would be great if you can start using 
something like that here, but I'm fine with keeping this like it is 
right now too.

>  		load_last_commit
>  
>  		# The amend request was rejected...
>  		#
>  		if {![string match amend* $commit_type]} {
> -			set selected_commit_type new
> +			set commit_type_is_amend 0
>  		}
>  	}
>  }

I tested it on my setup and it works fine. Thanks.

-- 
Regards,
Pratyush Yadav

Re: [PATCH 2/2] git-gui: add hotkey to toggle "Amend Last Commit" check button/menu

[Pratyush Yadav]

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.

-- 
Regards,
Pratyush Yadav

Re: [PATCH 2/2] git-gui: add hotkey to toggle "Amend Last Commit" check button/menu

[Birger Skogeng Pedersen]

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

Re: [PATCH 2/2] git-gui: add hotkey to toggle "Amend Last Commit" check button/menu

[Pratyush Yadav]

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.

-- 
Regards,
Pratyush Yadav

[PATCH v3] git-gui: add hotkey to toggle "Amend Last Commit"

[Birger Skogeng Pedersen]

Selecting whether to do a "New Commit" or "Amend Last Commit" 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>
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
 git-gui.sh | 41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index 5bc21b8..ebe267f 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -1445,7 +1445,7 @@ proc force_amend {} {
 	set commit_type $newType
 
 	set selected_commit_type amend
-	do_select_commit_type
+	ui_select_commit_type
 }
 
 proc rescan {after {honor_trustmtime 1}} {
@@ -2640,6 +2640,16 @@ proc show_less_context {} {
 	}
 }
 
+proc toggle_commit_type {} {
+	global selected_commit_type
+	if {[string match amend* $selected_commit_type]} {
+		set selected_commit_type new
+	} else {
+		set selected_commit_type amend
+	}
+	ui_select_commit_type
+}
+
 ######################################################################
 ##
 ## ui construction
@@ -2824,13 +2834,31 @@ proc commit_btn_caption {} {
 	}
 }
 
+proc ui_select_commit_type {} {
+	global selected_commit_type
+	global ui_commit_type_commit ui_commit_type_amend
+
+	do_select_commit_type
+	if {$selected_commit_type eq {new}} {
+		.mbar.commit entryconf [mc "New Commit"] \
+			-accelerator {}
+		.mbar.commit entryconf [mc "Amend Last Commit"] \
+			-accelerator $::M1T-E
+	} elseif {$selected_commit_type eq {amend}} {
+		.mbar.commit entryconf [mc "New Commit"] \
+			-accelerator $::M1T-E
+		.mbar.commit entryconf [mc "Amend Last Commit"] \
+			-accelerator {}
+	}
+}
+
 if {[is_enabled multicommit] || [is_enabled singlecommit]} {
 	menu .mbar.commit
 
 	if {![is_enabled nocommit]} {
 		.mbar.commit add radiobutton \
 			-label [mc "New Commit"] \
-			-command do_select_commit_type \
+			-command ui_select_commit_type \
 			-variable selected_commit_type \
 			-value new
 		lappend disable_on_lock \
@@ -2838,7 +2866,8 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} {
 
 		.mbar.commit add radiobutton \
 			-label [mc "Amend Last Commit"] \
-			-command do_select_commit_type \
+			-accelerator $M1T-E \
+			-command ui_select_commit_type \
 			-variable selected_commit_type \
 			-value amend
 		lappend disable_on_lock \
@@ -3315,14 +3344,14 @@ set ui_coml .vpane.lower.commarea.buffer.header.l
 if {![is_enabled nocommit]} {
 	${NS}::radiobutton .vpane.lower.commarea.buffer.header.new \
 		-text [mc "New Commit"] \
-		-command do_select_commit_type \
+		-command ui_select_commit_type \
 		-variable selected_commit_type \
 		-value new
 	lappend disable_on_lock \
 		[list .vpane.lower.commarea.buffer.header.new conf -state]
 	${NS}::radiobutton .vpane.lower.commarea.buffer.header.amend \
 		-text [mc "Amend Last Commit"] \
-		-command do_select_commit_type \
+		-command ui_select_commit_type \
 		-variable selected_commit_type \
 		-value amend
 	lappend disable_on_lock \
@@ -3837,6 +3866,8 @@ bind .   <$M1B-Key-j> do_revert_selection
 bind .   <$M1B-Key-J> do_revert_selection
 bind .   <$M1B-Key-i> do_add_all
 bind .   <$M1B-Key-I> do_add_all
+bind .   <$M1B-Key-e> toggle_commit_type
+bind .   <$M1B-Key-E> toggle_commit_type
 bind .   <$M1B-Key-minus> {show_less_context;break}
 bind .   <$M1B-Key-KP_Subtract> {show_less_context;break}
 bind .   <$M1B-Key-equal> {show_more_context;break}
-- 
2.21.0.windows.1

Re: [PATCH 1/2] git-gui: convert new/amend commit radiobutton to checketton

[Bert Wesarg]

On Wed, Sep 11, 2019 at 10:15 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
>
> Typo in the subject. s/checketton/checkbutton/
>
> On 05/09/19 10:09PM, Bert Wesarg wrote:
> > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> > ---
> >  git-gui.sh          | 36 +++++++++---------------------------
> >  lib/checkout_op.tcl |  6 +++---
> >  lib/commit.tcl      |  4 ++--
> >  lib/index.tcl       |  8 ++++----
> >  4 files changed, 18 insertions(+), 36 deletions(-)
> >
> > diff --git a/git-gui.sh b/git-gui.sh
> > index 5bc21b8..80a07d5 100755
> > --- a/git-gui.sh
> > +++ b/git-gui.sh
> > @@ -1341,6 +1341,7 @@ set HEAD {}
> >  set PARENT {}
> >  set MERGE_HEAD [list]
> >  set commit_type {}
> > +set commit_type_is_amend 0
> >  set empty_tree {}
> >  set current_branch {}
> >  set is_detached 0
> > @@ -1348,7 +1349,6 @@ set current_diff_path {}
> >  set is_3way_diff 0
> >  set is_submodule_diff 0
> >  set is_conflict_diff 0
> > -set selected_commit_type new
> >  set diff_empty_count 0
> >
> >  set nullid "0000000000000000000000000000000000000000"
> > @@ -1435,7 +1435,7 @@ proc PARENT {} {
> >  }
> >
> >  proc force_amend {} {
> > -     global selected_commit_type
> > +     global commit_type_is_amend
> >       global HEAD PARENT MERGE_HEAD commit_type
> >
> >       repository_state newType newHEAD newMERGE_HEAD
> > @@ -1444,7 +1444,7 @@ proc force_amend {} {
> >       set MERGE_HEAD $newMERGE_HEAD
> >       set commit_type $newType
> >
> > -     set selected_commit_type amend
> > +     set commit_type_is_amend 1
>
> Why do we need a separate variable for this? Why not just check
> commit_type to know whether it is an amend or not? My guess after
> reading the patch is that we need to associate a variable with the
> checkbutton to decide its state, and since there are multiple types of
> amend that commit_type can be (amend, amend-initial, amend-merge), it is
> not easy to use it directly. Am I guessing correctly?
>
> >       do_select_commit_type
> >  }
> >
> > @@ -2828,19 +2828,10 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} {
> >       menu .mbar.commit
> >
> >       if {![is_enabled nocommit]} {
> > -             .mbar.commit add radiobutton \
> > -                     -label [mc "New Commit"] \
> > -                     -command do_select_commit_type \
> > -                     -variable selected_commit_type \
> > -                     -value new
> > -             lappend disable_on_lock \
> > -                     [list .mbar.commit entryconf [.mbar.commit index last] -state]
> > -
> > -             .mbar.commit add radiobutton \
> > +             .mbar.commit add checkbutton \
> >                       -label [mc "Amend Last Commit"] \
> > -                     -command do_select_commit_type \
> > -                     -variable selected_commit_type \
> > -                     -value amend
> > +                     -variable commit_type_is_amend \
> > +                     -command do_select_commit_type
> >               lappend disable_on_lock \
> >                       [list .mbar.commit entryconf [.mbar.commit index last] -state]
> >
> > @@ -3313,18 +3304,10 @@ set ui_comm .vpane.lower.commarea.buffer.frame.t
> >  set ui_coml .vpane.lower.commarea.buffer.header.l
> >
> >  if {![is_enabled nocommit]} {
> > -     ${NS}::radiobutton .vpane.lower.commarea.buffer.header.new \
> > -             -text [mc "New Commit"] \
> > -             -command do_select_commit_type \
> > -             -variable selected_commit_type \
> > -             -value new
> > -     lappend disable_on_lock \
> > -             [list .vpane.lower.commarea.buffer.header.new conf -state]
> > -     ${NS}::radiobutton .vpane.lower.commarea.buffer.header.amend \
> > +     ${NS}::checkbutton .vpane.lower.commarea.buffer.header.amend \
> >               -text [mc "Amend Last Commit"] \
> > -             -command do_select_commit_type \
> > -             -variable selected_commit_type \
> > -             -value amend
> > +             -variable commit_type_is_amend \
> > +             -command do_select_commit_type
> >       lappend disable_on_lock \
> >               [list .vpane.lower.commarea.buffer.header.amend conf -state]
> >  }
> > @@ -3349,7 +3332,6 @@ pack $ui_coml -side left -fill x
> >
> >  if {![is_enabled nocommit]} {
> >       pack .vpane.lower.commarea.buffer.header.amend -side right
> > -     pack .vpane.lower.commarea.buffer.header.new -side right
> >  }
> >
> >  textframe .vpane.lower.commarea.buffer.frame
> > diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl
> > index 9e7412c..a522829 100644
> > --- a/lib/checkout_op.tcl
> > +++ b/lib/checkout_op.tcl
> > @@ -389,7 +389,7 @@ $err
> >  }
> >
> >  method _after_readtree {} {
> > -     global selected_commit_type commit_type HEAD MERGE_HEAD PARENT
> > +     global commit_type HEAD MERGE_HEAD PARENT
>
> This cleans up an unused variable declaration. Nice.
>
> >       global current_branch is_detached
> >       global ui_comm
> >
> > @@ -490,12 +490,12 @@ method _update_repo_state {} {
> >       #    amend mode our file lists are accurate and we can avoid
> >       #    the rescan.
> >       #
> > -     global selected_commit_type commit_type HEAD MERGE_HEAD PARENT
> > +     global commit_type_is_amend commit_type HEAD MERGE_HEAD PARENT
> >       global ui_comm
> >
> >       unlock_index
> >       set name [_name $this]
> > -     set selected_commit_type new
> > +     set commit_type_is_amend 0
> >       if {[string match amend* $commit_type]} {
> >               $ui_comm delete 0.0 end
> >               $ui_comm edit reset
> > diff --git a/lib/commit.tcl b/lib/commit.tcl
> > index 83620b7..384f18f 100644
> > --- a/lib/commit.tcl
> > +++ b/lib/commit.tcl
> > @@ -327,7 +327,7 @@ proc commit_writetree {curHEAD msg_p} {
> >  proc commit_committree {fd_wt curHEAD msg_p} {
> >       global HEAD PARENT MERGE_HEAD commit_type commit_author
> >       global current_branch
> > -     global ui_comm selected_commit_type
> > +     global ui_comm commit_type_is_amend
> >       global file_states selected_paths rescan_active
> >       global repo_config
> >       global env
> > @@ -461,8 +461,8 @@ A rescan will be automatically started now.
> >
> >       # -- Update in memory status
> >       #
> > -     set selected_commit_type new
> >       set commit_type normal
> > +     set commit_type_is_amend 0
> >       set HEAD $cmt_id
> >       set PARENT $cmt_id
> >       set MERGE_HEAD [list]
> > diff --git a/lib/index.tcl b/lib/index.tcl
> > index b588db1..e07b7a3 100644
> > --- a/lib/index.tcl
> > +++ b/lib/index.tcl
> > @@ -466,19 +466,19 @@ proc do_revert_selection {} {
> >  }
> >
> >  proc do_select_commit_type {} {
> > -     global commit_type selected_commit_type
> > +     global commit_type commit_type_is_amend
> >
> > -     if {$selected_commit_type eq {new}
> > +     if {$commit_type_is_amend == 0
> >               && [string match amend* $commit_type]} {
> >               create_new_commit
> > -     } elseif {$selected_commit_type eq {amend}
> > +     } elseif {$commit_type_is_amend == 1
> >               && ![string match amend* $commit_type]} {
>
> Not exactly related to your change, but shouldn't these "string match
> amend*" in the two ifs be assertions instead of checks? If
> $commit_type_is_amend == 0, then $commit_type should _always_ be amend*,
> and if $commit_type_is_amend == 1, then $commit_type should _never_ be
> amend*.
>

AFAIU this now is, that the former 'selected_commit_type' was also
used as a request for the commit type, you set it to the desired one,
than call do_select_commit_type, it will than check if a state change
actually happen, and if it was to request an amend, which may also
fail, it goes back to !amend.

Thus its not an assert.

> I don't see assertions being used anywhere, but I suppose we should look
> into them in the future. It would be great if you can start using
> something like that here, but I'm fine with keeping this like it is
> right now too.
>
> >               load_last_commit
> >
> >               # The amend request was rejected...
> >               #
> >               if {![string match amend* $commit_type]} {
> > -                     set selected_commit_type new
> > +                     set commit_type_is_amend 0
> >               }
> >       }
> >  }
>
> I tested it on my setup and it works fine. Thanks.
>
> --
> Regards,
> Pratyush Yadav

Re: [PATCH 1/2] git-gui: convert new/amend commit radiobutton to checketton

[Bert Wesarg]

On Wed, Sep 11, 2019 at 10:15 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
>
> Typo in the subject. s/checketton/checkbutton/\

Will re-roll and drop the actual keybinding patch, so that Birger can
resend his part,

Bert

>
> On 05/09/19 10:09PM, Bert Wesarg wrote:
> > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> > ---
> >  git-gui.sh          | 36 +++++++++---------------------------
> >  lib/checkout_op.tcl |  6 +++---
> >  lib/commit.tcl      |  4 ++--
> >  lib/index.tcl       |  8 ++++----
> >  4 files changed, 18 insertions(+), 36 deletions(-)
> >
> > diff --git a/git-gui.sh b/git-gui.sh
> > index 5bc21b8..80a07d5 100755
> > --- a/git-gui.sh
> > +++ b/git-gui.sh
> > @@ -1341,6 +1341,7 @@ set HEAD {}
> >  set PARENT {}
> >  set MERGE_HEAD [list]
> >  set commit_type {}
> > +set commit_type_is_amend 0
> >  set empty_tree {}
> >  set current_branch {}
> >  set is_detached 0
> > @@ -1348,7 +1349,6 @@ set current_diff_path {}
> >  set is_3way_diff 0
> >  set is_submodule_diff 0
> >  set is_conflict_diff 0
> > -set selected_commit_type new
> >  set diff_empty_count 0
> >
> >  set nullid "0000000000000000000000000000000000000000"
> > @@ -1435,7 +1435,7 @@ proc PARENT {} {
> >  }
> >
> >  proc force_amend {} {
> > -     global selected_commit_type
> > +     global commit_type_is_amend
> >       global HEAD PARENT MERGE_HEAD commit_type
> >
> >       repository_state newType newHEAD newMERGE_HEAD
> > @@ -1444,7 +1444,7 @@ proc force_amend {} {
> >       set MERGE_HEAD $newMERGE_HEAD
> >       set commit_type $newType
> >
> > -     set selected_commit_type amend
> > +     set commit_type_is_amend 1
>
> Why do we need a separate variable for this? Why not just check
> commit_type to know whether it is an amend or not? My guess after
> reading the patch is that we need to associate a variable with the
> checkbutton to decide its state, and since there are multiple types of
> amend that commit_type can be (amend, amend-initial, amend-merge), it is
> not easy to use it directly. Am I guessing correctly?
>
> >       do_select_commit_type
> >  }
> >
> > @@ -2828,19 +2828,10 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} {
> >       menu .mbar.commit
> >
> >       if {![is_enabled nocommit]} {
> > -             .mbar.commit add radiobutton \
> > -                     -label [mc "New Commit"] \
> > -                     -command do_select_commit_type \
> > -                     -variable selected_commit_type \
> > -                     -value new
> > -             lappend disable_on_lock \
> > -                     [list .mbar.commit entryconf [.mbar.commit index last] -state]
> > -
> > -             .mbar.commit add radiobutton \
> > +             .mbar.commit add checkbutton \
> >                       -label [mc "Amend Last Commit"] \
> > -                     -command do_select_commit_type \
> > -                     -variable selected_commit_type \
> > -                     -value amend
> > +                     -variable commit_type_is_amend \
> > +                     -command do_select_commit_type
> >               lappend disable_on_lock \
> >                       [list .mbar.commit entryconf [.mbar.commit index last] -state]
> >
> > @@ -3313,18 +3304,10 @@ set ui_comm .vpane.lower.commarea.buffer.frame.t
> >  set ui_coml .vpane.lower.commarea.buffer.header.l
> >
> >  if {![is_enabled nocommit]} {
> > -     ${NS}::radiobutton .vpane.lower.commarea.buffer.header.new \
> > -             -text [mc "New Commit"] \
> > -             -command do_select_commit_type \
> > -             -variable selected_commit_type \
> > -             -value new
> > -     lappend disable_on_lock \
> > -             [list .vpane.lower.commarea.buffer.header.new conf -state]
> > -     ${NS}::radiobutton .vpane.lower.commarea.buffer.header.amend \
> > +     ${NS}::checkbutton .vpane.lower.commarea.buffer.header.amend \
> >               -text [mc "Amend Last Commit"] \
> > -             -command do_select_commit_type \
> > -             -variable selected_commit_type \
> > -             -value amend
> > +             -variable commit_type_is_amend \
> > +             -command do_select_commit_type
> >       lappend disable_on_lock \
> >               [list .vpane.lower.commarea.buffer.header.amend conf -state]
> >  }
> > @@ -3349,7 +3332,6 @@ pack $ui_coml -side left -fill x
> >
> >  if {![is_enabled nocommit]} {
> >       pack .vpane.lower.commarea.buffer.header.amend -side right
> > -     pack .vpane.lower.commarea.buffer.header.new -side right
> >  }
> >
> >  textframe .vpane.lower.commarea.buffer.frame
> > diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl
> > index 9e7412c..a522829 100644
> > --- a/lib/checkout_op.tcl
> > +++ b/lib/checkout_op.tcl
> > @@ -389,7 +389,7 @@ $err
> >  }
> >
> >  method _after_readtree {} {
> > -     global selected_commit_type commit_type HEAD MERGE_HEAD PARENT
> > +     global commit_type HEAD MERGE_HEAD PARENT
>
> This cleans up an unused variable declaration. Nice.
>
> >       global current_branch is_detached
> >       global ui_comm
> >
> > @@ -490,12 +490,12 @@ method _update_repo_state {} {
> >       #    amend mode our file lists are accurate and we can avoid
> >       #    the rescan.
> >       #
> > -     global selected_commit_type commit_type HEAD MERGE_HEAD PARENT
> > +     global commit_type_is_amend commit_type HEAD MERGE_HEAD PARENT
> >       global ui_comm
> >
> >       unlock_index
> >       set name [_name $this]
> > -     set selected_commit_type new
> > +     set commit_type_is_amend 0
> >       if {[string match amend* $commit_type]} {
> >               $ui_comm delete 0.0 end
> >               $ui_comm edit reset
> > diff --git a/lib/commit.tcl b/lib/commit.tcl
> > index 83620b7..384f18f 100644
> > --- a/lib/commit.tcl
> > +++ b/lib/commit.tcl
> > @@ -327,7 +327,7 @@ proc commit_writetree {curHEAD msg_p} {
> >  proc commit_committree {fd_wt curHEAD msg_p} {
> >       global HEAD PARENT MERGE_HEAD commit_type commit_author
> >       global current_branch
> > -     global ui_comm selected_commit_type
> > +     global ui_comm commit_type_is_amend
> >       global file_states selected_paths rescan_active
> >       global repo_config
> >       global env
> > @@ -461,8 +461,8 @@ A rescan will be automatically started now.
> >
> >       # -- Update in memory status
> >       #
> > -     set selected_commit_type new
> >       set commit_type normal
> > +     set commit_type_is_amend 0
> >       set HEAD $cmt_id
> >       set PARENT $cmt_id
> >       set MERGE_HEAD [list]
> > diff --git a/lib/index.tcl b/lib/index.tcl
> > index b588db1..e07b7a3 100644
> > --- a/lib/index.tcl
> > +++ b/lib/index.tcl
> > @@ -466,19 +466,19 @@ proc do_revert_selection {} {
> >  }
> >
> >  proc do_select_commit_type {} {
> > -     global commit_type selected_commit_type
> > +     global commit_type commit_type_is_amend
> >
> > -     if {$selected_commit_type eq {new}
> > +     if {$commit_type_is_amend == 0
> >               && [string match amend* $commit_type]} {
> >               create_new_commit
> > -     } elseif {$selected_commit_type eq {amend}
> > +     } elseif {$commit_type_is_amend == 1
> >               && ![string match amend* $commit_type]} {
>
> Not exactly related to your change, but shouldn't these "string match
> amend*" in the two ifs be assertions instead of checks? If
> $commit_type_is_amend == 0, then $commit_type should _always_ be amend*,
> and if $commit_type_is_amend == 1, then $commit_type should _never_ be
> amend*.
>
> I don't see assertions being used anywhere, but I suppose we should look
> into them in the future. It would be great if you can start using
> something like that here, but I'm fine with keeping this like it is
> right now too.
>
> >               load_last_commit
> >
> >               # The amend request was rejected...
> >               #
> >               if {![string match amend* $commit_type]} {
> > -                     set selected_commit_type new
> > +                     set commit_type_is_amend 0
> >               }
> >       }
> >  }
>
> I tested it on my setup and it works fine. Thanks.
>
> --
> Regards,
> Pratyush Yadav

Re: [PATCH 2/2] git-gui: add hotkey to toggle "Amend Last Commit" check button/menu

[Marc Branchaud]

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.
> 

Re: [PATCH 2/2] git-gui: add hotkey to toggle "Amend Last Commit" check button/menu

[Philip Oakley]

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

Re: [PATCH 2/2] git-gui: add hotkey to toggle "Amend Last Commit" check button/menu

[Birger Skogeng Pedersen]

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

Re: [PATCH 2/2] git-gui: add hotkey to toggle "Amend Last Commit" check button/menu

[Marc Branchaud]

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
> 

Re: [PATCH v3] git-gui: add hotkey to toggle "Amend Last Commit"

[Pratyush Yadav]

Hi Birger,

I'm afraid you are working on an older version of this patch. You should 
be re-rolling [0], which works well with Bert's "amend check button" 
change.

[0] https://public-inbox.org/git/b82a00441ff1a6a9cea3fd235c1c33729ec31b71.1567713659.git.bert.wesarg@googlemail.com/

On 12/09/19 08:41PM, Birger Skogeng Pedersen wrote:
> Selecting whether to do a "New Commit" or "Amend Last Commit" 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>
> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> ---
>  git-gui.sh | 41 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 5bc21b8..ebe267f 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -1445,7 +1445,7 @@ proc force_amend {} {
>  	set commit_type $newType
>  
>  	set selected_commit_type amend
> -	do_select_commit_type
> +	ui_select_commit_type
>  }
>  
>  proc rescan {after {honor_trustmtime 1}} {
> @@ -2640,6 +2640,16 @@ proc show_less_context {} {
>  	}
>  }
>  
> +proc toggle_commit_type {} {
> +	global selected_commit_type
> +	if {[string match amend* $selected_commit_type]} {
> +		set selected_commit_type new
> +	} else {
> +		set selected_commit_type amend
> +	}
> +	ui_select_commit_type
> +}
> +
>  ######################################################################
>  ##
>  ## ui construction
> @@ -2824,13 +2834,31 @@ proc commit_btn_caption {} {
>  	}
>  }
>  
> +proc ui_select_commit_type {} {
> +	global selected_commit_type
> +	global ui_commit_type_commit ui_commit_type_amend
> +
> +	do_select_commit_type
> +	if {$selected_commit_type eq {new}} {
> +		.mbar.commit entryconf [mc "New Commit"] \
> +			-accelerator {}
> +		.mbar.commit entryconf [mc "Amend Last Commit"] \
> +			-accelerator $::M1T-E
> +	} elseif {$selected_commit_type eq {amend}} {
> +		.mbar.commit entryconf [mc "New Commit"] \
> +			-accelerator $::M1T-E
> +		.mbar.commit entryconf [mc "Amend Last Commit"] \
> +			-accelerator {}
> +	}
> +}
> +
>  if {[is_enabled multicommit] || [is_enabled singlecommit]} {
>  	menu .mbar.commit
>  
>  	if {![is_enabled nocommit]} {
>  		.mbar.commit add radiobutton \
>  			-label [mc "New Commit"] \
> -			-command do_select_commit_type \
> +			-command ui_select_commit_type \
>  			-variable selected_commit_type \
>  			-value new
>  		lappend disable_on_lock \
> @@ -2838,7 +2866,8 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} {
>  
>  		.mbar.commit add radiobutton \
>  			-label [mc "Amend Last Commit"] \
> -			-command do_select_commit_type \
> +			-accelerator $M1T-E \
> +			-command ui_select_commit_type \
>  			-variable selected_commit_type \
>  			-value amend
>  		lappend disable_on_lock \
> @@ -3315,14 +3344,14 @@ set ui_coml .vpane.lower.commarea.buffer.header.l
>  if {![is_enabled nocommit]} {
>  	${NS}::radiobutton .vpane.lower.commarea.buffer.header.new \
>  		-text [mc "New Commit"] \
> -		-command do_select_commit_type \
> +		-command ui_select_commit_type \
>  		-variable selected_commit_type \
>  		-value new
>  	lappend disable_on_lock \
>  		[list .vpane.lower.commarea.buffer.header.new conf -state]
>  	${NS}::radiobutton .vpane.lower.commarea.buffer.header.amend \
>  		-text [mc "Amend Last Commit"] \
> -		-command do_select_commit_type \
> +		-command ui_select_commit_type \
>  		-variable selected_commit_type \
>  		-value amend
>  	lappend disable_on_lock \
> @@ -3837,6 +3866,8 @@ bind .   <$M1B-Key-j> do_revert_selection
>  bind .   <$M1B-Key-J> do_revert_selection
>  bind .   <$M1B-Key-i> do_add_all
>  bind .   <$M1B-Key-I> do_add_all
> +bind .   <$M1B-Key-e> toggle_commit_type
> +bind .   <$M1B-Key-E> toggle_commit_type
>  bind .   <$M1B-Key-minus> {show_less_context;break}
>  bind .   <$M1B-Key-KP_Subtract> {show_less_context;break}
>  bind .   <$M1B-Key-equal> {show_more_context;break}
> -- 
> 2.21.0.windows.1
> 

-- 
Regards,
Pratyush Yadav

Re: [PATCH 1/2] git-gui: convert new/amend commit radiobutton to checketton

[Pratyush Yadav]

On 12/09/19 09:35PM, Bert Wesarg wrote:
> On Wed, Sep 11, 2019 at 10:15 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> >
> > Typo in the subject. s/checketton/checkbutton/
> >
> > On 05/09/19 10:09PM, Bert Wesarg wrote:
> > > diff --git a/lib/index.tcl b/lib/index.tcl
> > > index b588db1..e07b7a3 100644
> > > --- a/lib/index.tcl
> > > +++ b/lib/index.tcl
> > > @@ -466,19 +466,19 @@ proc do_revert_selection {} {
> > >  }
> > >
> > >  proc do_select_commit_type {} {
> > > -     global commit_type selected_commit_type
> > > +     global commit_type commit_type_is_amend
> > >
> > > -     if {$selected_commit_type eq {new}
> > > +     if {$commit_type_is_amend == 0
> > >               && [string match amend* $commit_type]} {
> > >               create_new_commit
> > > -     } elseif {$selected_commit_type eq {amend}
> > > +     } elseif {$commit_type_is_amend == 1
> > >               && ![string match amend* $commit_type]} {
> >
> > Not exactly related to your change, but shouldn't these "string match
> > amend*" in the two ifs be assertions instead of checks? If
> > $commit_type_is_amend == 0, then $commit_type should _always_ be amend*,
> > and if $commit_type_is_amend == 1, then $commit_type should _never_ be
> > amend*.
> >
> 
> AFAIU this now is, that the former 'selected_commit_type' was also
> used as a request for the commit type, you set it to the desired one,
> than call do_select_commit_type, it will than check if a state change
> actually happen, and if it was to request an amend, which may also
> fail, it goes back to !amend.
> 
> Thus its not an assert.

Thanks for explaining. While I'm not the biggest fan of this design, 
let's just keep it this way for now.

-- 
Regards,
Pratyush Yadav

Re: [PATCH 2/2] git-gui: add hotkey to toggle "Amend Last Commit" check button/menu

[Pratyush Yadav]

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

-- 
Regards,
Pratyush Yadav

[PATCH v4] git-gui: add hotkey to toggle "Amend Last Commit"

[Birger Skogeng Pedersen]

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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/git-gui.sh b/git-gui.sh
index 5bc21b8..d6e4631 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 radiobutton \
 			-label [mc "New Commit"] \
+			-accelerator $M1T-E \
 			-command do_select_commit_type \
 			-variable selected_commit_type \
 			-value new
@@ -3837,6 +3844,8 @@ bind .   <$M1B-Key-j> do_revert_selection
 bind .   <$M1B-Key-J> do_revert_selection
 bind .   <$M1B-Key-i> do_add_all
 bind .   <$M1B-Key-I> do_add_all
+bind .   <$M1B-Key-e> toggle_commit_type
+bind .   <$M1B-Key-E> toggle_commit_type
 bind .   <$M1B-Key-minus> {show_less_context;break}
 bind .   <$M1B-Key-KP_Subtract> {show_less_context;break}
 bind .   <$M1B-Key-equal> {show_more_context;break}
-- 
2.21.0.windows.1

Re: [PATCH v4] git-gui: add hotkey to toggle "Amend Last Commit"

[Birger Skogeng Pedersen]

On Fri, Sep 13, 2019 at 4:37 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> Hi Birger,
>
> I'm afraid you are working on an older version of this patch. You should
> be re-rolling [0], which works well with Bert's "amend check button"
> change.
>
> [0] https://public-inbox.org/git/b82a00441ff1a6a9cea3fd235c1c33729ec31b71.1567713659.git.bert.wesarg@googlemail.com/

Forgive me, I get a little bit confused. Should my patch be based on
"next" or "master" branch?
Also, is it an issue that this patch won't work unless you merge
Bert's 1/2 patch[0]?
Your feedback cannot be too specific, I want to learn how to do this
properly :-)

[0] https://public-inbox.org/git/ab1f68cc8552e405c9d04622be1e728ab81bda17.1567713659.git.bert.wesarg@googlemail.com/


Birger

Re: [PATCH v4] git-gui: add hotkey to toggle "Amend Last Commit"

[Pratyush Yadav]

+Cc Junio so you know what development model I'm using, and comment with 
your thoughts (if you want to).

On 13/09/19 11:32PM, Birger Skogeng Pedersen wrote:
> On Fri, Sep 13, 2019 at 4:37 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > Hi Birger,
> >
> > I'm afraid you are working on an older version of this patch. You should
> > be re-rolling [0], which works well with Bert's "amend check button"
> > change.
> >
> > [0] https://public-inbox.org/git/b82a00441ff1a6a9cea3fd235c1c33729ec31b71.1567713659.git.bert.wesarg@googlemail.com/
> 
> Forgive me, I get a little bit confused. Should my patch be based on
> "next" or "master" branch?

For git-gui, ignore "next" for now. I considered using a model similar 
to Git where patches first get queued to "next" (or "pu" depending on 
how finished they are). And then after some time letting people use 
them, they are merged to "master" which eventually goes in the release. 
But this seems to be too complicated to me without any clear benefit.

I think for now using just "master" for git-gui should be fine, since I 
won't directly release git-gui. Instead I'll periodically ask Junio to 
pull changes from my master. This will be our "release". So essentially 
my "master" for now acts as a place for people involved in development 
to test out all the changes, and then the rest of the world gets the new 
version along with Git's release.

(Junio, you have done this for much longer than I have, so is there a 
major problem with my workflow?)

If all this seems too complicated, just work on top of my "master". The 
rest of it is mostly my problem.

> Also, is it an issue that this patch won't work unless you merge
> Bert's 1/2 patch[0]?

Your patch is dependent on Bert's patch. This means I will have to merge 
his patch first, and then yours. And that makes complete sense. That's 
how dependent changes should work. So no, it is not an issue that this 
patch won't work unless I merge Bert's patch first.

So while my advice above was to work on top of "master", that does not 
apply in this case since your patch is dependent on someone's patch 
which isn't in master yet. So in this specific case, you should base 
your patch on top of Bert's. Otherwise there are two problems:

- Your patch in and of itself makes little sense, it probably would 
  crash or do some unexpected stuff. This hurts people doing a bisect, 
  where if they land on your patch stuff is broken, and they have to 
  manually move a commit up or down to continue.

- Your patch will textually conflict with Bert's. That means I'd first 
  merge Bert's patch since yours doesn't work without his. But then when 
  I merge yours, I'd get a nasty merge conflict that is not easy to 
  resolve and leaves chances for a subtle bug.

> Your feedback cannot be too specific, I want to learn how to do this
> properly :-)

I'm not sure how well I explained this, so feel free to ask more 
questions if I didn't explain something properly :).
 
> [0] https://public-inbox.org/git/ab1f68cc8552e405c9d04622be1e728ab81bda17.1567713659.git.bert.wesarg@googlemail.com/

-- 
Regards,
Pratyush Yadav

Re: [PATCH v4] git-gui: add hotkey to toggle "Amend Last Commit"

[Birger Skogeng Pedersen]

Thanks, I really appreciate you taking time to explain it thoroughly.

On Sat, Sep 14, 2019 at 12:11 AM Pratyush Yadav <me@yadavpratyush.com> wrote:
> So while my advice above was to work on top of "master", that does not
> apply in this case since your patch is dependent on someone's patch
> which isn't in master yet. So in this specific case, you should base
> your patch on top of Bert's.

Okay, I might have to make another re-roll then. I based v4 on master.

Birger

[PATCH v5] git-gui: add hotkey to toggle "Amend Last Commit"

[Birger Skogeng Pedersen]

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 c7d9103..790adf1 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -2664,6 +2664,12 @@ proc focus_widget {widget} {
 	}
 }
 
+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
@@ -3892,6 +3898,8 @@ bind .   <$M1B-Key-j> do_revert_selection
 bind .   <$M1B-Key-J> do_revert_selection
 bind .   <$M1B-Key-i> do_add_all
 bind .   <$M1B-Key-I> do_add_all
+bind .   <$M1B-Key-e> toggle_commit_type
+bind .   <$M1B-Key-E> toggle_commit_type
 bind .   <$M1B-Key-minus> {show_less_context;break}
 bind .   <$M1B-Key-KP_Subtract> {show_less_context;break}
 bind .   <$M1B-Key-equal> {show_more_context;break}
-- 
2.21.0.windows.1

Re: [PATCH v5] git-gui: add hotkey to toggle "Amend Last Commit"

[Pratyush Yadav]

You missed labelling the menu item of "Amend Last Commit" with the 
shortcut, like we do for other menu items bound to a hotkey like F5 for 
rescan, Ctrl-T for stage, etc. I added that locally.

Thanks for the re-roll. Will queue.

On 14/09/19 11:18AM, Birger Skogeng Pedersen wrote:
> 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 c7d9103..790adf1 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -2664,6 +2664,12 @@ proc focus_widget {widget} {
>  	}
>  }
>  
> +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
> @@ -3892,6 +3898,8 @@ bind .   <$M1B-Key-j> do_revert_selection
>  bind .   <$M1B-Key-J> do_revert_selection
>  bind .   <$M1B-Key-i> do_add_all
>  bind .   <$M1B-Key-I> do_add_all
> +bind .   <$M1B-Key-e> toggle_commit_type
> +bind .   <$M1B-Key-E> toggle_commit_type
>  bind .   <$M1B-Key-minus> {show_less_context;break}
>  bind .   <$M1B-Key-KP_Subtract> {show_less_context;break}
>  bind .   <$M1B-Key-equal> {show_more_context;break}
> -- 
> 2.21.0.windows.1
> 

-- 
Regards,
Pratyush Yadav

Re: [PATCH v5] git-gui: add hotkey to toggle "Amend Last Commit"

[Birger Skogeng Pedersen]

Hi,

On Sat, Sep 14, 2019 at 7:48 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> You missed labelling the menu item of "Amend Last Commit" with the
> shortcut, like we do for other menu items bound to a hotkey like F5 for
> rescan, Ctrl-T for stage, etc. I added that locally.

Sorry about that. Thanks!


Birger