Add concept of edit groups

This allows for multiple edits to be undone/redone in one go, as if they
were one edit.

Useful when a function is editing the commandline buffer via scripted
changes or via a keybinding so the internal changes to the buffer can be
abstracted away.

(Having extreme difficulty getting pexpect to play nice with the concept
of undo/redo...)

share/functions/fish_clipboard_paste.fish

@@ -38,7 +38,10 @@ function fish_clipboard_paste
         # so we don't trigger ignoring history.
         set data[1] (string trim -l -- $data[1])
     end
+
     if test -n "$data"
+        begin-undo-group
         commandline -i -- $data
+        end-undo-group
     end
 end

src/builtin_commandline.cpp

@@ -36,6 +36,9 @@ enum {
     APPEND_MODE        // insert at end of current token/command/buffer
 };
 
+/// Handle a single readline_cmd_t command out-of-band.
+void reader_handle_command(readline_cmd_t cmd);
+
 /// Replace/append/insert the selection with/at/after the specified string.
 ///
 /// \param begin beginning of selection
@@ -302,8 +305,18 @@ maybe_t<int> builtin_commandline(parser_t &parser, io_streams_t &streams, wchar_
                 if (mc == rl::repaint_mode || mc == rl::force_repaint || mc == rl::repaint) {
                     if (ld.is_repaint) continue;
                 }
-                // Inserts the readline function at the back of the queue.
-                reader_queue_ch(*mc);
+
+                // HACK: Execute these right here and now so they can affect any insertions/changes
+                // made via bindings. The correct solution is to change all `commandline`
+                // insert/replace operations into readline functions with associated data, so that
+                // all queued `commandline` operations - including buffer modifications - are
+                // executed in order
+                if (mc == rl::begin_undo_group || mc == rl::end_undo_group) {
+                    reader_handle_command(*mc);
+                } else {
+                    // Inserts the readline function at the back of the queue.
+                    reader_queue_ch(*mc);
+                }
             } else {
                 streams.err.append_format(_(L"%ls: Unknown input function '%ls'"), cmd, argv[i]);
                 builtin_print_error_trailer(parser, streams.err, cmd);

src/input.cpp

@@ -157,6 +157,8 @@ static const input_function_metadata_t input_function_metadata[] = {
     {readline_cmd_t::cancel, L"cancel"},
     {readline_cmd_t::undo, L"undo"},
     {readline_cmd_t::redo, L"redo"},
+    {readline_cmd_t::begin_undo_group, L"begin-undo-group"},
+    {readline_cmd_t::end_undo_group, L"end-undo-group"},
 };
 
 static_assert(sizeof(input_function_metadata) / sizeof(input_function_metadata[0]) ==

src/input_common.h

@@ -80,6 +80,8 @@ enum class readline_cmd_t {
     cancel,
     undo,
     redo,
+    begin_undo_group,
+    end_undo_group,
     repeat_jump,
     // NOTE: This one has to be last.
     reverse_repeat_jump

src/reader.cpp

@@ -200,7 +200,7 @@ static bool want_to_coalesce_insertion_of(const editable_line_t &el, const wcstr
     // Only consolidate single character inserts.
     if (str.size() != 1) return false;
     // Make an undo group after every space.
-    if (str.at(0) == L' ') return false;
+    if (str.at(0) == L' ' && !el.undo_history.try_coalesce) return false;
     assert(!el.undo_history.edits.empty());
     const edit_t &last_edit = el.undo_history.edits.back();
     // Don't add to the last edit if it deleted something.
@@ -211,19 +211,35 @@ static bool want_to_coalesce_insertion_of(const editable_line_t &el, const wcstr
 }
 
 bool editable_line_t::undo() {
-    if (undo_history.edits_applied == 0) return false;  // nothing to undo
-    const edit_t &edit = undo_history.edits.at(undo_history.edits_applied - 1);
-    undo_history.edits_applied--;
-    edit_t inverse = edit_t(edit.offset, edit.replacement.size(), L"");
-    inverse.replacement = edit.old;
-    size_t old_position = edit.cursor_position_before_edit;
-    apply_edit(&text_, inverse);
-    set_position(old_position);
+    bool did_undo = false;
+    maybe_t<int> last_group_id{-1};
+    while (undo_history.edits_applied != 0) {
+        const edit_t &edit = undo_history.edits.at(undo_history.edits_applied - 1);
+        if (did_undo && (!edit.group_id.has_value() || edit.group_id != last_group_id)) {
+            // We've restored all the edits in this logical undo group
+            break;
+        }
+        last_group_id = edit.group_id;
+        undo_history.edits_applied--;
+        edit_t inverse = edit_t(edit.offset, edit.replacement.size(), L"");
+        inverse.replacement = edit.old;
+        size_t old_position = edit.cursor_position_before_edit;
+        apply_edit(&text_, inverse);
+        set_position(old_position);
+        did_undo = true;
+    }
+
+    end_edit_group();
     undo_history.may_coalesce = false;
-    return true;
+    return did_undo;
 }
 
 void editable_line_t::push_edit(edit_t &&edit) {
+    // Assign a new group id or propagate the old one if we're in a logical grouping of edits
+    if (edit_group_level_ != -1) {
+        edit.group_id = edit_group_id_;
+    }
+
     bool edit_does_nothing = edit.length == 0 && edit.replacement.empty();
     if (edit_does_nothing) return;
     if (undo_history.edits_applied != undo_history.edits.size()) {
@@ -251,13 +267,48 @@ void editable_line_t::insert_coalesce(const wcstring &str) {
 }
 
 bool editable_line_t::redo() {
-    if (undo_history.edits_applied >= undo_history.edits.size()) return false;  // nothing to redo
-    const edit_t &edit = undo_history.edits.at(undo_history.edits_applied);
-    undo_history.edits_applied++;
-    apply_edit(&text_, edit);
-    set_position(cursor_position_after_edit(edit));
-    undo_history.may_coalesce = false;  // Make a new undo group here.
-    return true;
+    bool did_redo = false;
+
+    maybe_t<int> last_group_id{-1};
+    while (undo_history.edits_applied < undo_history.edits.size()) {
+        const edit_t &edit = undo_history.edits.at(undo_history.edits_applied);
+        if (did_redo && (!edit.group_id.has_value() || edit.group_id != last_group_id)) {
+            // We've restored all the edits in this logical undo group
+            break;
+        }
+        last_group_id = edit.group_id;
+        undo_history.edits_applied++;
+        apply_edit(&text_, edit);
+        set_position(cursor_position_after_edit(edit));
+        did_redo = true;
+    }
+
+    end_edit_group();
+    return did_redo;
+}
+
+void editable_line_t::begin_edit_group() {
+    if (++edit_group_level_ == 0) {
+        // Indicate that the next change must trigger the creation of a new history item
+        undo_history.may_coalesce = false;
+        // Indicate that future changes should be coalesced into the same edit if possible.
+        undo_history.try_coalesce = true;
+        // Assign a logical edit group id to future edits in this group
+        edit_group_id_ += 1;
+    }
+}
+
+void editable_line_t::end_edit_group() {
+    if (edit_group_level_ == -1) {
+        // Clamp the minimum value to -1 to prevent unbalanced end_edit_group() calls from breaking
+        // everything.
+        return;
+    }
+
+    if (--edit_group_level_ == -1) {
+        undo_history.try_coalesce = false;
+        undo_history.may_coalesce = false;
+    }
 }
 
 namespace {
@@ -1415,7 +1466,7 @@ void reader_data_t::insert_string(editable_line_t *el, const wcstring &str) {
         assert(el->undo_history.may_coalesce);
     } else {
         el->push_edit(edit_t(el->position(), 0, str));
-        el->undo_history.may_coalesce = (str.size() == 1);
+        el->undo_history.may_coalesce = el->undo_history.try_coalesce || (str.size() == 1);
     }
 
     if (el == &command_line) suppress_autosuggestion = false;
@@ -3710,7 +3761,17 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat
             }
             break;
         }
-            // Some commands should have been handled internally by inputter_t::readch().
+        case rl::begin_undo_group: {
+            editable_line_t *el = active_edit_line();
+            el->begin_edit_group();
+            break;
+        }
+        case rl::end_undo_group: {
+            editable_line_t *el = active_edit_line();
+            el->end_edit_group();
+            break;
+        }
+        // Some commands should have been handled internally by inputter_t::readch().
         case rl::self_insert:
         case rl::self_insert_notfirst:
         case rl::func_or:
@@ -3981,6 +4042,13 @@ void reader_schedule_prompt_repaint() {
     }
 }
 
+void reader_handle_command(readline_cmd_t cmd) {
+    if (reader_data_t *data = current_data_or_null()) {
+        readline_loop_state_t rls{};
+        data->handle_readline_command(cmd, rls);
+    }
+}
+
 void reader_queue_ch(const char_event_t &ch) {
     if (reader_data_t *data = current_data_or_null()) {
         data->inputter.queue_ch(ch);

src/reader.h

@@ -31,6 +31,10 @@ struct edit_t {
     /// The strings that are removed and added by this edit, respectively.
     wcstring old, replacement;
 
+    /// edit_t is only for contiguous changes, so to restore a group of arbitrary changes to the
+    /// command line we need to have a group id as forcibly coalescing changes is not enough.
+    maybe_t<int> group_id;
+
     explicit edit_t(size_t offset, size_t length, wcstring replacement)
         : offset(offset), length(length), replacement(std::move(replacement)) {}
 
@@ -61,6 +65,11 @@ struct undo_history_t {
     /// last one.
     bool may_coalesce = false;
 
+    /// Whether to be more aggressive in coalescing edits. Ideally, it would be "force coalesce"
+    /// with guaranteed atomicity but as `edit_t` is strictly for contiguous changes, that guarantee
+    /// can't be made at this time.
+    bool try_coalesce = false;
+
     /// Empty the history.
     void clear();
 };
@@ -72,6 +81,12 @@ class editable_line_t {
     /// The current position of the cursor in the command line.
     size_t position_ = 0;
 
+    /// The nesting level for atomic edits, so that recursive invocations of start_edit_group()
+    /// are not ended by one end_edit_group() call.
+    int32_t edit_group_level_ = -1;
+    /// Monotonically increasing edit group, ignored when edit_group_level_ is -1. Allowed to wrap.
+    uint32_t edit_group_id_ = -1;
+
    public:
     undo_history_t undo_history;
 
@@ -110,6 +125,11 @@ class editable_line_t {
 
     /// Redo the most recent undo. Returns true on success.
     bool redo();
+
+    /// Start a logical grouping of command line edits that should be undone/redone together.
+    void begin_edit_group();
+    /// End a logical grouping of command line edits that should be undone/redone together.
+    void end_edit_group();
 };
 
 /// Read commands from \c fd until encountering EOF.