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

[Phillip Lord]

Stefan Monnier <address@hidden> writes:

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

Okay.

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

Yeah, this was my mistake. I have fixed this now. I believe my code
means that other "amalgamating" commands could be added freely (i.e.
from within lisp) also.


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

I've moved all of this to lisp now. I don't currently have the
executing_kbd_macro check in, but I guess I could add.


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

Oops

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

Yes. Seemed like a good idea, but wasn't.



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

These and the other stylistic elements that you mentioned have been fixed.



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

I did think about this, or something similar.

But, after self-insert-command, actually, undo-undoably-changed-buffers
tells all the buffers that were modified since the last time we added an
auto-boundary. This will only be the same as the buffers which have
changed as a result of self-insert-command iff
undo-undoably-changed-buffers was nil before the command. It need not be
if buffers are undoably-changing as a result of a timer or a process for
instance.

My other concern is that after a self-insert-command, I can guarantee
that the current-buffer hasn't changed much (normally by one char). But,
for example, with lentic a self-insert-command in one buffer can in
worse case result in all the characters in another buffer changing. So
amalgamating these changes might result in a big buffer-undo-list.

It should be possible for package developers who care to run the
amalgamation code in the buffers of their choice, I think.

Phil