New undo element (fun . args)

New undo element (fun . args)

[Kim F. Storm]

I appreciate the new undo element (fun . args), but there are
two problems with the specific format chosen:

1) it does not work with the "undo-in-region" funtionality.

2) AFAICS, it effectively blocks any future extensions to the
   undo list format.

To fix both of these problems, I suggest to change the format to:

   (apply FUN ARGS BEG . END)

and fix the undo in region to understand it.

-- 
Kim F. Storm  http://www.cua.dk

Re: New undo element (fun . args)

[Miles Bader]

On Sun, 30 Jan 2005 01:47:02 +0100, Kim F. Storm <storm@cua.dk> wrote:
> To fix both of these problems, I suggest to change the format to:
> 
>    (apply FUN ARGS BEG . END)

How about:

   (apply FUN BEG END . ARGS)

?

Seems more natural...

-Miles
-- 
Do not taunt Happy Fun Ball.

Re: New undo element (fun . args)

[Stefan Monnier]

>> To fix both of these problems, I suggest to change the format to:
>> 
>> (apply FUN ARGS BEG . END)

> How about:

>    (apply FUN BEG END . ARGS)

> Seems more natural...

100% agreement.  I even suggest we pass the BEG and END args to FUN.
I.e. basically keep the current setup, except require the first 2 args to be
BEG and END.


        Stefan

Re: New undo element (fun . args)

[Kim F. Storm]

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

>>> To fix both of these problems, I suggest to change the format to:
>>> 
>>> (apply FUN ARGS BEG . END)
>
>> How about:
>
>>    (apply FUN BEG END . ARGS)
>
>> Seems more natural...
>
> 100% agreement.  I even suggest we pass the BEG and END args to FUN.
> I.e. basically keep the current setup, except require the first 2 args to be
> BEG and END.

I was about to suggest that, but I couldn't quite understand the implications
if BEG and END are modified in the undo-in-region undo list -- but probably
it it the correct thing to do.

However, it occurred to me that we need to record a value for the
undo-delta of this undo entry if we want this to work with
undo-in-region.  So something like:

   (apply FUN BEG END DELTA . ARGS)

seems to be needed.  IIUC, the DELTA is the amount of data insert in
or deleted from the region by this command (i.e. the number of bytes
added to or subtracted from END).  But I'm not into every detail of
the undo-in-region, so someone who understands that code should
comment on this, pls.

Maybe if DELTA is nil, this entry should be ignored during
undo-in-region (whatever the implications of that could be).

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: New undo element (fun . args)

[Kim F. Storm]

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

>>> To fix both of these problems, I suggest to change the format to:
>>> 
>>> (apply FUN ARGS BEG . END)
>
>> How about:
>
>>    (apply FUN BEG END . ARGS)
>
>> Seems more natural...
>
> 100% agreement.  I even suggest we pass the BEG and END args to FUN.
> I.e. basically keep the current setup, except require the first 2 args to be
> BEG and END.

Elaborating on my previous proposal, I think the format should be

  (apply FUN INFO . ARGS)

where INFO is either nil (if undo-in-region is not supported), or
(START END . DELTA) if it is.

This makes it easier to handle cases where the extra information is
not needed (e.g. for ses.el), specifically FUN needs just one extra
arg rather than three, and it is sufficient to specify nil for INFO.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: New undo element (fun . args)

[Richard Stallman]

    100% agreement.  I even suggest we pass the BEG and END args to FUN.
    I.e. basically keep the current setup, except require the first 2 args to be
    BEG and END.

That would be a nuisance; you would be unable to use existing functions
for this purpose.

By the way, note that we would have to change SES to handle the new
format, if we change the format.

Re: New undo element (fun . args)

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     100% agreement.  I even suggest we pass the BEG and END args to FUN.
>     I.e. basically keep the current setup, except require the first 2 args to be
>     BEG and END.
>
> That would be a nuisance; you would be unable to use existing functions
> for this purpose.
>
> By the way, note that we would have to change SES to handle the new
> format, if we change the format.

We have two othogonal issues:

a) the format of the undo entry

Here I guess we can agree on (apply DELTA BEG END FUN . ARGS)
And that FUN is called with ARGS only  (if necessary, the
BEG END etc. can be included explicitly in ARGS).

b) the undo-in-region support

I don't know, but in the case of ses, it may not make sense to have
undo in a region, and consequently may be quite hard to put something
sensible into DELTA, BEG and END.

For this purpose, I suppose that we have an alternate, short format
that omits the undo-in-region information:

    (apply FUN . ARGS)

For undo-in-region, this simply says that undo-in-region is not
supported beyond this element in the undo list.

It is trivial to distinguish the two formats - if second element is
an integer, it is the long format, otherwise it is the short format.

That makes it trivial to change ses.el.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: New undo element (fun . args)

[Kim F. Storm]

storm@cua.dk (Kim F. Storm) writes:

>> By the way, note that we would have to change SES to handle the new
>> format, if we change the format.

I just installed changes at the C level to support the two formats

  (apply FUNNAME . ARGS)
  (apply DELTA BEG END FUNNAME . ARGS)

and modified ses.el accordingly (using the first format).

I will look at using this in cua mode.  Maybe other modes which
remaps undo should/could use this too?

I did not implement the support for undo-in-region in simple.el;
somebody want to look into that?

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: New undo element (fun . args)

[Richard Stallman]

    I will look at using this in cua mode.  Maybe other modes which
    remaps undo should/could use this too?

It is very possible.  But we need to think about how to make
undo-in-region work right for these things.

It could be the undoable setq operations in ses.el are associated with
a change to some part of the buffer, in which case they ought to be
recorded that way.

Re: New undo element (fun . args)

[Richard Stallman]

    I don't know, but in the case of ses, it may not make sense to have
    undo in a region, and consequently may be quite hard to put something
    sensible into DELTA, BEG and END.

One of the undoable operations in SES is setting a variable.
This doesn't relate directly to buffer text, but it might
be associated with some region somehow.  I don't know
how this is used, and I think it is important to check.

Could someone check?

I sent another message asking Yavner to help with this.

Re: New undo element (fun . args)

[Stefan Monnier]

> One of the undoable operations in SES is setting a variable.
> This doesn't relate directly to buffer text, but it might
> be associated with some region somehow.  I don't know
> how this is used, and I think it is important to check.

Most of the vars that are thus set/unset are vars of the form "B6" or "C20",
i.e. vars that hold the content of a cell.  So they do have some relation
with one specific region of the buffer: the corresponding cell.  So we could
just replace (apply ses-set-with-undo ,sym ,(symbol-value sym)) with (apply
START END 0 ses-set-with-undo ,sym ,(symbol-value sym)) where START and END
are the boundaries of the cell.  But since the same code is used for other
vars as well, such as vars that relate to a particular column of
the spreadsheet, it'll take a bit more effort.


        Stefan

Re: New undo element (fun . args)

[Kim F. Storm]

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

>> One of the undoable operations in SES is setting a variable.
>> This doesn't relate directly to buffer text, but it might
>> be associated with some region somehow.  I don't know
>> how this is used, and I think it is important to check.
>
> Most of the vars that are thus set/unset are vars of the form "B6" or "C20",
> i.e. vars that hold the content of a cell.  So they do have some relation
> with one specific region of the buffer: the corresponding cell.  So we could
> just replace (apply ses-set-with-undo ,sym ,(symbol-value sym)) with (apply
> START END 0 ses-set-with-undo ,sym ,(symbol-value sym)) where START and END

Should be 0 START END.

> are the boundaries of the cell.  But since the same code is used for other
> vars as well, such as vars that relate to a particular column of
> the spreadsheet, it'll take a bit more effort.

I've tried to make cua use the new undo machinery, but it is more
complicated than I had imagined.

One obstacle is this piece of code in primitive-undo (undo.c):

It is slightly modified to add (apply cdr . nil) rather than (cdr .
nil) to the undo list in this case:

  /* Make sure this produces at least one undo entry,
     so the test in `undo' for continuing an undo series
     will work right.  */
  if (EQ (oldlist, current_buffer->undo_list))
    current_buffer->undo_list
      = Fcons (list3 (Qapply, Qcdr, nil), current_buffer->undo_list);


I don't understand why this is necessary.  The primitive-undo
continues to the next undo boundary, so why is it important what is
recorded "between undo boundaries" ?

The problem I have with cua is that I don't record buffer changes as
such, but rather a change in where the active rectangle is.  It works
to some extent, but as soon as undo hits one of the entries I added to
the undo list, it gets stuck -- and the pending undo list seems to
be stuck at that specific (apply cdr ...) entry.

So somehow what I do breaks the test mentioned above.  But what
exactly is that test trying to differentiate.  I've looked at the
code in simple.el, and I simply don't get it.

Any ideas...?

Re: New undo element (fun . args)

[Richard Stallman]

      /* Make sure this produces at least one undo entry,
	 so the test in `undo' for continuing an undo series
	 will work right.  */
      if (EQ (oldlist, current_buffer->undo_list))
	current_buffer->undo_list
	  = Fcons (list3 (Qapply, Qcdr, nil), current_buffer->undo_list);


    I don't understand why this is necessary.  The primitive-undo
    continues to the next undo boundary, so why is it important what is
    recorded "between undo boundaries" ?

I think it is necessary for this code to work:

		     ;; If something (a timer or filter?) changed the buffer
		     ;; since the previous command, don't continue the undo seq.
		     (let ((list buffer-undo-list))
		       (while (eq (car list) nil)
			 (setq list (cdr list)))
		       ;; If the last undo record made was made by undo
		       ;; it shows nothing else happened in between.
		       (gethash list undo-equiv-table))))

If undoing does not generate any undo entries, there will be nothing
for this hash mark to ride on.  However, I may have misunderstood
something.

      It works
    to some extent, but as soon as undo hits one of the entries I added to
    the undo list, it gets stuck -- and the pending undo list seems to
    be stuck at that specific (apply cdr ...) entry.

Could you debug precisely what is happening when it is "stuck" as you
say?  In what way does the undo process fail to work?  Where is the bug?

Re: New undo element (fun . args)

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>       /* Make sure this produces at least one undo entry,
> 	 so the test in `undo' for continuing an undo series
> 	 will work right.  */
>       if (EQ (oldlist, current_buffer->undo_list))
> 	current_buffer->undo_list
> 	  = Fcons (list3 (Qapply, Qcdr, nil), current_buffer->undo_list);
>
>     I don't understand why this is necessary.  The primitive-undo
>     continues to the next undo boundary, so why is it important what is
>     recorded "between undo boundaries" ?
>
> I think it is necessary for this code to work:
>
> 		     ;; If something (a timer or filter?) changed the buffer
> 		     ;; since the previous command, don't continue the undo seq.
> 		     (let ((list buffer-undo-list))
> 		       (while (eq (car list) nil)
> 			 (setq list (cdr list)))
> 		       ;; If the last undo record made was made by undo
> 		       ;; it shows nothing else happened in between.
> 		       (gethash list undo-equiv-table))))
>
> If undoing does not generate any undo entries, there will be nothing
> for this hash mark to ride on.  However, I may have misunderstood
> something.

I may have misunderstood something too, but IIUC, this checks to see
if the buffer-undo-list has not been modified since the last undo --
by comparing the current head of buffer-undo-list with the the head of
the buffer-undo-list at the end of the last undo as it was recorded in
undo-equiv-table.

So whether that last undo command added anything to buffer-undo-list
or not doesn't seem relevant, as the head of the buffer undo list
should be in the undo-equiv-table in both cases, and gethash should
succeed in both cases.

I will experiment with removing the (apply cdr nil) element and see
if it breaks things.



BTW, I suppose that the apply undo function is not allowed to change
current buffer.  And maybe we should signal an error in primitive-undo
if it does.



>
>       It works
>     to some extent, but as soon as undo hits one of the entries I added to
>     the undo list, it gets stuck -- and the pending undo list seems to
>     be stuck at that specific (apply cdr ...) entry.
>
> Could you debug precisely what is happening when it is "stuck" as you
> say?

I'm trying :-)

>   In what way does the undo process fail to work?  Where is the bug?

I'm not sure it is a bug in the undo machinery or the way I try to use
it in cua, or a combination...


PS: If it turns out that my understanding is wrong, my concern with
adding (apply cdr nil) is when the (apply ...) element is only one
among several undo elements between two undo boundaries--as long as
just one of those elements generate an undo element in
buffer-undo-list it should be sufficient to fulfill that requirement.

So this would be a problem only in the case where (apply ...) is the
only action between two undo boundaries. To work around this, it seems
to be sufficient to add just one element at the end of Fprimitive_undo
in case the undo list hasn't changed at all.


-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: New undo element (fun . args)

[Richard Stallman]

    I may have misunderstood something too, but IIUC, this checks to see
    if the buffer-undo-list has not been modified since the last undo --
    by comparing the current head of buffer-undo-list with the the head of
    the buffer-undo-list at the end of the last undo as it was recorded in
    undo-equiv-table.

That is right.

    So whether that last undo command added anything to buffer-undo-list
    or not doesn't seem relevant, as the head of the buffer undo list
    should be in the undo-equiv-table in both cases,

If an undo command does not generate any undo entries, it will have to
put into undo-equiv-table a head-of-list that was produced by some
other command, which may not have been an undo command.  That is
certainly not supposed to happen.  Whether it actually leads to
incorrect results, I don't know.

    So this would be a problem only in the case where (apply ...) is the
    only action between two undo boundaries. To work around this, it seems
    to be sufficient to add just one element at the end of Fprimitive_undo
    in case the undo list hasn't changed at all.

Yes, I think so.

    BTW, I suppose that the apply undo function is not allowed to change
    current buffer.

Yes it can.  That is why it can specify a DELTA.

Re: New undo element (fun . args)

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     So whether that last undo command added anything to buffer-undo-list
>     or not doesn't seem relevant, as the head of the buffer undo list
>     should be in the undo-equiv-table in both cases,
>
> If an undo command does not generate any undo entries, it will have to
> put into undo-equiv-table a head-of-list that was produced by some
> other command, which may not have been an undo command.  That is
> certainly not supposed to happen.  

I see, yes.

>     So this would be a problem only in the case where (apply ...) is the
>     only action between two undo boundaries. To work around this, it seems
>     to be sufficient to add just one element at the end of Fprimitive_undo
>     in case the undo list hasn't changed at all.
>
> Yes, I think so.

I'll change it to do so.

>
>     BTW, I suppose that the apply undo function is not allowed to change
>     current buffer.
>
> Yes it can.  That is why it can specify a DELTA.

I know.  What I meant was this:

I suppose that the apply undo function is not allowed to 
have SWITCHED BUFFER upon return.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: New undo element (fun . args)

[Richard Stallman]

    I suppose that the apply undo function is not allowed to 
    have SWITCHED BUFFER upon return.

Yes, that's right.

Re: New undo element (fun . args)

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     I suppose that the apply undo function is not allowed to 
>     have SWITCHED BUFFER upon return.
>
> Yes, that's right.

I have added a check to primitive-undo.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: New undo element (fun . args)

[Kim F. Storm]

storm@cua.dk (Kim F. Storm) writes:

>>   In what way does the undo process fail to work?  Where is the bug?
>
> I'm not sure it is a bug in the undo machinery or the way I try to use
> it in cua, or a combination...

I have installed change to cua to use the new undo apply element.

The problems reported earlier were just errors in my code.

It turned out to be quite tricky to get the usage right as an apply
function need to install a proper _redo_ entry which may not really do
anything but re-install the original _undo_ entry etc.


I also changed primitive-undo to install at most one dummy apply element.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: New undo element (fun . args)

[David Kastrup]

storm@cua.dk (Kim F. Storm) writes:

> storm@cua.dk (Kim F. Storm) writes:
>
>>>   In what way does the undo process fail to work?  Where is the
>>>   bug?
>>
>> I'm not sure it is a bug in the undo machinery or the way I try to
>> use it in cua, or a combination...
>
> I have installed change to cua to use the new undo apply element.
>
> The problems reported earlier were just errors in my code.
>
> It turned out to be quite tricky to get the usage right as an apply
> function need to install a proper _redo_ entry which may not really
> do anything but re-install the original _undo_ entry etc.
>
>
> I also changed primitive-undo to install at most one dummy apply
> element.

