bug#23906: 25.0.95; Undo boundary after process output is not consistent

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Markus Triska]

To reproduce this issue with emacs-25 git and cherry-picked commit
36e69bd82a0294b1f51d99a5eaf8e2c7661f7a16 from master, please do:

1) Download ediprolog.el from:

   https://www.metalevel.at/ediprolog/ediprolog.el

2) Download ceiled.pl from:

   https://www.metalevel.at/ei/ceiled.pl

3) Download simulans2.sh from:

   https://www.metalevel.at/ei/simulans2.sh

   and make it executable with:

   $ chmod +x simulans2.sh

4) Invoke Emacs with:

    emacs -Q ceiled.pl -fn "Bitstream Vera Sans Mono 15" \
         --eval "(load \"$PWD/ediprolog.el\")" \
         --eval "(setq ediprolog-program \"$PWD/simulans2.sh\")" \
         --eval "(global-set-key [f9] 'ediprolog-dwim)"

5) To move point to the start of line 15, press:

   M-g M-g 15 RET

6) We are now ready for the recipe, showing that undo after process
   output is not consistent. The issue I want to show occurs about 1 out
   of 7 times on average. The steps we now repeat are two key presses:

   F9 C-/

   In words, this is: "ediprolog-dwim, directly followed by undo".

   We repeat this over and over: F9 C-/ F9 C-/ F9 C-/ ...

7) Now the point: *Most* of the time, after undo, the buffer is exactly
   as it was before. But sometimes, about 1 out of 7 times, after undo
   is pressed, the characters "%@ " remain at the end of the buffer:

   %?- time(ceiled_square_root(2^10000, R)).
   %@ 

   These three characters ("%@ ") are inserted by ediprolog before
   receiving process output, and in most cases removed after the undo.

The issue in this case is not so much that the %@ appears in the buffer,
but that it is not handled consistently. Most often (and preferably), a
single undo removes both the process output _and_ the %@, but sometimes
a single undo removes *only* the process output, and the %@ remains. The
behaviour I desire is that C-/ consistently removes everything that was
inserted in direct sequence, both the "%@ " _and_ the process output.

Thank you for looking into this!

All the best,
Markus


In GNU Emacs 25.0.95.4 (x86_64-apple-darwin15.5.0, X toolkit, Xaw scroll bars)
 of 2016-07-05 built on mt-imac
Repository revision: e3b039d1a0e611d6619ed3ce67d125160d644ebc
Windowing system distributor 'The X.Org Foundation', version 11.0.11502000
Configured using:
 'configure --without-ns CFLAGS=-I/opt/local/include
 LDFLAGS=-L/opt/local/lib'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK GSETTINGS NOTIFY ACL GNUTLS
LIBXML2 FREETYPE XFT ZLIB TOOLKIT_SCROLL_BARS LUCID X11

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Eli Zaretskii]

> From: Markus Triska <triska@metalevel.at>
> Date: Wed, 06 Jul 2016 19:56:30 +0200
> 
> 7) Now the point: *Most* of the time, after undo, the buffer is exactly
>    as it was before. But sometimes, about 1 out of 7 times, after undo
>    is pressed, the characters "%@ " remain at the end of the buffer:
> 
>    %?- time(ceiled_square_root(2^10000, R)).
>    %@ 
> 
>    These three characters ("%@ ") are inserted by ediprolog before
>    receiving process output, and in most cases removed after the undo.
> 
> The issue in this case is not so much that the %@ appears in the buffer,
> but that it is not handled consistently. Most often (and preferably), a
> single undo removes both the process output _and_ the %@, but sometimes
> a single undo removes *only* the process output, and the %@ remains. The
> behaviour I desire is that C-/ consistently removes everything that was
> inserted in direct sequence, both the "%@ " _and_ the process output.

Given the fact that (AFAIU) undo for subprocesses is caught by code
that runs off a timer, isn't the above expected?  Phillip?

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Markus Triska <triska@metalevel.at>
>> Date: Wed, 06 Jul 2016 19:56:30 +0200
>> 
>> 7) Now the point: *Most* of the time, after undo, the buffer is exactly
>>    as it was before. But sometimes, about 1 out of 7 times, after undo
>>    is pressed, the characters "%@ " remain at the end of the buffer:
>> 
>>    %?- time(ceiled_square_root(2^10000, R)).
>>    %@ 
>> 
>>    These three characters ("%@ ") are inserted by ediprolog before
>>    receiving process output, and in most cases removed after the undo.
>> 
>> The issue in this case is not so much that the %@ appears in the buffer,
>> but that it is not handled consistently. Most often (and preferably), a
>> single undo removes both the process output _and_ the %@, but sometimes
>> a single undo removes *only* the process output, and the %@ remains. The
>> behaviour I desire is that C-/ consistently removes everything that was
>> inserted in direct sequence, both the "%@ " _and_ the process output.
>
> Given the fact that (AFAIU) undo for subprocesses is caught by code
> that runs off a timer, isn't the above expected?  Phillip?


Assuming that ceiled_square_root takes a significant length of time
(10/7 seconds in this case!), yes, I think this is the case.

I assume that the %@ is inserted first before the process is started,
then the results put in after the results come back? This would mean
that after the insertion of %@ there would be no undo-boundary.

You could check by adding %@ immediately before you insert data from
prolog, rather than after you send data to it. To me, this makes more
sense -- you are adding text over an elongated period (i.e. during the
evaluation) without the expectation of an undo boundary. Likewise,
forcing an undo-boundary immediately after %@ would give you consistent
behaviour (although different).

There are a number of fixes we could make for this in the undo system.
I could check for the size of the last undo, before forcing an
undo-boundary. Or, we could add an option to suppress the timer in a
specific buffer; although, this would add the possibility of
out-of-memory if it's not turned on again.

In this case, though, I am inclined toward suggesting changing
ediprolog.el.

Phil

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Markus Triska]

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

> I assume that the %@ is inserted first before the process is started,
> then the results put in after the results come back?

The precise sequence is (see `ediprolog-query' and `ediprolog-interact'):

1) "\n%@ " is inserted into the buffer
2) the Prolog process is started (if necessary)
3) the query is sent to the process
4) when process output arrives, it is appended to the buffer.

The buffer is modified only by (1) and (4). Somtimes there is an undo
boundary between them, and most often there is none (which I prefer).

> This would mean that after the insertion of %@ there would be no
> undo-boundary.

That's exactly what I expect to happen. But in some cases, there
unexpectedly *is* an undo boundary between two subsequent insertions of
text into the buffer, namely between (1) and (4) above.

> You could check by adding %@ immediately before you insert data from
> prolog, rather than after you send data to it. To me, this makes more
> sense -- you are adding text over an elongated period (i.e. during the
> evaluation) without the expectation of an undo boundary.

I will try this, thank you for the suggestion! For completeness: I now
insert %@ *before* I send data to Prolog: (1), then (3) above. I suppose
you means that I should do (1) immediately before (4), which in fact
means that I then *would* insert %@ *after* sending data to Prolog.

> Likewise, forcing an undo-boundary immediately after %@ would give you
> consistent behaviour (although different).

Yes, that would be consistent but rather the opposite of what I want: I
would like to avoid the undo-boundary in all cases.

> In this case, though, I am inclined toward suggesting changing
> ediprolog.el.

I am willing to do that if it achieves consistent results. However, a
bit of reflection is in order because other packages may benefit at the
same time by a more general solution. Please let me know if the use case
is now clear: To repeat, the issue I am reporting is that sometimes
there unexpectedly is an undo boundary although I do not want one. From
the above, it seems that you agree that there should never be an undo
boundary if the insertion of %@ is directly (except for timing) followed
by the insertion of arriving process output. Yet, the timing seems to be
critically involved. So, what would be a general way to always prevent an
undo boundary between insertions that are separated only by time?

I really like your suggestion to temporarily suppress the timer on a
specific buffer, say via a let-binding, while the interaction is in
progress. This would allow users to always undo the *whole* interaction
with Prolog using a single undo. This is what is typically needed when
using ediprolog: You interact with Prolog, and then undo the whole
interaction. In this use case, undo boundaries only get in the way. I
can make this configurable in ediprolog to perserve memory, if needed.

Thank you and all the best!
Markus

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Phillip Lord]

Markus Triska <triska@metalevel.at> writes:

> phillip.lord@russet.org.uk (Phillip Lord) writes:
>
>> I assume that the %@ is inserted first before the process is started,
>> then the results put in after the results come back?
>
> The precise sequence is (see `ediprolog-query' and `ediprolog-interact'):
>
> 1) "\n%@ " is inserted into the buffer
> 2) the Prolog process is started (if necessary)
> 3) the query is sent to the process
> 4) when process output arrives, it is appended to the buffer.
>
> The buffer is modified only by (1) and (4). Somtimes there is an undo
> boundary between them, and most often there is none (which I prefer).

Yes. So the issue here is that between 1) and 4) there is a
indeterminate length of time. In Emacs-24, for example, if you run any
command which modify a buffer between 1) and 4) then, also, you would
get an undo after %@.


>> This would mean that after the insertion of %@ there would be no
>> undo-boundary.
>
> That's exactly what I expect to happen. But in some cases, there
> unexpectedly *is* an undo boundary between two subsequent insertions of
> text into the buffer, namely between (1) and (4) above.

The problem here also is what happens if the process output is long.
Emacs will now add undo-boundaries periodically. Before, I think, the
undo list would have got longer and longer, until emacs errored.



>> You could check by adding %@ immediately before you insert data from
>> prolog, rather than after you send data to it. To me, this makes more
>> sense -- you are adding text over an elongated period (i.e. during the
>> evaluation) without the expectation of an undo boundary.
>
> I will try this, thank you for the suggestion! For completeness: I now
> insert %@ *before* I send data to Prolog: (1), then (3) above. I suppose
> you means that I should do (1) immediately before (4), which in fact
> means that I then *would* insert %@ *after* sending data to Prolog.


Yes. Or rather you could insert %@ immediately before getting data back
from prolog, perhaps with a process filter. Then both of your insertions
would be happening together.

If you want the %@ to still *appear* in the buffer after 2), then you
could use a text property to display something, without actually
changing the buffer, perhaps.


>> Likewise, forcing an undo-boundary immediately after %@ would give you
>> consistent behaviour (although different).
>
> Yes, that would be consistent but rather the opposite of what I want: I
> would like to avoid the undo-boundary in all cases.
>
>> In this case, though, I am inclined toward suggesting changing
>> ediprolog.el.
>
> I am willing to do that if it achieves consistent results. However, a
> bit of reflection is in order because other packages may benefit at the
> same time by a more general solution. Please let me know if the use case
> is now clear: To repeat, the issue I am reporting is that sometimes
> there unexpectedly is an undo boundary although I do not want one. From
> the above, it seems that you agree that there should never be an undo
> boundary if the insertion of %@ is directly (except for timing) followed
> by the insertion of arriving process output. Yet, the timing seems to be
> critically involved. So, what would be a general way to always prevent an
> undo boundary between insertions that are separated only by time?
>
> I really like your suggestion to temporarily suppress the timer on a
> specific buffer, say via a let-binding, while the interaction is in
> progress. This would allow users to always undo the *whole* interaction
> with Prolog using a single undo. This is what is typically needed when
> using ediprolog: You interact with Prolog, and then undo the whole
> interaction. In this use case, undo boundaries only get in the way. I
> can make this configurable in ediprolog to perserve memory, if needed.

I could add that, but the problem is that the timer is meant to prevent
the buffer-undo-list from over-running.

In this case, I'm still inclined toward thinking that the changed
semantics of undo are really uncovering an issue with ediprolog -- you
are expecting two events which are temporally separated to be a single
undo; with Emacs-24.5 it's possible to split them also just in a
different way -- for example if another process was generating output.
Also, what happens if point moves between 1) and 4)?

We could also add specific support for removing the last undo, which you
could do iff point is immediately after $@.

Stefan, any thoughts?

Phil

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Stefan Monnier]

> We could also add specific support for removing the last undo, which you
> could do iff point is immediately after $@.

I don't understand enough of Markus's situation to have
a definite opinion.
But I can see several options:
1- Stop worrying about the occasional presence of extra undo entries and
   just let the user hit undo an extra time when that happens.
   This won't work if undo is used semantically (e.g. if a partial undo
   results in an inconsistent state).
2- Refine the timer-based undo-boundaries along the lines of what you
   had earlier: only add a timer-based boundary if the last undo chunk
   is too long (same for undo-boundaries added in non-current buffers).
   It would still occasionally add "unwanted" boundaries, but only when
   not doing so would result in too large an undo list.
3- Have Ediprolog use the same trick used in Viper where we wipe out
   intermediate boundaries after the fact.
4- Add the kind of "do it manually" option you had added earlier, such
   that Ediprolog could request that Emacs refrain from auto-adding any
   undo-boundaries in its buffer.

My favorite is 1 if applicable, then 3, then 4, then 2.


        Stefan

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Markus Triska]

Hi Phillip, Stefan,

I post a bit of information about how ediprolog.el currently works so
that the use case is clear. I am perfectly willing to make any
adjustments to ediprolog so that undo is treated more consistently. At
the same time, I am also interested in improving Emacs generally, so
please read this as clarification of, not excuse for, the current state.

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

>> The precise sequence is (see `ediprolog-query' and `ediprolog-interact'):
>>
>> 1) "\n%@ " is inserted into the buffer
>> 2) the Prolog process is started (if necessary)
>> 3) the query is sent to the process
>> 4) when process output arrives, it is appended to the buffer.
>>
>> The buffer is modified only by (1) and (4). Somtimes there is an undo
>> boundary between them, and most often there is none (which I prefer).
>
> Yes. So the issue here is that between 1) and 4) there is a
> indeterminate length of time.

Yes, this is the core issue. In simulans2.sh which I posted, you can try
different time spans between 1) and 4) easily by modifying line 15.  For
example, if I change 1.5 to 10, I always get an undo boundary after the
initial %@. If I change it to 4 or 5, I still only sometimes get one. My
estimate is thus that the timer runs about once every 10 seconds ;-)


>> That's exactly what I expect to happen. But in some cases, there
>> unexpectedly *is* an undo boundary between two subsequent insertions of
>> text into the buffer, namely between (1) and (4) above.
>
> The problem here also is what happens if the process output is long.
> Emacs will now add undo-boundaries periodically. Before, I think, the
> undo list would have got longer and longer, until emacs errored.

To clearify, the use case of ediprolog indeed comprises a single flow of
insertions, either stemming from process output or user input, while the
Prolog query is in process: Everything the Prolog process outputs is
inserted. Everything the user inputs is also inserted into the buffer
(and sent to the process). The result is a complete transcript of the
Prolog interaction just as it would normally happen on the terminal.

For this particular case, I would be perfectly OK if undo would simply
undo the whole flow of insertions up to the point where the query was
started. I do not know much about the undo representation or your plans
about it, but what I think would work in this particular case is to
record it as "Emacs, to undo this, you need to remove all text between X
and Y", and Y simply increases as more text is inserted into the buffer.
This would keep the length of the undo list constant throughout the
whole interaction with the subprocess, at least in this concrete
case. Can I do this in ediprolog myself? If so, then I will do it.

> Yes. Or rather you could insert %@ immediately before getting data back
> from prolog, perhaps with a process filter. Then both of your insertions
> would be happening together.

I am already using a process filter: If the process outputs \n, then I
insert \n%@ into the buffer. Only for the very first %@ that I insert
directly after the query is started, I am currently doing it outside the
process filter. It is important to insert %@ immediately to give an
indication that the query has been sent to the process (which may take a
long time to produce output). Note that % starts a Prolog line comment,
so the process output that gets embedded in buffers does not interfere
with the Prolog program itself. For this reason, text properties would
not be a good fit for this use case: Users often want to store the query
results together with the source code, and using %@ allows that.

> Also, what happens if point moves between 1) and 4)?

In the typical interaction mode, point cannot move between 1) and 4):
Each key press is sent to the Prolog process, so that you can interact
with Prolog like in a terminal. However, it is indeed possible to escape
this loop with C-g and indeed move point and even insert text into the
buffer while the process still runs and may produce output that is also
inserted into the buffer by the process filter.

> Stefan, any thoughts?

As to the options outlined by Stefan:

   1- just let the user hit undo an extra time when that happens.

That is the current state that I consider a bit unsatisfying, so I would
like to choose between 2, 3 and 4:

   2- Refine the timer-based undo-boundaries along the lines of what you
      had earlier: only add a timer-based boundary if the last undo chunk
      is too long (same for undo-boundaries added in non-current buffers).
      It would still occasionally add "unwanted" boundaries, but only when
      not doing so would result in too large an undo list.

I think we can rule this out as a satisfying solution.

   3- Have Ediprolog use the same trick used in Viper where we wipe out
      intermediate boundaries after the fact.

This sounds like a great option. Could you please tell me a bit more?
The specific use case for me is: I have a process filter that
continuously (upon process output or user input) inserts text into the
buffer. I just want *all* this text gone when pressing C-/. If possible,
I hope that this can be done without the undo list growing too long.

   4- Add the kind of "do it manually" option you had added earlier, such
      that Ediprolog could request that Emacs refrain from auto-adding any
      undo-boundaries in its buffer.

To me, this sounds at least as great as (3) and maybe could be used by
both Viper and ediprolog if it were available?

Please note in this case that, as explained above, the output normally
is inserted continuously in a loop, but it is possible to break out of
the loop with C-g and edit text elsewhere in the buffer, and in that
case I still would like the normal undo behaviour for user
input. Ideally, only the output that stems from the interaction with the
Prolog process and user input during the interaction should be treated
as a single unit. This separation (i.e., treat insertions as a unit in
some parts of a buffer while applying the normal undo behaviour in other
parts) may of course be very hard to achieve, and I would already be
very satisfied if there is a way to make it work in the regular case. I
only mention for completeness that conceptually, I regard the text that
stems from the Prolog interaction different from editing operations.

Please try ediprolog if possible (any SWI version will do) and try for
example the query (M-x ediprolog-dwim RET with point on the line):

?- length(Ls, L).

You then interact with SWI-Prolog just as on a terminal. C-g will put
you out of the loop and let you edit other text in the buffer.
M-x ediprolog-toplevel RET puts you back to the toplevel.

You can stop the query at any time by pressing "." or "a". Ideally,
pressing C-/ after stopping the query undoes the whole interaction.

I hope that this clearifies the use case a bit.

Thank you for looking into this!

All the best,
Markus

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Stefan Monnier]

> with the Prolog program itself. For this reason, text properties would
> not be a good fit for this use case: Users often want to store the query
> results together with the source code, and using %@ allows that.

I think he meant to use text-properties (actually, I think it would
probably be an overlay) to *display* the special initial "%@" (which you
currently insert manually "to give an indication that the query has been
sent to the process") without actually inserting it in the buffer
(e.g. with an after-string or some such).

>    1- just let the user hit undo an extra time when that happens.
> That is the current state that I consider a bit unsatisfying, so I would
> like to choose between 2, 3 and 4.

Could you explain a bit why this is unsatisfying in ediprolog's case?

In my shell buffers, I sometimes use undo and am generally happier with
undo steps that are too fine grained than with undo steps which are too
coarse because it's easier to just repeat C-/ than to try and recreate
a missing intermediate point in the middle of an undo step.  So I'm
wondering to what extent your unsatisfaction is specific to ediprolog's
interaction or whether it's more a matter of user preference.
IOW, maybe we should we aim to provide that behavior not just in
ediprolog.el but also in comint, based on some user config.

>    3- Have Ediprolog use the same trick used in Viper where we wipe out
>       intermediate boundaries after the fact.
> This sounds like a great option. Could you please tell me a bit more?

In Viper, when you do an insertion (e.g. via the "i" key) here's what
happens: while performing the insertion, the undo list is handled
normally, with boundaries inserted automatically so that you can undo
parts of what you're in the process of inserting; but when you finally
exit with ESC Viper goes back through the buffer-undo-list and throws
away all the boundaries that were added during the insertion, so that an
undo gets rid of the whole insertion (i.e. all the steps that made up
the insertion end up amalgamated into a single undo step).

> The specific use case for me is: I have a process filter that
> continuously (upon process output or user input) inserts text into the
> buffer. I just want *all* this text gone when pressing C-/. If possible,
> I hope that this can be done without the undo list growing too long.

Using Viper's approach, you could for example throw away the
undo-boundaries added since the last process-send-string.  I'd expect
you'd do it right when you see Prolog's prompt.  So if you undo while
Prolog is still in the middle of responding you might only undo parts of
the current response (like now), but once a response is complete it will
always be undone as an indivisible step.

If the "undo parts of the current response" behavior doesn't suit you,
you could try to be more aggressive about getting rid of undo boundaries
and (for example) remove them whenever new data is received from the process.

>    4- Add the kind of "do it manually" option you had added earlier, such
>       that Ediprolog could request that Emacs refrain from auto-adding any
>       undo-boundaries in its buffer.
>
> To me, this sounds at least as great as (3) and maybe could be used by
> both Viper and ediprolog if it were available?

That's what we first used for Viper, but it turned out undesirable,
because it prevents the user from undoing fine grained steps of the
current insertion.

> Ideally, only the output that stems from the interaction with the
> Prolog process and user input during the interaction should be treated
> as a single unit. This separation (i.e., treat insertions as a unit in
> some parts of a buffer while applying the normal undo behaviour in other
> parts) may of course be very hard to achieve, and I would already be
> very satisfied if there is a way to make it work in the regular case. I
> only mention for completeness that conceptually, I regard the text that
> stems from the Prolog interaction different from editing operations.

Getting it "right" in all cases may indeed be difficult (if for no other
reason than because it's difficult to formally define what we mean by
"right" in the general case).


        Stefan

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Markus Triska]

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

>> with the Prolog program itself. For this reason, text properties would
>> not be a good fit for this use case: Users often want to store the query
>> results together with the source code, and using %@ allows that.
>
> I think he meant to use text-properties (actually, I think it would
> probably be an overlay) to *display* the special initial "%@" (which you
> currently insert manually "to give an indication that the query has been
> sent to the process") without actually inserting it in the buffer
> (e.g. with an after-string or some such).

Yes, I also understood it this way. Just to clearify: Eventually, there
actually must be a %@ as real text in the buffer, because otherwise what
the Prolog process emits will make the buffer no longer a valid Prolog
program. So, what the Prolog process emits must not only *appear* as
"commented out" in the buffer, but must actually *be* commented out.

A text property may get us over the initial %@ until process output
arrives, but at that point we will have to actually insert %@ into the
buffer, so that the query result does not interfere with the program
when users save it, or simply reconsult the buffer plus answer.

This is certainly doable, but not sufficient. An interaction with the
Prolog process can span several answers and even involve user input, and
I want all this to be removed again on undo. Please see below for more.


>>    1- just let the user hit undo an extra time when that happens.
>> That is the current state that I consider a bit unsatisfying, so I would
>> like to choose between 2, 3 and 4.
>
> Could you explain a bit why this is unsatisfying in ediprolog's case?

One important reason for this is the way that ediprolog is frequently
used: Typically, you use ediprolog to evaluate small self-contained test
cases and important queries directly within your buffer. Almost always
within a Prolog file, but of course also when composing e-mails to
discuss important Prolog features, bug reports, answer questions etc.

One very typical example: A user sends me a bug report together with
sample code, sample query and its result. Using ediprolog, I can -
directly from within the mail buffer - consult the code and evaluate the
query, and quickly see whether I can reproduce the problem. I leave this
evaluation in the buffer, can work on a fix in the library code,
reconsult the program, re-run the query, and extremely easily compare
the results because the previous result is also still in the buffer and
in fact placed directly on the line below the current one.

As such, I often use ediprolog to try out small self-contained test
cases while I am still developing a feature or working on a fix, to
track the current behaviour. At the same time, the test itself is often
also under development, so I cannot yet solidify this as an automated
unit test. Therefore, what it comes down to is often:

   *) work, work on fix
   *) peek, peek at result
   *) work, work on fix
   *) ...

And for such smaller peeks at the current results, I frequently need to
undo the whole interaction, and it is valuable during develpoment that
undo is extremely consistent in what it does. It is irritating to
always use C-/ for undo, except for 1 out of 10 times in such cases.


> In my shell buffers, I sometimes use undo and am generally happier with
> undo steps that are too fine grained than with undo steps which are too
> coarse because it's easier to just repeat C-/ than to try and recreate
> a missing intermediate point in the middle of an undo step.

I understand this. In the case of ediprolog, the good news is that the
whole interaction is *always* logged in a completely different buffer
too: *ediprolog-history* shows precisely what was sent to the Prolog
process, and what was emitted by the process, and users can always look
at that buffer if there is any doubt in retrospect about what was done.
Therefore, I would rather provide a consistent way for users to undo the
whole interaction at once. It can always be lookep up later if needed.

> IOW, maybe we should we aim to provide that behavior not just in
> ediprolog.el but also in comint, based on some user config.

Yes! Thank you for taking this general perspective. I must say at this
point that I have no experience with how such a "transactional" undo
would actually work out in practice. I can imagine that it is a bit
unusual at first. For ediprolog though, I am quite certain that I would
prefer it because, as explained above, the interactions are typically
short, and breaking up a short interaction into two or more parts only 1
out of N times is definitely worse than always treating it as a unit.

For comint and other modes, I can imagine that it could also be useful.


> In Viper, when you do an insertion ...

Thank you for the explanation!

> Using Viper's approach, you could for example throw away the
> undo-boundaries added since the last process-send-string.  I'd expect
> you'd do it right when you see Prolog's prompt.  So if you undo while
> Prolog is still in the middle of responding you might only undo parts of
> the current response (like now), but once a response is complete it will
> always be undone as an indivisible step.

This sounds like a great approach, thank you! I will try this.

All the best!
Markus

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Stefan Monnier]

> Yes, I also understood it this way. Just to clearify: Eventually, there
> actually must be a %@ as real text in the buffer, because otherwise what
> the Prolog process emits will make the buffer no longer a valid Prolog
> program. So, what the Prolog process emits must not only *appear* as
> "commented out" in the buffer, but must actually *be* commented out.

Yes, but the real "%@" will only be inserted into the buffer when actual
process output comes along (which you already do anyway, IIUC).

> A text property may get us over the initial %@ until process output
> arrives, but at that point we will have to actually insert %@ into the
> buffer, so that the query result does not interfere with the program
> when users save it, or simply reconsult the buffer plus answer.

That's right.

> This is certainly doable, but not sufficient. An interaction with the
> Prolog process can span several answers and even involve user input, and
> I want all this to be removed again on undo. Please see below for more.

Hmm... So are you saying that you want a single undo step to undo
something that corresponds to "some process output plus some user
input"?  This seems incompatible with the desire you expressed earlier
to insert undo-boundaries whenever the user actually inserts
something in the buffer (or did I misunderstand it?):

    Please note in this case that, as explained above, the output
    normally is inserted continuously in a loop, but it is possible to
    break out of the loop with C-g and edit text elsewhere in the
    buffer, and in that case I still would like the normal undo
    behaviour for user input.

Tho I guess you might be able to distinguish the two cases based on the C-g.
Hmm... probably doable, but sounds delicate.

> And for such smaller peeks at the current results, I frequently need to
> undo the whole interaction, and it is valuable during develpoment that
> undo is extremely consistent in what it does. It is irritating to
> always use C-/ for undo, except for 1 out of 10 times in such cases.

So it's not absolutely indispensable, but it's indeed more annoying than
in the "usual" comint case because undo is used more often and even
repeatedly, so predictability is more important than usual.
I see, makes sense.

>> Using Viper's approach, you could for example throw away the
>> undo-boundaries added since the last process-send-string.  I'd expect
>> you'd do it right when you see Prolog's prompt.  So if you undo while
>> Prolog is still in the middle of responding you might only undo parts of
>> the current response (like now), but once a response is complete it will
>> always be undone as an indivisible step.
> This sounds like a great approach, thank you! I will try this.

In case you haven't found it yet, "grep undo-boundary
lisp/emulation/viper*.el" gets you straight to the relevant code (in
emacs-25 or more recent).  Maybe we should add some support in some
"core" file (subr.el or something) for that operation.


        Stefan

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Markus Triska]

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

>> This is certainly doable, but not sufficient. An interaction with the
>> Prolog process can span several answers and even involve user input, and
>> I want all this to be removed again on undo. Please see below for more.
>
> Hmm... So are you saying that you want a single undo step to undo
> something that corresponds to "some process output plus some user
> input"?  This seems incompatible with the desire you expressed earlier
> to insert undo-boundaries whenever the user actually inserts
> something in the buffer (or did I misunderstand it?):

The interaction with Prolog (after the query is sent) consists of:

   a) output of the Prolog process
   b) user input to the Prolog process ("next answer", "exit" etc.)

*Both* lead to insertion of text in the buffer, and in this sense should
be treated completely alike: It all happens in the context of
"interaction with the Prolog process". When this interaction is over
(typically, no more answers or user pressed ".") and undo is desired,
*everything* that happened during this interaction should be undone at
once. The issue I reported was always that *too many* undo boundaries
were inserted in some cases, never too few. I want fewer boundaries, so
that I can always undo the entire interaction (output + input) at once.

> In case you haven't found it yet, "grep undo-boundary
> lisp/emulation/viper*.el" gets you straight to the relevant code (in
> emacs-25 or more recent).

Thank you, I have been looking at it already.

As far as I understand the code, this keeps track, via a special element
that normally does not occur in the undo list, of where the boundary
should be. Later, it removes all undo boundaries up to and including
that marker. I have a quite uneasy feeling about this: Can there be a
situation where the marker element unexpectedly remains in the undo
list? From a quick glance, maybe unwind-protect is what would be needed
in ediprolog to reliably remove such a marker also if C-g is pressed.

Other than that, I still do not understand why the timer is now
implemented in Emacs to insert undo boundaries during process output. I
have looked at the way the buffer-undo-list is stored, and it seems to
me that a continuous stream of process output that is inserted in the
buffer can be stored in the undo list quite efficiently, and that
inserting undo boundaries only further increases the length. So, how do
undo boundaries that are inserted by timers help to improve efficiency?

> Maybe we should add some support in some "core" file (subr.el or
> something) for that operation.

That would be awesome! Like "undo-begin-transaction", which could
prepare an atomic undo operation over many subsequent operations, and
"undo-end-transaction", which would remove all undo boundaries in it?

All the best!
Markus

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Stefan Monnier]

> As far as I understand the code, this keeps track, via a special element
> that normally does not occur in the undo list, of where the boundary
> should be.  Later, it removes all undo boundaries up to and including
> that marker.  I have a quite uneasy feeling about this: Can there be a
> situation where the marker element unexpectedly remains in the undo
> list?

Yes, Viper's implementation is not 100% satisfactory.
It could be improved in 2 ways:
- Don't add anything to the undo list; instead keep a reference to the
  original top of the list.
- Do add something, but make sure it's a valid element (e.g. a dummy
  "empty insertion" record).

There are also some issues that deserve care:
- The user may decide to undo past this special record, so it won't be
  found in buffer-undo-list any more.
- The buffer-undo-list may grow large enough that when Emacs's GC comes
  around to cut off its tail, it ends up throwing away the special record.

So the record may not be present in the end any more, and there can be
2 very different reasons for that (in one case what should be done is
to remove all undo-boundaries, and in the other to remove none).

> From a quick glance, maybe unwind-protect is what would be needed
> in ediprolog to reliably remove such a marker also if C-g is pressed.

Yes, if you have proper nesting, then unwind-protect should work well
and reliably.

> So, how do undo boundaries that are inserted by timers help to
> improve efficiency?

The undo logs may grow arbitrarily large, so Emacs's GC always
automatically trims the end of those logs (according to undo-*limit) to
make sure we don't end up using too much memory.  This trimming can only
be done at undo-boundaries, so in the absence of undo-boundaries, the
auto-trimming can't do it job.

>> Maybe we should add some support in some "core" file (subr.el or
>> something) for that operation.
> That would be awesome! Like "undo-begin-transaction", which could
> prepare an atomic undo operation over many subsequent operations, and
> "undo-end-transaction", which would remove all undo boundaries in it?

Yup.  It would seem to fit nicely next to atomic-change-group.


        Stefan

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Markus Triska]

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


>     Please note in this case that, as explained above, the output
>     normally is inserted continuously in a loop, but it is possible to
>     break out of the loop with C-g and edit text elsewhere in the
>     buffer, and in that case I still would like the normal undo
>     behaviour for user input.

To clearify this paragraph: Suppose the Prolog query is in progress and
takes a long time. The user presses C-g to quit waiting for process
output (the process continues of course) and continues to edit unrelated
parts of the program, or writes new queries etc. *These* edits are to be
regarded as completely normal edits, with everything as usual.

Only everything that happens in the context of "interaction with the
Prolog process" should always be undone as a single unit, without any
undo boundaries, even if user input happened during the interaction (as
it almast always does, because we ask for the next answer etc.).

However, user pressing C-g to temporarily get out of the Prolog
interaction (you can resume with M-x ediprolog-toplevel RET) is
comparatively unusual: Most ediprolog queries are small self-contained
test cases that quickly yield the desired answer, at least from my
experience. So I do not care that much what happens after C-g: For
example, if the undo "transaction" ends upon C-g, and normal undo
behaviour ensues for the rest of the interaction, that's very OK!

All the best,
Markus

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Phillip Lord]

Markus Triska <triska@metalevel.at> writes:

>> IOW, maybe we should we aim to provide that behavior not just in
>> ediprolog.el but also in comint, based on some user config.
>
> Yes! Thank you for taking this general perspective. I must say at this
> point that I have no experience with how such a "transactional" undo
> would actually work out in practice. I can imagine that it is a bit
> unusual at first. For ediprolog though, I am quite certain that I would
> prefer it because, as explained above, the interactions are typically
> short, and breaking up a short interaction into two or more parts only 1
> out of N times is definitely worse than always treating it as a unit.

Emacs undo is already transactional in the sense that it inserts undo's
between commands. If Emacs is doing stuff, running functions, and so
forth, it will not add an undo boundary unless you tell it to. This is
even true for the timer based boundaries, IIRC. Timers can only run in
one point of the command loop.


>> Using Viper's approach, you could for example throw away the
>> undo-boundaries added since the last process-send-string.  I'd expect
>> you'd do it right when you see Prolog's prompt.  So if you undo while
>> Prolog is still in the middle of responding you might only undo parts of
>> the current response (like now), but once a response is complete it will
>> always be undone as an indivisible step.
>
> This sounds like a great approach, thank you! I will try this.

So, I had a quick play, using this code (hey, my first piece of prolog
in a decade, and I copied it!).

rosetta_sleep(Time) :-
	writeln('Sleeping...'),
	sleep(Time),
	writeln('Awake!').

%?- rosetta_sleep(10).
%@ Sleeping...
%@ Awake!
%@ true.


This generalises the problem -- in this case, you are pretty much
guaranteed to get an undo-boundary between "Sleeping..." and "Awake!"
which you presumably want together.

You can actually get this behaviour -- this patch achieves it.


@@ -307,21 +307,24 @@
 for `ediprolog-consult' with a new process. With other prefix
 arguments, equivalent to `ediprolog-remove-interactions'."
   (interactive "P")
-  (cond ((eq arg 0)
-         (unless (ediprolog-running)
-           (error "No Prolog process running"))
-         (ediprolog-kill-prolog)
-         (message "Prolog process killed."))
-        ((eq arg 1) (ediprolog-consult))
-        ((eq arg 2) (ediprolog-consult t))
-        ((eq arg 7)
-         (unless (ediprolog-more-solutions)
-           (error "No query in progress"))
-         (ediprolog-toplevel))
-        ((equal arg '(4)) (ediprolog-consult) (ediprolog-query))
-        ((equal arg '(16)) (ediprolog-consult t) (ediprolog-query))
-        ((null arg) (unless (ediprolog-query) (ediprolog-consult)))
-        (t (ediprolog-remove-interactions))))
+  (cancel-timer undo-auto-current-boundary-timer)
+  (setq undo-auto-current-boundary-timer nil)
+  (let ((undo-auto-current-boundary-timer t))
+    (cond ((eq arg 0)
+           (unless (ediprolog-running)
+             (error "No Prolog process running"))
+           (ediprolog-kill-prolog)
+           (message "Prolog process killed."))
+          ((eq arg 1) (ediprolog-consult))
+          ((eq arg 2) (ediprolog-consult t))
+          ((eq arg 7)
+           (unless (ediprolog-more-solutions)
+             (error "No query in progress"))
+           (ediprolog-toplevel))
+          ((equal arg '(4)) (ediprolog-consult) (ediprolog-query))
+          ((equal arg '(16)) (ediprolog-consult t) (ediprolog-query))
+          ((null arg) (unless (ediprolog-query) (ediprolog-consult)))
+          (t (ediprolog-remove-interactions)))))
 
 (defun ediprolog-process-ready ()
   "Error if the previous query is still in progress."


Of course, this is pretty clunky and has global effect for the duration
of the let binding. Also easy to get wrong (as I did first time I tried
it).

But, if this is the behaviour you want, I think it can be added. I'll
just add a new buffer-local variable to disable the effect of the timer
(rather than the timer itself, as I have done here).

Phil

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Markus Triska]

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

> Emacs undo is already transactional in the sense that it inserts undo's
> between commands. If Emacs is doing stuff, running functions, and so
> forth, it will not add an undo boundary unless you tell it to. This is
> even true for the timer based boundaries, IIRC. Timers can only run in
> one point of the command loop.

In the case I posted, these atomic operations are too finely grained for
undo to revert all of them at once. I would like to state a transaction
over all commands that happen during the interaction with the process.

> This generalises the problem -- in this case, you are pretty much
> guaranteed to get an undo-boundary between "Sleeping..." and "Awake!"
> which you presumably want together.

Yes, exactly! These should be together.

> You can actually get this behaviour -- this patch achieves it.

Thank you for looking into this!

> Of course, this is pretty clunky and has global effect for the duration
> of the let binding. Also easy to get wrong (as I did first time I tried
> it).

I suppose in the first try, you forgot to cancel the scheduled timer in
addition to disabling its further invocation? As you mention, one
drawback of this is the global effect. And there's also another
drawback, which your example does not show: Please note that user input
can also happen during the interaction, for example, please try:

?- read(T).

and when asked, enter "test.":

?- read(T).
%@ |: test.
%@ 
%@ T = test.

Again, I want the whole interaction to be undone when pressing C-/, not
just up to the point the user was queried, i.e., after "|: ".

> But, if this is the behaviour you want, I think it can be added. I'll
> just add a new buffer-local variable to disable the effect of the timer
> (rather than the timer itself, as I have done here).

That's not sufficient to implement transactions in the way I need
them. I hope the example above shows why: I really need them to span all
buffer operations between two well defined points in time, not just text
that is inserted by process filters.

Thank you and all the best!
Markus

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Phillip Lord]

Markus Triska <triska@metalevel.at> writes:
>> You can actually get this behaviour -- this patch achieves it.
>
> Thank you for looking into this!
>
>> Of course, this is pretty clunky and has global effect for the duration
>> of the let binding. Also easy to get wrong (as I did first time I tried
>> it).
>
> I suppose in the first try, you forgot to cancel the scheduled timer in
> addition to disabling its further invocation?

No. I cancelled the timer but forgot to remove it from the variable it's
stored in. Since the timer (even once cancelled) is non-nil, it's never
rescheduled.

My inclination is make the timer private to be honest.


> As you mention, one drawback of this is the global effect. And there's
> also another drawback, which your example does not show: Please note
> that user input can also happen during the interaction, for example,
> please try:
>
> ?- read(T).
>
> and when asked, enter "test.":
>
> ?- read(T).
> %@ |: test.
> %@ 
> %@ T = test.
>
> Again, I want the whole interaction to be undone when pressing C-/, not
> just up to the point the user was queried, i.e., after "|: ".
>
>> But, if this is the behaviour you want, I think it can be added. I'll
>> just add a new buffer-local variable to disable the effect of the timer
>> (rather than the timer itself, as I have done here).
>
> That's not sufficient to implement transactions in the way I need
> them. I hope the example above shows why: I really need them to span all
> buffer operations between two well defined points in time, not just text
> that is inserted by process filters.

It is sufficient though, to restore the old behaviour and fix the
regression you have, which is the point of this bug!

We can think about doing something better (and which would work for
viper also -- as Stefan says, the current solution has some issues). But
I am not sure what this would look like at the moment.

Phil

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Markus Triska]

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

> It is sufficient though, to restore the old behaviour and fix the
> regression you have, which is the point of this bug!

> We can think about doing something better (and which would work for
> viper also -- as Stefan says, the current solution has some issues). But
> I am not sure what this would look like at the moment.

Please let us use this opportunity to fix the more general case
too. Stefan agreed that the following primitives would work:

    -) undo-begin-transaction
       Starts a new transaction.
    -) undo-end-transaction
       Ends the most recently started undo transaction.

The effects of all commands between would be undone as a single unit.

Introducing new variables that only change how the process timer works
would be made completely obsolete by such a solution: It would simply
fall out as one special case of such transactions, benefit ediprolog and
(very likely) Viper, and also other programs. Personally, I would have
no use case for only changing the process timer if such transactions
were available, since this is only one special case for me, even though
it is the one that is described in the very first post of this report.

I hope this approach or an equivalent one is possible in Emacs, and I
would be very grateful if you could look into this.

Thank you and all the best!
Markus

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Phillip Lord]

Markus Triska <triska@metalevel.at> writes:

> phillip.lord@russet.org.uk (Phillip Lord) writes:
>
>> It is sufficient though, to restore the old behaviour and fix the
>> regression you have, which is the point of this bug!
>
>> We can think about doing something better (and which would work for
>> viper also -- as Stefan says, the current solution has some issues). But
>> I am not sure what this would look like at the moment.
>
> Please let us use this opportunity to fix the more general case
> too. Stefan agreed that the following primitives would work:
>
>     -) undo-begin-transaction
>        Starts a new transaction.
>     -) undo-end-transaction
>        Ends the most recently started undo transaction.
>
> The effects of all commands between would be undone as a single unit.

Yep, but Stefan did say how to implement this! We could do the same
thing as viper, which works, but has the issues he pointed out. Off
hand, I do not see an easy solution.

It's also worth saying that that viper has two very well defined
modes -- insert and command -- which are core to its functionality. It
begins the transaction when it enters insert mode, then ends the
transaction when it leaves. I'm not seeing this with ediprolog. How are
you going to be sure that each "begin" is paired with an "end"?


> Introducing new variables that only change how the process timer works
> would be made completely obsolete by such a solution: It would simply
> fall out as one special case of such transactions, benefit ediprolog and
> (very likely) Viper, and also other programs. Personally, I would have
> no use case for only changing the process timer if such transactions
> were available, since this is only one special case for me, even though
> it is the one that is described in the very first post of this report.

My solution is simple and easy and fits within a let binding. The
solution you are looking for solves the much more complex case where you
want undo to behave one way up to a point in time, and then amalgamate
several undos post hoc.

> I hope this approach or an equivalent one is possible in Emacs, and I
> would be very grateful if you could look into this.

At the moment, I am interested in fixing the regressions that I've
caused by my changes to the undo system which I've nearly done -- you
have a workaround at least, and I'll put something easier onto master.

New undo functionality is not high on my list at the moment, am afraid.
Happy to provide feedback if you want to add this for yourself, though.

Phil

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Stefan Monnier]

> Yep, but Stefan did say how to implement this! We could do the same
> thing as viper, which works, but has the issues he pointed out. Off
> hand, I do not see an easy solution.

I think Viper's approach isn't fundamentally wrong.  The use of an
invalid element is a problem, but that shouldn't be hard to fix.

> At the moment, I am interested in fixing the regressions that I've
> caused by my changes to the undo system which I've nearly done -- you
> have a workaround at least, and I'll put something easier onto master.

I think what you consider as a regression is not worth fixing.
It just made an earlier problem slightly more visible.  Let's focus on
fixing the more general problem.

I'll see when I find time to devote to undo-begin/start-transaction.


        Stefan

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Stefan Monnier]

> Please let us use this opportunity to fix the more general case
> too. Stefan agreed that the following primitives would work:

>     -) undo-begin-transaction
>        Starts a new transaction.
>     -) undo-end-transaction
>        Ends the most recently started undo transaction.

> The effects of all commands between would be undone as a single unit.

How 'bout the patch below (see the viper part to get an idea about how
to use it in ediprolog)?


        Stefan


diff --git a/lisp/simple.el b/lisp/simple.el
index f114235..e54d4f2 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -3054,6 +3054,41 @@ undo-auto--undoable-change
   (undo-auto--boundary-ensure-timer))
 ;; End auto-boundary section
 
+(defun undo-amalgamate-change-group (handle)
+  "Amalgamate changes in change-group since HANDLE.
+Remove all undo boundaries between the state of HANDLE and now.
+HANDLE is as returned by `prepare-change-group'."
+  (dolist (elt handle)
+    (with-current-buffer (car elt)
+      (setq elt (cdr elt))
+      (when (consp buffer-undo-list)
+        (let ((old-car (car-safe elt))
+              (old-cdr (cdr-safe elt)))
+          (unwind-protect
+              (progn
+                ;; Temporarily truncate the undo log at ELT.
+                (when (consp elt)
+                  (setcar elt t) (setcdr elt nil))
+                (when
+                    (or (null elt)        ;The undo-log was empty.
+                        ;; `elt' is still in the log: normal case.
+                        (eq elt (last buffer-undo-list))
+                        ;; `elt' is not in the log any more, but that's because
+                        ;; the log is "all new", so we should remove all
+                        ;; boundaries from it.
+                        (not (eq (last buffer-undo-list) (last old-cdr))))
+                  (cl-callf (lambda (x) (delq nil x))
+                      (if (car buffer-undo-list)
+                          buffer-undo-list
+                        ;; Preserve the undo-boundaries at either ends of the
+                        ;; change-groups.
+                        (cdr buffer-undo-list)))))
+            ;; Reset the modified cons cell ELT to its original content.
+            (when (consp elt)
+              (setcar elt old-car)
+              (setcdr elt old-cdr))))))))
+
+
 (defcustom undo-ask-before-discard nil
   "If non-nil ask about discarding undo info for the current command.
 Normally, Emacs discards the undo info for the current command if
diff --git a/lisp/emulation/viper-cmd.el b/lisp/emulation/viper-cmd.el
index 3d9d1cc..3ce1b4d 100644
--- a/lisp/emulation/viper-cmd.el
+++ b/lisp/emulation/viper-cmd.el
@@ -1709,40 +1709,20 @@ viper-undo-more
 ;; The following two functions are used to set up undo properly.
 ;; In VI, unlike Emacs, if you open a line, say, and add a bunch of lines,
 ;; they are undone all at once.
-(defun viper-adjust-undo ()
-  (if viper-undo-needs-adjustment
-      (let ((inhibit-quit t)
-	    tmp tmp2)
-	(setq viper-undo-needs-adjustment nil)
-	(when (listp buffer-undo-list)
-          (let ((had-boundary (null (car buffer-undo-list))))
-            (if (setq tmp (memq viper-buffer-undo-list-mark buffer-undo-list))
-		(progn
-		  (setq tmp2 (cdr tmp)) ; the part after mark
-
-		  ;; cut tail from buffer-undo-list temporarily by direct
-		  ;; manipulation with pointers in buffer-undo-list
-		  (setcdr tmp nil)
-
-		  (setq buffer-undo-list (delq nil buffer-undo-list))
-		  (setq buffer-undo-list
-			(delq viper-buffer-undo-list-mark buffer-undo-list))
-		  ;; restore tail of buffer-undo-list
-		  (setq buffer-undo-list (nconc buffer-undo-list tmp2)))
-	      (setq buffer-undo-list (delq nil buffer-undo-list)))
-            ;; The top-level loop only adds boundaries if there has been
-            ;; modifications in the buffer, so make sure we don't accidentally
-            ;; drop the "final" boundary (bug#22295).
-	    (if had-boundary (undo-boundary)))))))
+(viper-deflocalvar viper--undo-change-group-handle nil)
+(put 'viper--undo-change-group-handle 'permanent-local t)
 
+(defun viper-adjust-undo ()
+  (when viper--undo-change-group-handle
+    (undo-amalgamate-change-group
+     (prog1 viper--undo-change-group-handle
+       (setq viper--undo-change-group-handle nil)))))
 
 (defun viper-set-complex-command-for-undo ()
-  (if (listp buffer-undo-list)
-      (if (not viper-undo-needs-adjustment)
-	  (let ((inhibit-quit t))
-	    (setq buffer-undo-list
-		  (cons viper-buffer-undo-list-mark buffer-undo-list))
-	    (setq viper-undo-needs-adjustment t)))))
+  (and (listp buffer-undo-list)
+       (not viper--undo-change-group-handle)
+       (setq viper--undo-change-group-handle
+             (prepare-change-group))))
 
 
 ;;; Viper's destructive Command ring utilities
diff --git a/lisp/emulation/viper-init.el b/lisp/emulation/viper-init.el
index 086daf2..838d846 100644
--- a/lisp/emulation/viper-init.el
+++ b/lisp/emulation/viper-init.el
@@ -369,15 +369,6 @@ viper-set-input-method
 
 ;; VI-style Undo
 
-;; Used to 'undo' complex commands, such as replace and insert commands.
-(viper-deflocalvar viper-undo-needs-adjustment nil)
-(put 'viper-undo-needs-adjustment 'permanent-local t)
-
-;; A mark that Viper puts on buffer-undo-list.  Marks the beginning of a
-;; complex command that must be undone atomically.  If inserted, it is
-;; erased by viper-change-state-to-vi and viper-repeat.
-(defconst viper-buffer-undo-list-mark 'viper)
-
 (defcustom viper-keep-point-on-undo nil
   "Non-nil means not to move point while undoing commands.
 This style is different from Emacs and Vi.  Try it to see if

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Markus Triska]

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

> How 'bout the patch below (see the viper part to get an idea about how
> to use it in ediprolog)?

It's pretty awesome, thank you!

I have added it to ediprolog.el with the patch below, please let me know
if you have any comments. I like particularly that your functions work
together in a pure way: It's OK if the handle is simply *not* used. At
least I *think* that is the case, i.e., prepare-change-group does not
alter the undo list or any other global state. I am particularly
interested in this case because in ediprolog, the following situation
may arise:

   1) a query is started with M-x ediprolog-dwim RET
   2) interaction with Prolog takes place
   3) critically, user presses C-g to exit the interaction (!)
   4) user makes (unrelated) changes elsewhere in the buffer
   5) user re-enters the interaction with M-x ediprolog-toplevel RET
   7) interaction resumes
   8) after a few more answers, interaction finishes
   9) C-/ (user presses undo)

To see what I mean, please try M-x ediprolog-dwim RET on the following
query, and press SPACE twice:

?- between(1, 3, X).
%@ X = 1 ;
%@ X = 2 

at this state, press C-g to exit the interaction, and (optionally) make
other changes in the buffer. Then do M-x ediprolog-toplevel RET to
resume interaction, and press SPACE once more. In total, you obtain:

?- between(1, 3, X).
%@ X = 1 ;
%@ X = 2 ;
%@ X = 3.

Ideally, if you then press C-/, the *whole* interaction would be undone,
and I may find a good way to do this. However, in its current state
(with the patch below), such interrupted and resumed interactions still
result in undo boundaries, which is also acceptable.

Thus, with the primitives you provide, the case I care most about,
namely a single continuous interaction, now works exactly as expected on
undo (it is undone as a unit on), and interruptions work acceptably
well.  Therefore, thank you very much for this!

One thing I noticed in the interaction above is that point is sometimes
at a very unexpected position after undo, in particular if the buffer
contents are modified after the query is interrupted. I will file a
separate issue for this.

And a question: Is there any reason not to write viper-adjust-undo as:

   (defun viper-adjust-undo ()
     (when viper--undo-change-group-handle
       (undo-amalgamate-change-group viper--undo-change-group-handle)
       (setq viper--undo-change-group-handle nil)))

The patch to ediprolog.el follows.

Thank you and all the best!
Markus

diff --git a/ediprolog.el b/ediprolog.el
index ee89095..408284b 100644
--- a/ediprolog.el
+++ b/ediprolog.el
@@ -347,11 +347,13 @@ arguments, equivalent to `ediprolog-remove-interactions'."
                  (error "Missing `.' at the end of this query")))
            (query (buffer-substring-no-properties from to)))
       (end-of-line)
