Patchwork [RFC] crecord: make an option for space key to move cursor down (issue5159) (RFC)

login
register
mail settings
Submitter Anton Shestakov
Date Jan. 8, 2017, 2:19 a.m.
Message ID <1afa1ffe6984bfe00273.1483841970@neuro>
Download mbox | patch
Permalink /patch/18135/
State Accepted
Headers show

Comments

Anton Shestakov - Jan. 8, 2017, 2:19 a.m.
# HG changeset patch
# User Anton Shestakov <av6@dwimlabs.net>
# Date 1483841309 -28800
#      Sun Jan 08 10:08:29 2017 +0800
# Node ID 1afa1ffe6984bfe0027336ce0c83486e82ba2b50
# Parent  cfd14b0508ddf8edf156d202e2c5d07b259c2f46
crecord: make an option for space key to move cursor down (issue5159) (RFC)

I really want to have an option of toggling a selection on a line and also
moving cursor down as a single keystroke. It also kinda makes sense for space
key to do this, because some other curses UIs in the wild do this (e.g. various
file managers, htop). So I got an idea to make a config option that defaults to
False for compatibility, but allows making crecord UI a lot more useful for
people with big hunks.

What do you people think? And in what section should I put the config?
Sean Farley - Jan. 8, 2017, 3:10 a.m.
Anton Shestakov <av6@dwimlabs.net> writes:

> # HG changeset patch
> # User Anton Shestakov <av6@dwimlabs.net>
> # Date 1483841309 -28800
> #      Sun Jan 08 10:08:29 2017 +0800
> # Node ID 1afa1ffe6984bfe0027336ce0c83486e82ba2b50
> # Parent  cfd14b0508ddf8edf156d202e2c5d07b259c2f46
> crecord: make an option for space key to move cursor down (issue5159) (RFC)
>
> I really want to have an option of toggling a selection on a line and also
> moving cursor down as a single keystroke. It also kinda makes sense for space
> key to do this, because some other curses UIs in the wild do this (e.g. various
> file managers, htop). So I got an idea to make a config option that defaults to
> False for compatibility, but allows making crecord UI a lot more useful for
> people with big hunks.
>
> What do you people think? And in what section should I put the config?
>
> diff --git a/mercurial/crecord.py b/mercurial/crecord.py
> --- a/mercurial/crecord.py
> +++ b/mercurial/crecord.py
> @@ -1588,6 +1588,8 @@ are you sure you want to review/edit and
>              return True
>          elif keypressed in [' '] or (test and keypressed in ["TOGGLE"]):
>              self.toggleapply()
> +            if self.ui.configbool('crecord', 'spacemovesdown'):
> +                self.downarrowevent()

I could be convinced this is a good idea in general. I usually shoot
first but I don't know if the old behavior is worth preserving?
Pierre-Yves David - Jan. 10, 2017, 4:27 p.m.
On 01/08/2017 03:19 AM, Anton Shestakov wrote:
> # HG changeset patch
> # User Anton Shestakov <av6@dwimlabs.net>
> # Date 1483841309 -28800
> #      Sun Jan 08 10:08:29 2017 +0800
> # Node ID 1afa1ffe6984bfe0027336ce0c83486e82ba2b50
> # Parent  cfd14b0508ddf8edf156d202e2c5d07b259c2f46
> crecord: make an option for space key to move cursor down (issue5159) (RFC)
>
> I really want to have an option of toggling a selection on a line and also
> moving cursor down as a single keystroke. It also kinda makes sense for space
> key to do this, because some other curses UIs in the wild do this (e.g. various
> file managers, htop). So I got an idea to make a config option that defaults to
> False for compatibility, but allows making crecord UI a lot more useful for
> people with big hunks.
>
> What do you people think? And in what section should I put the config?

What about using a modifier key for that ? eg: shift space select and 
move forward.

>
> diff --git a/mercurial/crecord.py b/mercurial/crecord.py
> --- a/mercurial/crecord.py
> +++ b/mercurial/crecord.py
> @@ -1588,6 +1588,8 @@ are you sure you want to review/edit and
>              return True
>          elif keypressed in [' '] or (test and keypressed in ["TOGGLE"]):
>              self.toggleapply()
> +            if self.ui.configbool('crecord', 'spacemovesdown'):
> +                self.downarrowevent()
>          elif keypressed in ['A']:
>              self.toggleall()
>          elif keypressed in ['e']:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Sean Farley - Jan. 11, 2017, 6:36 p.m.
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:

