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

From: Birger Skogeng Pedersen <birger.sp@gmail.com>
Date: Sep 1, 2019, 18:24 UTC
Message-ID: <CAGr--=+x6rdb9wexJ3bo+yZu3_zgNB48Ku-jDbcAnQ2We77cSQ@mail.gmail.com>
In-Reply-To: <20190901113218.3lfu4ifsxhzrsw4g@yadavpratyush.com>

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.

Show 8 quoted lines

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

Show 7 quoted lines

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

Show 6 quoted lines

> 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