Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change f59d1be: Move undo amalgamation to lisp.

[Phillip Lord]

Stefan Monnier <address@hidden> writes:

>>> I think this code should be called from self-insert-command rather than
>>> from pre-command-hook.
>> On a hook?  Or do I just directly call a function defined in lisp form C?
>
> Yes, you can just call a Lisp function directly from C.

Okay, that's straight-forward enough.

>> Does the same argument apply to the post-command-hook and
>> after-change-functions also?
>
> For the after-change-functions: yes, very much so.
>
> For the post-command-hook, I also think we could/should call the
> function directly rather than go through post-command-hook, but there
> are arguments in favor of either choice.
>
> At least, calling the function directly is safer in the sense that it
> is closer to the pre-existing code.

For me, it also has the advantage that it will occur at a defined time
wrt to any functions on post-command-hook, which should make the
behaviour more predictable.

Set against this, of course, is that it also becomes harder to change
from lisp. Does the

  call2 (Qmy_function,arg1,arg2)

work like a normal lisp call? I mean, can I redefine my-function, and
will it run the new definition? Is it still open to advice?

>>> And it should also be called from delete-char.
>> Yes, next on my list.
>
> Ah, fine, then.
>
>>> We don't actually know that (cdr last-before-nil) and (car
>>> last-before-nil) are numbers.  The previous self-insert-command might
>>> have performed all kinds of buffer modifications (via abbrev-expansion,
>>> post-self-insert-hook, ...).
>> Hmmm. That's unfortunate -- I was trying to avoid "global" state and
>> just user buffer state; the undo-list seemed like a sensible place to
>> get this knowledge from.
>
> The current logic in remove_excessive_undo_boundaries is far from
> perfect, but unless you have a really good idea how to do it
> differently, I recommend you just try to reproduce it in Elisp.


As I said, the difficulty comes about from trying to work out whether
the last undo-boundary is an "automatic" one (i.e. added by the C layer
and the command loop) or a "manual" one (i.e. added by a call to
undo-boundary).

This works like so:

in keyboard.c

#+begin_src c
  if (NILP (KVAR (current_kboard, Vprefix_arg))) /* FIXME: Why?  --Stef  */
    {
      Lisp_Object undo = BVAR (current_buffer, undo_list);
      Fundo_boundary ();
      last_undo_boundary
        = (EQ (undo, BVAR (current_buffer, undo_list))
           ? Qnil : BVAR (current_buffer, undo_list));
    }
  call1 (Qcommand_execute, Vthis_command);
#+end_src


in cmds.c (243) we have

#+begin_src c
  if (remove_boundary
      && CONSP (BVAR (current_buffer, undo_list))
      && NILP (XCAR (BVAR (current_buffer, undo_list)))
      /* Only remove auto-added boundaries, not boundaries
         added by explicit calls to undo-boundary.  */
      && EQ (BVAR (current_buffer, undo_list), last_undo_boundary))
    /* Remove the undo_boundary that was just pushed.  */
    bset_undo_list (current_buffer, XCDR (BVAR (current_buffer, undo_list)));
#+end_src


In otherwords, we remember the last undo-boundary put in by the command
loop, and we only remove the last undo-boundary from bufffer-undo-list
if it is that one.

What I didn't like about this logic is that it only works for a single
buffer; it assumes that there is only one last_undo_boundary. But a
self-insert-command might result in changes to more than one buffer, if
there is a function on post-command-hook or after-change-functions.

As I said before, the ultimate problem is that all undo-boundaries look
alike, because they are all nil. I had a brief look to see how hard it
would be to change this; I've found about four places in lisp that
depend on this "boundary as nil" behaviour.

 - primitive-undo (while (setq next (pop list)))
 - undo-make-selective-list (cond ((null undo-elt)))
 - undo-elt-in-region (eq undo-elt nil)
 - undo-elt-crosses-region (cond ((atom undo-elt) nil))

There are quite a few places in C also. Fixable, but I suspect I'd
introduce more problems than I would solve.

Anyway, I'll move the calls away from the hooks over the next few days,
and see how it goes!

Phil