> On 01/08/2017 03:19 AM, Anton Shestakov wrote:
>> # HG changeset patch
>> # User Anton Shestakov <av6@dwimlabs.net>
>> # Date 1483841309 -28800
>> #      Sun Jan 08 10:08:29 2017 +0800
>> # Node ID 1afa1ffe6984bfe0027336ce0c83486e82ba2b50
>> # Parent  cfd14b0508ddf8edf156d202e2c5d07b259c2f46
>> crecord: make an option for space key to move cursor down (issue5159) (RFC)
>>
>> I really want to have an option of toggling a selection on a line and also
>> moving cursor down as a single keystroke. It also kinda makes sense for space
>> key to do this, because some other curses UIs in the wild do this (e.g. various
>> file managers, htop). So I got an idea to make a config option that defaults to
>> False for compatibility, but allows making crecord UI a lot more useful for
>> people with big hunks.
>>
>> What do you people think? And in what section should I put the config?
>
> What about using a modifier key for that ? eg: shift space select and 
> move forward.

I think I've been tripped up enough about the current behavior that I'd
be in favor for making this the default and having shift+space (is there
a curses' best practice here?) as the "select but don't move". Not too
big a deal, though, so +0.
Anton Shestakov - Jan. 12, 2017, 10:32 a.m.
On Tue, 10 Jan 2017 17:27:32 +0100
Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:

> 
> 
> On 01/08/2017 03:19 AM, Anton Shestakov wrote:
> > # HG changeset patch
> > # User Anton Shestakov <av6@dwimlabs.net>
> > # Date 1483841309 -28800
> > #      Sun Jan 08 10:08:29 2017 +0800
> > # Node ID 1afa1ffe6984bfe0027336ce0c83486e82ba2b50
> > # Parent  cfd14b0508ddf8edf156d202e2c5d07b259c2f46
> > crecord: make an option for space key to move cursor down (issue5159) (RFC)
> >
> > I really want to have an option of toggling a selection on a line and also
> > moving cursor down as a single keystroke. It also kinda makes sense for space
> > key to do this, because some other curses UIs in the wild do this (e.g. various
> > file managers, htop). So I got an idea to make a config option that defaults to
> > False for compatibility, but allows making crecord UI a lot more useful for
> > people with big hunks.
> >
> > What do you people think? And in what section should I put the config?
> 
> What about using a modifier key for that ? eg: shift space select and 
> move forward.

I don't know. I can't think of a key combination and/or modifier that
would feel natural as a way to do "action" vs. "action+move". You know
how in some IM clients Enter sends the message and Shift+Enter just
inserts a newline? It's the closest I could think of and it's still
pretty different (there it's "action" vs. "newline").

The more I think about this, the more it feels to me that only
select+move makes sense anyway, because I can always go back to the
previous line and undo the previous decision. Compare this with the
regular chunk selector ("text"), which doesn't have a way to go back on
your decision, only restart the whole process. With curses UI it's
possible, and so optimizing the process for speed makes sense.

We could also try different ways to move cursor down for this
"action+move" key/combination, whatever we'd settle on. Maybe always
moving down is not the best way, maybe it should only work on lines,
not hunks/files, or maybe it should do what PageDown does and move to
the next block of the same kind (probably not the latter, but do chime
in).
Pierre-Yves David - Jan. 13, 2017, 12:29 p.m.
On 01/12/2017 11:32 AM, Anton Shestakov wrote:
> On Tue, 10 Jan 2017 17:27:32 +0100
> Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>
>> On 01/08/2017 03:19 AM, Anton Shestakov wrote:
>>> # HG changeset patch
>>> # User Anton Shestakov <av6@dwimlabs.net>
>>> # Date 1483841309 -28800
>>> #      Sun Jan 08 10:08:29 2017 +0800
>>> # Node ID 1afa1ffe6984bfe0027336ce0c83486e82ba2b50
>>> # Parent  cfd14b0508ddf8edf156d202e2c5d07b259c2f46
>>> crecord: make an option for space key to move cursor down (issue5159) (RFC)

TL;DR; I move the option in [experimental] and pushed the patch so that 
we can test the behavior.

It would be useful to have some test for this (I know crecord has some 
tests)

We have various options here

0) change nothing
1) get a config option to change the behavior
2) use a modifier key
3) change the default behavior
4) find another better idea

In my opinion, option (0: change nothing) is off the table as their have 
been enough people (including me) who express interests in having this 
available in some way.

(1: config option) is not very appealing because it makes the behavior 
hard to find and changes. I would rather have something easily 
accessible from. However, an experimental option might be a good way to 
test for (4: change behavior). In all case, I saw you introduced a new 
[crecord] section. That seems like a bad idea for couple of reason 
crecord is not a meaningful name and we try to not create too many new 
config section for just one value. Having section gathered around on 
theme help discovering other option when looking at the help.

