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

[Bert Wesarg <bert.wesarg@googlemail.com>]

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
>