Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change 0f3c1db: Changed semantics of first-undoable-change-hook.

[Phillip Lord]

Stefan Monnier <address@hidden> writes:

> Please capitalize and punctuate your comments.
> Please always add spaces before the open parens.
> Here as well (and ,many other places).
> Try to avoid adding spurious empty lines.
> It looks pretty good.  Remaining problems I noticed:
> - run_first_undo_hook is defined to take no argument, yet it's always
>   called with `current_buffer' as argument.

I am working my way through all of these!

> - in src/keyboard.c we push undo-boundaries after every command.
>   This is now made redundant by your code, so you might like to
>   remove it.
>
> - when trying to remove that code, you'll notice that
>   Fself_insert_command (and Fdelete_char, IIRC) depends on that code to
>   know whether the last boundary can be removed (so as to implement the
>   "bundling").


Hmmm. So, I had a look at this, and it's actually more complex than it
appears. The problem, ironically, is the bundling in
Fself_insert_command. My code (first-undoable-change-hook) only runs on
the first change after a boundary. And this doesn't mean "after a
boundary has been added" but "when there is a boundary on the head of
the list immediately after a change has happened". But,
self-insert-command removes this boundary. So my hook doesn't run. At
least I think that this is what is happening. The global state of Emacs
is fairly complex to reason about in this case.

So, I have to hook into post-command-hook to get this to work, which
obviously runs a lot and isn't quite working yet for me.

The bundling implemented by remove_excessive_undo_boundaries is making
me scratch my head also. This also depends on global stage
(last_undo_boundary); if I understand this code correctly, this is the
cons cell that was last added in `keyboard.c'. I have worked out how to
replicate this, but my use case still breaks this because of this use of
global state; last_undo_boundary only works because we are assuming that
Fself_insert_command is only going to add *one* undo boundary. And
that's not true if post-command-hook is doing other things.

The underlying problem here is that "undo-boundary" is marked by nil and
there's only one nil. So all undo boundaries are equivalent; we can't
distinguish between those inserted "automatically" and those inserted
"explicitly", except by looking at the cons. If we used symbols, say
something like

'(boundary reason)

where "boundary" was always the same symbol, and reason could any of a
set of symbols, then life would be easier, because I could distinguish
between boundaries from the command-loop and boundaries from everywhere
else by looking at buffer-undo-list rather than global state of a single
variable. But this is quite a big change.

I am sorry this is taking so long and so much discussion; I'm learning C
as I go which is a little traumatic.


> - It looks like run_first_undo_hook is often called in an "unclean"
>   state where the C code has set current_buffer rather than calling
>   set_buffer_internal.  This is an optimization that only works when we
>   know exactly the code that might possibly be run before current_buffer
>   is set back to its previous value, but it can't be used when running
>   arbitrary Elisp code.  So we should call run_first_undo_hook *before*
>   setting current_buffer, and run_first_undo_hook should
>   set_buffer_internal when needed.

Okay, will fix that.