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

[Stefan Monnier <monnier@iro.umontreal.ca>]

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

See some comments on the latest code below.

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

The current behavior is to call it right *before* running the command,
i.e. after running post-command-hook, and even after running the next
pre-command-hook.

I recommend you keep the bikeshed of the same color.
If someone wants to change the behavior she can use an advice anyway.
And we can move it to a hook later on if we want.

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

Try C-d C-d C-d C-d C-x u.  It should undo all 4 C-d rather than only
the last one.  This is new in Emacs-25.

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

Have you tested with Emacs<25 by any chance?

> current_kboard? KVAR?

    KVAR (current_kboard, Vlast_command)

is the C code needed to find the value of Elisp's `last-command',
because it is a "keyboard-local" variable.

> And why the "executing_kbd_macro" check?

IIUC this is inherited from Emacs<19 and I just blindly preserved it.
I recommend you just do the same unless it causes problems.

>   current_buffer = buf;
>   set_buffer_internal(b);

This break because set_buffer_internal needs to know what was the
previous current_buffer to do its job correctly.  IOW either you use
"current_buffer = buf;" and you're super extra careful to only do
trivial things, or you do "set_buffer_internal(b);", but you don't want
to do both.


        Stefan


>  ;;; Code:
> -
>  (eval-when-compile (require 'cl-lib))

Spurious change.

> +    ;; We need to set `undo-last-boundary' to nil as we are about to
> +    ;; delete the last boundary, so we want to not assume anything about
> +    ;; the boundary before this one
> +    (setq undo-last-boundary nil)

Why is this needed?  The undo-last-boundary handling should work for
"any" command without any extra code (except for self-insert-command or
delete-char, obviously) and I don't see why undo should be different here.

I could imagine the above code as something hypothetically useful as an
optimization, but if it's really needed, I think it hints at a problem
elsewhere.  E.g. maybe uses of undo-last-boundary should always check
(eq this-command last-command), or undo-last-boundary should (just like
the old last_undo_boundary) keep a reference to the value of
buffer-undo-list, or the *command-loop* should set undo-last-boundary to
nil whenever this-command is different from last-command, ...

> +;; This section helps to prevent undo-lists from getting too large. It
> +;; achieves by checking that no buffer has an undo-list which is large
> +;; and has no `undo-boundary', a condition that will block garbage
> +;; collection of that list. This happens silently and in most cases
> +;; not at all, as generally, buffers add their own undo-boundary.
> +;;
> +;; It will still fail if a large amount of material is added or
> +;; removed from a buffer with any rapidity and no undo-boundary. In
> +;; this case, the `undo-outer-limit' machinary will operate; this is
> +;; considered to be exceptional the user is warned.

I think this comment reflects an older version of the code.

> +(defun undo-needs-boundary-p ()

I recommend you use an "undo--" prefix for most of the funs&vars you've
introduced, unless you explicitly want to encourage people to make use
of it from third-party packages.

> +  "Returns non-nil if `buffer-undo-list' needs a boundary at the start."
      ^^^^^^^
M-x checkdoc-current-buffer RET will tell you how to fix this.

> +  (and
> +   ;; `buffer-undo-list' can be t.
> +   (listp buffer-undo-list)
> +   ;; The first element of buffer-undo-list is not nil.
> +   (car buffer-undo-list)))

aka (car-safe buffer-undo-list).

> +(defun undo-last-boundary-sic-p (last-boundary)

The arg is always undo-last-boundary, so you could just drop the arg.

> +  (mapc
> +   (lambda (b)

Use dolist instead.

> +(defun undo-auto-pre-self-insert-command()
                                          ^^
                                      Missing space
> +  (condition-case err
> +      (let ((last-sic-count
> +             (undo-last-boundary-sic-p undo-last-boundary)))
> +        (when
> +            last-sic-count
> +          (if
> +              (< last-sic-count 20)
> +              (progn (undo-auto-message "(changed) Removing last undo")
> +                     (setq buffer-undo-list
> +                           (cdr buffer-undo-list)))
> +            (progn (undo-auto-message "Reset sic to 0")
> +                   (setq undo-last-boundary
> +                         '(command self-insert-command 0))))))
> +    (error
> +     (undo-auto-message "pre-command-error %s"
> +                        (error-message-string err)))))

After self-insert-command, undo-undoably-changed-buffers now tells us
all the buffers that self-insert-command modified, so if we keep this
list in undo-last-boundary, we can (better: should) here remove the
boundaries in all those buffers.

> +  // PWL remove for now
> +  //if (XFASTINT (n) < 2)
> +  //remove_excessive_undo_boundaries ();
> +  call0(Qundo_auto_pre_self_insert_command);

Better keep the (XFASTINT (n) < 2) test.

> -            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));
> -	      }

Here's where you want to call your undo-auto-post-command-hook (which
should be renamed to describe what it does rather than when it's called).

> +  call0(Qundo_undoable_change);
         ^^
   Missing space.

> +  // PWL running with the wrong current-buffer

Please use /*...*/ comments rather than //...\n comments.  It's not
really important, but that's what we use everywhere else.

> +  Fset(Qundo_last_boundary,Qnil);
        ^^
Another missing space.

> +  DEFVAR_LISP ("undo-last-boundary",
> +               Vundo_last_boundary,
> +               doc: /* TODO
> +*/);

This belongs in the Lisp code.