git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Birger Skogeng Pedersen <birger.sp@gmail.com>
To: Pratyush Yadav <me@yadavpratyush.com>, Git List <git@vger.kernel.org>
Subject: Re: [PATCH] git-gui: Add hotkeys to set widget focus
Date: Sun, 1 Sep 2019 20:24:47 +0200	[thread overview]
Message-ID: <CAGr--=+x6rdb9wexJ3bo+yZu3_zgNB48Ku-jDbcAnQ2We77cSQ@mail.gmail.com> (raw)
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.


> 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

  parent reply	other threads:[~2019-09-01 18:25 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-20 13:32 git-gui, feature request: add hotkeys to focus different widgets Birger Skogeng Pedersen
2018-02-23 10:22 ` [PATCH] git-gui: Add hotkeys to change focus between ui widgets Birger Skogeng Pedersen
2018-02-23 16:42   ` Birger Skogeng Pedersen
2018-02-28 12:10 ` Birger Skogeng Pedersen
2018-03-05 16:55   ` Johannes Schindelin
2018-03-06 14:35     ` Birger Skogeng Pedersen
2018-03-06 19:28       ` Junio C Hamano
2019-08-31 12:23         ` [PATCH] git-gui: Add hotkeys to set widget focus Birger Skogeng Pedersen
2019-08-31 12:27           ` Birger Skogeng Pedersen
2019-09-01 11:32           ` Pratyush Yadav
2019-09-01 16:21             ` Junio C Hamano
2019-09-01 18:24             ` Birger Skogeng Pedersen [this message]
2019-09-01 19:01             ` Bert Wesarg
2019-09-01 19:36               ` [PATCH] " Birger Skogeng Pedersen
2019-09-02 18:19                 ` Pratyush Yadav
2019-09-02 18:35                   ` Birger Skogeng Pedersen
2019-09-02 18:53                     ` Pratyush Yadav
2019-09-02 19:05                       ` Birger Skogeng Pedersen
2019-09-02 19:42                 ` Bert Wesarg
2019-09-03 14:21                   ` Birger Skogeng Pedersen
2019-09-03 14:22                   ` Pratyush Yadav
2019-09-03 14:45                     ` [PATCH] git-gui: use path name instead of list index to track last clicked file Pratyush Yadav
2019-09-03 18:07                       ` [PATCH v4] git-gui: Add hotkeys to set widget focus Birger Skogeng Pedersen
2019-09-03 18:13                         ` Birger Skogeng Pedersen
2019-09-03 19:30                           ` Birger Skogeng Pedersen
2019-09-03 21:49                         ` Pratyush Yadav
2019-09-04 14:30                           ` [PATCH v5] " Birger Skogeng Pedersen
2019-09-04 18:59                             ` Johannes Sixt
2019-09-04 19:20                               ` Birger Skogeng Pedersen
2019-09-04 21:39                                 ` Johannes Sixt
2019-09-04 22:31                                   ` Pratyush Yadav
2019-09-04 23:38                                     ` Junio C Hamano
2019-09-05 12:33                                       ` Pratyush Yadav
2019-09-04 19:55                               ` Bert Wesarg
2019-09-04 21:45                                 ` Johannes Sixt
2019-09-10 19:12                             ` Pratyush Yadav
2019-09-11  6:49                               ` Birger Skogeng Pedersen
2019-09-11 17:48                                 ` Pratyush Yadav
2019-09-11 18:11                                   ` Johannes Sixt
2019-09-03 16:06                     ` [PATCH] [PATCH] " Bert Wesarg
2019-09-01 22:27             ` Philip Oakley
2019-09-02 12:25               ` Pratyush Yadav
2019-09-02 17:23                 ` Philip Oakley
2019-09-03 22:18                   ` Pratyush Yadav
2019-09-01 18:58           ` Bert Wesarg
2019-09-01 19:25             ` Birger Skogeng Pedersen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGr--=+x6rdb9wexJ3bo+yZu3_zgNB48Ku-jDbcAnQ2We77cSQ@mail.gmail.com' \
    --to=birger.sp@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=me@yadavpratyush.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).