| 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 <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?
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 >
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.
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).
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,
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.
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 >
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']: