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

From: Birger Skogeng Pedersen <birger.sp@gmail.com>
To: Bert Wesarg <bert.wesarg@googlemail.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH] [PATCH] git-gui: Add hotkeys to set widget focus
Date: Tue, 3 Sep 2019 16:21:42 +0200	[thread overview]
Message-ID: <CAGr--=L=iUxAOGwwbQwVB5OwmNoXorBXgehMfiC5gvfgUQEa9g@mail.gmail.com> (raw)
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.

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