Implement the redo command

[verdy89]

Overview

Implemented the redo method to enable reverting undone actions on a per-action basis.

Implementation Approach

By defining @position, I was able to implement the redo command using only @past_lines, which was originally defined for the undo command. By saving the current line state in @past_lines, the entire mechanism was simplified. Specifically, it works as follows:

• Normal input: Delete all elements after the current position in @past_lines, push the input into @past_lines, and increase @position by 1.
• Cursor movement: Update only the cursor position in @past_lines.
• Undo: Decrease @position by 1 and return @past_lines[position].
• Redo: Increase @position by 1 and return @past_lines[position].

[ima1zumi]

How about setting redo to M-C-_? Maybe it's used in Emacs.
C-g is assigned abort in GNU Readline, so I believe it is better to avoid using this.

https://www.gnu.org/savannah-checkouts/gnu/emacs/news/NEWS.User.20.html

Several commands in the group buffer can be undone with M-C-_

[verdy89]

Thank you for the advice. As you suggested, I have changed the key assignment.

Changed redo key assignment from \C-g to \M-\C-_

[ima1zumi]

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.

[verdy89]

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?

[ima1zumi]

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.

[verdy89]

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

[verdy89]

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.

[tompng]

I think this part needs to be implemented inside push_past_lines because:

• push_past_lines is also called from insert_pasted_text. This part is also needed in there.
• This will blow redo history even when cursor is just moved. This part should be done only when @old_buffer_of_lines != @buffer_of_lines

[verdy89]

Thank you for your comment. I've fixed it.

Fixed deletion of the redo history in regular input

[ima1zumi]

The instance variable name position feels too broad. Since the line_editor.rb is a large file, we want to narrow down what position refers to. For example, if it’s related to input_lines, we could use input_lines_position.

[verdy89]

I went with input_lines_position as well.

Rename @position to @input_lines_position

[ima1zumi]

Since we can now move through the past and future with undo and redo, the variable name past_lines doesn't seem very appropriate. As a suggestion, how about input_lines? If you have any other ideas, please let me know.

[verdy89]

Since I couldn't come up with a better name, I went with input_lines. Thank you.

Renamed variables: past_lines -> input_lines

[tompng]

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

[verdy89]

I've removed initialization code. Thank you.

Deleted unused variables: @old_byte_pointer and @old_line_index

[ima1zumi]

Nice work 👍 Thanks for your contribution!

[tompng]

👍