Re: Support for undo-amalgamate in a version of the atomic-change-group macro (with patch)

[Campbell Barton <ideasman42@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2622 bytes --]

On Mon, Nov 8, 2021 at 10:09 AM Campbell Barton <ideasman42@gmail.com> wrote:
>
> On Mon, Nov 8, 2021 at 12:21 AM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> >
> > > Here is an updated patch with a separate macro to amalgamate undo barriers.
> >
> > Looks pretty good, thanks.
> > I was about to install it into `master` but noticed the following:
> >
> > - You seem not to have signed copyright paperwork yet.  If you're OK
> >   with it, please fill the form below and send it to the FSF so they can
> >   send you the relevant paperwork to sign.
> >   [ The change is sufficiently small that we can accept it right away,
> >     but since the paperwork process takes some time, it's good to do it
> >     "in advance" so it's out of the way for your next contributions.  ]
>
> Thanks for checking the patch.
> I'll look into signing the paperwork, I may do other contributions in
> the future, so best get that handled sooner than later.
>
> > - I see the macro binds undo limits, but AFAICT this is an "accident"
> >   resulting from copy&pasting code from the other macro: for the
> >   atomic-change macro it's very important that undo info is not thrown
> >   away since the macro uses the undo info internally to cancel changes
> >   on error, but for this macro I can't see any harm in having the undo
> >   info truncated, so I think we shouldn't change the undo limit
> >   vars.  WDYT?
>
> This was intentional, the reasoning is that it should not be possible
> for the amalgamated undo information to form a partial undo step.
>
> In other words, I believe there should be a guarantee that a single
> undo restores the state before `with-undo-amalgamate-change-group' is
> used. Or, in the unlikely case a single undo-step exceeds memory
> limits, that undo fails entirely instead of undoing into a state
> part-way through the body within the macro.
> This is in keeping with how you would expect undo to work for any
> other command AFAICS.
>
> In my use case this is an important guarantee since undo/redoing an
> action needs to ensure the action was fully undone before performing
> the new action. Failure to do so will cause bugs that could be
> difficult to track down.
>
> This should be noted in the code-comments, if the rationale above
> seems reasonable I'll update the patch.
>
> Another note on naming, this could be called `with-undo-amalgamate',
> not sure if the "-change-group" suffix is worth including (I did this
> to fit in with existing names, although it seems a bit verbose).

Send the copyright assignment email to `assign(_at_)gnu.org'
updated the patch with added comment,

[-- Attachment #2: emacs-with-undo-amalgamate-change-group.diff --]
[-- Type: text/x-patch, Size: 1658 bytes --]

commit 204c5d03adf6407d7676ad241b139187f5a84ea1
Author: Campbell Barton <ideasman42@gmail.com>
Date:   Sun Nov 7 19:39:29 2021 +1100

    Add 'with-undo-amalgamate-change-group' macro
    
    This allows commands to be made without adding undo-barriers, e.g.
    kmacro-exec-ring-item.

diff --git a/lisp/subr.el b/lisp/subr.el
index f6dbd00532..c68b334e42 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3542,6 +3542,28 @@ atomic-change-group
 	     (accept-change-group ,handle)
 	   (cancel-change-group ,handle))))))
 
+(defmacro with-undo-amalgamate-change-group (&rest body)
+  "Like `progn' but perform BODY with amalgamated undo barriers.
+
+This allows multiple operations to be undone in a single step."
+  (declare (indent 0) (debug t))
+  (let ((handle (make-symbol "--change-group-handle--")))
+    `(let ((,handle (prepare-change-group))
+           ;; Don't truncate any undo data in the middle of this.
+           ;; This guarantees the resulting undo step is complete,
+           ;; so the user can't undo into an intermediate step
+           ;; performed within the body of this macro.
+           (undo-outer-limit nil)
+           (undo-limit most-positive-fixnum)
+           (undo-strong-limit most-positive-fixnum))
+       (unwind-protect
+           (progn
+             (activate-change-group ,handle)
+             ,@body)
+         (progn
+           (accept-change-group ,handle)
+           (undo-amalgamate-change-group ,handle))))))
+
 (defun prepare-change-group (&optional buffer)
   "Return a handle for the current buffer's state, for a change group.
 If you specify BUFFER, make a handle for BUFFER's state instead.