Re: An idea: combine-change-calls

[Alan Mackenzie]

Hello, Stefan.

On Sat, Mar 31, 2018 at 19:38:03 -0400, Stefan Monnier wrote:
> > >     (defun wrap-and-run-primitive-undo (beg end list)
> > >       (combine-change-calls
> > That's a refinement I haven't managed to get around to, yet.

> Any difficulty there?
> Naively, I'd expect that it's just a matter of:

>     (defun undo--wrap-and-run-primitive-undo (beg end list)
>       (combine-change-calls
>         (while list
>           (setq list (primitive-undo 1 list)))))
  
> [ and of course, making sure this function is defined after the
>   combine-change-calls macro.  ]

It was exactly that.  No difficulties.

[ .... ]

> > So, I think this exercise is worthwhile.  I have added some doc strings
> > to the functions.  I have introduced a variable to hinder a "recursive"
> > undo--wrap-and-run-primitive-undo appearing in the undo list.  This
> > happened with uncomment-region calling comment-region.

> That's weird: shouldn't the inhibit-modification-hooks already play this role?

I don't think so.  i-m-h is purely to do with whether the hooks should
be run now.  That has nothing to do with how items should be added to
the undo list.

> Oh, wait, I see you don't test inhibit-modification-hooks any more but
> you test undo--combining-change-calls instead.  Any particular reason
> for this change?

inhibit-modification-hooks is tested around the run-hooks-with-args
calls.  If i-m-h is set to non-nil by some function, and comment-region
is run, we still want the undo records to be enclosed by an (apply ...)
form.

> > Sometimes, point ends up at the wrong place after an undo (probably
> > because of primitive-undo removing POSITION elements from undo-lists, as
> > we've already discussed.)

> The code that removes those "position" elements is in `undo`, not in
> `primitive-undo`, so what you're describing must come from something
> else (unless you're seeing this when *redoing* or *reundoing*).

I'm not sure.  I think it's in re-undoing.  As you've already guessed, I
would be in favour of removing that code from `undo' that we don't see
what it's for.

Anyhow, here's another version of the code (below), with the following
changes:
1/- undo--w-a-r-p-undo is now a macro call of c-c-c.
2/- end-marker is now of type t, so that it is relocated when text is
inserted at the postion it points to.  (DELTA was coming out wrong.)
3/- end-marker is now set to point nowhere at the end of the function.
4/- A whole lot of `prog1's have been replaced by the variable
RESULT.
5/- c-c-c-1 no longer incorporates a timestamp undo record ((t . ...))
into the (apply ...) form.  This just seemed wrong.

I think it's almost time to commit this to master.  I propose that
combine-change-calls{-1,} go into subr.el, beside
combine-after-change-calls, and undo--wrap-and-run-primitive-undo go
into simple.el, beside the rest of the undo stuff.

I will introduce the variable comment-combine-change-calls in
newcomment.el, and modify {,un}comment-region to test it, and if set, to
use c-c-c.

I will document c-c-c in the Elisp manual on page "Change Hooks".

For (almost) the last time, here's the current version of the code:



(defvar undo--combining-change-calls nil
  "Non-nil when `combine-change-calls' is running.")

(defun combine-change-calls-1 (beg end body)
  "Evaluate BODY, running the change hooks just once, for region \(BEG END).

Firstly, `before-change-functions' is invoked for the region
\(BEG END), then BODY (a function) is evaluated with
`inhibit-modification-hooks' non-nil, then finally
`after-change-functions' is invoked on the updated region (BEG
NEW-END) with a calculated OLD-LEN argument.  If
`inhibit-modification-hooks' is initially non-nil, the change
hooks are not run.

The result of `comebine-change-calls-1' is the value returned by
BODY.  BODY must not make a different buffer current, except
temporarily.  It must not make any changes to the buffer outside
the specified region.  It must not change
`before-change-functions' or `after-change-functions'.

Additionally, the buffer modifications of BODY are recorded on
the buffer's undo list as a single \(apply ...) entry containing
the function `undo--wrap-and-run-primitive-undo'."
  (let ((old-bul buffer-undo-list)
        (end-marker (copy-marker end t))
        result)
    (if undo--combining-change-calls
        (setq result (funcall body))
      (let ((undo--combining-change-calls t))
        (if (not inhibit-modification-hooks)
            (run-hook-with-args 'before-change-functions beg end))
        (if (eq buffer-undo-list t)
            (setq result (funcall body))
          (let ((inhibit-modification-hooks t))
            (setq result (funcall body)))
          (let ((ap-elt
                 (list 'apply
                       (- end end-marker)
                       beg
                       (marker-position end-marker)
                       #'undo--wrap-and-run-primitive-undo
                       beg (marker-position end-marker) buffer-undo-list))
                (ptr buffer-undo-list))
            (if (not (eq buffer-undo-list old-bul))
                (progn
                  (while (and (not (eq (cdr ptr) old-bul))
                              ;; In case garbage collection has removed OLD-BUL.
                              (cdr ptr)
                              ;; Don't include a timestamp entry.
                              (not (and (consp (cdr ptr))
                                        (consp (cadr ptr))
                                        (eq (caadr ptr) t)
                                        (setq old-bul (cdr ptr)))))
                    (setq ptr (cdr ptr)))
                  (unless (cdr ptr)
                    (message "combine-change-calls: buffer-undo-list broken"))
                  (setcdr ptr nil)
                  (push ap-elt buffer-undo-list)
                  (setcdr buffer-undo-list old-bul)))))
        (if (not inhibit-modification-hooks)
            (run-hook-with-args 'after-change-functions
                                beg (marker-position end-marker)
                                (- end beg)))))
    (set-marker end-marker nil)
    result))

(defmacro combine-change-calls (beg end &rest forms)
  "Evaluate FORMS, running the change hooks just once.

Firstly, `before-change-functions' is invoked for the region
\(BEG END), then the FORMS are evaluated with
`inhibit-modification-hooks' non-nil, and finally
`after-change-functions' is invoked on the updated region.  The
change hooks are not run if `inhibit-modification-hooks' is
non-nil.

The result of `combine-change-calls' is the value returned by the
last of the FORMS to be evaluated.  FORMS may not make a
different buffer current, except temporarily.  FORMS may not
change the buffer outside the specified region.  It must not
change `before-change-functions' or `after-change-functions'.

Additionally, the buffer modifications of BODY are recorded on
the buffer's undo list as a single \(apply ...) entry containing
the function `undo--wrap-and-run-primitive-undo'. "
  `(combine-change-calls-1 ,beg ,end (lambda () ,@forms)))

(defun undo--wrap-and-run-primitive-undo (beg end list)
  "Call `primitive-undo' on the undo elements in LIST.

This function is intended to be called purely by `undo' as the
function in an \(apply DELTA BEG END FUNNAME . ARGS) undo
element.  It invokes `before-change-functions' and
`after-change-functions' once each for the entire region \(BEG
END) rather than once for each individual change.

Additionally the fresh \"redo\" elements which are generated on
`buffer-undo-list' will themselves be \"enclosed\" in
`undo--wrap-and-run-primitive-undo'.

Undo elements of this form are generated by the macro
`combine-change-calls'."
  (combine-change-calls beg end
                        (while list
                          (setq list (primitive-undo 1 list)))))


>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).