-      (insert "\n" ediprolog-indent-prefix ediprolog-prefix)
-      (ediprolog-interact
-       (format "%s\n" (mapconcat #'identity
-                                 ;; `%' can precede each query line
-                                 (split-string query "\n[ \t%]*") " "))))
+      (let ((handle (prepare-change-group)))
+        (insert "\n" ediprolog-indent-prefix ediprolog-prefix)
+        (ediprolog-interact
+         (format "%s\n" (mapconcat #'identity
+                                   ;; `%' can precede each query line
+                                   (split-string query "\n[ \t%]*") " ")))
+        (undo-amalgamate-change-group handle)))
     t))
 
 ;;;###autoload

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Stefan Monnier]

> together in a pure way: It's OK if the handle is simply *not* used.

Indeed, it's perfectly OK.

> And a question: Is there any reason not to write viper-adjust-undo as:

>    (defun viper-adjust-undo ()
>      (when viper--undo-change-group-handle
>        (undo-amalgamate-change-group viper--undo-change-group-handle)
>        (setq viper--undo-change-group-handle nil)))

No really strong reason, no, but it just seems safer to set
viper--undo-change-group-handle to nil before we start with
undo-amalgamate-change-group, in case it signals an error.


        Stefan

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Stefan Monnier]

> How 'bout the patch below (see the viper part to get an idea about how
> to use it in ediprolog)?

Thanks, pushed to master (described as 25.2 but probably really only to
be released as 26.1).


        Stefan

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
> How 'bout the patch below (see the viper part to get an idea about how
> to use it in ediprolog)?

Looks good. Could do with a test or two! Changing the undo system was
made harder the absence of tests I think.

> +(viper-deflocalvar viper--undo-change-group-handle nil)
> +(put 'viper--undo-change-group-handle 'permanent-local t)

Why the permanent-local? This will remove undo boundaries even over an
major-mode change surely?

Phil

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Stefan Monnier]

>> +(viper-deflocalvar viper--undo-change-group-handle nil)
>> +(put 'viper--undo-change-group-handle 'permanent-local t)
> Why the permanent-local?

IIRC this just reproduces the previous behavior.

> This will remove undo boundaries even over an major-mode
> change surely?

Right, and I think it's done on purpose.


        Stefan

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Lars Ingebrigtsen]

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

> How 'bout the patch below (see the viper part to get an idea about how
> to use it in ediprolog)?

[...]

> +(defun undo-amalgamate-change-group (handle)

If I'm skimming this thread correctly, this addition fixed the reported
bug, so I'm closing this bug report.  If there's more to be done here,
please respond to the debbugs address, and we'll reopen.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Phillip Lord]

Markus Triska <triska@metalevel.at> writes:
> Yes, this is the core issue. In simulans2.sh which I posted, you can try
> different time spans between 1) and 4) easily by modifying line 15.  For
> example, if I change 1.5 to 10, I always get an undo boundary after the
> initial %@. If I change it to 4 or 5, I still only sometimes get one. My
> estimate is thus that the timer runs about once every 10 seconds ;-)

Yep, every 10 seconds.

>
>
>>> That's exactly what I expect to happen. But in some cases, there
>>> unexpectedly *is* an undo boundary between two subsequent insertions of
>>> text into the buffer, namely between (1) and (4) above.
>>
>> The problem here also is what happens if the process output is long.
>> Emacs will now add undo-boundaries periodically. Before, I think, the
>> undo list would have got longer and longer, until emacs errored.
>
> To clearify, the use case of ediprolog indeed comprises a single flow of
> insertions, either stemming from process output or user input, while the
> Prolog query is in process: Everything the Prolog process outputs is
> inserted. Everything the user inputs is also inserted into the buffer
> (and sent to the process). The result is a complete transcript of the
> Prolog interaction just as it would normally happen on the terminal.
>
> For this particular case, I would be perfectly OK if undo would simply
> undo the whole flow of insertions up to the point where the query was
> started. I do not know much about the undo representation or your plans
> about it, but what I think would work in this particular case is to
> record it as "Emacs, to undo this, you need to remove all text between X
> and Y", and Y simply increases as more text is inserted into the buffer.
> This would keep the length of the undo list constant throughout the
> whole interaction with the subprocess, at least in this concrete
> case. Can I do this in ediprolog myself? If so, then I will do it.


And what happens if the prolog process spams and never stops? Y, gets
longer and longer. Eventually, Emacs signals an error with a "this is
probably a bug" message.



>> Yes. Or rather you could insert %@ immediately before getting data back
>> from prolog, perhaps with a process filter. Then both of your insertions
>> would be happening together.
>
> I am already using a process filter: If the process outputs \n, then I
> insert \n%@ into the buffer. Only for the very first %@ that I insert
> directly after the query is started, I am currently doing it outside the
> process filter. It is important to insert %@ immediately to give an
> indication that the query has been sent to the process (which may take a
> long time to produce output). Note that % starts a Prolog line comment,
> so the process output that gets embedded in buffers does not interfere
> with the Prolog program itself. For this reason, text properties would
> not be a good fit for this use case: Users often want to store the query
> results together with the source code, and using %@ allows that.

I think you are doing two things -- one is telling the user that
something has happened and one is storing the result. I think that this
is the problem.

My suggestion is add the %@ into the buffer when the result comes
through. If you want %@ to *appear* before this, then insert an overlay
with a display string.


>> Also, what happens if point moves between 1) and 4)?
>
> In the typical interaction mode, point cannot move between 1) and 4):
> Each key press is sent to the Prolog process, so that you can interact
> with Prolog like in a terminal.

Emacs blocks?

>    3- Have Ediprolog use the same trick used in Viper where we wipe out
>       intermediate boundaries after the fact.
>
> This sounds like a great option. Could you please tell me a bit more?
> The specific use case for me is: I have a process filter that
> continuously (upon process output or user input) inserts text into the
> buffer. I just want *all* this text gone when pressing C-/. If possible,
> I hope that this can be done without the undo list growing too long.

I don't think that this is needed here. Viper has this because it
removes undo-boundaries when it changes modes -- so it needs to add
undo-boundaries which it later takes out.



>    4- Add the kind of "do it manually" option you had added earlier, such
>       that Ediprolog could request that Emacs refrain from auto-adding any
>       undo-boundaries in its buffer.
>
> To me, this sounds at least as great as (3) and maybe could be used by
> both Viper and ediprolog if it were available?


I have no particular problem with adding this (although not for Emacs
25.1 now!). Again, the caveat is, if the process is long or continual
you MUST do something to call undo-boundary periodically, or Emacs will
error.


>
> Please note in this case that, as explained above, the output normally
> is inserted continuously in a loop, but it is possible to break out of
> the loop with C-g and edit text elsewhere in the buffer, and in that
> case I still would like the normal undo behaviour for user
> input.

Yeah, that's straight-forward -- add undo-boundary on non local exit.

My own feeling here is, still, that while ediprolog's current system is
straight-forward, it's probably not correct!

Phil

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Markus Triska]

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

>> about it, but what I think would work in this particular case is to
>> record it as "Emacs, to undo this, you need to remove all text between X
>> and Y", and Y simply increases as more text is inserted into the buffer.
>> This would keep the length of the undo list constant throughout the
>> whole interaction with the subprocess, at least in this concrete
>> case. Can I do this in ediprolog myself? If so, then I will do it.
>
>
> And what happens if the prolog process spams and never stops? Y, gets
> longer and longer. Eventually, Emacs signals an error with a "this is
> probably a bug" message.

The way I imagine it, Y does not get "longer and longer", but is simply
an integer that gets higher and higher when more output arrives. That
would be one way to merge subsequent insertions in the undo list.

> I think you are doing two things -- one is telling the user that
> something has happened and one is storing the result. I think that this
> is the problem.
>
> My suggestion is add the %@ into the buffer when the result comes
> through. If you want %@ to *appear* before this, then insert an overlay
> with a display string.

Please let us consider the more general issue of making *all*
insertions, the first _and also subsequent ones_ that happen during the
interaction with the Prolog process (and which can stem either from
process output *or* from user input) *atomic* in the sense that they are
all undone as a single unit. Please try out ediprolog to see what I
mean. In my view, the transaction concept that Stefan has proposed is a
very good solution for this and would benefit also other parts of Emacs.
It is this general issue that I would like to address and solve with a
suitable (likely: new) Emacs mechanism, not only the first %@.

>> In the typical interaction mode, point cannot move between 1) and 4):
>> Each key press is sent to the Prolog process, so that you can interact
>> with Prolog like in a terminal.
>
> Emacs blocks?

It is an interaction loop that can be left with C-g at any time. Please
try out ediprolog to see what I mean, for example on the query:

?- length(Ls, L).

> My own feeling here is, still, that while ediprolog's current system is
> straight-forward, it's probably not correct!

Please let me clearify: All that ediprolog does is inserting text into
the buffer. The text can either stem from process output, user input, or
(only in the very special case of the *first* %@ that is inserted when a
new interaction starts) even by ediprolog directly, as hardcoded string.

This seems completely normal and correct to me. Undo boundaries are
currently inserted without my control over them. I understand that this
is for efficiency reasons, to allow Emacs to trim the undo list.  I
would like to have more control over this, for example, by removing
these undo boundaries when the whole interaction is over. At that point,
we know that Emacs has not raised an error, so it should be very
safe. Several other solutions would also fit this particular case, such
as merging subsequent insertions in the undo list to keep its length
constant. These solutions should not depend on the timing of insertions.

We have found one case where I could prevent the undo boundary with
special-case code (for the initial %@), but a more general solution
would be much more welcome and work for all parts of the interaction.

All the best,
Markus

bug#23906: 25.0.95; Undo boundary after process output is not consistent

[Phillip Lord]

Markus Triska <triska@metalevel.at> writes:

> phillip.lord@russet.org.uk (Phillip Lord) writes:
>
>>> about it, but what I think would work in this particular case is to
>>> record it as "Emacs, to undo this, you need to remove all text between X
>>> and Y", and Y simply increases as more text is inserted into the buffer.
>>> This would keep the length of the undo list constant throughout the
>>> whole interaction with the subprocess, at least in this concrete
>>> case. Can I do this in ediprolog myself? If so, then I will do it.
>>
>>
>> And what happens if the prolog process spams and never stops? Y, gets
>> longer and longer. Eventually, Emacs signals an error with a "this is
>> probably a bug" message.
>
> The way I imagine it, Y does not get "longer and longer", but is simply
> an integer that gets higher and higher when more output arrives. That
> would be one way to merge subsequent insertions in the undo list.

Sorry, I didn't put that well. My point was, simply, that if the prolog
output is too long, you have to put an boundary sooner or later, or
Emacs is going to complain.


>> I think you are doing two things -- one is telling the user that
>> something has happened and one is storing the result. I think that this
>> is the problem.
>>
>> My suggestion is add the %@ into the buffer when the result comes
>> through. If you want %@ to *appear* before this, then insert an overlay
>> with a display string.
>
> Please let us consider the more general issue of making *all*
> insertions, the first _and also subsequent ones_ that happen during the
> interaction with the Prolog process (and which can stem either from
> process output *or* from user input) *atomic* in the sense that they are
> all undone as a single unit. Please try out ediprolog to see what I
> mean. In my view, the transaction concept that Stefan has proposed is a
> very good solution for this and would benefit also other parts of Emacs.
> It is this general issue that I would like to address and solve with a
> suitable (likely: new) Emacs mechanism, not only the first %@.

Already replied to this, in my other response. It's easy to add.


> This seems completely normal and correct to me. Undo boundaries are
> currently inserted without my control over them. I understand that this
> is for efficiency reasons, to allow Emacs to trim the undo list.

Just to be clear; it's not for reasons of efficiency. Emacs has to trim
the undo list or Emacs has a memory leak. It currently has heuristic
checks in place, which make Emacs produce a user visible error when it
forcibly discards undo information.

> I would like to have more control over this, for example, by removing
> these undo boundaries when the whole interaction is over. At that
> point, we know that Emacs has not raised an error, so it should be
> very safe.

That's true, but only for a defined set of prolog output.

Still, I guess, hitting the undo limits is not a problem for ediprolog
in general use, or you would have hit it already (as I say, it's pretty
obvious when you do hit it!)

> Several other solutions would also fit this particular case, such as
> merging subsequent insertions in the undo list to keep its length
> constant. These solutions should not depend on the timing of
> insertions.
>
> We have found one case where I could prevent the undo boundary with
> special-case code (for the initial %@), but a more general solution
> would be much more welcome and work for all parts of the interaction.

No worries, I'll add to master.

Phil