From: Jens Lideström <jens@lidestrom.se>
Date: Wed, 28 Jun 2023 07:12:50 GMT
Subject: Re: [PATCH 6/9] gitk: add keyboard bind for create and remove branch
Message-ID: <36b3ce90d4ecc9eb9fa5174aba0cf9d4@lidestrom.se>
In-Reply-To: <7c73cc47-302d-8706-dd7f-fd034ef8d945@kdbg.org>

>> +[mc "<%s-C>		Create branch on selected commit" $M1T]
> 
> ... "C"? Which one is it?

"C" is a mistake. Good catch, thanks!

I choose Ctrl-B to avoid a conflict with Ctrl-C for copying text.

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

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.

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

I'll do so.

/Jens

On 2023-06-28 07:59, Johannes Sixt wrote:
> Am 27.06.23 um 16:41 schrieb Jens Lidestrom via GitGitGadget:
>> From: Jens Lidestrom <jens@lidestrom.se>
>> 
>> Signed-off-by: Jens Lidestrom <jens@lidestrom.se>
>> ---
>>  gitk-git/gitk | 25 ++++++++++++++++---------
>>  1 file changed, 16 insertions(+), 9 deletions(-)
>> 
>> diff --git a/gitk-git/gitk b/gitk-git/gitk
>> index 596977abe89..0d83a72a424 100755
>> --- a/gitk-git/gitk
>> +++ b/gitk-git/gitk
>> @@ -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...
> 
>>      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
>> @@ -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?
> 
>> +[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
>> @@ -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
>> +    } elseif {$shouldComfirm} {
>> +        if {![confirm_popup [mc "Really delete branch %s?" $head]]} 
>> return
>>      }
>>      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