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

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

[Stefan Monnier]

> +(defun undo-auto-pre-command-hook()
> +  (when (and (eq last-command 'self-insert-command)
> +             (eq this-command 'self-insert-command))

I think this code should be called from self-insert-command rather than
from pre-command-hook.  And it should also be called from delete-char.

> +    ;; As last-command was s-i-c, there should be "insert" cons just
> +    ;; before this. We need to check that there have not been too many insertions
> +    (let ((last-before-nil
> +           (cadr buffer-undo-list)))
> +      (when
> +          (> 20
> +             (- (cdr last-before-nil)
> +                (car last-before-nil)))

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


        Stefan

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

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> +(defun undo-auto-pre-command-hook()
>> +  (when (and (eq last-command 'self-insert-command)
>> +             (eq this-command 'self-insert-command))
>
> 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?

Does the same argument apply to the post-command-hook and
after-change-functions also? I've been trying to use already existing
functionality where possible (as it stands, I've only reduced code in
the C layer -- the one thing I have added -- undo-size -- isn't used
anymore, and I may back out before I've finished).

> And it should also be called from delete-char.

Yes, next on my list. Wanted to do one thing first and check that it
didn't crash first.


>> +    ;; As last-command was s-i-c, there should be "insert" cons just
>> +    ;; before this. We need to check that there have not been too many insertions
>> +    (let ((last-before-nil
>> +           (cadr buffer-undo-list)))
>> +      (when
>> +          (> 20
>> +             (- (cdr last-before-nil)
>> +                (car last-before-nil)))
>
> 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.

Other ideas? A buffer-local "undo-auto-self-insert-counter" perhaps? Or
I could just use the logic above when it works and do nothing when it
doesn't which would be safe, but means a post-self-insert-hook might
break the blocks-of-20 amalgamation.

Phil

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

[Stefan Monnier]

>> 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.  Typically, you
do it as follows:

   ...
      call2 (Qmy_function, arg1, arg2);
   ...
   foo_syms ()
   {
     ...
     DEFSYM (Qmy_function, "my-function");
     ...
   }

Since these functions are somewhat internal, I like to call them with
a leading "internal-" prefix, but that's my own preference rather than
a real convention we follow.

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

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


        Stefan

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

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> 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

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

[Stefan Monnier]

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

Yes.  Qmy_function is just the symbol, so the C code will call whichever
function is bound to this symbol, like a call from Lisp would do.

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

You can solve it in the same way we do it no: save the auto-added boundary
in a variable (last_undo_boundary) when you add it, so you can
afterwards use an `eq' test to figure out if this boundary was
added automatically.

Since you're moving the "auto-adding" to Lisp, last_undo_boundary will
naturally move to Lisp as well.

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

No: last_undo_boundary can be buffer-local.  You can also make it
contain more info (e.g. add for example which was the command that
created the corresponding change, or which other buffers received
a boundary at the same, ...).


        Stefan

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

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> 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?
>
> Yes.  Qmy_function is just the symbol, so the C code will call whichever
> function is bound to this symbol, like a call from Lisp would do.

Okay, no worries.


>>> 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).
>
> You can solve it in the same way we do it no: save the auto-added boundary
> in a variable (last_undo_boundary) when you add it, so you can
> afterwards use an `eq' test to figure out if this boundary was
> added automatically.
>
> Since you're moving the "auto-adding" to Lisp, last_undo_boundary will
> naturally move to Lisp as well.
>
>> 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.
>
> No: last_undo_boundary can be buffer-local.  You can also make it
> contain more info (e.g. add for example which was the command that
> created the corresponding change, or which other buffers received
> a boundary at the same, ...).

Yes, that's true, once it's in lisp.

Phil

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 <monnier@iro.umontreal.ca> 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

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

[Stefan Monnier]

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

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

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> 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

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

[Stefan Monnier]

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

Indeed, with process filters and such there's a real probability that
this isn't the case.  I think we can avoid this problem by making
self-insert-command explicitly call undo-auto-boundaries at its end.

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

In the worst case self-insert-command can also change the whole buffer.
So the worst case is not nearly as important as the "reasonably expectable
cases".

> So amalgamating these changes might result in a big buffer-undo-list.

I don't see how/why the size of buffer-undo-list would be affected.


        Stefan

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

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> 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.
>
> Indeed, with process filters and such there's a real probability that
> this isn't the case.  I think we can avoid this problem by making
> self-insert-command explicitly call undo-auto-boundaries at its end.
>
>> 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.
>
> In the worst case self-insert-command can also change the whole buffer.
> So the worst case is not nearly as important as the "reasonably expectable
> cases".

In the case of lentic, the worst case is, unfortunately, not uncommon.
Original it happened every key press, not it is rare enough not to cause
noticable lag, but it is enough to add a lot of stuff into
buffer-undo-list.


>> So amalgamating these changes might result in a big buffer-undo-list.
>
> I don't see how/why the size of buffer-undo-list would be affected.

Oh, sorry, I mean, "big" in the sense of "big chunks between boundaries"
which in turn means "not open for GC".

Regardless, I've made the changes, and pushed them.

I have noticed one problem case. The *scratch* buffer is created without
an undo-boundary after the ";;; This buffer is..." message. I think this
is a bootstrap problem and can be fixed by adding an undo-boundary call
to startup.el.

Other than this, are these changes ready to go?

Phil

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

[Stefan Monnier]

> I have noticed one problem case. The *scratch* buffer is created without
> an undo-boundary after the ";;; This buffer is..." message. I think this
> is a bootstrap problem and can be fixed by adding an undo-boundary call
> to startup.el.

The boundary is supposed to be added just before the first command
(since undo-boundaries are added by the command loop right before
running a command).  so maybe the problem is that it doesn't get added
to your list of "buffers with undo elements"?

> Other than this, are these changes ready to go?

I'll let you know as soon as I find the time to review it,


        Stefan

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

[Phillip Lord]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> I have noticed one problem case. The *scratch* buffer is created without
>> an undo-boundary after the ";;; This buffer is..." message. I think this
>> is a bootstrap problem and can be fixed by adding an undo-boundary call
>> to startup.el.
>
> The boundary is supposed to be added just before the first command
> (since undo-boundaries are added by the command loop right before
> running a command).  so maybe the problem is that it doesn't get added
> to your list of "buffers with undo elements"?

I'm working on this, although it's hard to work out. Is Emacs even in
the command loop when it runs startup.el?

My inclination would be to just put an explicit "undo-boundary" into
startup, as it is more straight-forward than working my way through the
emacs boot process.


>> Other than this, are these changes ready to go?
>
> I'll let you know as soon as I find the time to review it,


Apologies! I didn't mean to appear to nag. I am quite patient
(especially at this time of year).

Phil

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

[Stefan Monnier]

>>> I have noticed one problem case. The *scratch* buffer is created without
>>> an undo-boundary after the ";;; This buffer is..." message. I think this
>>> is a bootstrap problem and can be fixed by adding an undo-boundary call
>>> to startup.el.
>> 
>> The boundary is supposed to be added just before the first command
>> (since undo-boundaries are added by the command loop right before
>> running a command).  so maybe the problem is that it doesn't get added
>> to your list of "buffers with undo elements"?

> I'm working on this, although it's hard to work out. Is Emacs even in
> the command loop when it runs startup.el?

No, but that doesn't matter: before you hit your first key, there won't
be any undo-boundary, but as long as the *scratch* buffer is in the list
of "buffers with undo elements", that's OK because it means that as
soon as you hit the first key, just before running this first command,
the command-loop (in which we are as soon as Emacs sits there waiting
for us to hit the first key) should call undo-auto-boundaries (assuming
your patch indeed calls undo-auto-boundaries *before* rather than
*after* running the command).

> My inclination would be to just put an explicit "undo-boundary" into
> startup, as it is more straight-forward than working my way through the
> emacs boot process.

That's fine, indeed.

>>> Other than this, are these changes ready to go?
>> I'll let you know as soon as I find the time to review it,
> Apologies! I didn't mean to appear to nag. I am quite patient
> (especially at this time of year).

No worries, I didn't read it as a nag.


        Stefan

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

[Phillip Lord]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>>> I have noticed one problem case. The *scratch* buffer is created without
>>>> an undo-boundary after the ";;; This buffer is..." message. I think this
>>>> is a bootstrap problem and can be fixed by adding an undo-boundary call
>>>> to startup.el.
>>> 
>>> The boundary is supposed to be added just before the first command
>>> (since undo-boundaries are added by the command loop right before
>>> running a command).  so maybe the problem is that it doesn't get added
>>> to your list of "buffers with undo elements"?
>
>> I'm working on this, although it's hard to work out. Is Emacs even in
>> the command loop when it runs startup.el?
>
> No, but that doesn't matter: before you hit your first key, there won't
> be any undo-boundary, but as long as the *scratch* buffer is in the list
> of "buffers with undo elements", that's OK because it means that as
> soon as you hit the first key, just before running this first command,
> the command-loop (in which we are as soon as Emacs sits there waiting
> for us to hit the first key) should call undo-auto-boundaries (assuming
> your patch indeed calls undo-auto-boundaries *before* rather than
> *after* running the command).


At the moment, it's more *after* rather than *before*. I just put the
call into immediately after the second of the two calls to
post-command-hook. I will move it. How does the error handling work? If
the lisp called by

call1(Qcommand_execute, Vthis_command)

errors, I am guessing the stuff after it doesn't run? Rather, execution
passes to command_loop_2, which then re-enters? This would be another
reason to add it before, because I guess we should add an undo-boundary
if the command errors?

The positive side of this *scratch* buffer does eventually get an
undo-boundary, but only because of the timer I implemented. So, at least
now I am sure that this is working.


>> My inclination would be to just put an explicit "undo-boundary" into
>> startup, as it is more straight-forward than working my way through the
>> emacs boot process.
>
> That's fine, indeed.
>
>>>> Other than this, are these changes ready to go?
>>> I'll let you know as soon as I find the time to review it,
>> Apologies! I didn't mean to appear to nag. I am quite patient
>> (especially at this time of year).
>
> No worries, I didn't read it as a nag.


Thanks!

Phil

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

[Stefan Monnier]

>> for us to hit the first key) should call undo-auto-boundaries (assuming
>> your patch indeed calls undo-auto-boundaries *before* rather than
>> *after* running the command).
> At the moment, it's more *after* rather than *before*.

The old code does it before.
See, you just got bitten because you gratuitously changed that ;-)


        Stefan

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

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> for us to hit the first key) should call undo-auto-boundaries (assuming
>>> your patch indeed calls undo-auto-boundaries *before* rather than
>>> *after* running the command).
>> At the moment, it's more *after* rather than *before*.
>
> The old code does it before.
> See, you just got bitten because you gratuitously changed that ;-)


Well, I strongly dispute this.

I had to learn C to make these changes, and I still haven't got a clue
what I am doing. So the changes were not gratuitous. They were made
entirely out of ignorance of what the code does, and fear that it might
stop working if I changed it again.

In the words of my four year old, are we there yet?

Phil

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

[Stefan Monnier]

>> The old code does it before.
>> See, you just got bitten because you gratuitously changed that ;-)
> Well, I strongly dispute this.

Should we get our kids together so they can throw a tantrum for us?


        Stefan

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

[David Kastrup]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> The old code does it before.
>>> See, you just got bitten because you gratuitously changed that ;-)
>> Well, I strongly dispute this.
>
> Should we get our kids together so they can throw a tantrum for us?

There's

<URL:http://www.splode.com/~friedman/software/emacs-lisp/src/flame.el>

for that.  I thought that it was distributed as part of Emacs at one
time but I might be mistaken.

-- 
David Kastrup

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

[Phillip Lord]

David Kastrup <dak@gnu.org> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>>> The old code does it before.
>>>> See, you just got bitten because you gratuitously changed that ;-)
>>> Well, I strongly dispute this.
>>
>> Should we get our kids together so they can throw a tantrum for us?
>
> There's
>
> <URL:http://www.splode.com/~friedman/software/emacs-lisp/src/flame.el>
>
> for that.  I thought that it was distributed as part of Emacs at one
> time but I might be mistaken.


Next you'll tell me that there is a

M-x generate-long-thread-on-emacs-devel

I'm sure that the current git thread has been recycled from before the
change, just with some changes in tense.

Phil

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

[David Kastrup]

phillip.lord@russet.org.uk (Phillip Lord) writes:

> David Kastrup <dak@gnu.org> writes:
>
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>
>>>>> The old code does it before.
>>>>> See, you just got bitten because you gratuitously changed that ;-)
>>>> Well, I strongly dispute this.
>>>
>>> Should we get our kids together so they can throw a tantrum for us?
>>
>> There's
>>
>> <URL:http://www.splode.com/~friedman/software/emacs-lisp/src/flame.el>
>>
>> for that.  I thought that it was distributed as part of Emacs at one
>> time but I might be mistaken.
>
>
> Next you'll tell me that there is a
>
> M-x generate-long-thread-on-emacs-devel

That's built-in.  When core memory was still a scarce resource, one had
to resort to less subtle means like putting an obtrusive help function
on the backspace key.

But now that functionality is scattered around less conspicuously.

-- 
David Kastrup