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

From: Birger Skogeng Pedersen <birger.sp@gmail.com>
Date: Sep 3, 2019, 14:21 UTC
Message-ID: <CAGr--=L=iUxAOGwwbQwVB5OwmNoXorBXgehMfiC5gvfgUQEa9g@mail.gmail.com>
In-Reply-To: <CAKPyHN0LtOG9mFQ6qxc-w-EdpJ3HGJmHAGdrGp2FKkq4uCf55A@mail.gmail.com>

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.

Show 10 quoted lines

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

Show 8 quoted lines

> 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