Patchwork [1,of,2] crecord: repurpose "a" key to toggle all selections (BC)

login
register
mail settings
Submitter Jordi Gutiérrez Hermoso
Date Dec. 10, 2019, 10:08 p.m.
Message ID <bd10bc4bdb994b875040.1576015706@chloe>
Download mbox | patch
Permalink /patch/43693/
State Superseded
Headers show

Comments

Jordi Gutiérrez Hermoso - Dec. 10, 2019, 10:08 p.m.
# HG changeset patch
# User Jordi Gutiérrez Hermoso <jordigh@octave.org>
# Date 1576015329 18000
#      Tue Dec 10 17:02:09 2019 -0500
# Node ID bd10bc4bdb994b875040c0c58f1c341df66ddd46
# Parent  ce088b38f92b6d480df2072dc45fb068443758dd
crecord: repurpose "a" key to toggle all selections (BC)

I really don't like "a". I keep accidentally hitting it when I
actually want "A", and then I'm suddenly in a state I don't want to be
in. There's a big wall of text telling me that I've turned amend mode
on or off (which one was I orginally in?), and this seems very
useless.

If I wanted to amend or not, I would have chosen that from the
command-line, not change my mind after I've already started picking
hunks apart.

It seems much better to repurpose this key to be a "weaker" version of
"A". It toggles all selections. This is pretty harmless if hit
accidentally, (can just hit "a" again to undo it), and has immediate
visual feedback that something happened: all the x's and blank spaces
get switched around. And unlike with amend, the current flipped state
is also immediately visible without having to read a wall of text.

I'm calling this a BC, however, because somewhere, someone out there
has probably really fallen in love with the old use of "a" and will
get angry that we took it away.
Denis Laxalde - Dec. 11, 2019, 9:41 a.m.
Jordi Gutiérrez Hermoso a écrit :
> # HG changeset patch
> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> # Date 1576015329 18000
> #      Tue Dec 10 17:02:09 2019 -0500
> # Node ID bd10bc4bdb994b875040c0c58f1c341df66ddd46
> # Parent  ce088b38f92b6d480df2072dc45fb068443758dd
> crecord: repurpose "a" key to toggle all selections (BC)
> 
> I really don't like "a". I keep accidentally hitting it when I
> actually want "A", and then I'm suddenly in a state I don't want to be
> in. There's a big wall of text telling me that I've turned amend mode
> on or off (which one was I orginally in?), and this seems very
> useless.

Sounds reasonable to me. Also this 'amend mode' seems only useful for
'commit --interactive'; but it's currently also available for other
interactive commands (e.g. 'revert --interactive', which makes no
sense) so it's already weird.

> If I wanted to amend or not, I would have chosen that from the
> command-line, not change my mind after I've already started picking
> hunks apart.

Agreed as well on this. IMHO, interactive diff hunks selection module
should only be concerned about selecting hunks.

> It seems much better to repurpose this key to be a "weaker" version of
> "A". It toggles all selections. This is pretty harmless if hit
> accidentally, (can just hit "a" again to undo it), and has immediate
> visual feedback that something happened: all the x's and blank spaces
> get switched around. And unlike with amend, the current flipped state
> is also immediately visible without having to read a wall of text.

Does "pretty harmless" mean that you can actually toggle back to the
previous selection? (If not, I wouldn't say this is harmless since the
user may have spend a fair amount of time carefully selecting hunks.)

Also, this breaks tests in test-commit-interactive-curses.t (and it'd be
nice to have some new test for the new feature).

> I'm calling this a BC, however, because somewhere, someone out there
> has probably really fallen in love with the old use of "a" and will
> get angry that we took it away.
> 
> diff --git a/mercurial/crecord.py b/mercurial/crecord.py
> --- a/mercurial/crecord.py
> +++ b/mercurial/crecord.py
> @@ -990,6 +990,13 @@ class curseschunkselector(object):
>                      self.toggleapply(item)
>          self.waslasttoggleallapplied = not self.waslasttoggleallapplied
>  
> +    def flipselections(self):
> +        b"flip all selections"

Triple " without 'b' would be nicer.

