Skip to content
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

Implement the redo command #707

Merged
merged 11 commits into from
May 27, 2024
+152 −19
Merged

Implement the redo command #707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8530294
Implement the redo command
verdy89 May 18, 2024
ef3653b
Commented out a test that does not pass
verdy89 May 19, 2024
1ddc14b
Changed key assignment for redo from "\C-[" to "\C-g"
verdy89 May 20, 2024
9e5e623
Changed redo key assignment from `\C-g` to `\M-\C-_`
verdy89 May 23, 2024
9e0d6ad
Revert the first implemantation
verdy89 May 23, 2024
a28ab63
Implemented redo by sharing `@past_lines` between undo and redo
verdy89 May 25, 2024
99cf6c1
Fixed the index of past_lines that is updated when the cursor is moved
verdy89 May 25, 2024
720cd98
Fixed deletion of the redo history in regular input
verdy89 May 26, 2024
0ab3c67
Renamed variables: past_lines -> input_lines
verdy89 May 27, 2024
7aee2ca
Rename @position to @input_lines_position
verdy89 May 27, 2024
95a7108
Deleted unused variables: `@old_byte_pointer` and `@old_line_index`
verdy89 May 27, 2024
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
2 changes: 1 addition & 1 deletion lib/reline/key_actor/emacs.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ class Reline::KeyActor::Emacs < Reline::KeyActor::Base
# 158 M-^^
:ed_unassigned,
# 159 M-^_
:ed_unassigned,
:redo,
# 160 M-SPACE
:em_set_mark,
# 161 M-!
Expand Down
47 changes: 30 additions & 17 deletions lib/reline/line_editor.rb
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

How about managing the history using only @past_lines with an index in an instance variable?
This way, it would be more efficient as it avoids moving entries between two arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we aim to use only @past_lines, it may be necessary to provide instance variables such as @position to implement the redo. It seems that this would result in a complex mechanism like below.

def normal_input
  if @past_lines.length > @position
    @past_lines = @past_lines.slice(0, @position + 1)
  else
    @past_lines.push(current_line)
  end
  @position += 1
end

def undo
  if @past_lines.length == @position
    @past_lines.push(current_line)
  end

  @position -= 1
  current_line = @past_lines[@position]
end

def redo
  if @past_lines.length - 1 > @position
    @position += 1
    current_line = @past_lines[@position]
  end
end

It may be true that moving elements between two arrays is not efficient, but even when using only @past_lines, the amount of data that needs to be stored remains the same. Additionally, having a more complex mechanism would make maintenance more challenging.

Is there a better way to achieve this?

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for writing the sample code! As you mentioned, we need to use instance variables like @position. Similar operations are performed in the methods that handle history, as shown here: https://github.com/ruby/reline/blob/master/lib/reline/line_editor.rb#L1601-L1843.
While this approach is more complex than using push/pop with two arrays, I don't think it reduces maintainability.

Personally, I feel there are too many instance variables in line_editor.rb, so I would like to reduce the number of instance variables with significant responsibilities. I find that using integers has a narrower scope of responsibility than using arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, I understand. Thank you for your comments.
With additional advice from @tompng , I was able to implement redo without using @next_lines!

https://github.com/ruby/reline/pull/707/files/9e0d6ad14e18173930419f1e617fb44608984e14..99cf6c1b1a8485ad78e1b6720eb935884a80ab7b

Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,8 @@ def reset_variables(prompt = '', encoding:)
@resized = false
@cache = {}
@rendered_screen = RenderedScreen.new(base_y: 0, lines: [], cursor_y: 0)
@past_lines = []
@input_lines = [[[""], 0, 0]]
@input_lines_position = 0
@undoing = false
reset_line
end
Expand Down Expand Up @@ -1137,7 +1138,7 @@ def input_key(key)
@completion_journey_state = nil
end

push_past_lines unless @undoing
push_input_lines unless @undoing
@undoing = false

if @in_pasting
Expand All @@ -1156,21 +1157,24 @@ def input_key(key)

def save_old_buffer
@old_buffer_of_lines = @buffer_of_lines.dup
@old_byte_pointer = @byte_pointer.dup
@old_line_index = @line_index.dup
end

def push_past_lines
if @old_buffer_of_lines != @buffer_of_lines
@past_lines.push([@old_buffer_of_lines, @old_byte_pointer, @old_line_index])
Copy link
Member

Choose a reason for hiding this comment

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

Looks like @old_byte_pointer and @old_line_index are not used anymore. We can remove initialization code from def save_old_buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed initialization code. Thank you.

Deleted unused variables: @old_byte_pointer and @old_line_index

def push_input_lines
if @old_buffer_of_lines == @buffer_of_lines
@input_lines[@input_lines_position] = [@buffer_of_lines.dup, @byte_pointer, @line_index]
else
@input_lines = @input_lines[0..@input_lines_position]
@input_lines_position += 1
@input_lines.push([@buffer_of_lines.dup, @byte_pointer, @line_index])
end
trim_past_lines
trim_input_lines
end

MAX_PAST_LINES = 100
def trim_past_lines
if @past_lines.size > MAX_PAST_LINES
@past_lines.shift
MAX_INPUT_LINES = 100
def trim_input_lines
if @input_lines.size > MAX_INPUT_LINES
@input_lines.shift
@input_lines_position -= 1
end
end

Expand Down Expand Up @@ -1352,7 +1356,7 @@ def insert_pasted_text(text)
@buffer_of_lines[@line_index, 1] = lines
@line_index += lines.size - 1
@byte_pointer = @buffer_of_lines[@line_index].bytesize - post.bytesize
push_past_lines
push_input_lines
end

def insert_text(text)
Expand Down Expand Up @@ -2529,13 +2533,22 @@ def finish
end

private def undo(_key)
return if @past_lines.empty?
@undoing = true

return if @input_lines_position <= 0

@input_lines_position -= 1
target_lines, target_cursor_x, target_cursor_y = @input_lines[@input_lines_position]
set_current_lines(target_lines.dup, target_cursor_x, target_cursor_y)
end

private def redo(_key)
@undoing = true

target_lines, target_cursor_x, target_cursor_y = @past_lines.last
set_current_lines(target_lines, target_cursor_x, target_cursor_y)
return if @input_lines_position >= @input_lines.size - 1

@past_lines.pop
@input_lines_position += 1
target_lines, target_cursor_x, target_cursor_y = @input_lines[@input_lines_position]
set_current_lines(target_lines.dup, target_cursor_x, target_cursor_y)
end
end
106 changes: 105 additions & 1 deletion test/reline/test_key_actor_emacs.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1498,11 +1498,115 @@ def test_undo_with_multiline
end

def test_undo_with_many_times
str = "a" + "b" * 100
str = "a" + "b" * 99
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I changed the implementation to push up to the current state into @past_lines, the number of actions stored in @past_lines is now one less than MAX_PAST_LINES. However, I believe that the value of MAX_PAST_LINES being set to 100 was chosen simply because 100 is a round number, without any other specific reason. Therefore, instead of changing the value of MAX_PAST_LINES, I addressed this by modifying the tests.

input_keys(str, false)
100.times { input_keys("\C-_", false) }
assert_line_around_cursor('a', '')
input_keys("\C-_", false)
assert_line_around_cursor('a', '')
end

def test_redo
input_keys("aあb", false)
assert_line_around_cursor('aあb', '')
input_keys("\M-\C-_", false)
assert_line_around_cursor('aあb', '')
input_keys("\C-_", false)
assert_line_around_cursor('aあ', '')
input_keys("\C-_", false)
assert_line_around_cursor('a', '')
input_keys("\M-\C-_", false)
assert_line_around_cursor('aあ', '')
input_keys("\M-\C-_", false)
assert_line_around_cursor('aあb', '')
input_keys("\C-_", false)
assert_line_around_cursor('aあ', '')
input_keys("c", false)
assert_line_around_cursor('aあc', '')
input_keys("\M-\C-_", false)
assert_line_around_cursor('aあc', '')
end

def test_redo_with_cursor_position
input_keys("abc\C-b\C-h", false)
assert_line_around_cursor('a', 'c')
input_keys("\M-\C-_", false)
assert_line_around_cursor('a', 'c')
input_keys("\C-_", false)
assert_line_around_cursor('ab', 'c')
input_keys("\M-\C-_", false)
assert_line_around_cursor('a', 'c')
end

def test_redo_with_multiline
@line_editor.multiline_on
@line_editor.confirm_multiline_termination_proc = proc {}
input_keys("1\n2\n3", false)
assert_whole_lines(["1", "2", "3"])
assert_line_index(2)
assert_line_around_cursor('3', '')

input_keys("\C-_", false)
assert_whole_lines(["1", "2", ""])
assert_line_index(2)
assert_line_around_cursor('', '')

input_keys("\C-_", false)
assert_whole_lines(["1", "2"])
assert_line_index(1)
assert_line_around_cursor('2', '')

input_keys("\M-\C-_", false)
assert_whole_lines(["1", "2", ""])
assert_line_index(2)
assert_line_around_cursor('', '')

input_keys("\M-\C-_", false)
assert_whole_lines(["1", "2", "3"])
assert_line_index(2)
assert_line_around_cursor('3', '')

input_keys("\C-p\C-h\C-h", false)
assert_whole_lines(["1", "3"])
assert_line_index(0)
assert_line_around_cursor('1', '')

input_keys("\C-n", false)
assert_whole_lines(["1", "3"])
assert_line_index(1)
assert_line_around_cursor('3', '')

input_keys("\C-_", false)
assert_whole_lines(["1", "", "3"])
assert_line_index(1)
assert_line_around_cursor('', '')

input_keys("\C-_", false)
assert_whole_lines(["1", "2", "3"])
assert_line_index(1)
assert_line_around_cursor('2', '')

input_keys("\M-\C-_", false)
assert_whole_lines(["1", "", "3"])
assert_line_index(1)
assert_line_around_cursor('', '')

input_keys("\M-\C-_", false)
assert_whole_lines(["1", "3"])
assert_line_index(1)
assert_line_around_cursor('3', '')
end

def test_redo_with_many_times
str = "a" + "b" * 98 + "c"
input_keys(str, false)
100.times { input_keys("\C-_", false) }
assert_line_around_cursor('a', '')
input_keys("\C-_", false)
assert_line_around_cursor('a', '')
100.times { input_keys("\M-\C-_", false) }
assert_line_around_cursor(str, '')
input_keys("\M-\C-_", false)
assert_line_around_cursor(str, '')
end
end
16 changes: 16 additions & 0 deletions test/reline/yamatanooroti/test_rendering.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,22 @@ def test_bracketed_paste_with_undo
EOC
end

def test_bracketed_paste_with_redo
omit if Reline.core.io_gate.win?
start_terminal(5, 30, %W{ruby -I#{@pwd}/lib #{@pwd}/test/reline/yamatanooroti/multiline_repl}, startup_message: 'Multiline REPL.')
write("abc")
write("\e[200~def hoge\r\t3\rend\e[201~")
write("\C-_")
write("\M-\C-_")
close
assert_screen(<<~EOC)
Multiline REPL.
prompt> abcdef hoge
prompt> 3
prompt> end
EOC
end

def test_backspace_until_returns_to_initial
start_terminal(5, 30, %W{ruby -I#{@pwd}/lib #{@pwd}/test/reline/yamatanooroti/multiline_repl}, startup_message: 'Multiline REPL.')
write("ABC")
Expand Down
Loading