gitk: improve keyboard support

Quoted 1 line

+[mc "<%s-C> Create branch on selected commit" $M1T]

... "C"? Which one is it?

The key binding to remove a branch does not make sense to me. It does happen that I have more than one branch on a commit, but there is no way to select which one to remove via the keyboard. I have to use the context menu. This needs more thought IMHO.

At a minimum, separate it out into its own commit.

Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:

Quoted 12 lines

From: Jens Lidestrom <[email protected]>

Signed-off-by Jens Lidestrom <[email protected]>

---
 gitk-git/gitk | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

gitk-git/gitk +0 -0

index 596977abe89..0d83a72a424 100755

gitk-git/gitk proc makewindow {} { · line 2698

Quoted 7 lines

gitk-git/gitk +2 -0

@@ -2692,6 +2692,8 @@ proc makewindow {} {
     bind $ctext <<Selection>> rehighlight_search_results
     bind . <$M1B-t> {resethead [selected_line_id]}
     bind . <$M1B-o> {checkout [selected_line_head] 
 [selected_line_id]}
+    bind . <$M1B-m> {rmbranch [selected_line_head] [selected_line_id] 
 1}
+    bind . <$M1B-b> {mkbranch [selected_line_id]}

"b" vs...

Quoted 16 lines

gitk-git/gitk +0 -0

     for {set i 1} {$i < 10} {incr i} {

         bind . <$M1B-Key-$i> [list go_to_parent $i]
     }

gitk-git/gitk +1 -1

@@ -2735,7 +2737,7 @@ proc makewindow {} {
     makemenu $headctxmenu {
         {mc "Check out this branch" command {checkout $headmenuhead 
 $headmenuid}}
         {mc "Rename this branch" command mvbranch}
-        {mc "Remove this branch" command rmbranch}
+        {mc "Remove this branch" command {rmbranch $headmenuhead 
 $headmenuid 0}}
         {mc "Copy branch name" command {clipboard clear; clipboard 

append $headmenuhead}}
}
$headctxmenu configure -tearoff 0

gitk-git/gitk proc keys {} { · line 3190

Quoted 4 lines

gitk-git/gitk +1 -0

@@ -3185,6 +3187,8 @@ proc keys {} {
 [mc "<F5>		Update"]
 [mc "<%s-T>		Reset current branch to selected commit" $M1T]
 [mc "<%s-O>		Check out selected commit" $M1T]
+[mc "<%s-C>		Create branch on selected commit" $M1T]

... "C"? Which one is it?

Quoted 52 lines

gitk-git/gitk +10 -8

+[mc "<%s-M>		Remove selected branch" $M1T]
 " \
             -justify left -bg $bgcolor -border 2 -relief groove
     pack $w.m -side top -fill both -padx 2 -pady 2

@@ -9576,13 +9580,13 @@ proc wrcomcan {} {
     unset wrcomtop
 }
 
-proc mkbranch {} {
-    global NS rowmenuid
+proc mkbranch {id} {
+    global NS
 
     set top .branchdialog
 
     set val(name) ""
-    set val(id) $rowmenuid
+    set val(id) $id
     set val(command) [list mkbrgo $top]
 
     set ui(title) [mc "Create branch"]

@@ -10054,13 +10058,14 @@ proc readcheckoutstat {fd newhead newheadref 
 newheadid} {
     }
 }
 
-proc rmbranch {} {
-    global headmenuid headmenuhead mainhead
+proc rmbranch {head id shouldComfirm} {
+    global mainhead
     global idheads
-
-    set head $headmenuhead
-    set id $headmenuid
     # this check shouldn't be needed any more...
+    if {$head eq ""} {
+        error_popup [mc "Cannot delete a detached head"]
+        return
+    }
     if {$head eq $mainhead} {
         error_popup [mc "Cannot delete the currently checked-out 

branch"]
return

gitk-git/gitk +0 -0

@@ -10070,6 +10075,8 @@ proc rmbranch {} {
         # the stuff on this branch isn't on any other branch
         if {![confirm_popup [mc "The commits on branch %s aren't on 
 any other\
                         branch.\nReally delete branch %s?" $head 

$head]]} return

gitk-git/gitk +2 -0

+    } elseif {$shouldComfirm} {
+        if {![confirm_popup [mc "Really delete branch %s?" $head]]} 

return

Quoted 3 lines

     }
     nowbusy rmbranch
     update

The key binding to remove a branch does not make sense to me. It does happen that I have more than one branch on a commit, but there is no way to select which one to remove via the keyboard. I have to use the context menu. This needs more thought IMHO. At a minimum, separate it out into its own commit.

-- Hannes

I would prefer to keep the default at "mixed" mode, set the focus on the radio button to make it easy to switch to "hard" mode by hitting the Down arrow key, and then make it so that Enter triggers the OK button.

Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:

Quoted 4 lines

From: Jens Lidestrom <[email protected]>

Reset hard is dangerous but also the most common reset type, and not having it pre-selected in the dialog is annoying to users.

I agree that the operation of the Reset dialog is clumsy before this series. However, this patch together with the previous patch turns it into a foot gun. It becomes far too easy to destroy uncommitted work.

I would prefer to keep the default at "mixed" mode, set the focus on the radio button to make it easy to switch to "hard" mode by hitting the Down arrow key, and then make it so that Enter triggers the OK button.

Quoted 2 lines

It is also less dangerous in the GUI where there is a confirmation dialog. Also, dangling commits remain in the GUI and can be recovered.

The problem with "hard" mode are not the commits. The real danger is that it blows away uncommitted changes. Besides of that, I do not consider this UI a confirmation dialog.

-- Hannes

Quoted 23 lines

Signed-off-by Jens Lidestrom <[email protected]>

---
 gitk-git/gitk | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

gitk-git/gitk +0 -0

index 9d93053e360..5b0a0ea46be 100755
@@ -9906,7 +9906,9 @@ proc resethead {reset_target_id} {
         [mc "Reset branch %s to %s?" $mainhead [commit_name 

$reset_target_id 1]]
pack $w.m -side top -fill x -padx 20 -pady 20 ${NS}::labelframe $w.f -text [mc "Reset type:"]

gitk-git/gitk +1 -1

-    set resettype mixed
+    # Reset hard is dangerous but also the most common reset type, 

and not

gitk-git/gitk +2 -0

+    # having it pre-selected in the dialog is annoying to users.
+    set resettype hard
     ${NS}::radiobutton $w.f.soft -value soft -variable resettype \
         -text [mc "Soft: Leave working tree and index untouched"]
     grid $w.f.soft -sticky w

From: Jens Lidestrom <[email protected]>

Reset hard is dangerous but also the most common reset type, and not having it pre-selected in the dialog is annoying to users.

It is also less dangerous in the GUI where there is a confirmation dialog. Also, dangling commits remain in the GUI and can be recovered.

Signed-off-by Jens Lidestrom <[email protected]>

---
 gitk-git/gitk | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

gitk-git/gitk +3 -1

index 9d93053e360..5b0a0ea46be 100755
@@ -9906,7 +9906,9 @@ proc resethead {reset_target_id} {
         [mc "Reset branch %s to %s?" $mainhead [commit_name $reset_target_id 1]]
     pack $w.m -side top -fill x -padx 20 -pady 20
     ${NS}::labelframe $w.f -text [mc "Reset type:"]
-    set resettype mixed
+    # Reset hard is dangerous but also the most common reset type, and not
+    # having it pre-selected in the dialog is annoying to users.
+    set resettype hard
     ${NS}::radiobutton $w.f.soft -value soft -variable resettype \
         -text [mc "Soft: Leave working tree and index untouched"]
     grid $w.f.soft -sticky w

Please note that gitk-git directory is in its own repository that is only subtree-merged into the Git repository. You should generate patches against git://git.ozlabs.org/~paulus/gitk (I don't know how difficult it would be for Paul to integrate patches that were generated by gitgitgadget).

Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:

Quoted 9 lines

It is often convenient to use the keyboard to navigate the gitk GUI and there are keyboard shortcut bindings for many operations such as searching and scrolling. There is however no keyboard binding for the most common operations on branches and commits: Check out, reset, cherry-pick, create and delete branches.

This PR adds keyboard bindings for these 5 commands. It also adjusts some GUI focus defaults to simplify keyboard navigation.

Quoted 6 lines

Some refactoring of the command implementation has been necessary. Originally the commands was using the mouse context menu to get info about the head and commit to act on. When using keyboard binds this information isn't available so instead the row that is selected in the GUI is used. By adding procedures for doing this the PR lays the groundwork for more similar keyboard binds in the future.

I like it when an application can be navigated with the keyboard. These changes are very much appreciated.

I've left some comments on individual commits. The important one is that I think it makes the Reset dialog way too easy to destroy uncommitted work.

Please note that gitk-git directory is in its own repository that is only subtree-merged into the Git repository. You should generate patches against git://git.ozlabs.org/~paulus/gitk (I don't know how difficult it would be for Paul to integrate patches that were generated by gitgitgadget).

-- Hannes

Quoted 24 lines

I'm including Paul Mackerras because he seems to be the maintainer of gitk. Can you review, Paul?

Jens Lidestrom (9):
gitk: add procedures to get commit info from selected row
gitk: use term "current branch" in gui
gitk: add keyboard bind for reset
gitk: show branch name in reset dialog
gitk: add keyboard bind for checkout
gitk: add keyboard bind for create and remove branch
gitk: add keyboard bind to cherry-pick
gitk: focus ok button in reset dialog
gitk: default select reset hard in dialog

 gitk-git/gitk | 132 
++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 96 insertions(+), 36 deletions(-)

base-commit: 94486b6763c29144c60932829a65fec0597e17b3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1551%2Fjensli%2Fkeyboard-for-gitk-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1551/jensli/keyboard-for-gitk-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1551

It is often convenient to use the keyboard to navigate the gitk GUI and there are keyboard shortcut bindings for many operations such as searching and scrolling. There is however no keyboard binding for the most common operations on branches and commits: Check out, reset, cherry-pick, create and delete branches.

This PR adds keyboard bindings for these 5 commands. It also adjusts some GUI focus defaults to simplify keyboard navigation.

Some refactoring of the command implementation has been necessary. Originally the commands was using the mouse context menu to get info about the head and commit to act on. When using keyboard binds this information isn't available so instead the row that is selected in the GUI is used. By adding procedures for doing this the PR lays the groundwork for more similar keyboard binds in the future.

I'm including Paul Mackerras because he seems to be the maintainer of gitk. Can you review, Paul?

Jens Lidestrom (9):
gitk: add procedures to get commit info from selected row
gitk: use term "current branch" in gui
gitk: add keyboard bind for reset
gitk: show branch name in reset dialog
gitk: add keyboard bind for checkout
gitk: add keyboard bind for create and remove branch
gitk: add keyboard bind to cherry-pick
gitk: focus ok button in reset dialog
gitk: default select reset hard in dialog

 gitk-git/gitk | 132 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 96 insertions(+), 36 deletions(-)

base-commit: 94486b6763c29144c60932829a65fec0597e17b3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1551%2Fjensli%2Fkeyboard-for-gitk-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1551/jensli/keyboard-for-gitk-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1551

From: Jens Lidestrom <[email protected]>

Signed-off-by Jens Lidestrom <[email protected]>

---
 gitk-git/gitk | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

gitk-git/gitk +0 -0

index 596977abe89..0d83a72a424 100755

gitk-git/gitk +1 -1

     for {set i 1} {$i < 10} {incr i} {
         bind . <$M1B-Key-$i> [list go_to_parent $i]
     }

@@ -2735,7 +2737,7 @@ proc makewindow {} {
     makemenu $headctxmenu {
         {mc "Check out this branch" command {checkout $headmenuhead $headmenuid}}
         {mc "Rename this branch" command mvbranch}
-        {mc "Remove this branch" command rmbranch}
+        {mc "Remove this branch" command {rmbranch $headmenuhead $headmenuid 0}}
         {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
     }
     $headctxmenu configure -tearoff 0

gitk-git/gitk +10 -8

+[mc "<%s-M>		Remove selected branch" $M1T]
 " \
             -justify left -bg $bgcolor -border 2 -relief groove
     pack $w.m -side top -fill both -padx 2 -pady 2

@@ -9576,13 +9580,13 @@ proc wrcomcan {} {
     unset wrcomtop
 }
 
-proc mkbranch {} {
-    global NS rowmenuid
+proc mkbranch {id} {
+    global NS
 
     set top .branchdialog
 
     set val(name) ""
-    set val(id) $rowmenuid
+    set val(id) $id
     set val(command) [list mkbrgo $top]
 
     set ui(title) [mc "Create branch"]

@@ -10054,13 +10058,14 @@ proc readcheckoutstat {fd newhead newheadref newheadid} {
     }
 }
 
-proc rmbranch {} {
-    global headmenuid headmenuhead mainhead
+proc rmbranch {head id shouldComfirm} {
+    global mainhead
     global idheads
-
-    set head $headmenuhead
-    set id $headmenuid
     # this check shouldn't be needed any more...
+    if {$head eq ""} {
+        error_popup [mc "Cannot delete a detached head"]
+        return
+    }
     if {$head eq $mainhead} {
         error_popup [mc "Cannot delete the currently checked-out branch"]
         return

Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:

Quoted 9 lines

It is often convenient to use the keyboard to navigate the gitk GUI and there are keyboard shortcut bindings for many operations such as searching and scrolling. There is however no keyboard binding for the most common operations on branches and commits: Check out, reset, cherry-pick, create and delete branches.

This PR adds keyboard bindings for these 5 commands. It also adjusts some GUI focus defaults to simplify keyboard navigation.

Quoted 6 lines

Some refactoring of the command implementation has been necessary. Originally the commands was using the mouse context menu to get info about the head and commit to act on. When using keyboard binds this information isn't available so instead the row that is selected in the GUI is used. By adding procedures for doing this the PR lays the groundwork for more similar keyboard binds in the future.

I like it when an application can be navigated with the keyboard. These changes are very much appreciated.

I've left some comments on individual commits. The important one is that I think it makes the Reset dialog way too easy to destroy uncommitted work.

Please note that gitk-git directory is in its own repository that is only subtree-merged into the Git repository. You should generate patches against git://git.ozlabs.org/~paulus/gitk (I don't know how difficult it would be for Paul to integrate patches that were generated by gitgitgadget).

-- Hannes

Quoted 22 lines

I'm including Paul Mackerras because he seems to be the maintainer of gitk. Can you review, Paul?

Jens Lidestrom (9):
gitk: add procedures to get commit info from selected row
gitk: use term "current branch" in gui
gitk: add keyboard bind for reset
gitk: show branch name in reset dialog
gitk: add keyboard bind for checkout
gitk: add keyboard bind for create and remove branch
gitk: add keyboard bind to cherry-pick
gitk: focus ok button in reset dialog
gitk: default select reset hard in dialog

  gitk-git/gitk | 132 ++++++++++++++++++++++++++++++++++++--------------
  1 file changed, 96 insertions(+), 36 deletions(-)

base-commit: 94486b6763c29144c60932829a65fec0597e17b3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1551%2Fjensli%2Fkeyboard-for-gitk-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1551/jensli/keyboard-for-gitk-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1551

My intention is to always remove the first branch head that is displayed for a single commit in the GUI. This caters to the common use case, with only one branch for a single commit. If there are multiple branch heads on a commit and the users don't want to remove the first one then they need to use the mouse context menu to choose which one to delete.

I could change the implementation to display a dialog that lets the user choose in case of multiple branch heads.

In that case, should I do that as part of this PR, or as a follow up? I would prefer to finish this one first.

@Hannes: I choose key combinations with Ctrl+<another-key>.

Another possibility is to use Ctrl+Shift+<another-key>. That is more complicated to press, but it creates a nice distinction between two categories of commands:

Searching and navigation command (existing): Ctrl+<another-key>

Branch modification commands (new): Ctrl+Shift+<another-key>

Do you have any opinion on this? Only Ctrl, or Ctrl+Shift for the new commands?

IMO a selection dialog is the second-best solution. The best one is to make the branch labels selectable somehow via keyboard navigation. I am not a fan of the here implement behavior, because it is only a half-solution.

Am 28.06.23 um 09:12 schrieb Jens Lideström:

Quoted 8 lines

My intention is to always remove the first branch head that is displayed for a single commit in the GUI. This caters to the common use case, with only one branch for a single commit. If there are multiple branch heads on a commit and the users don't want to remove the first one then they need to use the mouse context menu to choose which one to delete.

I could change the implementation to display a dialog that lets the user choose in case of multiple branch heads.

IMO a selection dialog is the second-best solution. The best one is to make the branch labels selectable somehow via keyboard navigation. I am not a fan of the here implement behavior, because it is only a half-solution.

Also, the order of branch labels on a line is not 100% deterministic:
when you create a branch, it goes last, but when you refresh the view, branch labels are sorted alphabetically (I think). This means you can't create a branch and delete it right away if there is already a branch on the commit.

Quoted 2 lines

In that case, should I do that as part of this PR, or as a follow up? I would prefer to finish this one first.

My preference would be to not implement this behavior until it is clear what it should be.

-- Hannes

Quoted 3 lines

I would prefer to keep the default at "mixed" mode, set the focus on the radio button to make it easy to switch to "hard" mode by hitting the Down arrow key, and then make it so that Enter triggers the OK button.

That indeed sounds better! Safer but still convenient. I'll change the solution to this.

I noticed that some dialogues have a key bind to close with success using Enter.

/Jens

Quoted 45 lines · Johannes Sixt

Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:

Quoted 4 lines

From: Jens Lidestrom <[email protected]>

Reset hard is dangerous but also the most common reset type, and not having it pre-selected in the dialog is annoying to users.

I agree that the operation of the Reset dialog is clumsy before this series. However, this patch together with the previous patch turns it into a foot gun. It becomes far too easy to destroy uncommitted work.

I would prefer to keep the default at "mixed" mode, set the focus on the radio button to make it easy to switch to "hard" mode by hitting the Down arrow key, and then make it so that Enter triggers the OK button.

Quoted 2 lines

It is also less dangerous in the GUI where there is a confirmation dialog. Also, dangling commits remain in the GUI and can be recovered.

The problem with "hard" mode are not the commits. The real danger is that it blows away uncommitted changes. Besides of that, I do not consider this UI a confirmation dialog.

-- Hannes

Quoted 22 lines

Signed-off-by Jens Lidestrom <[email protected]>

---
 gitk-git/gitk | 4 +++-

 1 file changed, 3 insertions(+), 1 deletion(-)

gitk-git/gitk +0 -0

index 9d93053e360..5b0a0ea46be 100755
@@ -9906,7 +9906,9 @@ proc resethead {reset_target_id} {

         [mc "Reset branch %s to %s?" $mainhead [commit_name $reset_target_id 1]]
     pack $w.m -side top -fill x -padx 20 -pady 20 ${NS}::labelframe $w.f -text [mc "Reset type:"]
-    set resettype mixed
+    # Reset hard is dangerous but also the most common reset type, and not +    # having it pre-selected in the dialog is annoying to users. +    set resettype hard
     ${NS}::radiobutton $w.f.soft -value soft -variable resettype \
         -text [mc "Soft: Leave working tree and index untouched"] grid $w.f.soft -sticky w

From: Jens Lidestrom <[email protected]>

This also introduces the ability to check out detatched heads. This shouldn't result any problems, because gitk already works with detatched heads if they are checked out using the terminal.

Signed-off-by Jens Lidestrom <[email protected]>

---
 gitk-git/gitk | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

gitk-git/gitk +16 -12

index bfe912983f4..596977abe89 100755
@@ -2691,6 +2691,7 @@ proc makewindow {} {
     bind $ctext <Button-1> {focus %W}
     bind $ctext <<Selection>> rehighlight_search_results
     bind . <$M1B-t> {resethead [selected_line_id]}
+    bind . <$M1B-o> {checkout [selected_line_head] [selected_line_id]}
     for {set i 1} {$i < 10} {incr i} {
         bind . <$M1B-Key-$i> [list go_to_parent $i]
     }

@@ -2707,7 +2708,7 @@ proc makewindow {} {
         {mc "Create tag" command mktag}
         {mc "Copy commit reference" command copyreference}
         {mc "Write commit to file" command writecommit}
-        {mc "Create new branch" command mkbranch}
+        {mc "Create new branch" command {mkbranch $rowmenuid}}
         {mc "Cherry-pick this commit" command cherrypick}
         {mc "Reset current branch to here" command {resethead $rowmenuid}}
         {mc "Mark this commit" command markhere}

@@ -2732,7 +2733,7 @@ proc makewindow {} {
 
     set headctxmenu .headctxmenu
     makemenu $headctxmenu {
-        {mc "Check out this branch" command cobranch}
+        {mc "Check out this branch" command {checkout $headmenuhead $headmenuid}}
         {mc "Rename this branch" command mvbranch}
         {mc "Remove this branch" command rmbranch}
         {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}

@@ -3183,6 +3184,7 @@ proc keys {} {
 [mc "<%s-minus>	Decrease font size" $M1T]
 [mc "<F5>		Update"]
 [mc "<%s-T>		Reset current branch to selected commit" $M1T]
+[mc "<%s-O>		Check out selected commit" $M1T]
 " \
             -justify left -bg $bgcolor -border 2 -relief groove
     pack $w.m -side top -fill both -padx 2 -pady 2

@@ -9978,25 +9980,26 @@ proc headmenu {x y id head} {
     tk_popup $headctxmenu $x $y
 }
 
-proc cobranch {} {
-    global headmenuid headmenuhead headids
+proc checkout {newhead newheadid} {
+    global headids
     global showlocalchanges
 
     # check the tree is clean first??
-    set newhead $headmenuhead
+
+    # The ref is either the head, if it exists, or the ID
+    set newheadref [expr {$newhead ne "" ? $newhead : $newheadid}]
+
     set command [list | git checkout]
     if {[string match "remotes/*" $newhead]} {
         set remote $newhead
         set newhead [string range $newhead [expr [string last / $newhead] + 1] end]
-        # The following check is redundant - the menu option should
-        # be disabled to begin with...
         if {[info exists headids($newhead)]} {
             error_popup [mc "A local branch named %s exists already" $newhead]
             return
         }
         lappend command -b $newhead --track $remote
     } else {
-        lappend command $newhead
+        lappend command $newheadref
     }
     lappend command 2>@1
     nowbusy checkout [mc "Checking out"]

@@ -10011,11 +10014,11 @@ proc cobranch {} {
             dodiffindex
         }
     } else {
-        filerun $fd [list readcheckoutstat $fd $newhead $headmenuid]
+        filerun $fd [list readcheckoutstat $fd $newhead $newheadref $newheadid]
     }
 }
 
-proc readcheckoutstat {fd newhead newheadid} {
+proc readcheckoutstat {fd newhead newheadref newheadid} {
     global mainhead mainheadid headids idheads showlocalchanges progresscoords
     global viewmainheadid curview
 

@@ -10034,12 +10037,13 @@ proc readcheckoutstat {fd newhead newheadid} {
         return
     }
     set oldmainid $mainheadid
-    if {! [info exists headids($newhead)]} {
+
+    if {$newhead ne "" && ! [info exists headids($newhead)]} {
         set headids($newhead) $newheadid
         lappend idheads($newheadid) $newhead
         addedhead $newheadid $newhead
     }
-    set mainhead $newhead
+    set mainhead $newheadref
     set mainheadid $newheadid
     set viewmainheadid($curview) $newheadid
     redrawtags $oldmainid

Please note that gitk-git directory is in its own repository that is only subtree-merged into the Git repository. You should generate patches against git://git.ozlabs.org/~paulus/gitk (I don't know how difficult it would be for Paul to integrate patches that were generated by gitgitgadget).

Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:

Quoted 9 lines

It is often convenient to use the keyboard to navigate the gitk GUI and there are keyboard shortcut bindings for many operations such as searching and scrolling. There is however no keyboard binding for the most common operations on branches and commits: Check out, reset, cherry-pick, create and delete branches.

This PR adds keyboard bindings for these 5 commands. It also adjusts some GUI focus defaults to simplify keyboard navigation.

Quoted 6 lines

Some refactoring of the command implementation has been necessary. Originally the commands was using the mouse context menu to get info about the head and commit to act on. When using keyboard binds this information isn't available so instead the row that is selected in the GUI is used. By adding procedures for doing this the PR lays the groundwork for more similar keyboard binds in the future.

I like it when an application can be navigated with the keyboard. These changes are very much appreciated.

I've left some comments on individual commits. The important one is that I think it makes the Reset dialog way too easy to destroy uncommitted work.

Please note that gitk-git directory is in its own repository that is only subtree-merged into the Git repository. You should generate patches against git://git.ozlabs.org/~paulus/gitk (I don't know how difficult it would be for Paul to integrate patches that were generated by gitgitgadget).

-- Hannes

Quoted 23 lines

I'm including Paul Mackerras because he seems to be the maintainer of gitk. Can you review, Paul?

Jens Lidestrom (9):
gitk: add procedures to get commit info from selected row
gitk: use term "current branch" in gui
gitk: add keyboard bind for reset
gitk: show branch name in reset dialog
gitk: add keyboard bind for checkout
gitk: add keyboard bind for create and remove branch
gitk: add keyboard bind to cherry-pick
gitk: focus ok button in reset dialog
gitk: default select reset hard in dialog

 gitk-git/gitk | 132 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 96 insertions(+), 36 deletions(-)

base-commit: 94486b6763c29144c60932829a65fec0597e17b3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1551%2Fjensli%2Fkeyboard-for-gitk-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1551/jensli/keyboard-for-gitk-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1551

From: Jens Lidestrom <[email protected]>

This also introduces the ability to check out detatched heads. This shouldn't result any problems, because gitk already works with detatched heads if they are checked out using the terminal.

Signed-off-by Jens Lidestrom <[email protected]>

---
 gitk-git/gitk | 125 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 98 insertions(+), 27 deletions(-)

gitk-git/gitk +34 -4

index 642cd7f652a..8364033ad58 100755
@@ -2691,6 +2691,7 @@ proc makewindow {} {
     bind $ctext <Button-1> {focus %W}
     bind $ctext <<Selection>> rehighlight_search_results
     bind . <$M1B-t> {resethead [selected_line_id]}
+    bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
     for {set i 1} {$i < 10} {incr i} {
         bind . <$M1B-Key-$i> [list go_to_parent $i]
     }

@@ -2732,7 +2733,7 @@ proc makewindow {} {
 
     set headctxmenu .headctxmenu
     makemenu $headctxmenu {
-        {mc "Check out this branch" command cobranch}
+        {mc "Check out this branch" command {checkout [list $headmenuhead] $headmenuid}}
         {mc "Rename this branch" command mvbranch}
         {mc "Remove this branch" command rmbranch}
         {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}

@@ -3183,6 +3184,7 @@ proc keys {} {
 [mc "<%s-minus>	Decrease font size" $M1T]
 [mc "<F5>		Update"]
 [mc "<%s-T>		Reset current branch to selected commit" $M1T]
+[mc "<%s-O>		Check out selected commit" $M1T]
 " \
             -justify left -bg $bgcolor -border 2 -relief groove
     pack $w.m -side top -fill both -padx 2 -pady 2

@@ -9978,25 +9980,93 @@ proc headmenu {x y id head} {
     tk_popup $headctxmenu $x $y
 }
 
-proc cobranch {} {
-    global headmenuid headmenuhead headids
+proc checkout {heads_on_commit id_to_checkout} {
+    global headids mainhead
     global showlocalchanges
+    global sel_ix confirm_ok NS
 
     # check the tree is clean first??
-    set newhead $headmenuhead
+
+    # Filter out remote branches if local branch is also present
+    foreach remote_ix [lsearch -all $heads_on_commit "remotes/*"] {
+        set remote_head [lindex $heads_on_commit $remote_ix]
+        set local_head [string range $remote_head [expr [string last / $remote_head] + 1] end]
+        if {$local_head in $heads_on_commit} {
+            set heads_on_commit [lreplace $heads_on_commit $remote_ix $remote_ix]
+        }
+    }
+
+    if {[llength $heads_on_commit] == 1 && [lindex $heads_on_commit 0] eq $mainhead} {
+        # Only the currently active branch
+        return
+    }
+
+    # Filter out mainhead
+    set mainhead_ix [lsearch $heads_on_commit $mainhead]
+    if {$mainhead_ix != -1} {
+        set heads_on_commit [lreplace $heads_on_commit $mainhead_ix $mainhead_ix]
+    }
+    set nr_heads_on_commit [llength $heads_on_commit]
+
+    # The number of heads on the commit determines how to select what to checkout
+    if {$nr_heads_on_commit == 0} {
+        set head_to_checkout ""
+        set ref_to_checkout $id_to_checkout
+    } elseif {$nr_heads_on_commit == 1} {
+        set head_to_checkout [lindex $heads_on_commit 0]

gitk-git/gitk +46 -23

+            bind $w.f.id_$i <Key-Up> "set sel_ix [expr ($i - 1) % $nr_heads_on_commit]"
+            bind $w.f.id_$i <Key-Down> "set sel_ix [expr ($i + 1) % $nr_heads_on_commit]"
+            grid $w.f.id_$i -sticky w -padx 20
+        }
+
+        pack $w.f -side top -fill x -padx 4
+        ${NS}::button $w.ok -text [mc OK] -command "set confirm_ok 1; destroy $w"
+        bind $w <Key-Return> "set confirm_ok 1; destroy $w"
+        pack $w.ok -side left -fill x -padx 20 -pady 20
+        ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
+        bind $w <Key-Escape> [list destroy $w]
+        pack $w.cancel -side right -fill x -padx 20 -pady 20
+        bind $w <Visibility> "grab $w; focus $w.f.id_$sel_ix"
+
+        tkwait window $w
+
+        if {!$confirm_ok} return
+
+        set head_to_checkout [lindex $heads_on_commit $sel_ix]
+        set ref_to_checkout $head_to_checkout
+    }
+
+    # Handle remote branches
     set command [list | git checkout]
-    if {[string match "remotes/*" $newhead]} {
-        set remote $newhead
-        set newhead [string range $newhead [expr [string last / $newhead] + 1] end]
-        # The following check is redundant - the menu option should
-        # be disabled to begin with...
-        if {[info exists headids($newhead)]} {
-            error_popup [mc "A local branch named %s exists already" $newhead]
+    if {[string match "remotes/*" $head_to_checkout]} {
+        set remote $head_to_checkout
+        set head_to_checkout [string range $head_to_checkout [expr [string last / $head_to_checkout] + 1] end]
+        set ref_to_checkout $head_to_checkout
+        if {[info exists headids($head_to_checkout)]} {
+            error_popup [mc "A local branch named %s exists already" $head_to_checkout]
             return
         }
-        lappend command -b $newhead --track $remote
+        lappend command -b $head_to_checkout --track $remote
     } else {
-        lappend command $newhead
+        lappend command $ref_to_checkout
     }
     lappend command 2>@1
     nowbusy checkout [mc "Checking out"]

@@ -10011,11 +10081,11 @@ proc cobranch {} {
             dodiffindex
         }
     } else {
-        filerun $fd [list readcheckoutstat $fd $newhead $headmenuid]
+        filerun $fd [list readcheckoutstat $fd $head_to_checkout $ref_to_checkout $id_to_checkout]
     }
 }
 
-proc readcheckoutstat {fd newhead newheadid} {
+proc readcheckoutstat {fd head_to_checkout ref_to_checkout id_to_checkout} {
     global mainhead mainheadid headids idheads showlocalchanges progresscoords
     global viewmainheadid curview
 

@@ -10033,18 +10103,19 @@ proc readcheckoutstat {fd newhead newheadid} {
         error_popup $err
         return
     }
-    set oldmainid $mainheadid
-    if {! [info exists headids($newhead)]} {
-        set headids($newhead) $newheadid
-        lappend idheads($newheadid) $newhead
-        addedhead $newheadid $newhead
-    }
-    set mainhead $newhead
-    set mainheadid $newheadid
-    set viewmainheadid($curview) $newheadid
-    redrawtags $oldmainid
-    redrawtags $newheadid
-    selbyid $newheadid
+    set old_main_id $mainheadid
+
+    if {$head_to_checkout ne "" && ! [info exists headids($head_to_checkout)]} {
+        set headids($head_to_checkout) $id_to_checkout
+        lappend idheads($id_to_checkout) $head_to_checkout
+        addedhead $id_to_checkout $head_to_checkout
+    }
+    set mainhead $ref_to_checkout
+    set mainheadid $id_to_checkout
+    set viewmainheadid($curview) $id_to_checkout
+    redrawtags $old_main_id
+    redrawtags $id_to_checkout
+    selbyid $id_to_checkout
     if {$showlocalchanges} {
         dodiffindex
     }

From: Jens Lidestrom <[email protected]>

Make it more convenient to use the reset dialog using keyboard.

* Set focus to the combo box.
* Accept with Return key.
* Auto-select combo items when navigating in menu with up/down keys.

Signed-off-by Jens Lidestrom <[email protected]>

---
 gitk-git/gitk | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

gitk-git/gitk +14 -1

index f559e279b7a..fafff2b1a5b 100755
@@ -9902,27 +9902,40 @@ proc resethead {reset_target_id} {
     ttk_toplevel $w
     make_transient $w .
     wm title $w [mc "Confirm reset"]
+
     ${NS}::label $w.m -text \
         [mc "Reset branch %s to %s?" $mainhead [commit_name $reset_target_id 1]]
     pack $w.m -side top -fill x -padx 20 -pady 20
     ${NS}::labelframe $w.f -text [mc "Reset type:"]
+
     set resettype mixed
+
     ${NS}::radiobutton $w.f.soft -value soft -variable resettype \
         -text [mc "Soft: Leave working tree and index untouched"]
+    bind $w.f.soft <Key-Up> "set resettype hard"
+    bind $w.f.soft <Key-Down> "set resettype mixed"
     grid $w.f.soft -sticky w
+
     ${NS}::radiobutton $w.f.mixed -value mixed -variable resettype \
         -text [mc "Mixed: Leave working tree untouched, reset index"]
+    bind $w.f.mixed <Key-Up> "set resettype soft"
+    bind $w.f.mixed <Key-Down> "set resettype hard"
     grid $w.f.mixed -sticky w
+
     ${NS}::radiobutton $w.f.hard -value hard -variable resettype \
         -text [mc "Hard: Reset working tree and index\n(discard ALL local changes)"]
+    bind $w.f.hard <Key-Up> "set resettype mixed"
+    bind $w.f.hard <Key-Down> "set resettype soft"
     grid $w.f.hard -sticky w
     pack $w.f -side top -fill x -padx 4
+
     ${NS}::button $w.ok -text [mc OK] -command "set confirm_ok 1; destroy $w"
+    bind $w <Key-Return> "set confirm_ok 1; destroy $w"
     pack $w.ok -side left -fill x -padx 20 -pady 20
     ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
     bind $w <Key-Escape> [list destroy $w]
     pack $w.cancel -side right -fill x -padx 20 -pady 20
-    bind $w <Visibility> "grab $w; focus $w"
+    bind $w <Visibility> "grab $w; focus $w.f.mixed"
     tkwait window $w
     if {!$confirm_ok} return
     if {[catch {set fd [open \

From: Jens Lidestrom <[email protected]>

Signed-off-by Jens Lidestrom <[email protected]>

---
 gitk-git/gitk | 104 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 89 insertions(+), 15 deletions(-)

gitk-git/gitk +0 -0

index 8364033ad58..65ca11becca 100755

gitk-git/gitk +1 -1

     for {set i 1} {$i < 10} {incr i} {
         bind . <$M1B-Key-$i> [list go_to_parent $i]
     }

@@ -2735,7 +2737,7 @@ proc makewindow {} {
     makemenu $headctxmenu {
         {mc "Check out this branch" command {checkout [list $headmenuhead] $headmenuid}}
         {mc "Rename this branch" command mvbranch}
-        {mc "Remove this branch" command rmbranch}
+        {mc "Remove this branch" command {rmbranch [list $headmenuhead] $headmenuid 0}}
         {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
     }
     $headctxmenu configure -tearoff 0

gitk-git/gitk +49 -11

 " \
             -justify left -bg $bgcolor -border 2 -relief groove
     pack $w.m -side top -fill both -padx 2 -pady 2

@@ -10121,32 +10125,102 @@ proc readcheckoutstat {fd head_to_checkout ref_to_checkout id_to_checkout} {
     }
 }
 
-proc rmbranch {} {
-    global headmenuid headmenuhead mainhead
+proc rmbranch {heads_on_commit id shouldComfirm} {
+    global mainhead
     global idheads
+    global confirm_ok sel_ix NS
 
-    set head $headmenuhead
-    set id $headmenuid
-    # this check shouldn't be needed any more...
-    if {$head eq $mainhead} {
+    if {[llength $heads_on_commit] == 0} {
+        error_popup [mc "Cannot delete a detached head"]
+        return
+    }
+
+    if {[llength $heads_on_commit] == 1 && [lindex $heads_on_commit 0] eq $mainhead} {
         error_popup [mc "Cannot delete the currently checked-out branch"]
         return
     }
-    set dheads [descheads $id]
-    if {[llength $dheads] == 1 && $idheads($dheads) eq $head} {
-        # the stuff on this branch isn't on any other branch
-        if {![confirm_popup [mc "The commits on branch %s aren't on any other\
-                        branch.\nReally delete branch %s?" $head $head]]} return
+
+    # Filter out mainhead
+    set mainhead_ix [lsearch $heads_on_commit $mainhead]
+    if {$mainhead_ix != -1} {
+        set heads_on_commit [lreplace $heads_on_commit $mainhead_ix $mainhead_ix]
+    }
+
+    # Filter out remote branches
+    foreach head_ix [lsearch -all $heads_on_commit "remotes/*"] {
+        set heads_on_commit [lreplace $heads_on_commit $head_ix $head_ix]
+    }
+
+    if {[llength $heads_on_commit] == 0} {
+        # Probably only current branch and its remote branch on commit
+        error_popup [mc "Cannot delete branch"]
+        return
     }
+
+    set nr_heads_on_commit [llength $heads_on_commit]
+    set first_head [lindex $heads_on_commit 0]
+
+    if {$nr_heads_on_commit == 1} {
+        # Only a single head, simple comfirm dialogs
+
+        set head_to_remove $first_head
+        set dheads [descheads $id]
+
+        if {[llength $dheads] == 1 && $idheads($dheads) eq $head_to_remove} {
+            # the stuff on this branch isn't on any other branch
+            if {![confirm_popup [mc "The commits on branch %s aren't on any other\
+                            branch.\nReally delete branch %s?" $head_to_remove $head_to_remove]]} return
+        } elseif {$shouldComfirm} {
+            if {![confirm_popup [mc "Really delete branch %s?" $head_to_remove]]} return
+        }
+    } else {
+        # Select branch dialog
+
+        set confirm_ok 0
+
+        set w ".selectbranch"
+        ttk_toplevel $w

gitk-git/gitk +25 -3

+            bind $w.f.id_$i <Key-Up> "set sel_ix [expr ($i - 1) % $nr_heads_on_commit]"
+            bind $w.f.id_$i <Key-Down> "set sel_ix [expr ($i + 1) % $nr_heads_on_commit]"
+            grid $w.f.id_$i -sticky w -padx 20
+        }
+
+        pack $w.f -side top -fill x -padx 4
+        ${NS}::button $w.ok -text [mc OK] -command "set confirm_ok 1; destroy $w"
+        bind $w <Key-Return> "set confirm_ok 1; destroy $w"
+        pack $w.ok -side left -fill x -padx 20 -pady 20
+        ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
+        bind $w <Key-Escape> [list destroy $w]
+        pack $w.cancel -side right -fill x -padx 20 -pady 20
+        bind $w <Visibility> "grab $w; focus $w.f.id_$sel_ix"
+
+        tkwait window $w
+        if {!$confirm_ok} return
+
+        set head_to_remove [lindex $heads_on_commit $sel_ix]
+    }
+
+    # Perform the command
+
     nowbusy rmbranch
     update
-    if {[catch {exec git branch -D $head} err]} {
+    if {[catch {exec git branch -D $head_to_remove} err]} {
         notbusy rmbranch
         error_popup $err
         return
     }
-    removehead $id $head
-    removedhead $id $head
+    removehead $id $head_to_remove
+    removedhead $id $head_to_remove
     redrawtags $id
     notbusy rmbranch
     dispneartags 0

From: Jens Lidestrom <[email protected]>

Signed-off-by Jens Lidestrom <[email protected]>

---
 gitk-git/gitk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

gitk-git/gitk +1 -2

index 351b88f10c0..f559e279b7a 100755
@@ -9582,13 +9582,13 @@ proc wrcomcan {} {
     unset wrcomtop
 }
 
-proc mkbranch {} {
-    global NS rowmenuid
+proc mkbranch {id} {

From: Jens Lidestrom <[email protected]>

Signed-off-by Jens Lidestrom <[email protected]>

---
 gitk-git/gitk | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

gitk-git/gitk +1 -0

index 65ca11becca..351b88f10c0 100755
@@ -2690,6 +2690,7 @@ proc makewindow {} {
     bind $ctext $ctxbut {pop_diff_menu %W %X %Y %x %y}
     bind $ctext <Button-1> {focus %W}
     bind $ctext <<Selection>> rehighlight_search_results
+    bind . <$M1B-p> {cherrypick [selected_line_id]}
     bind . <$M1B-t> {resethead [selected_line_id]}
     bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
     bind . <$M1B-m> {rmbranch [selected_line_heads] [selected_line_id] 1}

gitk-git/gitk +1 -1

         {mc "Reset current branch to here" command {resethead $rowmenuid}}
         {mc "Mark this commit" command markhere}
         {mc "Return to mark" command gotomark}

@@ -3186,6 +3187,7 @@ proc keys {} {
 [mc "<%s-minus>	Decrease font size" $M1T]
 [mc "<F5>		Update"]
 [mc "<%s-T>		Reset current branch to selected commit" $M1T]
+[mc "<%s-P>		Cherry-pick selected commit to current branch" $M1T]
 [mc "<%s-O>		Check out selected commit" $M1T]
 [mc "<%s-B>		Create branch on selected commit" $M1T]
 [mc "<%s-M>		Remove selected branch" $M1T]

@@ -9758,24 +9760,29 @@ proc exec_citool {tool_args {baseid {}}} {
     array set env $save_env
 }
 
-proc cherrypick {} {

gitk-git/gitk +5 -4

+
     set oldhead [exec git rev-parse HEAD]
-    set dheads [descheads $rowmenuid]
+    set dheads [descheads $id]
     if {$dheads ne {} && [lsearch -exact $dheads $oldhead] >= 0} {
         set ok [confirm_popup [mc "Commit %s is already\
                 included in branch %s -- really re-apply it?" \
-                                   [string range $rowmenuid 0 7] $mainhead]]
+                                   [string range $id 0 7] $mainhead]]
         if {!$ok} return
     }
     nowbusy cherrypick [mc "Cherry-picking"]
     update
     # Unfortunately git-cherry-pick writes stuff to stderr even when
     # no error occurs, and exec takes that as an indication of error...
-    if {[catch {exec sh -c "git cherry-pick -r $rowmenuid 2>&1"} err]} {
+    if {[catch {exec sh -c "git cherry-pick -r $id 2>&1"} err]} {
         notbusy cherrypick
         if {[regexp -line \
                  {Entry '(.*)' (would be overwritten by merge|not uptodate)} \

@@ -9791,7 +9798,7 @@ proc cherrypick {} {
                         resolve it?"]]} {
                 # Force citool to read MERGE_MSG
                 file delete [file join $gitdir "GITGUI_MSG"]
-                exec_citool {} $rowmenuid
+                exec_citool {} $id
             }
         } else {
             error_popup $err

gitk-git/gitk proc makewindow {} { · line 2715

Quoted 2 lines

gitk-git/gitk +2 -0

+        {mc "Create new branch" command {mkbranch $rowmenuid}}
+        {mc "Cherry-pick this commit" command {cherrypick $rowmenuid}}

The change regarding Create new branch is not related to this commit's topic and should be elsewhere.

gitk-git/gitk proc exec_citool {tool_args {baseid {}}} { · line 9771

Quoted 4 lines

gitk-git/gitk +4 -0

+    if {! [info exists headids($mainhead)]} {
+        error_popup [mc "Cannot cherry-pick to a detached head"]
+        return
+    }

Why is it necessary to forbid this now? It was not forbidden before.

Am 03.07.23 um 20:45 schrieb Jens Lidestrom via GitGitGadget:

Quoted 20 lines

From: Jens Lidestrom <[email protected]>

Signed-off-by Jens Lidestrom <[email protected]>

---
 gitk-git/gitk | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

gitk-git/gitk +1 -0

index 65ca11becca..351b88f10c0 100755
@@ -2690,6 +2690,7 @@ proc makewindow {} {
     bind $ctext $ctxbut {pop_diff_menu %W %X %Y %x %y}
     bind $ctext <Button-1> {focus %W}
     bind $ctext <<Selection>> rehighlight_search_results
+    bind . <$M1B-p> {cherrypick [selected_line_id]}
     bind . <$M1B-t> {resethead [selected_line_id]}
     bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
     bind . <$M1B-m> {rmbranch [selected_line_heads] [selected_line_id] 1}

gitk-git/gitk proc makewindow {} { · line 2715

Quoted 7 lines

gitk-git/gitk +2 -2

@@ -2710,8 +2711,8 @@ proc makewindow {} {
         {mc "Create tag" command mktag}
         {mc "Copy commit reference" command copyreference}
         {mc "Write commit to file" command writecommit}
-        {mc "Create new branch" command mkbranch}
-        {mc "Cherry-pick this commit" command cherrypick}
+        {mc "Create new branch" command {mkbranch $rowmenuid}}
+        {mc "Cherry-pick this commit" command {cherrypick $rowmenuid}}

The change regarding Create new branch is not related to this commit's topic and should be elsewhere.

Quoted 16 lines

gitk-git/gitk +1 -1

         {mc "Reset current branch to here" command {resethead $rowmenuid}}
         {mc "Mark this commit" command markhere}
         {mc "Return to mark" command gotomark}

@@ -3186,6 +3187,7 @@ proc keys {} {
 [mc "<%s-minus>	Decrease font size" $M1T]
 [mc "<F5>		Update"]
 [mc "<%s-T>		Reset current branch to selected commit" $M1T]
+[mc "<%s-P>		Cherry-pick selected commit to current branch" $M1T]
 [mc "<%s-O>		Check out selected commit" $M1T]
 [mc "<%s-B>		Create branch on selected commit" $M1T]
 [mc "<%s-M>		Remove selected branch" $M1T]

@@ -9758,24 +9760,29 @@ proc exec_citool {tool_args {baseid {}}} {
     array set env $save_env
 }
 
-proc cherrypick {} {

gitk-git/gitk proc exec_citool {tool_args {baseid {}}} { · line 9771

Quoted 10 lines

gitk-git/gitk +6 -1

-    global rowmenuid curview
+proc cherrypick {id} {
+    global curview headids
     global mainhead mainheadid
     global gitdir
 
+    if {! [info exists headids($mainhead)]} {
+        error_popup [mc "Cannot cherry-pick to a detached head"]
+        return
+    }

Why is it necessary to forbid this now? It was not forbidden before.

Quoted 29 lines

gitk-git/gitk +5 -4

+
     set oldhead [exec git rev-parse HEAD]
-    set dheads [descheads $rowmenuid]
+    set dheads [descheads $id]
     if {$dheads ne {} && [lsearch -exact $dheads $oldhead] >= 0} {
         set ok [confirm_popup [mc "Commit %s is already\
                 included in branch %s -- really re-apply it?" \
-                                   [string range $rowmenuid 0 7] $mainhead]]
+                                   [string range $id 0 7] $mainhead]]
         if {!$ok} return
     }
     nowbusy cherrypick [mc "Cherry-picking"]
     update
     # Unfortunately git-cherry-pick writes stuff to stderr even when
     # no error occurs, and exec takes that as an indication of error...
-    if {[catch {exec sh -c "git cherry-pick -r $rowmenuid 2>&1"} err]} {
+    if {[catch {exec sh -c "git cherry-pick -r $id 2>&1"} err]} {
         notbusy cherrypick
         if {[regexp -line \
                  {Entry '(.*)' (would be overwritten by merge|not uptodate)} \

@@ -9791,7 +9798,7 @@ proc cherrypick {} {
                         resolve it?"]]} {
                 # Force citool to read MERGE_MSG
                 file delete [file join $gitdir "GITGUI_MSG"]
-                exec_citool {} $rowmenuid
+                exec_citool {} $id
             }
         } else {
             error_popup $err

There is already a branch selection dialog (hit F2 to bring it up). Could you please have a look whether it can be reused here? Because...

At a minimum, there should be a confirmation when a transition from a branch to a detached head happens

I find it irritating that it is possible to check out a detached head with Ctrl-O, but not via the context menu.

...

I propose that you do not include the ability to check out a detached head in this commit, but make it a new feature once this series has settled.

I find it irritating that it is possible to check out a detached head with Ctrl-O, but not via the context menu.

Playing around a bit with this feature, it may be a bit too easy to check out a different commit with a simple key press and without confirmation. At a minimum, there should be a confirmation when a transition from a branch to a detached head happens, because otherwise inexperienced users end up in inconvenient territories.

I propose that you do not include the ability to check out a detached head in this commit, but make it a new feature once this series has settled.

Am 03.07.23 um 20:45 schrieb Jens Lidestrom via GitGitGadget:

Quoted 5 lines

From: Jens Lidestrom <[email protected]>

This also introduces the ability to check out detatched heads. This shouldn't result any problems, because gitk already works with detatched heads if they are checked out using the terminal.

I find it irritating that it is possible to check out a detached head with Ctrl-O, but not via the context menu.

Playing around a bit with this feature, it may be a bit too easy to check out a different commit with a simple key press and without confirmation. At a minimum, there should be a confirmation when a transition from a branch to a detached head happens, because otherwise inexperienced users end up in inconvenient territories.

I propose that you do not include the ability to check out a detached head in this commit, but make it a new feature once this series has settled.

Quoted 76 lines

Signed-off-by Jens Lidestrom <[email protected]>

---
 gitk-git/gitk | 125 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 98 insertions(+), 27 deletions(-)

gitk-git/gitk +34 -4

index 642cd7f652a..8364033ad58 100755
@@ -2691,6 +2691,7 @@ proc makewindow {} {
     bind $ctext <Button-1> {focus %W}
     bind $ctext <<Selection>> rehighlight_search_results
     bind . <$M1B-t> {resethead [selected_line_id]}
+    bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
     for {set i 1} {$i < 10} {incr i} {
         bind . <$M1B-Key-$i> [list go_to_parent $i]
     }

@@ -2732,7 +2733,7 @@ proc makewindow {} {
 
     set headctxmenu .headctxmenu
     makemenu $headctxmenu {
-        {mc "Check out this branch" command cobranch}
+        {mc "Check out this branch" command {checkout [list $headmenuhead] $headmenuid}}
         {mc "Rename this branch" command mvbranch}
         {mc "Remove this branch" command rmbranch}
         {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}

@@ -3183,6 +3184,7 @@ proc keys {} {
 [mc "<%s-minus>	Decrease font size" $M1T]
 [mc "<F5>		Update"]
 [mc "<%s-T>		Reset current branch to selected commit" $M1T]
+[mc "<%s-O>		Check out selected commit" $M1T]
 " \
             -justify left -bg $bgcolor -border 2 -relief groove
     pack $w.m -side top -fill both -padx 2 -pady 2

@@ -9978,25 +9980,93 @@ proc headmenu {x y id head} {
     tk_popup $headctxmenu $x $y
 }
 
-proc cobranch {} {
-    global headmenuid headmenuhead headids
+proc checkout {heads_on_commit id_to_checkout} {
+    global headids mainhead
     global showlocalchanges
+    global sel_ix confirm_ok NS
 
     # check the tree is clean first??
-    set newhead $headmenuhead
+
+    # Filter out remote branches if local branch is also present
+    foreach remote_ix [lsearch -all $heads_on_commit "remotes/*"] {
+        set remote_head [lindex $heads_on_commit $remote_ix]
+        set local_head [string range $remote_head [expr [string last / $remote_head] + 1] end]
+        if {$local_head in $heads_on_commit} {
+            set heads_on_commit [lreplace $heads_on_commit $remote_ix $remote_ix]
+        }
+    }
+
+    if {[llength $heads_on_commit] == 1 && [lindex $heads_on_commit 0] eq $mainhead} {
+        # Only the currently active branch
+        return
+    }
+
+    # Filter out mainhead
+    set mainhead_ix [lsearch $heads_on_commit $mainhead]
+    if {$mainhead_ix != -1} {
+        set heads_on_commit [lreplace $heads_on_commit $mainhead_ix $mainhead_ix]
+    }
+    set nr_heads_on_commit [llength $heads_on_commit]
+
+    # The number of heads on the commit determines how to select what to checkout
+    if {$nr_heads_on_commit == 0} {
+        set head_to_checkout ""
+        set ref_to_checkout $id_to_checkout
+    } elseif {$nr_heads_on_commit == 1} {
+        set head_to_checkout [lindex $heads_on_commit 0]

gitk-git/gitk proc headmenu {x y id head} { · line 10026

Quoted 10 lines

gitk-git/gitk +10 -0

+        set ref_to_checkout $head_to_checkout
+    } else {
+        # Branch selection dialog
+
+        set confirm_ok 0
+
+        set w ".selectbranch"
+        ttk_toplevel $w
+        make_transient $w .
+        wm title $w [mc "Check out branch"]

There is already a branch selection dialog (hit F2 to bring it up). Could you please have a look whether it can be reused here? Because...

gitk-git/gitk proc headmenu {x y id head} { · line 10034

Quoted 8 lines

gitk-git/gitk +8 -0

+        ${NS}::label $w.m -text [mc "Check out which branch?"]
+        pack $w.m -side top -fill x -padx 20 -pady 20
+        ${NS}::frame $w.f
+
+        set sel_ix 0
+        for {set i 0} {$i < $nr_heads_on_commit} {incr i} {
+            ${NS}::radiobutton $w.f.id_$i -value $i -variable sel_ix \
+                -text [lindex $heads_on_commit $i]

... presenting a list of items in the form of radio buttons is a very non-standard user-interface, IMO. I would have expected a list widget in which an entry is highlighted. Also, if the list is long, a list would have a scroll bar, but a radio button group is limited to the available screen height.

Quoted 93 lines

gitk-git/gitk +46 -23

+            bind $w.f.id_$i <Key-Up> "set sel_ix [expr ($i - 1) % $nr_heads_on_commit]"
+            bind $w.f.id_$i <Key-Down> "set sel_ix [expr ($i + 1) % $nr_heads_on_commit]"
+            grid $w.f.id_$i -sticky w -padx 20
+        }
+
+        pack $w.f -side top -fill x -padx 4
+        ${NS}::button $w.ok -text [mc OK] -command "set confirm_ok 1; destroy $w"
+        bind $w <Key-Return> "set confirm_ok 1; destroy $w"
+        pack $w.ok -side left -fill x -padx 20 -pady 20
+        ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
+        bind $w <Key-Escape> [list destroy $w]
+        pack $w.cancel -side right -fill x -padx 20 -pady 20
+        bind $w <Visibility> "grab $w; focus $w.f.id_$sel_ix"
+
+        tkwait window $w
+
+        if {!$confirm_ok} return
+
+        set head_to_checkout [lindex $heads_on_commit $sel_ix]
+        set ref_to_checkout $head_to_checkout
+    }
+
+    # Handle remote branches
     set command [list | git checkout]
-    if {[string match "remotes/*" $newhead]} {
-        set remote $newhead
-        set newhead [string range $newhead [expr [string last / $newhead] + 1] end]
-        # The following check is redundant - the menu option should
-        # be disabled to begin with...
-        if {[info exists headids($newhead)]} {
-            error_popup [mc "A local branch named %s exists already" $newhead]
+    if {[string match "remotes/*" $head_to_checkout]} {
+        set remote $head_to_checkout
+        set head_to_checkout [string range $head_to_checkout [expr [string last / $head_to_checkout] + 1] end]
+        set ref_to_checkout $head_to_checkout
+        if {[info exists headids($head_to_checkout)]} {
+            error_popup [mc "A local branch named %s exists already" $head_to_checkout]
             return
         }
-        lappend command -b $newhead --track $remote
+        lappend command -b $head_to_checkout --track $remote
     } else {
-        lappend command $newhead
+        lappend command $ref_to_checkout
     }
     lappend command 2>@1
     nowbusy checkout [mc "Checking out"]

@@ -10011,11 +10081,11 @@ proc cobranch {} {
             dodiffindex
         }
     } else {
-        filerun $fd [list readcheckoutstat $fd $newhead $headmenuid]
+        filerun $fd [list readcheckoutstat $fd $head_to_checkout $ref_to_checkout $id_to_checkout]
     }
 }
 
-proc readcheckoutstat {fd newhead newheadid} {
+proc readcheckoutstat {fd head_to_checkout ref_to_checkout id_to_checkout} {
     global mainhead mainheadid headids idheads showlocalchanges progresscoords
     global viewmainheadid curview
 

@@ -10033,18 +10103,19 @@ proc readcheckoutstat {fd newhead newheadid} {
         error_popup $err
         return
     }
-    set oldmainid $mainheadid
-    if {! [info exists headids($newhead)]} {
-        set headids($newhead) $newheadid
-        lappend idheads($newheadid) $newhead
-        addedhead $newheadid $newhead
-    }
-    set mainhead $newhead
-    set mainheadid $newheadid
-    set viewmainheadid($curview) $newheadid
-    redrawtags $oldmainid
-    redrawtags $newheadid
-    selbyid $newheadid
+    set old_main_id $mainheadid
+
+    if {$head_to_checkout ne "" && ! [info exists headids($head_to_checkout)]} {
+        set headids($head_to_checkout) $id_to_checkout
+        lappend idheads($id_to_checkout) $head_to_checkout
+        addedhead $id_to_checkout $head_to_checkout
+    }
+    set mainhead $ref_to_checkout
+    set mainheadid $id_to_checkout
+    set viewmainheadid($curview) $id_to_checkout
+    redrawtags $old_main_id
+    redrawtags $id_to_checkout
+    selbyid $id_to_checkout
     if {$showlocalchanges} {
         dodiffindex
     }

This second line should not be in this commit, should it?

The same comment as in the earlier commit about the checkout command applies here: I don't think that radio buttons are the correct UI for a branch selection.

In general, such duplication of code should not happen. The dialog only differs in the title and help text and should be factored into a helper function.

Am 03.07.23 um 20:45 schrieb Jens Lidestrom via GitGitGadget:

Quoted 12 lines

From: Jens Lidestrom <[email protected]>

Signed-off-by Jens Lidestrom <[email protected]>

---
 gitk-git/gitk | 104 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 89 insertions(+), 15 deletions(-)

gitk-git/gitk +0 -0

index 8364033ad58..65ca11becca 100755

gitk-git/gitk proc makewindow {} { · line 2696

Quoted 5 lines

gitk-git/gitk +2 -0

@@ -2692,6 +2692,8 @@ proc makewindow {} {
     bind $ctext <<Selection>> rehighlight_search_results
     bind . <$M1B-t> {resethead [selected_line_id]}
     bind . <$M1B-o> {checkout [selected_line_heads] [selected_line_id]}
+    bind . <$M1B-m> {rmbranch [selected_line_heads] [selected_line_id] 1}
+    bind . <$M1B-b> {mkbranch [selected_line_id]}

This second line should not be in this commit, should it?

Quoted 13 lines

gitk-git/gitk +1 -1

     for {set i 1} {$i < 10} {incr i} {
         bind . <$M1B-Key-$i> [list go_to_parent $i]
     }

@@ -2735,7 +2737,7 @@ proc makewindow {} {
     makemenu $headctxmenu {
         {mc "Check out this branch" command {checkout [list $headmenuhead] $headmenuid}}
         {mc "Rename this branch" command mvbranch}
-        {mc "Remove this branch" command rmbranch}
+        {mc "Remove this branch" command {rmbranch [list $headmenuhead] $headmenuid 0}}
         {mc "Copy branch name" command {clipboard clear; clipboard append $headmenuhead}}
     }
     $headctxmenu configure -tearoff 0

gitk-git/gitk proc keys {} { · line 3191

Quoted 5 lines

gitk-git/gitk +2 -0

@@ -3185,6 +3187,8 @@ proc keys {} {
 [mc "<F5>		Update"]
 [mc "<%s-T>		Reset current branch to selected commit" $M1T]
 [mc "<%s-O>		Check out selected commit" $M1T]
+[mc "<%s-B>		Create branch on selected commit" $M1T]
+[mc "<%s-M>		Remove selected branch" $M1T]

Same here.

Quoted 73 lines

gitk-git/gitk +49 -11

 " \
             -justify left -bg $bgcolor -border 2 -relief groove
     pack $w.m -side top -fill both -padx 2 -pady 2

@@ -10121,32 +10125,102 @@ proc readcheckoutstat {fd head_to_checkout ref_to_checkout id_to_checkout} {
     }
 }
 
-proc rmbranch {} {
-    global headmenuid headmenuhead mainhead
+proc rmbranch {heads_on_commit id shouldComfirm} {
+    global mainhead
     global idheads
+    global confirm_ok sel_ix NS
 
-    set head $headmenuhead
-    set id $headmenuid
-    # this check shouldn't be needed any more...
-    if {$head eq $mainhead} {
+    if {[llength $heads_on_commit] == 0} {
+        error_popup [mc "Cannot delete a detached head"]
+        return
+    }
+
+    if {[llength $heads_on_commit] == 1 && [lindex $heads_on_commit 0] eq $mainhead} {
         error_popup [mc "Cannot delete the currently checked-out branch"]
         return
     }
-    set dheads [descheads $id]
-    if {[llength $dheads] == 1 && $idheads($dheads) eq $head} {
-        # the stuff on this branch isn't on any other branch
-        if {![confirm_popup [mc "The commits on branch %s aren't on any other\
-                        branch.\nReally delete branch %s?" $head $head]]} return
+
+    # Filter out mainhead
+    set mainhead_ix [lsearch $heads_on_commit $mainhead]
+    if {$mainhead_ix != -1} {
+        set heads_on_commit [lreplace $heads_on_commit $mainhead_ix $mainhead_ix]
+    }
+
+    # Filter out remote branches
+    foreach head_ix [lsearch -all $heads_on_commit "remotes/*"] {
+        set heads_on_commit [lreplace $heads_on_commit $head_ix $head_ix]
+    }
+
+    if {[llength $heads_on_commit] == 0} {
+        # Probably only current branch and its remote branch on commit
+        error_popup [mc "Cannot delete branch"]
+        return
     }
+
+    set nr_heads_on_commit [llength $heads_on_commit]
+    set first_head [lindex $heads_on_commit 0]
+
+    if {$nr_heads_on_commit == 1} {
+        # Only a single head, simple comfirm dialogs
+
+        set head_to_remove $first_head
+        set dheads [descheads $id]
+
+        if {[llength $dheads] == 1 && $idheads($dheads) eq $head_to_remove} {
+            # the stuff on this branch isn't on any other branch
+            if {![confirm_popup [mc "The commits on branch %s aren't on any other\
+                            branch.\nReally delete branch %s?" $head_to_remove $head_to_remove]]} return
+        } elseif {$shouldComfirm} {
+            if {![confirm_popup [mc "Really delete branch %s?" $head_to_remove]]} return
+        }
+    } else {
+        # Select branch dialog
+
+        set confirm_ok 0
+
+        set w ".selectbranch"
+        ttk_toplevel $w

gitk-git/gitk proc readcheckoutstat {fd head_to_checkout ref_to_checkout id_to_checkout} { · line 10192

Quoted 10 lines

gitk-git/gitk +10 -0

+        make_transient $w .
+        wm title $w [mc "Delete branch"]
+        ${NS}::label $w.m -text [mc "Which branch to delete?"]
+        pack $w.m -side top -fill x -padx 20 -pady 20
+        ${NS}::frame $w.f
+
+        set sel_ix 0
+        for {set i 0} {$i < $nr_heads_on_commit} {incr i} {
+            ${NS}::radiobutton $w.f.id_$i -value $i -variable sel_ix \
+                -text [lindex $heads_on_commit $i]

The same comment as in the earlier commit about the checkout command applies here: I don't think that radio buttons are the correct UI for a branch selection.

In general, such duplication of code should not happen. The dialog only differs in the title and help text and should be factored into a helper function.

Quoted 37 lines

gitk-git/gitk +25 -3

+            bind $w.f.id_$i <Key-Up> "set sel_ix [expr ($i - 1) % $nr_heads_on_commit]"
+            bind $w.f.id_$i <Key-Down> "set sel_ix [expr ($i + 1) % $nr_heads_on_commit]"
+            grid $w.f.id_$i -sticky w -padx 20
+        }
+
+        pack $w.f -side top -fill x -padx 4
+        ${NS}::button $w.ok -text [mc OK] -command "set confirm_ok 1; destroy $w"
+        bind $w <Key-Return> "set confirm_ok 1; destroy $w"
+        pack $w.ok -side left -fill x -padx 20 -pady 20
+        ${NS}::button $w.cancel -text [mc Cancel] -command "destroy $w"
+        bind $w <Key-Escape> [list destroy $w]
+        pack $w.cancel -side right -fill x -padx 20 -pady 20
+        bind $w <Visibility> "grab $w; focus $w.f.id_$sel_ix"
+
+        tkwait window $w
+        if {!$confirm_ok} return
+
+        set head_to_remove [lindex $heads_on_commit $sel_ix]
+    }
+
+    # Perform the command
+
     nowbusy rmbranch
     update
-    if {[catch {exec git branch -D $head} err]} {
+    if {[catch {exec git branch -D $head_to_remove} err]} {
         notbusy rmbranch
         error_popup $err
         return
     }
-    removehead $id $head
-    removedhead $id $head
+    removehead $id $head_to_remove
+    removedhead $id $head_to_remove
     redrawtags $id
     notbusy rmbranch
     dispneartags 0