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

[Phillip Lord]

So, I think I mostly have this now, although I need to do some more
testing on it to make sure, and clearly the code needs cleaning up (all
the debug infrastructure will need to go, and docstrings be put in
place). I have a few questions inline, however.

Stefan Monnier <address@hidden> writes:
> 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.


In the end, I have left this on post-command-hook. It is this hook that
actually does all the work and using the hook gives the (other)
developer the option of disabling this "intelligent" behavior if it
turns out not to work. This seems sensible to me.

The alternative is sticking a direct call to simple.el into the command
loop, but I would just put it immediately before post-command-hook. It
seems duplicative to me. Against this, is the use of a hook for "core"
functionality rather than an extension. Happy to change if you prefer.


>
> At least, calling the function directly is safer in the sense that it
> is closer to the pre-existing code.
>
>>> And it should also be called from delete-char.
>> Yes, next on my list.
>
> Ah, fine, then.


delete-char does indeed call "remove_excessive_boundaries", but for the
life of me I can not reproduce any affect that this has at the user
level.

If I type "abcde" into a clean buffer, then "undo" all the letters
disappear at once (since they have amalgamated). If, though I have
"abcde" in the buffer and do 5x delete followed by "undo" the letters
reappear one at a time.



>
>>> 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.


I am also a bit confused by this code from cmds.c

#+begin_src c
   if (!EQ (Vthis_command, KVAR (current_kboard, Vlast_command)))
    nonundocount = 0;

   if (NILP (Vexecuting_kbd_macro))
    {
      if (nonundocount <= 0 || nonundocount >= 20)
        {
          remove_boundary = false;
          nonundocount = 0;
        }
      nonundocount++;
    }
#+end_src

current_kboard? KVAR? And why the "executing_kbd_macro" check?

And, secondly, in undo.c I now have the following.

#+begin_src c
  /* Allocate a cons cell to be the undo boundary after this command.  */
  if (NILP (pending_boundary))
    pending_boundary = Fcons (Qnil, Qnil);

  /* Switch temporarily to the buffer that was changed.  */
  current_buffer = buf;

  // PWL running with the wrong current-buffer
  run_undoable_change ();

  if (MODIFF <= SAVE_MODIFF)
    record_first_change ();
#+end_src

Now, I know that you have told me previously that this is bad because
the `current-buffer = buf` line only impacts on C and the call to lisp
is likely to be wrong.

Do I just change the middle bit to ...

#+begin_src c
  /* Switch temporarily to the buffer that was changed.  */
  current_buffer = buf;

  // PWL running with the wrong current-buffer
  set_buffer_internal(b);
  run_undoable_change ();
#+end_src


and then switch back again later like so?

#+begin_src c
  current_buffer = obuf;
  set_buffer_internal(current_buffer);
#+end_src

Cheers!

Phil