Redo #812
Redo #812
Conversation
bpython/curtsiesfrontend/repl.py
Outdated
| @@ -775,6 +775,13 @@ def process_key_event(self, e): | |||
| self.on_tab(back=True) | |||
| elif e in key_dispatch[self.config.undo_key]: # ctrl-r for undo | |||
| self.prompt_undo() | |||
| elif e in ["<Ctrl-g>"]: # for redo | |||
There was a problem hiding this comment.
👍 this seems like a safe choice, bpython doesn't seem to use Ctrl-g for anything and don't know if you checked something like https://en.wikipedia.org/wiki/GNU_Readline but it doesn't sound like something people are clamoring for. It would be nice to make it configurable like self.config.undo_key.
There was a problem hiding this comment.
Also it'd be nice to
- follow the pattern of other keys of calling a method to keep this switch-like if/elif/elif/... structure shorter
- also for symmetry, use a tuple here instead of a list
bpython/curtsiesfrontend/repl.py
Outdated
| @@ -775,6 +775,13 @@ def process_key_event(self, e): | |||
| self.on_tab(back=True) | |||
| elif e in key_dispatch[self.config.undo_key]: # ctrl-r for undo | |||
| self.prompt_undo() | |||
| elif e in ["<Ctrl-g>"]: # for redo | |||
| if (self.could_be_redone): | |||
There was a problem hiding this comment.
I know we chose this together but I don't like it anymore, it sounds to me like a boolean. How about redo_stack?
bpython/curtsiesfrontend/repl.py
Outdated
| @@ -775,6 +775,13 @@ def process_key_event(self, e): | |||
| self.on_tab(back=True) | |||
| elif e in key_dispatch[self.config.undo_key]: # ctrl-r for undo | |||
| self.prompt_undo() | |||
| elif e in ["<Ctrl-g>"]: # for redo | |||
There was a problem hiding this comment.
It'd be nice to follow the pattern of the other keys here, calling a method in the body of the if.
bpython/repl.py
Outdated
| @@ -439,7 +439,8 @@ def __init__(self, interp, config): | |||
| duplicates=config.hist_duplicates, hist_size=config.hist_length | |||
| ) | |||
| self.s_hist = [] | |||
| self.history = [] | |||
| self.history = [] # History of commands | |||
There was a problem hiding this comment.
how about "commands executed since beginning of session," this comment isn't helping me much now.
bpython/repl.py
Outdated
| @@ -1009,6 +1010,10 @@ def undo(self, n=1): | |||
|
|
|||
| entries = list(self.rl_history.entries) | |||
|
|
|||
| #Most recently undone command | |||
| lastEntry = self.history[-n:] | |||
There was a problem hiding this comment.
Could you change lastEntry -> last_entries; plural because it's a list, and snake_case
bpython/curtsiesfrontend/repl.py
Outdated
| @@ -860,6 +867,9 @@ def readline_kill(self, e): | |||
|
|
|||
| def on_enter(self, insert_into_history=True, reset_rl_history=True): | |||
| # so the cursor isn't touching a paren TODO: necessary? | |||
| if insert_into_history: | |||
There was a problem hiding this comment.
This if statement reads confusingly to me, I think we should:
- rename the
insert_into_historyparameter inRepl.reevaluate()tonew_code; this is what it means, it's about whether the lines might be new, so they should be added to readline history. - rename the
insert_into_historyparameter inRepl.on_enter()tonew_codeas well. Then this if becomes "if the line being executed is new, then we can no longer use our redo stack." - not rename insert_into_history anywhere else
My rationale for the name is that new_code describes the situation/circumstance instead of the behavior we want, which is important since we're now using this flag to determine a new, different behavior.
And in reviewing this I found that the self.reevaluate(insert_into_history=True) in clear_modules_and_reevaluate() should probably be False instead, we can fix in another PR.
Could do, I don't think it's a big improvement.
I'd move it to a method on this Repl class, maybe right below |
bpython/curtsiesfrontend/repl.py
Outdated
| @@ -1811,7 +1816,15 @@ def prompt_for_undo(): | |||
|
|
|||
| greenlet.greenlet(prompt_for_undo).switch() | |||
|
|
|||
| def reevaluate(self, insert_into_history=False): | |||
| def prompt_redo(self): | |||
There was a problem hiding this comment.
I think just redo, "prompt_undo" is because it may prompt you for how many lines you want to undo.
bpython/cli.py
Outdated
| @@ -905,7 +905,7 @@ def p_key(self, key): | |||
| if n > 0: | |||
| self.undo(n=n) | |||
| return "" | |||
|
|
|||
There was a problem hiding this comment.
Could you remove this extraneous change?
|
Ok one more thing! (I'd say on last thing but that's hard to promise) Could you add this to the example config file, as a commented out key like undo: Line 69 in d6bd2e5 |
I think I fixed it! But it does feel a little "hack-y." You previously suggested we make history a class, do you still think that's a good idea? You also suggested we move redo somewhere else, though I don't know if you had ideas of where-- do you still have thoughts on that? Thanks!