(2: modifier key) the initial proposal "shift+space" can apparently 
create issues with some input system and the combination itself did not 
too convincing to Anton. So its currently a dead end.

(3: change behavior) might be a solution, its a curse UI and relatively 
new, so I think we have some room to change things here. However I'm not 
sure it is the right thing to do. Having an experimental config option 
to turn that on seems a good way to play more with it a get an opinion.

Given the above, I've taken the current patch moving the option in 
experimental. Please tests the option so that we can have an informed 
discussion in the future.

(4: better solution?) Someone might have a breakthrough idea, here is a 
small brainstorm:

maybe we could have a "selection" concept: you can selects line using 
"shift+arrow" and then you can change the state of that whole section.

Maybe we could have a "break" key. That would "split" the current chunk 
into two, so that one could select it independently.

Maybe we could have a "select this and everything below(or above?)" key?

Cheer,
Sean Farley - Jan. 13, 2017, 11:43 p.m.
Anton Shestakov <av6@dwimlabs.net> writes:

> On Tue, 10 Jan 2017 17:27:32 +0100
> Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>
>> 
>> 
>> On 01/08/2017 03:19 AM, Anton Shestakov wrote:
>> > # HG changeset patch
>> > # User Anton Shestakov <av6@dwimlabs.net>
>> > # Date 1483841309 -28800
>> > #      Sun Jan 08 10:08:29 2017 +0800
>> > # Node ID 1afa1ffe6984bfe0027336ce0c83486e82ba2b50
>> > # Parent  cfd14b0508ddf8edf156d202e2c5d07b259c2f46
>> > crecord: make an option for space key to move cursor down (issue5159) (RFC)
>> >
>> > I really want to have an option of toggling a selection on a line and also
>> > moving cursor down as a single keystroke. It also kinda makes sense for space
>> > key to do this, because some other curses UIs in the wild do this (e.g. various
>> > file managers, htop). So I got an idea to make a config option that defaults to
>> > False for compatibility, but allows making crecord UI a lot more useful for
>> > people with big hunks.
>> >
>> > What do you people think? And in what section should I put the config?
>> 
>> What about using a modifier key for that ? eg: shift space select and 
>> move forward.
>
> I don't know. I can't think of a key combination and/or modifier that
> would feel natural as a way to do "action" vs. "action+move". You know
> how in some IM clients Enter sends the message and Shift+Enter just
> inserts a newline? It's the closest I could think of and it's still
> pretty different (there it's "action" vs. "newline").
>
> The more I think about this, the more it feels to me that only
> select+move makes sense anyway, because I can always go back to the
> previous line and undo the previous decision. Compare this with the
> regular chunk selector ("text"), which doesn't have a way to go back on
> your decision, only restart the whole process. With curses UI it's
> possible, and so optimizing the process for speed makes sense.

Yeah, after reading your email, I definitely agree with you.
via Mercurial-devel - Jan. 14, 2017, 6:25 a.m.
On Fri, Jan 13, 2017, 15:43 Sean Farley <sean@farley.io> wrote:

> Anton Shestakov <av6@dwimlabs.net> writes:
>
> > On Tue, 10 Jan 2017 17:27:32 +0100
> > Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> >
> >>
> >>
> >> On 01/08/2017 03:19 AM, Anton Shestakov wrote:
> >> > # HG changeset patch
> >> > # User Anton Shestakov <av6@dwimlabs.net>
> >> > # Date 1483841309 -28800
> >> > #      Sun Jan 08 10:08:29 2017 +0800
> >> > # Node ID 1afa1ffe6984bfe0027336ce0c83486e82ba2b50
> >> > # Parent  cfd14b0508ddf8edf156d202e2c5d07b259c2f46
> >> > crecord: make an option for space key to move cursor down (issue5159)
> (RFC)
> >> >
> >> > I really want to have an option of toggling a selection on a line and
> also
> >> > moving cursor down as a single keystroke. It also kinda makes sense
> for space
> >> > key to do this, because some other curses UIs in the wild do this
> (e.g. various
> >> > file managers, htop). So I got an idea to make a config option that
> defaults to
> >> > False for compatibility, but allows making crecord UI a lot more
> useful for
> >> > people with big hunks.
> >> >
> >> > What do you people think? And in what section should I put the config?
> >>
> >> What about using a modifier key for that ? eg: shift space select and
> >> move forward.
> >
> > I don't know. I can't think of a key combination and/or modifier that
> > would feel natural as a way to do "action" vs. "action+move". You know
> > how in some IM clients Enter sends the message and Shift+Enter just
> > inserts a newline? It's the closest I could think of and it's still
> > pretty different (there it's "action" vs. "newline").
>


I thought Gmail had something like this for selecting conversations and
advancing, but it doesn't seem like it. However, I found that it uses '['
and ']' to navigate between conversations and '{' and '}' (which on US
keyboard are shifted '[' and ']'). By analogy, since crecord uses 'j' and
'k' to navigate (or arrow keys), perhaps 'J' and 'K' could be used to
select/deselect and move down/up?



> >
> > The more I think about this, the more it feels to me that only
> > select+move makes sense anyway, because I can always go back to the
> > previous line and undo the previous decision. Compare this with the
> > regular chunk selector ("text"), which doesn't have a way to go back on
> > your decision, only restart the whole process. With curses UI it's
> > possible, and so optimizing the process for speed makes sense.
>
> Yeah, after reading your email, I definitely agree with you.


I'm not sure I do, but if we changed it and I missed the current behavior,
we could always add e.g. 'x' for just selecting/deselecting.


> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Anton Shestakov - Jan. 16, 2017, 5:04 p.m.
On Fri, 13 Jan 2017 22:25:24 -0800
Martin von Zweigbergk <martinvonz@google.com> wrote:

> On Fri, Jan 13, 2017, 15:43 Sean Farley <sean@farley.io> wrote:
> 
> > Anton Shestakov <av6@dwimlabs.net> writes:
> >
> > > On Tue, 10 Jan 2017 17:27:32 +0100
> > > Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> > >
> > >>
> > >>
> > >> On 01/08/2017 03:19 AM, Anton Shestakov wrote:
> > >> > # HG changeset patch
> > >> > # User Anton Shestakov <av6@dwimlabs.net>
> > >> > # Date 1483841309 -28800
> > >> > #      Sun Jan 08 10:08:29 2017 +0800
> > >> > # Node ID 1afa1ffe6984bfe0027336ce0c83486e82ba2b50
> > >> > # Parent  cfd14b0508ddf8edf156d202e2c5d07b259c2f46
> > >> > crecord: make an option for space key to move cursor down (issue5159)
> > (RFC)
> > >> >
> > >> > I really want to have an option of toggling a selection on a line and
> > also
> > >> > moving cursor down as a single keystroke. It also kinda makes sense
> > for space
> > >> > key to do this, because some other curses UIs in the wild do this
> > (e.g. various
> > >> > file managers, htop). So I got an idea to make a config option that
> > defaults to
> > >> > False for compatibility, but allows making crecord UI a lot more
> > useful for
> > >> > people with big hunks.
> > >> >
> > >> > What do you people think? And in what section should I put the config?
> > >>
> > >> What about using a modifier key for that ? eg: shift space select and
> > >> move forward.
> > >
> > > I don't know. I can't think of a key combination and/or modifier that
> > > would feel natural as a way to do "action" vs. "action+move". You know
> > > how in some IM clients Enter sends the message and Shift+Enter just
> > > inserts a newline? It's the closest I could think of and it's still
> > > pretty different (there it's "action" vs. "newline").
> >
> 
> 
> I thought Gmail had something like this for selecting conversations and
> advancing, but it doesn't seem like it. However, I found that it uses '['
> and ']' to navigate between conversations and '{' and '}' (which on US
> keyboard are shifted '[' and ']'). By analogy, since crecord uses 'j' and
> 'k' to navigate (or arrow keys), perhaps 'J' and 'K' could be used to
> select/deselect and move down/up?

Okay, this could work. Personally, I'm more used to arrow keys, so if
it's possible to read Shift+Arrow keys in curses - sounds good.

There's one issue that I can see in this idea though, because using
Shift key could both feel like you're "dragging a selection" over a
number of lines or not feel like that at all. Let's consider a number
of lines in a hunk; some of the lines are selected for commit (let's
say even), some are not (let's say odd). When you think about
Shift-ArrowDown as dragging a selection, it should make all of the
lines selected (or all deselected, depending on the state of the first
line in the "selection"). I never thought about this originally, but
this behavior could be even more useful. Or not, it's hard to tell
without trying it out.

I'll try and see if it works for me after I add tests for
experimental.spacemovesdown (which should happen soon-ish).

Patch

diff --git a/mercurial/crecord.py b/mercurial/crecord.py
--- a/mercurial/crecord.py
+++ b/mercurial/crecord.py
@@ -1588,6 +1588,8 @@  are you sure you want to review/edit and
             return True
         elif keypressed in [' '] or (test and keypressed in ["TOGGLE"]):
             self.toggleapply()
+            if self.ui.configbool('crecord', 'spacemovesdown'):
+                self.downarrowevent()
         elif keypressed in ['A']:
             self.toggleall()
         elif keypressed in ['e']: