Skip to content

Redo #812

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thomasballinger merged 6 commits into from
Jun 30, 2020
Merged

Redo #812

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
586ad3c
Redo in progress; still buggy
allgo27 Jun 24, 2020
6a929a6
Fixed redo; added redone lines to history
allgo27 Jun 24, 2020
a8f5789
Updated redo with Tom's suggestions: made redo configurable and impro…
allgo27 Jun 25, 2020
2c86652
prompt_undo() -> undo(), removed extraneous whitespace
allgo27 Jun 25, 2020
beb4509
Updated tests with new redo-friendly variable names (insert_into_hist…
allgo27 Jun 25, 2020
babad30
Added redo to sample-config
allgo27 Jun 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Redo in progress; still buggy
  • Loading branch information
@allgo27
allgo27 committed Jun 24, 2020
commit 586ad3c8f2dae749fbf534be142a52e3d870c33a
2 changes: 1 addition & 1 deletion bpython/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -905,7 +905,7 @@ def p_key(self, key):
if n > 0:
self.undo(n=n)
return ""

Copy link
Copy Markdown
Member

@thomasballinger thomasballinger Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove this extraneous change?

elif key in key_dispatch[config.search_key]:
self.search()
return ""
Expand Down
12 changes: 11 additions & 1 deletion bpython/curtsiesfrontend/repl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,12 @@ 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
Copy link
Copy Markdown
Member

@thomasballinger thomasballinger Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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.

Copy link
Copy Markdown
Member

@thomasballinger thomasballinger Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@thomasballinger thomasballinger Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to follow the pattern of the other keys here, calling a method in the body of the if.

if (self.could_be_redone):
Copy link
Copy Markdown
Member

@thomasballinger thomasballinger Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we chose this together but I don't like it anymore, it sounds to me like a boolean. How about redo_stack?

self.push(self.could_be_redone.pop())
open("testFile.txt", "a").write(str(self.history) + "\n")
else:
self.status_bar.message("Nothing to redo.")
elif e in key_dispatch[self.config.save_key]: # ctrl-s for save
greenlet.greenlet(self.write2file).switch()
elif e in key_dispatch[self.config.pastebin_key]: # F8 for pastebin
Expand Down Expand Up @@ -860,6 +866,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:
Copy link
Copy Markdown
Member

@thomasballinger thomasballinger Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if statement reads confusingly to me, I think we should:

  • rename the insert_into_history parameter in Repl.reevaluate() to new_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_history parameter in Repl.on_enter() to new_code as 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.

self.could_be_redone = []

self._set_cursor_offset(-1, update_completion=False)
if reset_rl_history:
self.rl_history.reset()
Expand Down Expand Up @@ -1151,6 +1160,7 @@ def push(self, line, insert_into_history=True):

if insert_into_history:
self.insert_into_history(line)
#self.history.append(line)
self.buffer.append(line)

code_to_run = "\n".join(self.buffer)
Expand Down Expand Up @@ -1812,7 +1822,7 @@ def prompt_for_undo():
greenlet.greenlet(prompt_for_undo).switch()

def reevaluate(self, insert_into_history=False):
"""bpython.Repl.undo calls this"""
"""bpython.Repl.undo calls this"""
if self.watcher:
self.watcher.reset()
old_logical_lines = self.history
Expand Down
7 changes: 6 additions & 1 deletion bpython/repl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Member

@thomasballinger thomasballinger Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about "commands executed since beginning of session," this comment isn't helping me much now.

self.could_be_redone = []
self.evaluating = False
self.matches_iter = MatchesIterator()
self.funcprops = None
Expand Down Expand Up @@ -1009,6 +1010,10 @@ def undo(self, n=1):

entries = list(self.rl_history.entries)

#Most recently undone command
lastEntry = self.history[-n:]
Copy link
Copy Markdown
Member

@thomasballinger thomasballinger Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change lastEntry -> last_entries; plural because it's a list, and snake_case

lastEntry.reverse()
self.could_be_redone += lastEntry
self.history = self.history[:-n]
self.reevaluate()

Expand Down