> +        for header in self.headerlist:
> +            for hunk in header.allchildren():
> +                for line in hunk.allchildren():
> +                    self.toggleapply(line)
> +
>      def toggleallbetween(self):
>          """toggle applied on or off for all items in range [lastapplied,
>          current]. """
> @@ -1637,7 +1644,7 @@ the following are valid keystrokes:
>                   ctrl-l : scroll the selected line to the top of the screen
>                        m : edit / resume editing the commit message
>                        e : edit the currently selected hunk
> -                      a : toggle amend mode, only with commit -i
> +                      a : toggle all selections
>                        c : confirm selected changes
>                        r : review/edit and confirm selected changes
>                        q : quit without confirming (no changes will be made)
> @@ -1913,7 +1920,7 @@ are you sure you want to review/edit and
>          elif keypressed in ["q"]:
>              raise error.Abort(_(b'user quit'))
>          elif keypressed in ['a']:
> -            self.toggleamend(self.opts, test)
> +            self.flipselections()
>          elif keypressed in ["c"]:
>              return True
>          elif keypressed in ["r"]:
Jordi Gutiérrez Hermoso - Dec. 11, 2019, 3:31 p.m.
On Wed, 2019-12-11 at 10:41 +0100, Denis Laxalde wrote:
Jordi Gutiérrez Hermoso a écrit :
> > # HG changeset patch
> > # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> > # Date 1576015329 18000
> > #      Tue Dec 10 17:02:09 2019 -0500
> > # Node ID bd10bc4bdb994b875040c0c58f1c341df66ddd46
> > # Parent  ce088b38f92b6d480df2072dc45fb068443758dd
> > crecord: repurpose "a" key to toggle all selections (BC)
> > 

> Also this 'amend mode' seems only useful for 'commit --interactive';
> but it's currently also available for other interactive commands
> (e.g. 'revert --interactive', which makes no sense) so it's already
> weird.

Oh, right, I forgot about that. I've amended my commit message to also
include this point.

> Does "pretty harmless" mean that you can actually toggle back to the
> previous selection?

Yeah, all selections get flipped. Just hit "a" again to the same state
before you flipped them.

Honestly, the time I would use this most is when everything is
selected so I can start from zero selections. I so very often reach
for "A" at the beginning of my hunk selecting but accidentally hit "a"
and then I panic 'cause I forget what state it's in, so I just quit
the hunk selector and start over again from the command line.

> Also, this breaks tests in test-commit-interactive-curses.t (and it'd be
> nice to have some new test for the new feature).

Oh, oops. I thought I had ran the tests. There, tests fixed, and I
wrote a new test for toggling all lines.

> > diff --git a/mercurial/crecord.py b/mercurial/crecord.py
> > --- a/mercurial/crecord.py
> > +++ b/mercurial/crecord.py
> > @@ -990,6 +990,13 @@ class curseschunkselector(object):
> >                      self.toggleapply(item)
> >          self.waslasttoggleallapplied = not self.waslasttoggleallapplied
> >  
> > +    def flipselections(self):
> > +        b"flip all selections"
> 
> Triple " without 'b' would be nicer.

Fixed. Thanks.

Patch

diff --git a/mercurial/crecord.py b/mercurial/crecord.py
--- a/mercurial/crecord.py
+++ b/mercurial/crecord.py
@@ -990,6 +990,13 @@  class curseschunkselector(object):
                     self.toggleapply(item)
         self.waslasttoggleallapplied = not self.waslasttoggleallapplied
 
+    def flipselections(self):
+        b"flip all selections"
+        for header in self.headerlist:
+            for hunk in header.allchildren():
+                for line in hunk.allchildren():
+                    self.toggleapply(line)
+
     def toggleallbetween(self):
         """toggle applied on or off for all items in range [lastapplied,
         current]. """
@@ -1637,7 +1644,7 @@  the following are valid keystrokes:
                  ctrl-l : scroll the selected line to the top of the screen
                       m : edit / resume editing the commit message
                       e : edit the currently selected hunk
-                      a : toggle amend mode, only with commit -i
+                      a : toggle all selections
                       c : confirm selected changes
                       r : review/edit and confirm selected changes
                       q : quit without confirming (no changes will be made)
@@ -1913,7 +1920,7 @@  are you sure you want to review/edit and
         elif keypressed in ["q"]:
             raise error.Abort(_(b'user quit'))
         elif keypressed in ['a']:
-            self.toggleamend(self.opts, test)
+            self.flipselections()
         elif keypressed in ["c"]:
             return True
         elif keypressed in ["r"]: