Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change c7a6601 1/5: undo-size can count number of boundaries.

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change c7a6601 1/5: undo-size can count number of boundaries.

[Stefan Monnier]

Hi Philip,

Here are some nitpicks and suggestions on your code.

> +  if(n){

Please try to reproduce the code layout you see around.
(and/or (re)read the GNU Coding Standards which describe the coding
style to use).

In the above:
- Add a space before the open paren.
- Please the open brace on its own line.

> +          // we have a boundary, so check we do not have too many

Please capitalize and punctuate your comments.

> +          if (XCAR (elt) == Qnil)

Don't compare Lisp_Objects with == but with "EQ (x, y)" or in this case
with "NILP (elt)".  While hacking on the C code I recommend you

   ./configure --enable-checking --enable-check-lisp-object-type

which will help you catch those problems and a few others.

> +  if(NILP (Vundo_buffer_undoably_changed)){
> +    Fset (Qundo_buffer_undoably_changed,Qt);
> +    safe_run_hooks (Qundo_first_undoable_change_hook);
> +  }

Why do you need Vundo_buffer_undoably_changed?
Doesn't (car-safe buffer-undo-list) give you the same information?

> +               doc: /* Normal hook run when a buffer has its first recent undo-able change.

Rather than "recent undo-able change", I'd talk about the "first undo
record after a boundary".

> +This hook will be run with `current-buffer' as the buffer that
> +has changed. Recent means since the value of `undo-buffer-undoably-changed'
              ^^^
Please try and follow the sentence-end-double-space convention.


        Stefan

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change c7a6601 1/5: undo-size can count number of boundaries.

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> Here are some nitpicks and suggestions on your code.
>
>> +  if(n){
>
> Please try to reproduce the code layout you see around.
>
>> +          // we have a boundary, so check we do not have too many
>
> Please capitalize and punctuate your comments.
>
>> +          if (XCAR (elt) == Qnil)
>
> Don't compare Lisp_Objects with == but with "EQ (x, y)" or in this case
> with "NILP (elt)".  While hacking on the C code I recommend you
>
>    ./configure --enable-checking > (and/or (re)read the GNU Coding Standards which describe the coding
> style to use).
>
> In the above:
> - Add a space before the open paren.
> - Please the open brace on its own line.
--enable-check-lisp-object-type
>
> which will help you catch those problems and a few others.

All those should be fixed now.

>
>> +  if(NILP (Vundo_buffer_undoably_changed)){
>> +    Fset (Qundo_buffer_undoably_changed,Qt);
>> +    safe_run_hooks (Qundo_first_undoable_change_hook);
>> +  }
>
> Why do you need Vundo_buffer_undoably_changed?
> Doesn't (car-safe buffer-undo-list) give you the same information?

No. The point is that this can be reset when ever I choose, and so it
may well occur *not* after a boundary.

Still your question tells me that my name is not very good.
"first-recent-change" hook might be better, and
"buffer-recently-changed", perhaps.

You should be able to see how this all fits together now. Part of my
purpose is that the meaning and semantics of "recent" are now in the
hands of the lisp developer, since they can reset
"buffer-unably-changed" as they want, which will result in a new call to
the hook.

I have chosen not to add a "undoable-change" hook, because I didn't need
it. The first change hook calls rarely, so the performance issues with
handling this change in lisp are, I think, unimportant.



>> +               doc: /* Normal hook run when a buffer has its first recent undo-able change.
>
> Rather than "recent undo-able change", I'd talk about the "first undo
> record after a boundary".

I would have, iff it did this.

>> +This hook will be run with `current-buffer' as the buffer that
>> +has changed. Recent means since the value of `undo-buffer-undoably-changed'
>               ^^^
> Please try and follow the sentence-end-double-space convention.

Fixed now, hopefully.

Phil

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change c7a6601 1/5: undo-size can count number of boundaries.

[Stefan Monnier]

>>> +  if(NILP (Vundo_buffer_undoably_changed)){
>>> +    Fset (Qundo_buffer_undoably_changed,Qt);
>>> +    safe_run_hooks (Qundo_first_undoable_change_hook);
>>> +  }
>> Why do you need Vundo_buffer_undoably_changed?
>> Doesn't (car-safe buffer-undo-list) give you the same information?
> No. The point is that this can be reset when ever I choose, and so it
> may well occur *not* after a boundary.

But why would you want to set it not after a boundary?

> hands of the lisp developer, since they can reset
> "buffer-unably-changed" as they want, which will result in a new call
> to the hook.

When is this needed/useful?

> I have chosen not to add a "undoable-change" hook, because I didn't need
> it. The first change hook calls rarely, so the performance issues with
> handling this change in lisp are, I think, unimportant.

Agreed.


        Stefan

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change c7a6601 1/5: undo-size can count number of boundaries.

[Phillip Lord]

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

>>>> +  if(NILP (Vundo_buffer_undoably_changed)){
>>>> +    Fset (Qundo_buffer_undoably_changed,Qt);
>>>> +    safe_run_hooks (Qundo_first_undoable_change_hook);
>>>> +  }
>>> Why do you need Vundo_buffer_undoably_changed?
>>> Doesn't (car-safe buffer-undo-list) give you the same information?
>> No. The point is that this can be reset when ever I choose, and so it
>> may well occur *not* after a boundary.
>
> But why would you want to set it not after a boundary?

Because I am using it to provide a backdrop, default, automatically add
a boundary heuristic. Waiting for a boundary to be added defeats the point.

>> hands of the lisp developer, since they can reset
>> "buffer-unably-changed" as they want, which will result in a new call
>> to the hook.
>
> When is this needed/useful?

When you want to store a list of all the buffers which have had undoable
changes recently. In this case, I am using this to work out which
buffers may need boundary. That's the point.

Also, it kicks off the timer which checks all buffers in this list to
see if any of them need a boundary.

Having it in lisp, I don't think you would need it. But you might need
it NOT to happen. The old heuristic of "add an undo if the any other
buffer has been written" caused my problems with lentic. This heuristic
will break sometime, somewhere (it's in the nature of heuristics). If it
is in lisp, it can be prevented, fixed or changed without having to
change the Emacs C layer and on a per buffer basis.

>> I have chosen not to add a "undoable-change" hook, because I didn't need
>> it. The first change hook calls rarely, so the performance issues with
>> handling this change in lisp are, I think, unimportant.
>
> Agreed.

Phil

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change c7a6601 1/5: undo-size can count number of boundaries.

[Stefan Monnier]

> Because I am using it to provide a backdrop, default, automatically add
> a boundary heuristic.  Waiting for a boundary to be added defeats the point.

I think we're misunderstanding each other.

What I'm suggesting is to replace:

   if(NILP (Vundo_buffer_undoably_changed)){
     Fset (Qundo_buffer_undoably_changed,Qt);
     safe_run_hooks (Qundo_first_undoable_change_hook);
   }

with

   if (NILP (CAR (BVAR (current_buffer, undo_list)))
     safe_run_hooks (Qundo_first_undoable_change_hook);

and do it right before we add something to the undo-list (so the test of
undo_list indeed tells us if this is the first new change pushed since
the last boundary).  It should give us the same behavior but without the
need for that variable.


        Stefan

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change c7a6601 1/5: undo-size can count number of boundaries.

[Phillip Lord]

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

>> Because I am using it to provide a backdrop, default, automatically add
>> a boundary heuristic.  Waiting for a boundary to be added defeats the point.
>
> I think we're misunderstanding each other.
>
> What I'm suggesting is to replace:
>
>    if(NILP (Vundo_buffer_undoably_changed)){
>      Fset (Qundo_buffer_undoably_changed,Qt);
>      safe_run_hooks (Qundo_first_undoable_change_hook);
>    }
>
> with
>
>    if (NILP (CAR (BVAR (current_buffer, undo_list)))
>      safe_run_hooks (Qundo_first_undoable_change_hook);
>
> and do it right before we add something to the undo-list (so the test of
> undo_list indeed tells us if this is the first new change pushed since
> the last boundary).  It should give us the same behavior but without the
> need for that variable.

Because, that triggers the hook only after a boundary. With
"undo-buffer-undoably-changed", the hook is triggered after
undo-buffer-undoably-changed is set to nil which may or may not relate
to the addition of a boundary. And actually, currently it doesn't.

Anyway, my code is current safe but doesn't work (I didn't realise that
the GC buffer compaction normally leaves the first element in the
buffer-undo-list), so I need to fix that which is going to take a couple
of days. Hopefully, it will all make sense once it's finished!

Phil

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change c7a6601 1/5: undo-size can count number of boundaries.

[Stefan Monnier]

>> and do it right before we add something to the undo-list (so the test of
>> undo_list indeed tells us if this is the first new change pushed since
>> the last boundary).  It should give us the same behavior but without the
>> need for that variable.
> Because, that triggers the hook only after a boundary.  With
> "undo-buffer-undoably-changed", the hook is triggered after
> undo-buffer-undoably-changed is set to nil which may or may not relate
> to the addition of a boundary. And actually, currently it doesn't.

So the question becomes: why doesn't it?


        Stefan

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change c7a6601 1/5: undo-size can count number of boundaries.

[Phillip Lord]

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

>>> and do it right before we add something to the undo-list (so the test of
>>> undo_list indeed tells us if this is the first new change pushed since
>>> the last boundary).  It should give us the same behavior but without the
>>> need for that variable.
>> Because, that triggers the hook only after a boundary.  With
>> "undo-buffer-undoably-changed", the hook is triggered after
>> undo-buffer-undoably-changed is set to nil which may or may not relate
>> to the addition of a boundary. And actually, currently it doesn't.
>
> So the question becomes: why doesn't it?


Okay. So, I'll recap, and then explain the logic that I am trying to
implement.

I've removed the behaviour from undo.c which calls undo-boundary in a
buffer whenever a change has happened in any other buffer because it
breaks my use case (and others using post-command-hook).

This removes a default which ensures that an undo-boundary were inserted
in long-running processes periodically, so avoiding the
"undo-outer-limit" situation which largely happens in buggy situations
(or when a user has done something really big).

So, the logic I am trying to implement (or think I have!) is:

- record all buffers which have had undo-able changes in them as we go

- every ten seconds check the size of the undo list, and iff it is
  bigger than undo-limit, and has less than 2 boundaries, add one at the
  start.

We need 2 boundaries because the undo-list compact always leaves upto
the first boundary.

In reality, I have made two optimisations which make life slightly more
complex.

- The timer actually runs 10 seconds only after the first undoable
  change in any buffer, since the last time we checked (i.e. it will run
  once in an idle emacs).

- I have not added a "undoable-change-hook" into lisp space because it
  will get called a lot. Rather, we have "first-undoable-change" which
  is called only if "undo-buffer-undoably-changed" is nil. The timer
  resets the buffer-local values of this to nil when it runs.

I've pushed some more changes and will test further tomorrow. It's
starting to behave as I expected now (my C was broken amoung other
things -- initializing local variables! What's that all about?).

I need to rename "undo-buffer-undoably-changed" to "-recently-changed" I
think.

Just to repeat everything that others have said, your patience and
kindness at dealing with dumb questions is rather humbling. Good luck to
you!

Phil

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change c7a6601 1/5: undo-size can count number of boundaries.

[Stefan Monnier]

> Okay. So, I'll recap, and then explain the logic that I am trying to
> implement.

So, IIUC this undo-buffer-undoably-changed is meant to distinguish "new
change since boundary" from "new change since last timer run").

OK, so we were indeed miscommunicating.

But I get the impression that you didn't pay attention to another part
of the logic that we need: the part that pushes a boundary at the end of
a command.  Currently this is done in the C code and is applied only to
the current buffer, but we should change this so it is applied to all
buffers that were modified during the last command.

> - every ten seconds check the size of the undo list, and iff it is
>   bigger than undo-limit, and has less than 2 boundaries, add one at the
>   start.

Why not just unconditionally add a boundary (i.e. regardless of the undo
list size)?  That would make it more useful (because more predictable
and avoid too large undo steps), and would remove the need for
undo-buffer-undoably-changed.


        Stefan

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change c7a6601 1/5: undo-size can count number of boundaries.

[Phillip Lord]

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

>> Okay. So, I'll recap, and then explain the logic that I am trying to
>> implement.
>
> So, IIUC this undo-buffer-undoably-changed is meant to distinguish "new
> change since boundary" from "new change since last timer run").

Not sure "distinguish" is correct. Yes, it implements the "new change
since last timer run" logic. I am not using "new change since boundary"
(which as you say can be determined by looking at buffer-undo-list).


> OK, so we were indeed miscommunicating.
>
> But I get the impression that you didn't pay attention to another part
> of the logic that we need: the part that pushes a boundary at the end of
> a command.  Currently this is done in the C code and is applied only to
> the current buffer, but we should change this so it is applied to all
> buffers that were modified during the last command.

Hmmm, indeed, no I have not done that. I can add this. Does this not
obviate the need for the timer. A long running process buffer in an
otherwise idle emacs would be a counter example, perhaps?

>> - every ten seconds check the size of the undo list, and iff it is
>>   bigger than undo-limit, and has less than 2 boundaries, add one at the
>>   start.
>
> Why not just unconditionally add a boundary (i.e. regardless of the undo
> list size)?  That would make it more useful (because more predictable
> and avoid too large undo steps)

I am not so sure about predictable. The issue here is that the timer
runs 10 seconds after any change at all. So, for a change in a
individual buffer, it could be at any time (10 seconds or less) after an
undoable-change.

But it's easy to make the change. My thought was simply to leave the
undo-list as untouched as much as possible.

> and would remove the need for undo-buffer-undoably-changed.

Unfortunately not! I am using this to enable the "first change"
functionality. I have to know which buffers have had a *recent* undoable
change *or* I have to signal all undoable changes.

I could remove this variable and use a list instead. In fact, that list
actually exists (undo-undoably-change-buffers). Equally, I could remove
that list, and filter all buffers for those where
undo-buffer-undoably-changed is t. Probably I should do at least the
latter since it's simpler, and cycling through all buffers at most every
10 seconds is not going to be a high cost operation.

Phil

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change c7a6601 1/5: undo-size can count number of boundaries.

[Stefan Monnier]

>> But I get the impression that you didn't pay attention to another part
>> of the logic that we need: the part that pushes a boundary at the end of
>> a command.  Currently this is done in the C code and is applied only to
>> the current buffer, but we should change this so it is applied to all
>> buffers that were modified during the last command.
> Hmmm, indeed, no I have not done that.  I can add this.

To me, this is the most important part that relates to your original problem.

> Does this not obviate the need for the timer.

Not more than the current "push boundary when modifying another buffer".

> A long running process buffer in an otherwise idle Emacs would be
> a counter example, perhaps?

That's right.

> But it's easy to make the change. My thought was simply to leave the
> undo-list as untouched as much as possible.

Since it would only apply to changes made outside of commands,
i.e. changes from process filters and timers, I don't think "leave the
undo-list as untouched as much as possible" is desirable.  The fact that
we currently never push a boundary (except when some other buffer
happens to be modified) is just an accident.

>> and would remove the need for undo-buffer-undoably-changed.
> Unfortunately not!

I meant in the C code.  The C code would only need to trigger the Elisp
code when a change is made in a buffer which was previously "at an undo
boundary".  Of course, the Elisp code will then want to remember those
buffers "in need of a boundary" somehow (probably via a list like
undo-undoably-change-buffers).


        Stefan

Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change c7a6601 1/5: undo-size can count number of boundaries.

[Phillip Lord]

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

>>> But I get the impression that you didn't pay attention to another part
>>> of the logic that we need: the part that pushes a boundary at the end of
>>> a command.  Currently this is done in the C code and is applied only to
>>> the current buffer, but we should change this so it is applied to all
>>> buffers that were modified during the last command.
>> Hmmm, indeed, no I have not done that.  I can add this.
>
> To me, this is the most important part that relates to your original problem.
>
>> Does this not obviate the need for the timer.
>
> Not more than the current "push boundary when modifying another buffer".
>
>> A long running process buffer in an otherwise idle Emacs would be
>> a counter example, perhaps?
>
> That's right.
>
>> But it's easy to make the change. My thought was simply to leave the
>> undo-list as untouched as much as possible.
>
> Since it would only apply to changes made outside of commands,
> i.e. changes from process filters and timers, I don't think "leave the
> undo-list as untouched as much as possible" is desirable.  The fact that
> we currently never push a boundary (except when some other buffer
> happens to be modified) is just an accident.
>
>>> and would remove the need for undo-buffer-undoably-changed.
>> Unfortunately not!
>
> I meant in the C code.  The C code would only need to trigger the Elisp
> code when a change is made in a buffer which was previously "at an undo
> boundary".  Of course, the Elisp code will then want to remember those
> buffers "in need of a boundary" somehow (probably via a list like
> undo-undoably-change-buffers).


Okay this is, I believe, done now. It's a good deal simpler than it was,
which has to be a positive thing. I am still not entirely convinced -- I
think that the "default" undo-boundary behaviour should be to do as
little as possible, but the logic is implemented in lisp now, so it can
be over-ridden more straight-forwardly if needed. It would also be
possible to add a buffer-local variable `undo-no-auto-boundary' to
switch this behaviour off, as opposed to changing global state, but
adding that seems like overkill at the moment.

I've put some ad-hoc logging in; the code runs post-command and on a
timer and mostly silently, so it's hard to test it in actual use in any
other way. Obviously, I plan to remove that afterwards.

Comments welcome.

Phil