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