A _lot_ of people are annoyed at the way `undo' also redos things.
For that reason, a package redo.el exists, and people get told about
it frequently on Emacs lists and groups.

I consider it likely that with the new changes, this package will no
longer work.  I also have detected the variable undo-no-redo (not
customizable, though) which wouls appear from its name and description
to solve one half of the problem, the remaining half being that an
actual redo command working in the opposite direction would be
required.

Even if we don't find an actual keybinding for that (one could see
what redo.el uses), using undo with a negative argument could be used
instead.

Since we are likely to break redo.el, wouldn't it be sensible to
provide the functionality from within Emacs once and for all?  It does
not seem like much would be missing for implementing this, and one
could even make do without an extra keybinding by using negative
prefixes to undo.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: New undo element (fun . args)

[Stefan Monnier]

> A _lot_ of people are annoyed at the way `undo' also redos things.
> For that reason, a package redo.el exists, and people get told about
> it frequently on Emacs lists and groups.

> I consider it likely that with the new changes, this package will no
> longer work.

It's possible of course, I haven't looked at how it's written, but I find
it unlikely.  The most obvious way to implement it doesn't need to look at
the undo elements.

> I also have detected the variable undo-no-redo (not customizable, though)
> which wouls appear from its name and description to solve one half of the
> problem,

The command is called `undo-only' and I had originally bound it to
C-x U (i.e. a stronger version of C-x u since it skips over matching
undo-redo pairs in the undo-list), but Richard reminded me that we do
not like to bind different commands to uppercase and lowercase commands and
I haven't bothered to try and find another binding for it.

> the remaining half being that an actual redo command working in
> the opposite direction would be required.

C-f C-/ C-/ C-/ ...

Maybe we should arrange that calling `undo' after `undo-only' does not
continue undoing but switches to redo instead, so that redo.el would amount
to (defalias 'redo-undo 'undo-only) (defalias 'redo-redo 'undo).

> Since we are likely to break redo.el, wouldn't it be sensible to
> provide the functionality from within Emacs once and for all?

I find redo.el to remove functionality rather than to provide it.
But I did go through the trouble to implement undo-only.


        Stefan

Re: New undo element (fun . args)

[Richard Stallman]

    Since we are likely to break redo.el, wouldn't it be sensible to
    provide the functionality from within Emacs once and for all?

That might be a good idea--but it depends on what the specific
functionality is, and what part of it is already supported by
the existing options.

Re: New undo element (fun . args)

[Richard Stallman]

    It turned out to be quite tricky to get the usage right as an apply
    function need to install a proper _redo_ entry which may not really do
    anything but re-install the original _undo_ entry etc.

Ifr the extensible undo entry does some other nontrivial job,
surely its redo entry should redo that nontrivial job, right?
So how can it be that the redo entry only reinstalls the undo entry?

Re: New undo element (fun . args)

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

>     It turned out to be quite tricky to get the usage right as an apply
>     function need to install a proper _redo_ entry which may not really do
>     anything but re-install the original _undo_ entry etc.
>
> Ifr the extensible undo entry does some other nontrivial job,
> surely its redo entry should redo that nontrivial job, right?
> So how can it be that the redo entry only reinstalls the undo entry?

That was the tricky part :-)

It actually took me some time and deep thought to realize what
was (not) needed.

The cua undo stuff only deals with rectangle highlighting, so it only
makes sense to highlight anything when the rectangle data is actually
present/restored.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: New undo element (fun . args)

[Richard Stallman]

      But since the same code is used for other
    vars as well, such as vars that relate to a particular column of
    the spreadsheet, it'll take a bit more effort.

The problem is to decide which part of the text to associate these
variables with.  Is there text at the top of the column that would
make sense to associate them with?

Re: New undo element (fun . args)

[Richard Stallman]

    1) it does not work with the "undo-in-region" funtionality.

Yes, that is true.  However, to make this work required more
than just adding a range to the undo element.  It also
has to say the size change it makes within that region.
undo-elt-in-region and undo-delta have to handle it.
And the code in undo-make-selective-list has to be updated
to modify these elements for other elements that get skipped.

    2) AFAICS, it effectively blocks any future extensions to the
       undo list format.

Not really, but there's no need to argue about it.

I think this format is best:

   (apply DELTA BEG END FUN . ARGS)

It should be documented that BEG and END should be the