Patchwork histedit: make actions toggleables

login
register
mail settings
Submitter Yu Feng
Date May 10, 2019, 5:15 p.m.
Message ID <23bc04dc2d149829133d.1557508531@feyu.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/39995/
State Accepted
Headers show

Comments

Yu Feng - May 10, 2019, 5:15 p.m.
# HG changeset patch
# User feyu@google.com
# Date 1557508115 25200
#      Fri May 10 10:08:35 2019 -0700
# Node ID 23bc04dc2d149829133db571f6a922e95843c9f9
# Parent  458dc948aff9f1217718b7679f890fea510d54f7
histedit: make actions toggleables.

If the same action is applied twice, revert to pick.

For example, the current state
#0  pick   104:xxxxxxxxx   Collect progress

pressing e once
#0  edit   104:xxxxxxxxx   Collect progress

pressing e again toggles it back to pick.
Instead of keeping at e (behavior before this patch)
#0  pick   104:xxxxxxxxx   Collect progress

The rationale is that giving a response when a key is pressed
makes happy users. Toggling seems to be a reasonable response in
this scenario.
Anton Shestakov - May 11, 2019, 8:54 a.m.
On Fri, 10 May 2019 10:15:31 -0700
Yu Feng <rainwoodman@gmail.com> wrote:

> # HG changeset patch
> # User feyu@google.com
> # Date 1557508115 25200
> #      Fri May 10 10:08:35 2019 -0700
> # Node ID 23bc04dc2d149829133db571f6a922e95843c9f9
> # Parent  458dc948aff9f1217718b7679f890fea510d54f7
> histedit: make actions toggleables.
> 
> If the same action is applied twice, revert to pick.
> 
> For example, the current state
> #0  pick   104:xxxxxxxxx   Collect progress
> 
> pressing e once
> #0  edit   104:xxxxxxxxx   Collect progress
> 
> pressing e again toggles it back to pick.
> Instead of keeping at e (behavior before this patch)
> #0  pick   104:xxxxxxxxx   Collect progress
> 
> The rationale is that giving a response when a key is pressed
> makes happy users. Toggling seems to be a reasonable response in
> this scenario.

This rationale may be good for GUI, but text-based interfaces don't
traditionally react to every possible key press. And one way to see
this patch is that it makes histedit do pretty much the opposite to what
it's told to do: I press e and it goes back to "pick". That's
surprising because action field is not a toggle, it's a choice.

Also, users don't have to change histedit plan in one go, they can
leave it waiting for input for days on end, or open another window to
see the graph and mentally think up the list of actions to mechanically
punch it in on the keyboard without looking at the current state of
histedit plan. If they know that commits A+B+C need to be edited, it
makes sense to let them press (e, down)x3 on A without making sure that
any of the actions wasn't already on "edit" because of their previous
input.

This could be a config option, that would make people knowingly opt-in
and not be surprised by the behavior.
Yu Feng - May 13, 2019, 3:17 a.m.
Hi Anton,
To clarify, the change I propose is in the ncurses based _chistedit UI,
which does display the graph.
It does not change the behaviour of TUI version of _histedit.

The behaviour change is to consistently redefine the keypress actions from
set-a-rule to toggle-a-rule.
I can imagine in scenarios, one (even myself) may smash the keyboard
repeatedly in order to set the rule list,
so I do agree with you the definition of 'responsive' is subjective in this
situation.

I will look into adding a config option for this and file a new patch.

Thanks!

On Sat, May 11, 2019 at 1:56 AM Anton Shestakov <av6@dwimlabs.net> wrote:

> On Fri, 10 May 2019 10:15:31 -0700
> Yu Feng <rainwoodman@gmail.com> wrote:
>
> > # HG changeset patch
> > # User feyu@google.com
> > # Date 1557508115 25200
> > #      Fri May 10 10:08:35 2019 -0700
> > # Node ID 23bc04dc2d149829133db571f6a922e95843c9f9
> > # Parent  458dc948aff9f1217718b7679f890fea510d54f7
> > histedit: make actions toggleables.
> >
> > If the same action is applied twice, revert to pick.
> >
> > For example, the current state
> > #0  pick   104:xxxxxxxxx   Collect progress
> >
> > pressing e once
> > #0  edit   104:xxxxxxxxx   Collect progress
> >
> > pressing e again toggles it back to pick.
> > Instead of keeping at e (behavior before this patch)
> > #0  pick   104:xxxxxxxxx   Collect progress
> >
> > The rationale is that giving a response when a key is pressed
> > makes happy users. Toggling seems to be a reasonable response in
> > this scenario.
>
> This rationale may be good for GUI, but text-based interfaces don't
> traditionally react to every possible key press. And one way to see
> this patch is that it makes histedit do pretty much the opposite to what
> it's told to do: I press e and it goes back to "pick". That's
> surprising because action field is not a toggle, it's a choice.
>
> Also, users don't have to change histedit plan in one go, they can
> leave it waiting for input for days on end, or open another window to
> see the graph and mentally think up the list of actions to mechanically
> punch it in on the keyboard without looking at the current state of
> histedit plan. If they know that commits A+B+C need to be edited, it
> makes sense to let them press (e, down)x3 on A without making sure that
> any of the actions wasn't already on "edit" because of their previous
> input.
>
> This could be a config option, that would make people knowingly opt-in
> and not be surprised by the behavior.
>
Jordi Gutiérrez Hermoso - May 13, 2019, 6:14 p.m.
On Sun, 2019-05-12 at 20:17 -0700, Feng Yu wrote:
> To clarify, the change I propose is in the ncurses based _chistedit UI, which does display the graph.
> It does not change the behaviour of TUI version of _histedit. 

"TUI" and "curses" mean the same thing, generally. Maybe we should
call the other histedit interface editor-based?

> I will look into adding a config option for this and file a new
> patch. 

Wait, no, not more configs. That seems like unnecessary complication.
Let's come to a consensus first. I'm not sure which way makes most
sense. Both ways seem to make sense to me.

Do you have examples of users who were confused by the lack of
toggleability?

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -1111,7 +1111,10 @@  def changeaction(state, pos, action):
     """Change the action state on the given position to the new action"""
     rules = state['rules']
     assert 0 <= pos < len(rules)
-    rules[pos].action = action
+    if rules[pos].action != action:
+        rules[pos].action = action
+    else: # applying the same action twice reverts to the default
+        rules[pos].action = KEY_LIST[0]
 
 def cycleaction(state, pos, next=False):
     """Changes the action state the next or the previous action from