git-gui, feature request: add hotkeys to focus different widgets

git-gui, feature request: add hotkeys to focus different widgets

[Birger Skogeng Pedersen]

To fully use git-gui with keyboard-only, a few more hotkeys are
needed. I am missing hotkeys to change focus between the "Unstanged
Changes", "Staged Changes", diff viewer and "Commit Message" widgets.
I would like to be able to stage, browse, unstage and commit in
git-gui, all without using the mouse.

I propose that CTRL+(number) could be used as hotkeys to change the
focus between the four widgets I've mentioned.


Best regards,
Birger Skogeng Pedersen

[PATCH] git-gui: Add hotkeys to change focus between ui widgets

[Birger Skogeng Pedersen]

The user cannot change focus between the list of files, the diff view
and the commit message widgets without using the mouse (clicking either of
the four widgets ).

Hotkeys CTRL/CMD+number (1-4) now focuses the first file of either the
"Unstaged Changes" or "Staged Changes", the diff view or the
commit message dialog widgets, respectively. This enables the user to
select/unselect files, view the diff and create a commit in git-gui
using keyboard-only.

Signed-off-by: Birger Skogeng Pedersen <birgersp@gmail.com>
---
 git-gui/git-gui.sh | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 91c00e648..bdbe166f7 100755

(First timere here, any feedback is highly appreciated)

--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -2664,6 +2664,35 @@ proc show_less_context {} {
 	}
 }
 
+proc select_first_path {w} {
+	global file_lists last_clicked selected_paths
+	if {[llength $file_lists($w)] > 0} {
+		focus $w
+		set last_clicked [list $w 1]
+		show_diff [lindex $file_lists($w) 0] $w
+	}
+}
+
+proc select_first_unstaged_changes_path {} {
+	global ui_workdir
+	select_first_path $ui_workdir
+}
+
+proc select_first_staged_changes_path {} {
+	global ui_index
+	select_first_path $ui_index
+}
+
+proc focus_diff {} {
+	global ui_diff
+	focus $ui_diff
+}
+
+proc focus_commit_message {} {
+	global ui_comm
+	focus $ui_comm
+}
+
 ######################################################################
 ##
 ## ui construction
@@ -3876,6 +3905,11 @@ foreach i [list $ui_index $ui_workdir] {
 }
 unset i
 
+bind . <$M1B-Key-1> {select_first_unstaged_changes_path}
+bind . <$M1B-Key-2> {select_first_staged_changes_path}
+bind . <$M1B-Key-3> {focus_diff}
+bind . <$M1B-Key-4> {focus_commit_message}
+
 set file_lists($ui_index) [list]
 set file_lists($ui_workdir) [list]
 
-- 
2.16.2.266.g75bb9601e

Re: [PATCH] git-gui: Add hotkeys to change focus between ui widgets

[Birger Skogeng Pedersen]

Hi,

I've discovered a bug, I'll be sending a new version soon.

br
Birger

On Fri, Feb 23, 2018 at 11:22 AM, Birger Skogeng Pedersen
<birgersp@gmail.com> wrote:
> The user cannot change focus between the list of files, the diff view
> and the commit message widgets without using the mouse (clicking either of
> the four widgets ).
>
> Hotkeys CTRL/CMD+number (1-4) now focuses the first file of either the
> "Unstaged Changes" or "Staged Changes", the diff view or the
> commit message dialog widgets, respectively. This enables the user to
> select/unselect files, view the diff and create a commit in git-gui
> using keyboard-only.
>
> Signed-off-by: Birger Skogeng Pedersen <birgersp@gmail.com>
> ---
>  git-gui/git-gui.sh | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
> index 91c00e648..bdbe166f7 100755
>
> (First timere here, any feedback is highly appreciated)
>
> --- a/git-gui/git-gui.sh
> +++ b/git-gui/git-gui.sh
> @@ -2664,6 +2664,35 @@ proc show_less_context {} {
>         }
>  }
>
> +proc select_first_path {w} {
> +       global file_lists last_clicked selected_paths
> +       if {[llength $file_lists($w)] > 0} {
> +               focus $w
> +               set last_clicked [list $w 1]
> +               show_diff [lindex $file_lists($w) 0] $w
> +       }
> +}
> +
> +proc select_first_unstaged_changes_path {} {
> +       global ui_workdir
> +       select_first_path $ui_workdir
> +}
> +
> +proc select_first_staged_changes_path {} {
> +       global ui_index
> +       select_first_path $ui_index
> +}
> +
> +proc focus_diff {} {
> +       global ui_diff
> +       focus $ui_diff
> +}
> +
> +proc focus_commit_message {} {
> +       global ui_comm
> +       focus $ui_comm
> +}
> +
>  ######################################################################
>  ##
>  ## ui construction
> @@ -3876,6 +3905,11 @@ foreach i [list $ui_index $ui_workdir] {
>  }
>  unset i
>
> +bind . <$M1B-Key-1> {select_first_unstaged_changes_path}
> +bind . <$M1B-Key-2> {select_first_staged_changes_path}
> +bind . <$M1B-Key-3> {focus_diff}
> +bind . <$M1B-Key-4> {focus_commit_message}
> +
>  set file_lists($ui_index) [list]
>  set file_lists($ui_workdir) [list]
>
> --
> 2.16.2.266.g75bb9601e
>

[PATCH] git-gui: Add hotkeys to change focus between ui widgets

[Birger Skogeng Pedersen]

The user cannot change focus between the list of files, the diff view
and the commit message widgets without using the mouse (clicking either of
the four widgets ).

Hotkeys CTRL/CMD+number (1-4) now focuses the first file of either the
"Unstaged Changes" or "Staged Changes", the diff view or the
commit message dialog widgets, respectively. This enables the user to
select/unselect files, view the diff and create a commit in git-gui
using keyboard-only.

Signed-off-by: Birger Skogeng Pedersen <birgersp@gmail.com>
---
 git-gui/git-gui.sh | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 91c00e648..f96c0a6b8 100755

(This is my first patch ever, any feedback is highly appreciated)

--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -2664,6 +2664,38 @@ proc show_less_context {} {
 	}
 }
 
+proc select_first_path {w} {
+	global file_lists last_clicked selected_paths
+	if {[llength $file_lists($w)] > 0} {
+		focus $w
+		set last_clicked [list $w 1]
+		set path [lindex $file_lists($w) 0]
+		array unset selected_paths
+		set selected_paths($path) 1
+		show_diff $path $w
+	}
+}
+
+proc select_first_unstaged_changes_path {} {
+	global ui_workdir
+	select_first_path $ui_workdir
+}
+
+proc select_first_staged_changes_path {} {
+	global ui_index
+	select_first_path $ui_index
+}
+
+proc focus_diff {} {
+	global ui_diff
+	focus $ui_diff
+}
+
+proc focus_commit_message {} {
+	global ui_comm
+	focus $ui_comm
+}
+
 ######################################################################
 ##
 ## ui construction
@@ -3876,6 +3908,11 @@ foreach i [list $ui_index $ui_workdir] {
 }
 unset i
 
+bind . <$M1B-Key-1> {select_first_unstaged_changes_path}
+bind . <$M1B-Key-2> {select_first_staged_changes_path}
+bind . <$M1B-Key-3> {focus_diff}
+bind . <$M1B-Key-4> {focus_commit_message}
+
 set file_lists($ui_index) [list]
 set file_lists($ui_workdir) [list]
 
-- 
2.16.2.268.g7f9c27f2f

Re: [PATCH] git-gui: Add hotkeys to change focus between ui widgets

[Johannes Schindelin]

Hi Birger,

On Wed, 28 Feb 2018, Birger Skogeng Pedersen wrote:

> The user cannot change focus between the list of files, the diff view
> and the commit message widgets without using the mouse (clicking either of
> the four widgets ).
> 
> Hotkeys CTRL/CMD+number (1-4) now focuses the first file of either the
> "Unstaged Changes" or "Staged Changes", the diff view or the
> commit message dialog widgets, respectively. This enables the user to
> select/unselect files, view the diff and create a commit in git-gui
> using keyboard-only.

I like this!

> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
> index 91c00e648..f96c0a6b8 100755
> 
> (This is my first patch ever, any feedback is highly appreciated)

I am not an expert in Tcl/Tk, but I'll do my best to comment on this
patch.

> --- a/git-gui/git-gui.sh
> +++ b/git-gui/git-gui.sh
> @@ -2664,6 +2664,38 @@ proc show_less_context {} {
>  	}
>  }
>  
> +proc select_first_path {w} {
> +	global file_lists last_clicked selected_paths
> +	if {[llength $file_lists($w)] > 0} {
> +		focus $w
> +		set last_clicked [list $w 1]
> +		set path [lindex $file_lists($w) 0]
> +		array unset selected_paths
> +		set selected_paths($path) 1
> +		show_diff $path $w
> +	}
> +}

Do you think there is a way to focus on the last-selected path? That would
make this feature even more convenient, I think.

I am not sure that this information is still there if switching back from
another component...

> +proc select_first_unstaged_changes_path {} {
> +	global ui_workdir
> +	select_first_path $ui_workdir
> +}
> +
> +proc select_first_staged_changes_path {} {
> +	global ui_index
> +	select_first_path $ui_index
> +}
> +
> +proc focus_diff {} {
> +	global ui_diff
> +	focus $ui_diff
> +}
> +
> +proc focus_commit_message {} {
> +	global ui_comm
> +	focus $ui_comm
> +}
> +
>  ######################################################################
>  ##
>  ## ui construction
> @@ -3876,6 +3908,11 @@ foreach i [list $ui_index $ui_workdir] {
>  }
>  unset i
>  
> +bind . <$M1B-Key-1> {select_first_unstaged_changes_path}
> +bind . <$M1B-Key-2> {select_first_staged_changes_path}
> +bind . <$M1B-Key-3> {focus_diff}
> +bind . <$M1B-Key-4> {focus_commit_message}
> +
>  set file_lists($ui_index) [list]
>  set file_lists($ui_workdir) [list]

Looks good!

We are currently without an active Git GUI maintainer, so I hope that
Junio (the Git maintainer) will pick this up.

Ciao,
Johannes

Re: [PATCH] git-gui: Add hotkeys to change focus between ui widgets

[Birger Skogeng Pedersen]

Thanks for the feedback.

On Mon, Mar 5, 2018 at 5:55 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Do you think there is a way to focus on the last-selected path? That would
> make this feature even more convenient, I think.

Yes, good idea. I'll implement it and create a second version.

> I am not sure that this information is still there if switching back from
> another component...

I don't think so. But I can add a variable to hold the last selected
(clicked) path for both widgets.

Thanks (again),
Birger

On Mon, Mar 5, 2018 at 5:55 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Birger,
>
> On Wed, 28 Feb 2018, Birger Skogeng Pedersen wrote:
>
>> The user cannot change focus between the list of files, the diff view
>> and the commit message widgets without using the mouse (clicking either of
>> the four widgets ).
>>
>> Hotkeys CTRL/CMD+number (1-4) now focuses the first file of either the
>> "Unstaged Changes" or "Staged Changes", the diff view or the
>> commit message dialog widgets, respectively. This enables the user to
>> select/unselect files, view the diff and create a commit in git-gui
>> using keyboard-only.
>
> I like this!
>
>> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
>> index 91c00e648..f96c0a6b8 100755
>>
>> (This is my first patch ever, any feedback is highly appreciated)
>
> I am not an expert in Tcl/Tk, but I'll do my best to comment on this
> patch.
>
>> --- a/git-gui/git-gui.sh
>> +++ b/git-gui/git-gui.sh
>> @@ -2664,6 +2664,38 @@ proc show_less_context {} {
>>       }
>>  }
>>
>> +proc select_first_path {w} {
>> +     global file_lists last_clicked selected_paths
>> +     if {[llength $file_lists($w)] > 0} {
>> +             focus $w
>> +             set last_clicked [list $w 1]
>> +             set path [lindex $file_lists($w) 0]
>> +             array unset selected_paths
>> +             set selected_paths($path) 1
>> +             show_diff $path $w
>> +     }
>> +}
>
> Do you think there is a way to focus on the last-selected path? That would
> make this feature even more convenient, I think.
>
> I am not sure that this information is still there if switching back from
> another component...
>
>> +proc select_first_unstaged_changes_path {} {
>> +     global ui_workdir
>> +     select_first_path $ui_workdir
>> +}
>> +
>> +proc select_first_staged_changes_path {} {
>> +     global ui_index
>> +     select_first_path $ui_index
>> +}
>> +
>> +proc focus_diff {} {
>> +     global ui_diff
>> +     focus $ui_diff
>> +}
>> +
>> +proc focus_commit_message {} {
>> +     global ui_comm
>> +     focus $ui_comm
>> +}
>> +
>>  ######################################################################
>>  ##
>>  ## ui construction
>> @@ -3876,6 +3908,11 @@ foreach i [list $ui_index $ui_workdir] {
>>  }
>>  unset i
>>
>> +bind . <$M1B-Key-1> {select_first_unstaged_changes_path}
>> +bind . <$M1B-Key-2> {select_first_staged_changes_path}
>> +bind . <$M1B-Key-3> {focus_diff}
>> +bind . <$M1B-Key-4> {focus_commit_message}
>> +
>>  set file_lists($ui_index) [list]
>>  set file_lists($ui_workdir) [list]
>
> Looks good!
>
> We are currently without an active Git GUI maintainer, so I hope that
> Junio (the Git maintainer) will pick this up.
>
> Ciao,
> Johannes

Re: [PATCH] git-gui: Add hotkeys to change focus between ui widgets

[Junio C Hamano]

Birger Skogeng Pedersen <birgersp@gmail.com> writes:

> Thanks for the feedback.
>
> On Mon, Mar 5, 2018 at 5:55 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> Do you think there is a way to focus on the last-selected path? That would
>> make this feature even more convenient, I think.
>
> Yes, good idea. I'll implement it and create a second version.

Please make it a patch against the main git-gui project, not against
our tree.  That is, your patch text would look like this:

    diff --git a/git-gui.sh b/git-gui.sh
    index 5bc21b878d..39e80ebafa 100755
    --- a/git-gui.sh
    +++ b/git-gui.sh
    @@ -3843,6 +3843,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-KP_Enter> do_commit
     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 }

We've seen three patches to git-gui from three different people in
the past week.  The project seems to be abandoned and we need to
find a volunteer (or a few) to take it over, it seems.  In the
meantime I have blindly been picking and queuing git-gui changes
but because I am not even a casual user of it, I know I will not do
a good job maintaining it in the longer term.

[PATCH] git-gui: Add hotkeys to set widget focus

[Birger Skogeng Pedersen]

The user cannot change focus between the list of files, the diff view and
the commit message widgets without using the mouse (clicking either of
the four widgets ).

Hotkeys CTRL/CMD+number (1-4) now focuses a previously selected path from
either the "Unstaged Changes" or "Staged Changes", the diff view or the
commit message dialog widgets, respectively. This enables the user to
select/unselect files, view the diff and create a commit in git-gui
using keyboard-only.

Signed-off-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
---
 git-gui/git-gui.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 6de74ce639..cbd0b69804 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -2494,7 +2494,7 @@ proc force_first_diff {after} {
 
 proc toggle_or_diff {mode w args} {
 	global file_states file_lists current_diff_path ui_index ui_workdir
-	global last_clicked selected_paths
+	global last_clicked selected_paths file_lists_last_clicked
 
 	if {$mode eq "click"} {
 		foreach {x y} $args break
@@ -2551,6 +2551,8 @@ proc toggle_or_diff {mode w args} {
 	$ui_index tag remove in_sel 0.0 end
 	$ui_workdir tag remove in_sel 0.0 end
 
+	set file_lists_last_clicked($w) $lno
+
 	# Determine the state of the file
 	if {[info exists file_states($path)]} {
 		set state [lindex $file_states($path) 0]
@@ -2664,6 +2666,51 @@ proc show_less_context {} {
 	}
 }
 
+proc select_first_path {w} {
+	global file_lists last_clicked selected_paths ui_workdir
+	global file_lists_last_clicked
+
+	set _list_length [llength $file_lists($w)]
+
+	if {$_list_length > 0} {
+
+		set _index $file_lists_last_clicked($w)
+
+		if {$_index eq {}} {
+			set _index 1
+		} elseif {$_index > $_list_length} {
+			set _index $_list_length
+		}
+
+		focus $w
+		set last_clicked [list $w $_index]
+		set path [lindex $file_lists($w) [expr $_index - 1]]
+		array unset selected_paths
+		set selected_paths($path) 1
+		show_diff $path $w
+	}
+}
+
+proc select_first_unstaged_changes_path {} {
+	global ui_workdir
+	select_first_path $ui_workdir
+}
+
+proc select_first_staged_changes_path {} {
+	global ui_index
+	select_first_path $ui_index
+}
+
+proc focus_diff {} {
+	global ui_diff
+	focus $ui_diff
+}
+
+proc focus_commit_message {} {
+	global ui_comm
+	focus $ui_comm
+}
+
 ######################################################################
 ##
 ## ui construction
@@ -3877,6 +3924,14 @@ foreach i [list $ui_index $ui_workdir] {
 }
 unset i
 
+bind . <$M1B-Key-1> {select_first_unstaged_changes_path}
+bind . <$M1B-Key-2> {select_first_staged_changes_path}
+bind . <$M1B-Key-3> {focus_diff}
+bind . <$M1B-Key-4> {focus_commit_message}
+
+set file_lists_last_clicked($ui_index) {}
+set file_lists_last_clicked($ui_workdir) {}
+
 set file_lists($ui_index) [list]
 set file_lists($ui_workdir) [list]
 
-- 
2.23.0.37.g745f681289

Re: [PATCH] git-gui: Add hotkeys to set widget focus

[Birger Skogeng Pedersen]

(Finally picking this up again)

I've been using this feature for about one year now. If I may say so,
I think it is actually really great. It's really helpful for me to be
able to use git-gui with just the keyboard, and I'm hoping others will
find it useful, too.

Let me know what you guys think.

Re: [PATCH] git-gui: Add hotkeys to set widget focus

[Pratyush Yadav]

Hi Birger,

In case you haven't been following the list, Pat has been inactive 
recently, so I am acting as the interim maintainer of git-gui for now, 
because no one else stepped up and Junio would rather not maintain it.

You can find my fork over at https://github.com/prati0100/git-gui. I 
munged your patches to apply on my tree (which is separate from the 
git.git tree), but it would be great if you base them on my tree next 
time around.

On 31/08/19 02:23PM, Birger Skogeng Pedersen wrote:
> The user cannot change focus between the list of files, the diff view and
> the commit message widgets without using the mouse (clicking either of
> the four widgets ).

Nit: s/widgets )/widgets)/

> 
> Hotkeys CTRL/CMD+number (1-4) now focuses a previously selected path from
> either the "Unstaged Changes" or "Staged Changes", the diff view or the
> commit message dialog widgets, respectively. This enables the user to
> select/unselect files, view the diff and create a commit in git-gui
> using keyboard-only.
> 
> Signed-off-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
> ---
>  git-gui/git-gui.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
> index 6de74ce639..cbd0b69804 100755
> --- a/git-gui/git-gui.sh
> +++ b/git-gui/git-gui.sh
> @@ -2494,7 +2494,7 @@ proc force_first_diff {after} {
>  
>  proc toggle_or_diff {mode w args} {
>  	global file_states file_lists current_diff_path ui_index ui_workdir
> -	global last_clicked selected_paths
> +	global last_clicked selected_paths file_lists_last_clicked
>  
>  	if {$mode eq "click"} {
>  		foreach {x y} $args break
> @@ -2551,6 +2551,8 @@ proc toggle_or_diff {mode w args} {
>  	$ui_index tag remove in_sel 0.0 end
>  	$ui_workdir tag remove in_sel 0.0 end
>  
> +	set file_lists_last_clicked($w) $lno
> +
>  	# Determine the state of the file
>  	if {[info exists file_states($path)]} {
>  		set state [lindex $file_states($path) 0]
> @@ -2664,6 +2666,51 @@ proc show_less_context {} {
>  	}
>  }
>  
> +proc select_first_path {w} {

I have recently been going through the git-gui code, and my biggest 
gripe was non-descriptive single letter variable names. So maybe 
s/w/window/

> +	global file_lists last_clicked selected_paths ui_workdir
> +	global file_lists_last_clicked

Aside: this almost made me ask why you declared file_lists_last_clicked 
twice :D

> +
> +	set _list_length [llength $file_lists($w)]
> +
> +	if {$_list_length > 0} {
> +

Nit: Drop the blank line.

> +		set _index $file_lists_last_clicked($w)

If some files are added/removed via an external command, that means the 
index we choose won't be the file the user last looked at, correct? What 
about using path names instead, so we know exactly which file to 
display, even though its index might have changed?

But if it is not a trivial change, and needs a lot of work, I'm fine 
with the way things are. If the user changes stuff outside of git-gui, 
some side effects are to be expected.

> +
> +		if {$_index eq {}} {
> +			set _index 1
> +		} elseif {$_index > $_list_length} {
> +			set _index $_list_length

Just to be sure: _index should start at 1 right, and not 0?

> +		}
> +
> +		focus $w
> +		set last_clicked [list $w $_index]
> +		set path [lindex $file_lists($w) [expr $_index - 1]]
> +		array unset selected_paths
> +		set selected_paths($path) 1
> +		show_diff $path $w
> +	}

If _list_length is 0 (iow, no files are staged/unstaged), this won't 
change the focus at all. Are you sure this is the desired behaviour?  
Would it make no sense if we switch to an empty pane? The user did 
explicitly hit the button combo to go there. Yes, they won't be able to 
actually do anything, but what is the harm in switching focus if the 
user explicitly requests it?

> +}
> +
> +proc select_first_unstaged_changes_path {} {
> +	global ui_workdir
> +	select_first_path $ui_workdir
> +}
> +
> +proc select_first_staged_changes_path {} {
> +	global ui_index
> +	select_first_path $ui_index
> +}
> +
> +proc focus_diff {} {
> +	global ui_diff
> +	focus $ui_diff
> +}
> +
> +proc focus_commit_message {} {
> +	global ui_comm
> +	focus $ui_comm
> +}
> +

Do you expect these functions to be re-used somewhere in the near 
future? Otherwise...

>  ######################################################################
>  ##
>  ## ui construction
> @@ -3877,6 +3924,14 @@ foreach i [list $ui_index $ui_workdir] {
>  }
>  unset i
>  
> +bind . <$M1B-Key-1> {select_first_unstaged_changes_path}
> +bind . <$M1B-Key-2> {select_first_staged_changes_path}
> +bind . <$M1B-Key-3> {focus_diff}
> +bind . <$M1B-Key-4> {focus_commit_message}

... why not just put their bodies directly in here? Something like:

  bind . <$M1B-Key-1> {
	global $ui_workdir
	select_first_path $ui_workdir
  }

> +
> +set file_lists_last_clicked($ui_index) {}
> +set file_lists_last_clicked($ui_workdir) {}
> +
>  set file_lists($ui_index) [list]
>  set file_lists($ui_workdir) [list]

Overall, IMO it is a great idea. I tested it, and it works fine on my 
setup. Thanks.

-- 
Regards,
Pratyush Yadav

Re: [PATCH] git-gui: Add hotkeys to set widget focus

[Junio C Hamano]

Pratyush Yadav <me@yadavpratyush.com> writes:

> ... I am acting as the interim maintainer of git-gui for now, 
> ...
> You can find my fork over at https://github.com/prati0100/git-gui.

I am aware of three topics (call-do-quit-before-exit, reload-config
and revert-hunks-lines) of your own, plus your 'master' over there.
I'd expect that you'd also queue others' topics in the repository as
you act as the (interim) maintainer, updating the changes as they
get reviews, and then eventually when a topic matures enough [*1*],
you'd merge it to your 'master' and tell me to pull from there.

	side note *1*.  Decidinging when a topic gets mature enough
	is at your discretion as a subsystem maintainer.  With your
	reputation on line, I think everybody on the list would
	trust that you'd make a good judgment ;-).

Thanks again for volunteering.

Re: [PATCH] git-gui: Add hotkeys to set widget focus

[Birger Skogeng Pedersen]

Hello Pratyush,


(New patch according to our discussion coming up)


On Sun, Sep 1, 2019 at 1:32 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> In case you haven't been following the list, Pat has been inactive
> recently, so I am acting as the interim maintainer of git-gui for now,
> because no one else stepped up and Junio would rather not maintain it.

I saw a discussion about it some time ago. But I didn't catch that you
are the maintainer. That's great! I use git-gui all the time, so I'm
happy to hear someone is going to maintain it.


> ... it would be great if you base them on my tree next
> time around.

Okay, I will.


> I have recently been going through the git-gui code, and my biggest
> gripe was non-descriptive single letter variable names. So maybe
> s/w/window/

I agree about non-descriptive variables. I was following the style of
the rest of git-gui.


> If some files are added/removed via an external command, that means the
> index we choose won't be the file the user last looked at, correct? What
> about using path names instead, so we know exactly which file to
> display, even though its index might have changed?
>
> But if it is not a trivial change, and needs a lot of work, I'm fine
> with the way things are. If the user changes stuff outside of git-gui,
> some side effects are to be expected.

A somewhat non-trivial change, for me at least. To implement what
you're suggesting, I'm gonna need some help or the patch will be
delayed quite a lot...

So honestly, I'd appreciate it if we could leave it like this (for
now, at least).


> > +
> > +             if {$_index eq {}} {
> > +                     set _index 1
> > +             } elseif {$_index > $_list_length} {
> > +                     set _index $_list_length
>
> Just to be sure: _index should start at 1 right, and not 0?

I'm quite sure this is correct. Setting the index to 0 throws an error.


> If _list_length is 0 (iow, no files are staged/unstaged), this won't
> change the focus at all. Are you sure this is the desired behaviour?
> Would it make no sense if we switch to an empty pane? The user did
> explicitly hit the button combo to go there. Yes, they won't be able to
> actually do anything, but what is the harm in switching focus if the
> user explicitly requests it?

An error is thrown if we force focus to the "Unstaged Changes" widget
when it has no files listed. The same goes for the "Staged Changes"
widget. That's why I put the condition there.


> Do you expect these functions to be re-used somewhere in the near
> future?

Not really, but I feel the "key bindings" section of the script should
have as little logic as possible (and just be a bunch of key bindings
invoking functions).
Also I think function names are a good way to describe what the code
is doing, so personally I actually think it's better like this.
But if you feel strongly that it should be like you suggested, I'm open to it.


Best regards,
Birger

Re: [PATCH] git-gui: Add hotkeys to set widget focus

[Bert Wesarg]

Hi Birger,

On Sat, Aug 31, 2019 at 2:23 PM Birger Skogeng Pedersen
<birger.sp@gmail.com> wrote:
>
> The user cannot change focus between the list of files, the diff view and
> the commit message widgets without using the mouse (clicking either of
> the four widgets ).

that bugged me two, but never come up with a good idea.

>
> Hotkeys CTRL/CMD+number (1-4) now focuses a previously selected path from
> either the "Unstaged Changes" or "Staged Changes", the diff view or the
> commit message dialog widgets, respectively. This enables the user to
> select/unselect files, view the diff and create a commit in git-gui
> using keyboard-only.

But I don't understand this in full. Does this mean pressing CTRL+1 or
+2 does also changes the file selection? Why isn't it sufficient to
just focus the respective file list widget? And than have bindings to
change the selection?

Bert

>
> Signed-off-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
> ---
>  git-gui/git-gui.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
> index 6de74ce639..cbd0b69804 100755
> --- a/git-gui/git-gui.sh
> +++ b/git-gui/git-gui.sh
> @@ -2494,7 +2494,7 @@ proc force_first_diff {after} {
>
>  proc toggle_or_diff {mode w args} {
>         global file_states file_lists current_diff_path ui_index ui_workdir
> -       global last_clicked selected_paths
> +       global last_clicked selected_paths file_lists_last_clicked
>
>         if {$mode eq "click"} {
>                 foreach {x y} $args break
> @@ -2551,6 +2551,8 @@ proc toggle_or_diff {mode w args} {
>         $ui_index tag remove in_sel 0.0 end
>         $ui_workdir tag remove in_sel 0.0 end
>
> +       set file_lists_last_clicked($w) $lno
> +
>         # Determine the state of the file
>         if {[info exists file_states($path)]} {
>                 set state [lindex $file_states($path) 0]
> @@ -2664,6 +2666,51 @@ proc show_less_context {} {
>         }
>  }
>
> +proc select_first_path {w} {
> +       global file_lists last_clicked selected_paths ui_workdir
> +       global file_lists_last_clicked
> +
> +       set _list_length [llength $file_lists($w)]
> +
> +       if {$_list_length > 0} {
> +
> +               set _index $file_lists_last_clicked($w)
> +
> +               if {$_index eq {}} {
> +                       set _index 1
> +               } elseif {$_index > $_list_length} {
> +                       set _index $_list_length
> +               }
> +
> +               focus $w
> +               set last_clicked [list $w $_index]
> +               set path [lindex $file_lists($w) [expr $_index - 1]]
> +               array unset selected_paths
> +               set selected_paths($path) 1
> +               show_diff $path $w
> +       }
> +}
> +
> +proc select_first_unstaged_changes_path {} {
> +       global ui_workdir
> +       select_first_path $ui_workdir
> +}
> +
> +proc select_first_staged_changes_path {} {
> +       global ui_index
> +       select_first_path $ui_index
> +}
> +
> +proc focus_diff {} {
> +       global ui_diff
> +       focus $ui_diff
> +}
> +
> +proc focus_commit_message {} {
> +       global ui_comm
> +       focus $ui_comm
> +}
> +
>  ######################################################################
>  ##
>  ## ui construction
> @@ -3877,6 +3924,14 @@ foreach i [list $ui_index $ui_workdir] {
>  }
>  unset i
>
> +bind . <$M1B-Key-1> {select_first_unstaged_changes_path}
> +bind . <$M1B-Key-2> {select_first_staged_changes_path}
> +bind . <$M1B-Key-3> {focus_diff}
> +bind . <$M1B-Key-4> {focus_commit_message}
> +
> +set file_lists_last_clicked($ui_index) {}
> +set file_lists_last_clicked($ui_workdir) {}
> +
>  set file_lists($ui_index) [list]
>  set file_lists($ui_workdir) [list]
>
> --
> 2.23.0.37.g745f681289
>

Re: [PATCH] git-gui: Add hotkeys to set widget focus

[Bert Wesarg]

On Sun, Sep 1, 2019 at 1:32 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > +
> > +proc select_first_unstaged_changes_path {} {
> > +     global ui_workdir
> > +     select_first_path $ui_workdir
> > +}
> > +
> > +proc select_first_staged_changes_path {} {
> > +     global ui_index
> > +     select_first_path $ui_index
> > +}
> > +
> > +proc focus_diff {} {
> > +     global ui_diff
> > +     focus $ui_diff
> > +}
> > +
> > +proc focus_commit_message {} {
> > +     global ui_comm
> > +     focus $ui_comm
> > +}
> > +
>
> Do you expect these functions to be re-used somewhere in the near
> future? Otherwise...
>
> >  ######################################################################
> >  ##
> >  ## ui construction
> > @@ -3877,6 +3924,14 @@ foreach i [list $ui_index $ui_workdir] {
> >  }
> >  unset i
> >
> > +bind . <$M1B-Key-1> {select_first_unstaged_changes_path}
> > +bind . <$M1B-Key-2> {select_first_staged_changes_path}
> > +bind . <$M1B-Key-3> {focus_diff}
> > +bind . <$M1B-Key-4> {focus_commit_message}
>
> ... why not just put their bodies directly in here? Something like:
>
>   bind . <$M1B-Key-1> {
>         global $ui_workdir
>         select_first_path $ui_workdir
>   }

that should also work:

bind . <$M1B-Key-1> {select_first_path $::ui_workdir}

Re: [PATCH] git-gui: Add hotkeys to set widget focus

[Birger Skogeng Pedersen]

Hello Bert,


> But I don't understand this in full. Does this mean pressing CTRL+1 or
> +2 does also changes the file selection? Why isn't it sufficient to
> just focus the respective file list widget? And than have bindings to
> change the selection?
>
> Bert

I don't think it's feasible to focus either of those two widgets
without explicitly selecting a file. I tried it at first, but I
couldn't get it working. I'm not going to claim it's impossible (it's
probably possible), but I gave it up.
Consider this: when you click either of those two widgets with your
mouse, you're not really clicking the widget. You're clicking a file
listed in that widget. Clicking just the widget (in the blank space
beneath the files) does nothing in git-gui. You have to actually click
a file for the focus to change.

(To be precise, pressing CTRL+1/2 selects the same file that was
already selected in that widget)

Reading your question, I realised my wording in the commit message
wasn't very good. I'll try to improve it.


Best regards,
Birger

[PATCH] [PATCH] git-gui: Add hotkeys to set widget focus

[Birger Skogeng Pedersen]

The user cannot change focus between the list of files, the diff view and
the commit message widgets without using the mouse (clicking either of
the four widgets).

With this patch, the user may set ui focus to the previously selected path
in either the "Unstaged Changes" or "Staged Changes" widgets, using
CTRL/CMD+1 or CTRL/CMD+2.

The user may also set the ui focus to the diff view widget with
CTRL/CMD+3, or to the commit message widget with CTRL/CMD+4.

This enables the user to select/unselect files, view the diff and create a
commit in git-gui using keyboard-only.

Signed-off-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
---
 git-gui.sh | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/git-gui.sh b/git-gui.sh
index 5bc21b8..ce620f1 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -2495,7 +2495,7 @@ proc force_first_diff {after} {
 
 proc toggle_or_diff {mode w args} {
 	global file_states file_lists current_diff_path ui_index ui_workdir
-	global last_clicked selected_paths
+	global last_clicked selected_paths file_lists_last_clicked
 
 	if {$mode eq "click"} {
 		foreach {x y} $args break
@@ -2527,6 +2527,8 @@ proc toggle_or_diff {mode w args} {
 	$ui_index tag remove in_sel 0.0 end
 	$ui_workdir tag remove in_sel 0.0 end
 
+	set file_lists_last_clicked($w) $lno
+
 	# Determine the state of the file
 	if {[info exists file_states($path)]} {
 		set state [lindex $file_states($path) 0]
@@ -2640,6 +2642,29 @@ proc show_less_context {} {
 	}
 }
 
+proc select_path_in {widget} {
+	global file_lists last_clicked selected_paths ui_workdir
+	global file_lists_last_clicked
+
+	set _list_length [llength $file_lists($widget)]
+	if {$_list_length > 0} {
+
+		set _index $file_lists_last_clicked($widget)
+		if {$_index eq {}} {
+			set _index 1
+		} elseif {$_index > $_list_length} {
+			set _index $_list_length
+		}
+
+		focus $widget
+		set last_clicked [list $widget $_index]
+		set path [lindex $file_lists($widget) [expr $_index - 1]]
+		array unset selected_paths
+		set selected_paths($path) 1
+		show_diff $path $widget
+	}
+}
+
 ######################################################################
 ##
 ## ui construction
@@ -3852,6 +3877,14 @@ foreach i [list $ui_index $ui_workdir] {
 }
 unset i
 
+bind . <$M1B-Key-1> {select_path_in $::ui_workdir}
+bind . <$M1B-Key-2> {select_path_in $::ui_index}
+bind . <$M1B-Key-3> {focus $::ui_diff}
+bind . <$M1B-Key-4> {focus $::ui_comm}
+
+set file_lists_last_clicked($ui_index) {}
+set file_lists_last_clicked($ui_workdir) {}
+
 set file_lists($ui_index) [list]
 set file_lists($ui_workdir) [list]
 
-- 
2.23.0.37.g745f681289

Re: [PATCH] git-gui: Add hotkeys to set widget focus

[Philip Oakley]

Hi Pratyus,
On 01/09/2019 12:32, Pratyush Yadav wrote:
> Hi Birger,
>
> In case you haven't been following the list, Pat has been inactive
> recently, so I am acting as the interim maintainer of git-gui for now,
> because no one else stepped up and Junio would rather not maintain it.
>
> You can find my fork over athttps://github.com/prati0100/git-gui. I
> munged your patches to apply on my tree (which is separate from the
> git.git tree), but it would be great if you base them on my tree next
> time around.

Are there any plans or thoughts about creating a more inclusive man page 
for the git-gui?

Such things as the Options dialog linkages [1], and how to drive the 
command line options are areas I've wondered about over the years.

Not exactly sure how our plain text man pages and formatted HTML would 
fare for describing the gui layout and where to click. One thing I am 
noting is that these hotkey nicely have numbers so can easily be used 
for reference..

Philip

[1] 
https://stackoverflow.com/questions/6007823/is-there-a-help-page-for-the-git-gui-options-dialog

Re: [PATCH] git-gui: Add hotkeys to set widget focus

[Pratyush Yadav]

On 01/09/19 11:27PM, Philip Oakley wrote:
> Hi Pratyus,
> On 01/09/2019 12:32, Pratyush Yadav wrote:
> > Hi Birger,
> > 
> > In case you haven't been following the list, Pat has been inactive
> > recently, so I am acting as the interim maintainer of git-gui for now,
> > because no one else stepped up and Junio would rather not maintain it.
> > 
> > You can find my fork over athttps://github.com/prati0100/git-gui. I
> > munged your patches to apply on my tree (which is separate from the
> > git.git tree), but it would be great if you base them on my tree next
> > time around.
> 
> Are there any plans or thoughts about creating a more inclusive man page for
> the git-gui?
 
Having better documentation has been one of the things I have in my 
future plans, but I can't really say when I can get to it depending on 
my schedule and time available. I have a couple other topics active 
which I'd like to get resolved first.

Of course, if someone else is willing to take the initiative, I'm happy 
to help :)

> Such things as the Options dialog linkages [1], and how to drive the command
> line options are areas I've wondered about over the years.
> 
> Not exactly sure how our plain text man pages and formatted HTML would fare
> for describing the gui layout and where to click. One thing I am noting is
> that these hotkey nicely have numbers so can easily be used for reference..
 
For the options dialog, I think a "tooltip" (something like what you get 
when you hover over a image in a browser) that describes the option is a 
better idea than having a separate man page. I don't expect the option 
descriptions to be too long or complicated. This approach has the added 
benefit of not having to maintain a separate man page. Whenever someone 
adds a new options, they have to add its description as well.

I also think the "tools" feature needs some documentation, especially 
about what environment variables we export.

Other than these two, I don't see many places that need too much 
documentation. Rest of the UI is pretty self-intuitive, at least to me.

> Philip
> 
> [1] https://stackoverflow.com/questions/6007823/is-there-a-help-page-for-the-git-gui-options-dialog

-- 
Regards,
Pratyush Yadav

Re: [PATCH] git-gui: Add hotkeys to set widget focus

[Philip Oakley]

On 02/09/2019 13:25, Pratyush Yadav wrote:
> On 01/09/19 11:27PM, Philip Oakley wrote:
>> Hi Pratyus,
>> On 01/09/2019 12:32, Pratyush Yadav wrote:
>>> Hi Birger,
>>>
>>> In case you haven't been following the list, Pat has been inactive
>>> recently, so I am acting as the interim maintainer of git-gui for now,
>>> because no one else stepped up and Junio would rather not maintain it.
>>>
>>> You can find my fork over athttps://github.com/prati0100/git-gui. I
>>> munged your patches to apply on my tree (which is separate from the
>>> git.git tree), but it would be great if you base them on my tree next
>>> time around.
>> Are there any plans or thoughts about creating a more inclusive man page for
>> the git-gui?
>   
> Having better documentation has been one of the things I have in my
> future plans, but I can't really say when I can get to it depending on
> my schedule and time available. I have a couple other topics active
> which I'd like to get resolved first.
>
> Of course, if someone else is willing to take the initiative, I'm happy
> to help :)

The main aspect that would help for providing a contribution would be to 
at least decide the (rough) framework/format for a full Gui 'man page'. 
The existing one 
https://github.com/git/git/blob/master/Documentation/git-gui.txt is 
rather short. (would also need the sub-tree integration to be finessed)

e.g.
1. how much should it be done via 'include' files (like the git-config 
man page now does include::config.txt[] and onwards).

2. Does it use the doc-book man-page format, or something akin to the 
former tutorial format? (everything appears to have shifted to the man 
page format, so looks like man format is the one.. [1,2,3,4]

I'm thinking that, as it is a big job, it will need the documentation to 
be split over a number of small include files so that more folk can be 
contributors.

>> Such things as the Options dialog linkages [1], and how to drive the command
>> line options are areas I've wondered about over the years.
>>
>> Not exactly sure how our plain text man pages and formatted HTML would fare
>> for describing the gui layout and where to click. One thing I am noting is
>> that these hotkey nicely have numbers so can easily be used for reference..
>   
> For the options dialog, I think a "tooltip" (something like what you get
> when you hover over a image in a browser) that describes the option is a
> better idea than having a separate man page. I don't expect the option
> descriptions to be too long or complicated. This approach has the added
> benefit of not having to maintain a separate man page. Whenever someone
> adds a new options, they have to add its description as well.
A tool tip that says 'see git help config.. ' could be done. Any 
pointers to an existing one for trying a cookie cutter approach getting 
started on those ones?
>
> I also think the "tools" feature needs some documentation, especially
> about what environment variables we export.
>
> Other than these two, I don't see many places that need too much
> documentation. Rest of the UI is pretty self-intuitive, at least to me.
>
>> Philip
>>
>> [1] https://stackoverflow.com/questions/6007823/is-there-a-help-page-for-the-git-gui-options-dialog
[1,2,3,4] 
https://github.com/git/git/blob/master/Documentation/giteveryday.txt
https://github.com/git/git/blob/master/Documentation/gittutorial.txt
https://github.com/git/git/blob/master/Documentation/gitcore-tutorial.txt
https://github.com/git/git/blob/master/Documentation/everyday.txto

Re: [PATCH] [PATCH] git-gui: Add hotkeys to set widget focus

[Pratyush Yadav]

Hi Birger,

Other than a minor change that I can do locally, this looks good. I'll 
test it a bit and then merge it. Thanks.

On 01/09/19 09:36PM, Birger Skogeng Pedersen wrote:
[snip]
> +proc select_path_in {widget} {
> +	global file_lists last_clicked selected_paths ui_workdir
> +	global file_lists_last_clicked
> +
> +	set _list_length [llength $file_lists($widget)]
> +	if {$_list_length > 0} {
> +

You missed removing this extra blank line. Will fix it up locally.

> +		set _index $file_lists_last_clicked($widget)
[snip]

-- 
Regards,
Pratyush Yadav

Re: [PATCH] [PATCH] git-gui: Add hotkeys to set widget focus

[Birger Skogeng Pedersen]

On Mon, Sep 2, 2019 at 8:19 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> You missed removing this extra blank line. Will fix it up locally.
>
> > +             set _index $file_lists_last_clicked($widget)
> [snip]


To be honest I felt the blank line there was appropriate, in an
attempt to follow the same code style as the rest of the git-gui.sh.
For instance, the "add_range_to_selection" function
(https://github.com/prati0100/git-gui/blob/master/git-gui.sh#L2601-L2625)
has blank lines like this;


proc add_range_to_selection {w x y} {
    global file_lists last_clicked selected_paths

    if {[lindex $last_clicked 0] ne $w} {
        toggle_or_diff click $w $x $y
        return
    }

    set lno [lindex [split [$w index @$x,$y] .] 0]
    set lc [lindex $last_clicked 1]
    if {$lc < $lno} {
        set begin $lc
        set end $lno
    } else {
        set begin $lno
        set end $lc
    }

    foreach path [lrange $file_lists($w) \
        [expr {$begin - 1}] \
        [expr {$end - 1}]] {
        set selected_paths($path) 1
    }
    $w tag add in_sel $begin.0 [expr {$end + 1}].0
}


Not saying the blank line should defintely be there, I'm just saying
my reasoning for keeping it there in the first place :-)


Birger

Re: [PATCH] [PATCH] git-gui: Add hotkeys to set widget focus

[Pratyush Yadav]

On 02/09/19 08:35PM, Birger Skogeng Pedersen wrote:
> On Mon, Sep 2, 2019 at 8:19 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > You missed removing this extra blank line. Will fix it up locally.
> >
> > > +             set _index $file_lists_last_clicked($widget)
> > [snip]
> 
> 
> To be honest I felt the blank line there was appropriate, in an
> attempt to follow the same code style as the rest of the git-gui.sh.
> For instance, the "add_range_to_selection" function
> (https://github.com/prati0100/git-gui/blob/master/git-gui.sh#L2601-L2625)
> has blank lines like this;
 
Oh yes, these types of blank lines are perfectly fine in my opinion too.  
I actually prefer this alternative over not having any spacing at all.  
This way you divide the function in logical sections/steps.

But that was not what I pointed out. The blank line I pointed out was 
like this:

  if {condition} {
  	
	foo
	bar
  }

My nitpick was with the blank line _just after_ the if.

> 
> proc add_range_to_selection {w x y} {
>     global file_lists last_clicked selected_paths
> 

So in this example, the above blank line is fine...

>     if {[lindex $last_clicked 0] ne $w} {

...but if I add a blank line here like:

  if {[lindex $last_clicked 0] ne $w} {

	toggle_or_diff click $w $x $y
	return
  }

instead of like:

  if {[lindex $last_clicked 0] ne $w} {
	toggle_or_diff click $w $x $y
	return
  }

It is the blank line just after the start of your if statement that I 
was pointing out. I did leave other blank lines untouched in your patch.

[snip]

-- 
Regards,
Pratyush Yadav

Re: [PATCH] [PATCH] git-gui: Add hotkeys to set widget focus

[Birger Skogeng Pedersen]

Ah, sorry I misunderstood you.

Thanks,
Birger

Re: [PATCH] [PATCH] git-gui: Add hotkeys to set widget focus

[Bert Wesarg]

On Sun, Sep 1, 2019 at 9:37 PM Birger Skogeng Pedersen
<birger.sp@gmail.com> wrote:
>
> The user cannot change focus between the list of files, the diff view and
> the commit message widgets without using the mouse (clicking either of
> the four widgets).
>
> With this patch, the user may set ui focus to the previously selected path
> in either the "Unstaged Changes" or "Staged Changes" widgets, using
> CTRL/CMD+1 or CTRL/CMD+2.
>
> The user may also set the ui focus to the diff view widget with
> CTRL/CMD+3, or to the commit message widget with CTRL/CMD+4.
>
> This enables the user to select/unselect files, view the diff and create a
> commit in git-gui using keyboard-only.
>
> Signed-off-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
> ---
>  git-gui.sh | 35 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index 5bc21b8..ce620f1 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -2495,7 +2495,7 @@ proc force_first_diff {after} {
>
>  proc toggle_or_diff {mode w args} {
>         global file_states file_lists current_diff_path ui_index ui_workdir
> -       global last_clicked selected_paths
> +       global last_clicked selected_paths file_lists_last_clicked
>
>         if {$mode eq "click"} {
>                 foreach {x y} $args break
> @@ -2527,6 +2527,8 @@ proc toggle_or_diff {mode w args} {
>         $ui_index tag remove in_sel 0.0 end
>         $ui_workdir tag remove in_sel 0.0 end
>
> +       set file_lists_last_clicked($w) $lno

So we only remember the lno in the widget, that could mean, that we
select the wrong file after a rescan, which shifted the previous path
one down. Can we remember the pathname instead, and try to find this
again in the file list?

> +
>         # Determine the state of the file
>         if {[info exists file_states($path)]} {
>                 set state [lindex $file_states($path) 0]
> @@ -2640,6 +2642,29 @@ proc show_less_context {} {
>         }
>  }
>
> +proc select_path_in {widget} {

can we name it 'focus_and_select_path_in', as the main job ob this
function is to focus the widget. It makes also the 'bind' command
below more readily, because than all bind commands start with 'focus'.

> +       global file_lists last_clicked selected_paths ui_workdir

ui_workdir not referenced in this function

> +       global file_lists_last_clicked
> +
> +       set _list_length [llength $file_lists($widget)]
> +       if {$_list_length > 0} {
> +
> +               set _index $file_lists_last_clicked($widget)

I have the impression that variables starting with '_' are mainly used
as read-only global variables, see the list at line 158, and not that
often as temporal local variables.

> +               if {$_index eq {}} {
> +                       set _index 1
> +               } elseif {$_index > $_list_length} {
> +                       set _index $_list_length
> +               }
> +
> +               focus $widget
> +               set last_clicked [list $widget $_index]
> +               set path [lindex $file_lists($widget) [expr $_index - 1]]
> +               array unset selected_paths
> +               set selected_paths($path) 1
> +               show_diff $path $widget
> +       }
> +}
> +
>  ######################################################################
>  ##
>  ## ui construction
> @@ -3852,6 +3877,14 @@ foreach i [list $ui_index $ui_workdir] {
>  }
>  unset i
>
> +bind . <$M1B-Key-1> {select_path_in $::ui_workdir}
> +bind . <$M1B-Key-2> {select_path_in $::ui_index}
> +bind . <$M1B-Key-3> {focus $::ui_diff}
> +bind . <$M1B-Key-4> {focus $::ui_comm}

I would like to bring up a proposal: AFAICS, more or less all CTRL
bindings have a menu entry. But it does not make sense to have a menu
entry for these bindings. And I think we could add more bindings for
keyboard-afine users. Thus I would like to propose to use ALT as the
modifier for these bindings, which would give us a nice binding
classification.

How about that?

Bert

> +
> +set file_lists_last_clicked($ui_index) {}
> +set file_lists_last_clicked($ui_workdir) {}
> +
>  set file_lists($ui_index) [list]
>  set file_lists($ui_workdir) [list]
>
> --
> 2.23.0.37.g745f681289
>

Re: [PATCH] [PATCH] git-gui: Add hotkeys to set widget focus

[Birger Skogeng Pedersen]

Hi Bert,


On Mon, Sep 2, 2019 at 9:42 PM Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> So we only remember the lno in the widget, that could mean, that we
> select the wrong file after a rescan, which shifted the previous path
> one down. Can we remember the pathname instead, and try to find this
> again in the file list?

Seems to me like a rescan makes git-gui select the first file in the
list, regardless of this feature (patch) or not. Ideally, git-gui
would not do this. It should select the same file that was selected
before (if it can). But honestly it seems like a separate (perhaps
broader) issue. My biggest issue is not having the hotkeys, and that's
what I'm trying to mend :-)


> can we name it 'focus_and_select_path_in', as the main job ob this
> function is to focus the widget. It makes also the 'bind' command
> below more readily, because than all bind commands start with 'focus'.

Agreed.


> > +       global file_lists_last_clicked
> > +
> > +       set _list_length [llength $file_lists($widget)]
> > +       if {$_list_length > 0} {
> > +
> > +               set _index $file_lists_last_clicked($widget)
>
> I have the impression that variables starting with '_' are mainly used
> as read-only global variables, see the list at line 158, and not that
> often as temporal local variables.

Agreed, I'll rename it. It's been almost a year since I wrote this
patch so I can't really remember my reasoning for putting the
underscore there.


> I would like to bring up a proposal: AFAICS, more or less all CTRL
> bindings have a menu entry. But it does not make sense to have a menu
> entry for these bindings. And I think we could add more bindings for
> keyboard-afine users. Thus I would like to propose to use ALT as the
> modifier for these bindings, which would give us a nice binding
> classification.
>
> How about that?

Makes sense. And the ALT+1/2/3/4 seems to be unused for anything else.
At least on Windows, maybe on other systems those keystrokes are
already in use?


Birger

Re: [PATCH] [PATCH] git-gui: Add hotkeys to set widget focus

[Pratyush Yadav]

On 02/09/19 09:42PM, Bert Wesarg wrote:
> On Sun, Sep 1, 2019 at 9:37 PM Birger Skogeng Pedersen
> <birger.sp@gmail.com> wrote:
> >
> > The user cannot change focus between the list of files, the diff view and
> > the commit message widgets without using the mouse (clicking either of
> > the four widgets).
> >
> > With this patch, the user may set ui focus to the previously selected path
> > in either the "Unstaged Changes" or "Staged Changes" widgets, using
> > CTRL/CMD+1 or CTRL/CMD+2.
> >
> > The user may also set the ui focus to the diff view widget with
> > CTRL/CMD+3, or to the commit message widget with CTRL/CMD+4.
> >
> > This enables the user to select/unselect files, view the diff and create a
> > commit in git-gui using keyboard-only.
> >
> > Signed-off-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
> > ---
> >  git-gui.sh | 35 ++++++++++++++++++++++++++++++++++-
> >  1 file changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/git-gui.sh b/git-gui.sh
> > index 5bc21b8..ce620f1 100755
> > --- a/git-gui.sh
> > +++ b/git-gui.sh
> > @@ -2495,7 +2495,7 @@ proc force_first_diff {after} {
> >
> >  proc toggle_or_diff {mode w args} {
> >         global file_states file_lists current_diff_path ui_index ui_workdir
> > -       global last_clicked selected_paths
> > +       global last_clicked selected_paths file_lists_last_clicked
> >
> >         if {$mode eq "click"} {
> >                 foreach {x y} $args break
> > @@ -2527,6 +2527,8 @@ proc toggle_or_diff {mode w args} {
> >         $ui_index tag remove in_sel 0.0 end
> >         $ui_workdir tag remove in_sel 0.0 end
> >
> > +       set file_lists_last_clicked($w) $lno
> 
> So we only remember the lno in the widget, that could mean, that we
> select the wrong file after a rescan, which shifted the previous path
> one down. Can we remember the pathname instead, and try to find this
> again in the file list?
 
I raised a similar concern. Birger's response was that it is not a 
trivial change for him, and he needs help with it. So I decided to keep 
it like it is.

But now I thought about it a bit more, and I don't think it should be 
too difficult. A quick look tells me that $file_lists should be a sorted 
list with unique entries (git-gui.sh::display_file_helper{}), so it 
shouldn't be too difficult to find a given path. display_file_helper 
does:

  set lno [lsearch -sorted -exact $file_lists($w) $path]

which should also be what we want.

I have a quick-and-dirty fix for it. Haven't tested it too well, but it 
seems to work for some basic things like removing a file and refreshing, 
and then selecting focus again. Do give it a spin. I'll send the patch 
in reply to this email.

> > +
> >         # Determine the state of the file
> >         if {[info exists file_states($path)]} {
> >                 set state [lindex $file_states($path) 0]
> > @@ -2640,6 +2642,29 @@ proc show_less_context {} {
> >         }
> >  }
> >
> > +proc select_path_in {widget} {
> 
> can we name it 'focus_and_select_path_in', as the main job ob this
> function is to focus the widget. It makes also the 'bind' command
> below more readily, because than all bind commands start with 'focus'.

Ah, I was kind of uneasy with the function name, but couldn't come up 
with a good one. This sounds all right. Another suggestion that I'd put 
out: "focus_widget". As you said, focusing a widget is the main job of 
this function. Selecting paths is secondary. IMO it should be fine to 
not have "select_path_in" in the function name because you select the 
path in the process of focusing on widget.
 
> > +       global file_lists last_clicked selected_paths ui_workdir
> 
> ui_workdir not referenced in this function

Nice catch.

> > +       global file_lists_last_clicked
> > +
> > +       set _list_length [llength $file_lists($widget)]
> > +       if {$_list_length > 0} {
> > +
> > +               set _index $file_lists_last_clicked($widget)
> 
> I have the impression that variables starting with '_' are mainly used
> as read-only global variables, see the list at line 158, and not that
> often as temporal local variables.
> 
> > +               if {$_index eq {}} {
> > +                       set _index 1
> > +               } elseif {$_index > $_list_length} {
> > +                       set _index $_list_length
> > +               }
> > +
> > +               focus $widget
> > +               set last_clicked [list $widget $_index]
> > +               set path [lindex $file_lists($widget) [expr $_index - 1]]
> > +               array unset selected_paths
> > +               set selected_paths($path) 1
> > +               show_diff $path $widget
> > +       }
> > +}
> > +
> >  ######################################################################
> >  ##
> >  ## ui construction
> > @@ -3852,6 +3877,14 @@ foreach i [list $ui_index $ui_workdir] {
> >  }
> >  unset i
> >
> > +bind . <$M1B-Key-1> {select_path_in $::ui_workdir}
> > +bind . <$M1B-Key-2> {select_path_in $::ui_index}
> > +bind . <$M1B-Key-3> {focus $::ui_diff}
> > +bind . <$M1B-Key-4> {focus $::ui_comm}
> 
> I would like to bring up a proposal: AFAICS, more or less all CTRL
> bindings have a menu entry. But it does not make sense to have a menu
> entry for these bindings. And I think we could add more bindings for
> keyboard-afine users. Thus I would like to propose to use ALT as the
> modifier for these bindings, which would give us a nice binding
> classification.
> 
> How about that?
 
FWIW, I agree with this. To add to this, it would also somewhat match 
with bindings in other programs. For example, in Firefox you can choose 
tabs 0-9 with Alt+0-9. In gnome-terminal, you can also choose tabs with 
Alt+0-9. AFAIK Chrome does this too.

My one concern is, does an Alt key exist on Mac (AFAIK it does, but I 
want to be sure)? I don't think we have any existing bindings with Alt, 
so will they work well with Macs?

-- 
Regards,
Pratyush Yadav

[PATCH] git-gui: use path name instead of list index to track last clicked file

[Pratyush Yadav]

Birger,

You would probably want to squash this patch with yours when you send a
re-roll. Of course, I'd like some comments and tests on the patch before
considering it "done". Just letting you know that I'd like to have this
change in your original patch/commit, not as a separate commit. I put it
in a separate patch for now for easier readability.

Also, FYI, pass '--scissors' to git-am when applying this to not get the
above text in the commit message.

-- >8 --
For using the hotkeys CTRL/CMD+1/2/3/4, we save the index of the last
clicked file. This index may change if some external command changes the
repo state. So use the path name of the file instead.

Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
---
 git-gui.sh | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index ce620f1..9be1b6a 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -2527,7 +2527,7 @@ proc toggle_or_diff {mode w args} {
 	$ui_index tag remove in_sel 0.0 end
 	$ui_workdir tag remove in_sel 0.0 end

-	set file_lists_last_clicked($w) $lno
+	set file_lists_last_clicked($w) $path

 	# Determine the state of the file
 	if {[info exists file_states($path)]} {
@@ -2648,17 +2648,15 @@ proc select_path_in {widget} {

 	set _list_length [llength $file_lists($widget)]
 	if {$_list_length > 0} {
-
-		set _index $file_lists_last_clicked($widget)
-		if {$_index eq {}} {
-			set _index 1
-		} elseif {$_index > $_list_length} {
-			set _index $_list_length
+		set path $file_lists_last_clicked($widget)
+		set index [lsearch -sorted -exact $file_lists($widget) $path]
+		if {$index < 0} {
+			set index 0
+			set path [lindex $file_lists($widget) $index]
 		}

 		focus $widget
-		set last_clicked [list $widget $_index]
-		set path [lindex $file_lists($widget) [expr $_index - 1]]
+		set last_clicked [list $widget [expr $index + 1]]
 		array unset selected_paths
 		set selected_paths($path) 1
 		show_diff $path $widget
--
2.21.0

Re: [PATCH] [PATCH] git-gui: Add hotkeys to set widget focus

[Bert Wesarg]

On Tue, Sep 3, 2019 at 4:22 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
>
> On 02/09/19 09:42PM, Bert Wesarg wrote:
> > On Sun, Sep 1, 2019 at 9:37 PM Birger Skogeng Pedersen
> > <birger.sp@gmail.com> wrote:
> > >
> > > The user cannot change focus between the list of files, the diff view and
> > > the commit message widgets without using the mouse (clicking either of
> > > the four widgets).
> > >
> > > With this patch, the user may set ui focus to the previously selected path
> > > in either the "Unstaged Changes" or "Staged Changes" widgets, using
> > > CTRL/CMD+1 or CTRL/CMD+2.
> > >
> > > The user may also set the ui focus to the diff view widget with
> > > CTRL/CMD+3, or to the commit message widget with CTRL/CMD+4.
> > >
> > > This enables the user to select/unselect files, view the diff and create a
> > > commit in git-gui using keyboard-only.
> > >
> > > Signed-off-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
> > > ---
> > >  git-gui.sh | 35 ++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 34 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/git-gui.sh b/git-gui.sh
> > > index 5bc21b8..ce620f1 100755
> > > --- a/git-gui.sh
> > > +++ b/git-gui.sh
> > > @@ -2495,7 +2495,7 @@ proc force_first_diff {after} {
> > >
> > >  proc toggle_or_diff {mode w args} {
> > >         global file_states file_lists current_diff_path ui_index ui_workdir
> > > -       global last_clicked selected_paths
> > > +       global last_clicked selected_paths file_lists_last_clicked
> > >
> > >         if {$mode eq "click"} {
> > >                 foreach {x y} $args break
> > > @@ -2527,6 +2527,8 @@ proc toggle_or_diff {mode w args} {
> > >         $ui_index tag remove in_sel 0.0 end
> > >         $ui_workdir tag remove in_sel 0.0 end
> > >
> > > +       set file_lists_last_clicked($w) $lno
> >
> > So we only remember the lno in the widget, that could mean, that we
> > select the wrong file after a rescan, which shifted the previous path
> > one down. Can we remember the pathname instead, and try to find this
> > again in the file list?
>
> I raised a similar concern. Birger's response was that it is not a
> trivial change for him, and he needs help with it. So I decided to keep
> it like it is.
>
> But now I thought about it a bit more, and I don't think it should be
> too difficult. A quick look tells me that $file_lists should be a sorted
> list with unique entries (git-gui.sh::display_file_helper{}), so it
> shouldn't be too difficult to find a given path. display_file_helper
> does:
>
>   set lno [lsearch -sorted -exact $file_lists($w) $path]
>
> which should also be what we want.
>
> I have a quick-and-dirty fix for it. Haven't tested it too well, but it
> seems to work for some basic things like removing a file and refreshing,
> and then selecting focus again. Do give it a spin. I'll send the patch
> in reply to this email.
>
> > > +
> > >         # Determine the state of the file
> > >         if {[info exists file_states($path)]} {
> > >                 set state [lindex $file_states($path) 0]
> > > @@ -2640,6 +2642,29 @@ proc show_less_context {} {
> > >         }
> > >  }
> > >
> > > +proc select_path_in {widget} {
> >
> > can we name it 'focus_and_select_path_in', as the main job ob this
> > function is to focus the widget. It makes also the 'bind' command
> > below more readily, because than all bind commands start with 'focus'.
>
> Ah, I was kind of uneasy with the function name, but couldn't come up
> with a good one. This sounds all right. Another suggestion that I'd put
> out: "focus_widget". As you said, focusing a widget is the main job of
> this function. Selecting paths is secondary. IMO it should be fine to
> not have "select_path_in" in the function name because you select the
> path in the process of focusing on widget.
>
> > > +       global file_lists last_clicked selected_paths ui_workdir
> >
> > ui_workdir not referenced in this function
>
> Nice catch.
>
> > > +       global file_lists_last_clicked
> > > +
> > > +       set _list_length [llength $file_lists($widget)]
> > > +       if {$_list_length > 0} {
> > > +
> > > +               set _index $file_lists_last_clicked($widget)
> >
> > I have the impression that variables starting with '_' are mainly used
> > as read-only global variables, see the list at line 158, and not that
> > often as temporal local variables.
> >
> > > +               if {$_index eq {}} {
> > > +                       set _index 1
> > > +               } elseif {$_index > $_list_length} {
> > > +                       set _index $_list_length
> > > +               }
> > > +
> > > +               focus $widget
> > > +               set last_clicked [list $widget $_index]
> > > +               set path [lindex $file_lists($widget) [expr $_index - 1]]
> > > +               array unset selected_paths
> > > +               set selected_paths($path) 1
> > > +               show_diff $path $widget
> > > +       }
> > > +}
> > > +
> > >  ######################################################################
> > >  ##
> > >  ## ui construction
> > > @@ -3852,6 +3877,14 @@ foreach i [list $ui_index $ui_workdir] {
> > >  }
> > >  unset i
> > >
> > > +bind . <$M1B-Key-1> {select_path_in $::ui_workdir}
> > > +bind . <$M1B-Key-2> {select_path_in $::ui_index}
> > > +bind . <$M1B-Key-3> {focus $::ui_diff}
> > > +bind . <$M1B-Key-4> {focus $::ui_comm}
> >
> > I would like to bring up a proposal: AFAICS, more or less all CTRL
> > bindings have a menu entry. But it does not make sense to have a menu
> > entry for these bindings. And I think we could add more bindings for
> > keyboard-afine users. Thus I would like to propose to use ALT as the
> > modifier for these bindings, which would give us a nice binding
> > classification.
> >
> > How about that?
>
> FWIW, I agree with this. To add to this, it would also somewhat match
> with bindings in other programs. For example, in Firefox you can choose
> tabs 0-9 with Alt+0-9. In gnome-terminal, you can also choose tabs with
> Alt+0-9. AFAIK Chrome does this too.

ttk::notebook only provides Ctrl+Tab, Ctrl+Shift+Tab, and
Alt+<mnemonic> keybindings as default tab traversal, thus this should
not conflict with Alt+1-4 to begin with.

>
> My one concern is, does an Alt key exist on Mac (AFAIK it does, but I
> want to be sure)? I don't think we have any existing bindings with Alt,
> so will they work well with Macs?

they should ;-)

https://www.macworld.co.uk/how-to/mac/mac-keyboard-shortcuts-3504584/

Bert

>
> --
> Regards,
> Pratyush Yadav

[PATCH v4] git-gui: Add hotkeys to set widget focus

[Birger Skogeng Pedersen]

The user cannot change focus between the list of files, the diff view and
the commit message widgets without using the mouse (clicking either of
the four widgets).

With this patch, the user may set ui focus to the previously selected path
in either the "Unstaged Changes" or "Staged Changes" widgets, using
ALT+1 or ALT+2.

The user may also set the ui focus to the diff view widget with
ALT+3, or to the commit message widget with ALT+4.

This enables the user to select/unselect files, view the diff and create a
commit in git-gui using keyboard-only.

Signed-off-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
---
 git-gui.sh | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/git-gui.sh b/git-gui.sh
index 5bc21b8..3a2df2f 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -2495,7 +2495,7 @@ proc force_first_diff {after} {
 
 proc toggle_or_diff {mode w args} {
 	global file_states file_lists current_diff_path ui_index ui_workdir
-	global last_clicked selected_paths
+	global last_clicked selected_paths file_lists_last_clicked
 
 	if {$mode eq "click"} {
 		foreach {x y} $args break
@@ -2527,6 +2527,8 @@ proc toggle_or_diff {mode w args} {
 	$ui_index tag remove in_sel 0.0 end
 	$ui_workdir tag remove in_sel 0.0 end
 
+	set file_lists_last_clicked($w) $path
+
 	# Determine the state of the file
 	if {[info exists file_states($path)]} {
 		set state [lindex $file_states($path) 0]
@@ -2640,6 +2642,27 @@ proc show_less_context {} {
 	}
 }
 
+proc select_path_in {widget} {
+	global file_lists last_clicked selected_paths
+	global file_lists_last_clicked
+
+	set _list_length [llength $file_lists($widget)]
+	if {$_list_length > 0} {
+		set path $file_lists_last_clicked($widget)
+		set index [lsearch -sorted -exact $file_lists($widget) $path]
+		if {$index < 0} {
+			set index 0
+			set path [lindex $file_lists($widget) $index]
+		}
+
+		focus $widget
+		set last_clicked [list $widget [expr $index + 1]]
+		array unset selected_paths
+		set selected_paths($path) 1
+		show_diff $path $widget
+	}
+}
+
 ######################################################################
 ##
 ## ui construction
@@ -3852,6 +3875,14 @@ foreach i [list $ui_index $ui_workdir] {
 }
 unset i
 
+bind . <Alt-Key-1> {select_path_in $::ui_workdir}
+bind . <Alt-Key-2> {select_path_in $::ui_index}
+bind . <Alt-Key-3> {focus $::ui_diff}
+bind . <Alt-Key-4> {focus $::ui_comm}
+
+set file_lists_last_clicked($ui_index) {}
+set file_lists_last_clicked($ui_workdir) {}
+
 set file_lists($ui_index) [list]
 set file_lists($ui_workdir) [list]
 
-- 
2.21.0.windows.1

Re: [PATCH v4] git-gui: Add hotkeys to set widget focus

[Birger Skogeng Pedersen]

Hi,

(Sorry that I hadn't used the proper version in the subject before,
I'm new (as you could probably tell already))
In addition to your changes, I removed the unused ui_workdir variable
and modified the bindings to be ALT+1/2/3/4.
Shoud I have listed you in the commit? Or did I do it according to
your suggestion?

Thanks,
Birger

Re: [PATCH v4] git-gui: Add hotkeys to set widget focus

[Birger Skogeng Pedersen]

Hi Pratyush,

I just realised I had forgotten about the local variable prefixed with
an underscore. So v5 of the patch will be coming up.
Also I got quite uncertain, should I have added you in the commit msg
somehow? I've seen elsewhere that people add the "Signed-off-by" line
with the name of the other developer.

Birger

Re: [PATCH v4] git-gui: Add hotkeys to set widget focus

[Pratyush Yadav]

Thanks for the re-roll. Before you send out the v5, a couple more things 
you missed. Other than those, LGTM.

On 03/09/19 08:07PM, Birger Skogeng Pedersen wrote:
[snip]
> @@ -2640,6 +2642,27 @@ proc show_less_context {} {
>  	}
>  }
>  
> +proc select_path_in {widget} {

There was a suggestion from me and Bert about changing the function 
name.  I suggested "focus_widget", and Bert suggested 
"focus_and_select_path_in".  Choose whichever you feel is better.

> +	global file_lists last_clicked selected_paths
> +	global file_lists_last_clicked
> +
> +	set _list_length [llength $file_lists($widget)]
> +	if {$_list_length > 0} {

Since we only use $_list_length in one place after my suggestion, why 
not just remove it entirely?

That makes your if statement:

if {[llength $file_lists($widget)] > 0} {
	...

[snip]

-- 
Regards,
Pratyush Yadav

Re: [PATCH] git-gui: Add hotkeys to set widget focus

[Pratyush Yadav]

On 02/09/19 06:23PM, Philip Oakley wrote:
> On 02/09/2019 13:25, Pratyush Yadav wrote:
> > On 01/09/19 11:27PM, Philip Oakley wrote:
> > > Hi Pratyus,
[snip]
> > > Are there any plans or thoughts about creating a more inclusive 
> > > man page for
> > > the git-gui?
> > Having better documentation has been one of the things I have in my
> > future plans, but I can't really say when I can get to it depending on
> > my schedule and time available. I have a couple other topics active
> > which I'd like to get resolved first.
> > 
> > Of course, if someone else is willing to take the initiative, I'm happy
> > to help :)
> 
> The main aspect that would help for providing a contribution would be to at
> least decide the (rough) framework/format for a full Gui 'man page'. The
> existing one
> https://github.com/git/git/blob/master/Documentation/git-gui.txt is rather
> short. (would also need the sub-tree integration to be finessed)
> 
> e.g.
> 1. how much should it be done via 'include' files (like the git-config man
> page now does include::config.txt[] and onwards).
> 
> 2. Does it use the doc-book man-page format, or something akin to the former
> tutorial format? (everything appears to have shifted to the man page format,
> so looks like man format is the one.. [1,2,3,4]
> 
> I'm thinking that, as it is a big job, it will need the documentation to be
> split over a number of small include files so that more folk can be
> contributors.

What exactly do you think we should document?  From what I can see, the 
major topics are "options", "key bindings", and "tools".  Maybe also the 
blame viewer.

If we do options inside the dialog with tooltips, that leaves key 
bindings.  If there is not much else, we might as well do it in the one 
single man page we have, and worry about splitting later when it grows 
in size.

If you intend to have a more comprehensive documentation where we 
demonstrate the UI stuff, then using a man page will handicap us.  In 
that case a HTML page is a better idea.  Although I'm not too sure what 
warrants documentation is the general UI.  It all seems pretty intuitive 
to me, but them I am a "power user" so maybe I'm assuming too much.

> > For the options dialog, I think a "tooltip" (something like what you 
> > get
> > when you hover over a image in a browser) that describes the option is a
> > better idea than having a separate man page. I don't expect the option
> > descriptions to be too long or complicated. This approach has the added
> > benefit of not having to maintain a separate man page. Whenever someone
> > adds a new options, they have to add its description as well.
> A tool tip that says 'see git help config.. ' could be done. Any pointers to
> an existing one for trying a cookie cutter approach getting started on those
> ones?

The "Choose a revision" dialog shows a tooltip. You can get it by 
creating a tool with the "Ask the user to select a revision" option 
selected. Look in lib/choose_rev.tcl.

The blame viewer also uses tooltips. When you hover over a line, it 
shows the commit message of the last commit that touched it. Look in 
lib/blame.tcl.

Then there is Tk's tooltip package [0]. I haven't used it, so can't 
really say what the differences are, and which is better.

If you do end up using the tooltip implemented in choose_rev.tcl and 
blame.tcl, I think it is a good idea to move the common tooltip code in 
a "tooltip framework".

Although I understand that it is a lot of work (especially if you decide 
to refactor the existing tooltip code), and not exactly documentation, 
so it might not be what you want to do.

[0] https://core.tcl-lang.org/tklib/doc/trunk/embedded/www/tklib/files/modules/tooltip/tooltip.html

-- 
Regards,
Pratyush Yadav

[PATCH v5] git-gui: Add hotkeys to set widget focus

[Birger Skogeng Pedersen]

The user cannot change focus between the list of files, the diff view and
the commit message widgets without using the mouse (clicking either of
the four widgets).

With this patch, the user may set ui focus to the previously selected path
in either the "Unstaged Changes" or "Staged Changes" widgets, using
ALT+1 or ALT+2.

The user may also set the ui focus to the diff view widget with
ALT+3, or to the commit message widget with ALT+4.

This enables the user to select/unselect files, view the diff and create a
commit in git-gui using keyboard-only.

Signed-off-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
---
 git-gui.sh | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/git-gui.sh b/git-gui.sh
index 5bc21b8..5dae8da 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -2495,7 +2495,7 @@ proc force_first_diff {after} {
 
 proc toggle_or_diff {mode w args} {
 	global file_states file_lists current_diff_path ui_index ui_workdir
-	global last_clicked selected_paths
+	global last_clicked selected_paths file_lists_last_clicked
 
 	if {$mode eq "click"} {
 		foreach {x y} $args break
@@ -2527,6 +2527,8 @@ proc toggle_or_diff {mode w args} {
 	$ui_index tag remove in_sel 0.0 end
 	$ui_workdir tag remove in_sel 0.0 end
 
+	set file_lists_last_clicked($w) $path
+
 	# Determine the state of the file
 	if {[info exists file_states($path)]} {
 		set state [lindex $file_states($path) 0]
@@ -2640,6 +2642,26 @@ proc show_less_context {} {
 	}
 }
 
+proc focus_widget {widget} {
+	global file_lists last_clicked selected_paths
+	global file_lists_last_clicked
+
+	if {[llength $file_lists($widget)] > 0} {
+		set path $file_lists_last_clicked($widget)
+		set index [lsearch -sorted -exact $file_lists($widget) $path]
+		if {$index < 0} {
+			set index 0
+			set path [lindex $file_lists($widget) $index]
+		}
+
+		focus $widget
+		set last_clicked [list $widget [expr $index + 1]]
+		array unset selected_paths
+		set selected_paths($path) 1
+		show_diff $path $widget
+	}
+}
+
 ######################################################################
 ##
 ## ui construction
@@ -3852,6 +3874,14 @@ foreach i [list $ui_index $ui_workdir] {
 }
 unset i
 
+bind .   <Alt-Key-1> {focus_widget $::ui_workdir}
+bind .   <Alt-Key-2> {focus_widget $::ui_index}
+bind .   <Alt-Key-3> {focus $::ui_diff}
+bind .   <Alt-Key-4> {focus $::ui_comm}
+
+set file_lists_last_clicked($ui_index) {}
+set file_lists_last_clicked($ui_workdir) {}
+
 set file_lists($ui_index) [list]
 set file_lists($ui_workdir) [list]
 
-- 
2.21.0.windows.1

Re: [PATCH v5] git-gui: Add hotkeys to set widget focus

[Johannes Sixt]

Am 04.09.19 um 16:30 schrieb Birger Skogeng Pedersen:
> The user cannot change focus between the list of files, the diff view and
> the commit message widgets without using the mouse (clicking either of
> the four widgets).
> 
> With this patch, the user may set ui focus to the previously selected path
> in either the "Unstaged Changes" or "Staged Changes" widgets, using
> ALT+1 or ALT+2.
> 
> The user may also set the ui focus to the diff view widget with
> ALT+3, or to the commit message widget with ALT+4.

Many keyboards do not have a right Alt-key. That means that Alt+1 to
Alt+4 combinations must be typed single-handed with the left hand. This
is mildly awkward for Alt+4. Can we please have the very important
commit widget *not* at Alt+4? I could live with Alt+3.

-- Hannes

Re: [PATCH v5] git-gui: Add hotkeys to set widget focus

[Birger Skogeng Pedersen]

Hi Johannes,

On Wed, Sep 4, 2019 at 8:59 PM Johannes Sixt <j6t@kdbg.org> wrote:
> Many keyboards do not have a right Alt-key. That means that Alt+1 to
> Alt+4 combinations must be typed single-handed with the left hand. This
> is mildly awkward for Alt+4. Can we please have the very important
> commit widget *not* at Alt+4? I could live with Alt+3.

(RightAlt wouldn't be used by Europeans, anyways)
Are you suggesting to keep Alt+1/2/3 for the files/staged/diff
widgets, but use something other than Alt+4 for the commit dialog? If
so, which one would you prefer?
The initial propsal from me was to use CTRL/CMD+1/2/3/4. What do you
think of using the CTRL/CMD key instead of ALT?

Birger

Re: [PATCH v5] git-gui: Add hotkeys to set widget focus

[Bert Wesarg]

Dear Hannes,

On Wed, Sep 4, 2019 at 8:59 PM Johannes Sixt <j6t@kdbg.org> wrote:
>
> Am 04.09.19 um 16:30 schrieb Birger Skogeng Pedersen:
> > The user cannot change focus between the list of files, the diff view and
> > the commit message widgets without using the mouse (clicking either of
> > the four widgets).
> >
> > With this patch, the user may set ui focus to the previously selected path
> > in either the "Unstaged Changes" or "Staged Changes" widgets, using
> > ALT+1 or ALT+2.
> >
> > The user may also set the ui focus to the diff view widget with
> > ALT+3, or to the commit message widget with ALT+4.
>
> Many keyboards do not have a right Alt-key. That means that Alt+1 to
> Alt+4 combinations must be typed single-handed with the left hand. This
> is mildly awkward for Alt+4. Can we please have the very important
> commit widget *not* at Alt+4? I could live with Alt+3.
>

I use my left thumb to press the left Alt key and it does not feel
mildly awkward. As Alt is also used for the mnemonics, there will
probably more of mildly awkward key combinations, wont there?

Bert

> -- Hannes

Re: [PATCH v5] git-gui: Add hotkeys to set widget focus

[Johannes Sixt]

Am 04.09.19 um 21:20 schrieb Birger Skogeng Pedersen:
> On Wed, Sep 4, 2019 at 8:59 PM Johannes Sixt <j6t@kdbg.org> wrote:
>> Many keyboards do not have a right Alt-key. That means that Alt+1 to
>> Alt+4 combinations must be typed single-handed with the left hand. This
>> is mildly awkward for Alt+4. Can we please have the very important
>> commit widget *not* at Alt+4? I could live with Alt+3.
> 
> (RightAlt wouldn't be used by Europeans, anyways)
> Are you suggesting to keep Alt+1/2/3 for the files/staged/diff
> widgets, but use something other than Alt+4 for the commit dialog? If
> so, which one would you prefer?

I was suggesting Alt+3 for the commit message widget, but my preferences
are actually Alt+1, Alt+2, Alt+3, in this order. My preference for the
diff widget would be Alt+4 (the awkward one) because I do not foresee
that I would use it a lot. Use what remains for the two file lists.

> The initial propsal from me was to use CTRL/CMD+1/2/3/4. What do you
> think of using the CTRL/CMD key instead of ALT?

I would not mind Ctrl instead of Alt. Take your pick.

-- Hannes

Re: [PATCH v5] git-gui: Add hotkeys to set widget focus

[Johannes Sixt]

Am 04.09.19 um 21:55 schrieb Bert Wesarg:
> I use my left thumb to press the left Alt key and it does not feel
> mildly awkward. As Alt is also used for the mnemonics, there will
> probably more of mildly awkward key combinations, wont there?

That may well be the case. However, it is the commit message widget that
I would want to access with a hotkey most of the time. It would be nice
to have one that is the least stressfull for the hand. See my message
nearby for my preferences.

-- Hannes

Re: [PATCH v5] git-gui: Add hotkeys to set widget focus

[Pratyush Yadav]

On 04/09/19 11:39PM, Johannes Sixt wrote:
> Am 04.09.19 um 21:20 schrieb Birger Skogeng Pedersen:
> > On Wed, Sep 4, 2019 at 8:59 PM Johannes Sixt <j6t@kdbg.org> wrote:
> >> Many keyboards do not have a right Alt-key. That means that Alt+1 to
> >> Alt+4 combinations must be typed single-handed with the left hand. This
> >> is mildly awkward for Alt+4. Can we please have the very important
> >> commit widget *not* at Alt+4? I could live with Alt+3.
> > 
> > (RightAlt wouldn't be used by Europeans, anyways)
> > Are you suggesting to keep Alt+1/2/3 for the files/staged/diff
> > widgets, but use something other than Alt+4 for the commit dialog? If
> > so, which one would you prefer?
> 
> I was suggesting Alt+3 for the commit message widget, but my preferences
> are actually Alt+1, Alt+2, Alt+3, in this order. My preference for the
> diff widget would be Alt+4 (the awkward one) because I do not foresee
> that I would use it a lot. Use what remains for the two file lists.

I wonder if that binding is very intuitive.  If we do 1/2 for the top 
and bottom panes on the left side, and 3/4 for the top and bottom panes 
on the right side, that makes some sense.  Doing it your way makes it a 
counter-clockwise motion.

I am not arguing for or against this proposal, just pointing something 
worth thinking about.  Either way, I suppose after a while it becomes 
muscle memory so I'm not sure how much difference this subtle thing will 
make.

> 
> > The initial propsal from me was to use CTRL/CMD+1/2/3/4. What do you
> > think of using the CTRL/CMD key instead of ALT?
> 
> I would not mind Ctrl instead of Alt. Take your pick.

FWIW, I vote for sticking with Alt.

-- 
Regards,
Pratyush Yadav

Re: [PATCH v5] git-gui: Add hotkeys to set widget focus

[Junio C Hamano]

Pratyush Yadav <me@yadavpratyush.com> writes:

> On 04/09/19 11:39PM, Johannes Sixt wrote:
>> Am 04.09.19 um 21:20 schrieb Birger Skogeng Pedersen:
>> > On Wed, Sep 4, 2019 at 8:59 PM Johannes Sixt <j6t@kdbg.org> wrote:
>> >> Many keyboards do not have a right Alt-key. That means that Alt+1 to
>> >> Alt+4 combinations must be typed single-handed with the left hand. This
>> >> is mildly awkward for Alt+4. Can we please have the very important
>> >> commit widget *not* at Alt+4? I could live with Alt+3.
>> > 
>> > (RightAlt wouldn't be used by Europeans, anyways)
>> > Are you suggesting to keep Alt+1/2/3 for the files/staged/diff
>> > widgets, but use something other than Alt+4 for the commit dialog? If
>> > so, which one would you prefer?
>> 
>> I was suggesting Alt+3 for the commit message widget, but my preferences
>> are actually Alt+1, Alt+2, Alt+3, in this order. My preference for the
>> diff widget would be Alt+4 (the awkward one) because I do not foresee
>> that I would use it a lot. Use what remains for the two file lists.
>
> I wonder if that binding is very intuitive.  If we do 1/2 for the top 
> and bottom panes on the left side, and 3/4 for the top and bottom panes 
> on the right side, that makes some sense.  Doing it your way makes it a 
> counter-clockwise motion.
>
> I am not arguing for or against this proposal, just pointing something 
> worth thinking about.  Either way, I suppose after a while it becomes 
> muscle memory so I'm not sure how much difference this subtle thing will 
> make.
>
>> 
>> > The initial propsal from me was to use CTRL/CMD+1/2/3/4. What do you
>> > think of using the CTRL/CMD key instead of ALT?
>> 
>> I would not mind Ctrl instead of Alt. Take your pick.
>
> FWIW, I vote for sticking with Alt.

Can't these differences in personal preference be solved by
configurable key binding?

Re: [PATCH v5] git-gui: Add hotkeys to set widget focus

[Pratyush Yadav]

On 04/09/19 04:38PM, Junio C Hamano wrote:
> Pratyush Yadav <me@yadavpratyush.com> writes:
> 
> > On 04/09/19 11:39PM, Johannes Sixt wrote:
> >> Am 04.09.19 um 21:20 schrieb Birger Skogeng Pedersen:
> >> > On Wed, Sep 4, 2019 at 8:59 PM Johannes Sixt <j6t@kdbg.org> wrote:
> >> >> Many keyboards do not have a right Alt-key. That means that Alt+1 to
> >> >> Alt+4 combinations must be typed single-handed with the left hand. This
> >> >> is mildly awkward for Alt+4. Can we please have the very important
> >> >> commit widget *not* at Alt+4? I could live with Alt+3.
> >> > 
> >> > (RightAlt wouldn't be used by Europeans, anyways)
> >> > Are you suggesting to keep Alt+1/2/3 for the files/staged/diff
> >> > widgets, but use something other than Alt+4 for the commit dialog? If
> >> > so, which one would you prefer?
> >> 
> >> I was suggesting Alt+3 for the commit message widget, but my preferences
> >> are actually Alt+1, Alt+2, Alt+3, in this order. My preference for the
> >> diff widget would be Alt+4 (the awkward one) because I do not foresee
> >> that I would use it a lot. Use what remains for the two file lists.
> >
> > I wonder if that binding is very intuitive.  If we do 1/2 for the top 
> > and bottom panes on the left side, and 3/4 for the top and bottom panes 
> > on the right side, that makes some sense.  Doing it your way makes it a 
> > counter-clockwise motion.
> >
> > I am not arguing for or against this proposal, just pointing something 
> > worth thinking about.  Either way, I suppose after a while it becomes 
> > muscle memory so I'm not sure how much difference this subtle thing will 
> > make.
> >
> >> 
> >> > The initial propsal from me was to use CTRL/CMD+1/2/3/4. What do you
> >> > think of using the CTRL/CMD key instead of ALT?
> >> 
> >> I would not mind Ctrl instead of Alt. Take your pick.
> >
> > FWIW, I vote for sticking with Alt.
> 
> Can't these differences in personal preference be solved by
> configurable key binding?

They can be. But as of now, there is no existing mechanism to specify 
keybindings in git-gui that I know of, so that is not a trivial task.  
And if we do go with configurable keybindings, that raises the question 
of whether we should allow all existing keybindings to be changed.  
Again, this is a significant refactor.

And in the end we still have to come up with a reasonable default, so 
this discussion is still worth having IMO. We can keep configurable key 
keybindings as a future feature. Of course, if the contributors are 
willing to implement this feature along with this patch it would be 
great, but I don't think that is reason enough to block this patch for 
too long.

-- 
Regards,
Pratyush Yadav

Re: [PATCH v5] git-gui: Add hotkeys to set widget focus

[Pratyush Yadav]

+Cc j6t

This patch LGTM, but I'm not sure how to resolve the keybindings 
problem. Junio suggested we have configurable keybindings, and I agree 
with him, but until we do, something has to be agreed upon. And we also 
need to come up with a reasonable default.

So, I don't have any preferences for either using Alt+3 for the commit 
message buffer, or Alt+4. Unless someone has objections, I'll go with 
Alt+3 for the commit message buffer, and Alt+4 for the diff.

Thanks for the patch Birger. I'll change the keybindings locally before 
pushing out, no need to send a re-roll for something so trivial.

On 04/09/19 04:30PM, Birger Skogeng Pedersen wrote:
> The user cannot change focus between the list of files, the diff view and
> the commit message widgets without using the mouse (clicking either of
> the four widgets).
> 
> With this patch, the user may set ui focus to the previously selected path
> in either the "Unstaged Changes" or "Staged Changes" widgets, using
> ALT+1 or ALT+2.
> 
> The user may also set the ui focus to the diff view widget with
> ALT+3, or to the commit message widget with ALT+4.
> 
> This enables the user to select/unselect files, view the diff and create a
> commit in git-gui using keyboard-only.
> 
> Signed-off-by: Birger Skogeng Pedersen <birger.sp@gmail.com>
> ---
>  git-gui.sh | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 5bc21b8..5dae8da 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -2495,7 +2495,7 @@ proc force_first_diff {after} {
>  
>  proc toggle_or_diff {mode w args} {
>  	global file_states file_lists current_diff_path ui_index ui_workdir
> -	global last_clicked selected_paths
> +	global last_clicked selected_paths file_lists_last_clicked
>  
>  	if {$mode eq "click"} {
>  		foreach {x y} $args break
> @@ -2527,6 +2527,8 @@ proc toggle_or_diff {mode w args} {
>  	$ui_index tag remove in_sel 0.0 end
>  	$ui_workdir tag remove in_sel 0.0 end
>  
> +	set file_lists_last_clicked($w) $path
> +
>  	# Determine the state of the file
>  	if {[info exists file_states($path)]} {
>  		set state [lindex $file_states($path) 0]
> @@ -2640,6 +2642,26 @@ proc show_less_context {} {
>  	}
>  }
>  
> +proc focus_widget {widget} {
> +	global file_lists last_clicked selected_paths
> +	global file_lists_last_clicked
> +
> +	if {[llength $file_lists($widget)] > 0} {
> +		set path $file_lists_last_clicked($widget)
> +		set index [lsearch -sorted -exact $file_lists($widget) $path]
> +		if {$index < 0} {
> +			set index 0
> +			set path [lindex $file_lists($widget) $index]
> +		}
> +
> +		focus $widget
> +		set last_clicked [list $widget [expr $index + 1]]
> +		array unset selected_paths
> +		set selected_paths($path) 1
> +		show_diff $path $widget
> +	}
> +}
> +
>  ######################################################################
>  ##
>  ## ui construction
> @@ -3852,6 +3874,14 @@ foreach i [list $ui_index $ui_workdir] {
>  }
>  unset i
>  
> +bind .   <Alt-Key-1> {focus_widget $::ui_workdir}
> +bind .   <Alt-Key-2> {focus_widget $::ui_index}
> +bind .   <Alt-Key-3> {focus $::ui_diff}
> +bind .   <Alt-Key-4> {focus $::ui_comm}
> +
> +set file_lists_last_clicked($ui_index) {}
> +set file_lists_last_clicked($ui_workdir) {}
> +
>  set file_lists($ui_index) [list]
>  set file_lists($ui_workdir) [list]
>  
> -- 
> 2.21.0.windows.1
> 

-- 
Regards,
Pratyush Yadav

Re: [PATCH v5] git-gui: Add hotkeys to set widget focus

[Birger Skogeng Pedersen]

Hi Pratyush,

On Tue, Sep 10, 2019 at 9:12 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> This patch LGTM, but I'm not sure how to resolve the keybindings
> problem. Junio suggested we have configurable keybindings, and I agree
> with him, but until we do, something has to be agreed upon. And we also
> need to come up with a reasonable default.
>
> So, I don't have any preferences for either using Alt+3 for the commit
> message buffer, or Alt+4. Unless someone has objections, I'll go with
> Alt+3 for the commit message buffer, and Alt+4 for the diff.

I honestly don't quite follow the argumentation to use Alt+3 for the
commit message widget. Is Alt+4 (really) too awkward? And if it is,
how is Alt+3 better?
If you want to see it merged now (which I do, too), I propose we leave
it at Alt+3 for the diff, and Alt+4 for the commit message buffer.

As David A. mentioned in his email[1], git-cola utilizes CTRL+J/K/L
for navigation, maybe we should consider(?):
Alt+i: focus unstaged
Alt+j: focus staged
Alt+k: focus commit widget
Alt+l: focus diff view

[1] https://public-inbox.org/git/20190910085446.GB32239@gmail.com/#t

Best regards,
Birger

Re: [PATCH v5] git-gui: Add hotkeys to set widget focus

[Pratyush Yadav]

On 11/09/19 08:49AM, Birger Skogeng Pedersen wrote:
> Hi Pratyush,
> 
> On Tue, Sep 10, 2019 at 9:12 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > This patch LGTM, but I'm not sure how to resolve the keybindings
> > problem. Junio suggested we have configurable keybindings, and I agree
> > with him, but until we do, something has to be agreed upon. And we also
> > need to come up with a reasonable default.
> >
> > So, I don't have any preferences for either using Alt+3 for the commit
> > message buffer, or Alt+4. Unless someone has objections, I'll go with
> > Alt+3 for the commit message buffer, and Alt+4 for the diff.
> 
> I honestly don't quite follow the argumentation to use Alt+3 for the
> commit message widget. Is Alt+4 (really) too awkward? And if it is,
> how is Alt+3 better?

It isn't really much more than personal preference.

> If you want to see it merged now (which I do, too), I propose we leave
> it at Alt+3 for the diff, and Alt+4 for the commit message buffer.

Since this entire debate essentially boils down to personal preference, 
there is no clear answer. So I'll just go with the author's 
implementation.

Do note that I fixed a small nitpick locally. Changed the subject from 
"git-gui: Add..." to "git-gui: add...".
 
> As David A. mentioned in his email[1], git-cola utilizes CTRL+J/K/L
> for navigation, maybe we should consider(?):
> Alt+i: focus unstaged
> Alt+j: focus staged
> Alt+k: focus commit widget
> Alt+l: focus diff view

David's suggestion was to try to use the vim keys, which are h,j,k,l, 
and not i,j,k,l. Also, in git-cola (I learned this from reading his 
email, I don't use git-cola myself), Ctrl+j is for focussing on the 
lower part of the UI, which is their diff viewer. j is used for moving 
down in vim. Ctrl+k focusses on the status widget, which is in the upper 
part of the UI. k is used for moving up in vim.

But we can't do something similar with git-gui. Why would Alt+k make 
more sense when bound to diff instead of the unstaged changes widget? 
Both are in the upper part of the UI. Similar argument for why Alt+j 
would make more sense bound to commit message buffer instead of the 
staged changes widget? Both are in the lower part of the UI. And you 
can't fit h or l in this similar analogy, because they don't make sense 
to me at all.

I realise that the bindings I mentioned are not what you proposed, but 
they are more in line with the vim keys.

My point being, it is not simple to bind our 4 widgets to 4 directional 
keys, unlike in git-cola where you only have an up and down.

So I don't see any reasonable bindings what would work with the 4 vim 
directional keys: h,j,k,l. Maybe I'm missing something?

-- 
Regards,
Pratyush Yadav

Re: [PATCH v5] git-gui: Add hotkeys to set widget focus

[Johannes Sixt]

Am 11.09.19 um 19:48 schrieb Pratyush Yadav:
> Since this entire debate essentially boils down to personal preference, 
> there is no clear answer. So I'll just go with the author's 
> implementation.

Fair enough.

-- Hannes