bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

To reproduce this issue, please perform the following steps:

1. Install SWI-Prolog >= 7.2. I am using 7.3.23 in the following.

2. Copy ediprolog.el to the current directory via:

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

3. Copy ceiled.pl to the current directory via:

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

4. With ediprolog.el and ceiled.pl in the current directory,
   start Emacs with:

     $ emacs -Q ceiled.pl -fn "Bitstream Vera Sans Mono 15" \
          --eval "(load \"$PWD/ediprolog.el\")"

   Please see the screenshot for how this looks for me:

      https://www.metalevel.at/ei/ceil1.png
      

5. Press:

       M-x ediprolog-dwim RET

   This consults the buffer. If everything works as intended, you see
   "Buffer consulted." in the message area.

6. Press:

      M-g M-g 15 RET

   This moves point at the beginning of "%?- time(....)".

7. Press:

      M-x ediprolog-dwim RET

   This evaluates the Prolog query at point. It only takes a few
   milliseconds to produce a very long line that spans several visual
   lines.

   Please see the screenshot:

      https://www.metalevel.at/ei/ceil2.png

8. Press:

      C-_

   This *undoes* the insertion of text that resulted from step (7).

9. As expected, the insertion is undone, but unexpectedly a completely blank
   buffer remains.

   Please see the screenshot:

      https://www.metalevel.at/ei/ceil3.png

  When I press C-p to move to the previous line, the expected part
  of the buffer is shown.

  Please see the screenshot:

      https://www.metalevel.at/ei/ceil4.png

In summary, step 8 leads to 2 problems:

(a) a blank buffer is shown
(b) point position after the undo is not where point was before the insertion.

Please note that #1095 ("Unexpected point position after undo") may be
related: Point after undo is sometimes at an unexpected position.

Thank you for looking into this!
Markus


In GNU Emacs 25.1.50.1 (x86_64-apple-darwin15.5.0, X toolkit, Xaw scroll bars)
 of 2016-05-30 built on mt-imac
Repository revision: 190942baeff3f541abf2a937e0fb4d3f9ea104be
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 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#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Eli Zaretskii]

> From: Markus Triska <triska@metalevel.at>
> Date: Wed, 29 Jun 2016 23:47:19 +0200
> 
> In summary, step 8 leads to 2 problems:
> 
> (a) a blank buffer is shown
> (b) point position after the undo is not where point was before the insertion.

I suspect this is a redisplay problem, not an undo problem.  How do
you see that point is in the wrong position?  What does "C-x =" say
immediately after the undo?

Anyway, I tried to reproduce the problem, but couldn't: SWI-Prolog
7.3.x seems not to support the OS of my main development machine, and
all my attempts to simulate the situation by having a subprocess
insert a long string into a buffer failed: after undo I always see a
window with the single original line, the one that corresponds to your
line 15.  As expected.

Can you try coming up with a recipe that doesn't involve SWI-Prolog?

Also, how important is it to have exactly the font you mention and
exactly that size?  (I tried that font and that size, as well as a few
others, but that didn't help in my case.)

Thanks.

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

Eli Zaretskii <eliz@gnu.org> writes:


> I suspect this is a redisplay problem, not an undo problem.  How do
> you see that point is in the wrong position?  What does "C-x =" say
> immediately after the undo?

I see from two facts that it is in the wrong position:

1) I need to press C-p to go to the position *before* the undo. From
   this I see that point after undo is at least one line off.
   
2) C-x = immediately after undo confirms this, saying:

   point=323 of 322 (EOB) column=0

   While point before undo is point 281 of 322.


> Can you try coming up with a recipe that doesn't involve SWI-Prolog?

Please see #21722. It shows that point position after undo is different
from what it was before, just as in this case. I constructed the case
shown in #21722 due to the issues with undo after process output, and I
suspect these issues are related. Redisplay may also be involved.


> Also, how important is it to have exactly the font you mention and
> exactly that size?  (I tried that font and that size, as well as a few
> others, but that didn't help in my case.)

The font and even window size definitely have an impact on this
issue. Please try a different window size, and I hope you can then
reproduce the issue, or at least #21722.

Thank you and all the best!
Markus

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Eli Zaretskii]

> From: Markus Triska <triska@metalevel.at>
> Cc: 23871@debbugs.gnu.org
> Date: Thu, 30 Jun 2016 20:00:31 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> 
> > I suspect this is a redisplay problem, not an undo problem.  How do
> > you see that point is in the wrong position?  What does "C-x =" say
> > immediately after the undo?
> 
> I see from two facts that it is in the wrong position:
> 
> 1) I need to press C-p to go to the position *before* the undo. From
>    this I see that point after undo is at least one line off.
>    
> 2) C-x = immediately after undo confirms this, saying:
> 
>    point=323 of 322 (EOB) column=0
> 
>    While point before undo is point 281 of 322.

I'm not sure this is not the expected behavior.  This means the blank
buffer you see is just the last empty line with EOB.  The Emacs
display engine should not normally allow that, it should show the last
line before EOB.

> > Can you try coming up with a recipe that doesn't involve SWI-Prolog?
> 
> Please see #21722. It shows that point position after undo is different
> from what it was before, just as in this case.

When I repeat the recipe there, both with Emacs 25.0.95 and 25.1.50,
point is positioned at the end of the line, both after "C-M-x C-/" and
"C-h f C-g C-M-x C-/".  So I see consistent behavior.  Perhaps undo
gurus could comment whether this is also the correct behavior.

> The font and even window size definitely have an impact on this
> issue. Please try a different window size, and I hope you can then
> reproduce the issue, or at least #21722.

Unfortunately, I can't reproduce any of them.

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Eli Zaretskii]

> Date: Thu, 30 Jun 2016 21:21:31 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 23871@debbugs.gnu.org
> 
> > 2) C-x = immediately after undo confirms this, saying:
> > 
> >    point=323 of 322 (EOB) column=0
> > 
> >    While point before undo is point 281 of 322.
> 
> I'm not sure this is not the expected behavior.  This means the blank
> buffer you see is just the last empty line with EOB.  The Emacs
> display engine should not normally allow that, it should show the last
> line before EOB.

The above meant to say that it could be the expected undo behavior,
but if so, the display engine probably has some issue in this
situation.

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

Markus Triska <triska@metalevel.at> writes:
> Please see #21722. It shows that point position after undo is different
> from what it was before, just as in this case. I constructed the case
> shown in #21722 due to the issues with undo after process output, and I
> suspect these issues are related. Redisplay may also be involved.
>
>
>> Also, how important is it to have exactly the font you mention and
>> exactly that size?  (I tried that font and that size, as well as a few
>> others, but that didn't help in my case.)
>
> The font and even window size definitely have an impact on this
> issue. Please try a different window size, and I hope you can then
> reproduce the issue, or at least #21722.

I believe the following patch fixes #21722.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-missing-point-information-in-undo.patch --]
[-- Type: text/x-diff, Size: 783 bytes --]

From 6687d6162941b7d5a5d7e41c3a7e1b1a1527538f Mon Sep 17 00:00:00 2001
From: Phillip Lord <phillip.lord@russet.org.uk>
Date: Thu, 30 Jun 2016 22:06:00 +0100
Subject: [PATCH] Fix missing point information in undo

* src/undo.c (record_insert): Use record_point instead of
  prepare_record.

Addresses Bug# 21722
---
 src/undo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/undo.c b/src/undo.c
index be5b270..cafe351 100644
--- a/src/undo.c
+++ b/src/undo.c
@@ -83,7 +83,7 @@ record_insert (ptrdiff_t beg, ptrdiff_t length)
   if (EQ (BVAR (current_buffer, undo_list), Qt))
     return;
 
-  prepare_record ();
+  record_point (beg);
 
   /* If this is following another insertion and consecutive with it
      in the buffer, combine the two.  */
-- 
2.9.0

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

Hi Phillip,

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

> I believe the following patch fixes #21722.

I tried it, and can still reproduce both #21722 and #23871 also with the
patch applied to the current emacs-25 branch.

All the best,
Markus

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Eli Zaretskii]

> From: Markus Triska <triska@metalevel.at>
> Cc: Eli Zaretskii <eliz@gnu.org>,  23871@debbugs.gnu.org
> Date: Fri, 01 Jul 2016 08:31:10 +0200
> 
> phillip.lord@russet.org.uk (Phillip Lord) writes:
> 
> > I believe the following patch fixes #21722.
> 
> I tried it, and can still reproduce both #21722 and #23871 also with the
> patch applied to the current emacs-25 branch.

Which doesn't surprise me, since point is at the insertion position
just before the insertion, which is when record_insert is called,
AFAIU.

I think the question here is why does undo record the position of
point at the end of the sexp, and not where point was when C-M-x was
invoked.  This has something to do with code that runs off C-M-x, I
presume.

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

[-- Attachment #1: Type: text/plain, Size: 1202 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Markus Triska <triska@metalevel.at>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  23871@debbugs.gnu.org
>> Date: Fri, 01 Jul 2016 08:31:10 +0200
>> 
>> phillip.lord@russet.org.uk (Phillip Lord) writes:
>> 
>> > I believe the following patch fixes #21722.
>> 
>> I tried it, and can still reproduce both #21722 and #23871 also with the
>> patch applied to the current emacs-25 branch.
>
> Which doesn't surprise me, since point is at the insertion position
> just before the insertion, which is when record_insert is called,
> AFAIU.
>
> I think the question here is why does undo record the position of
> point at the end of the sexp, and not where point was when C-M-x was
> invoked.

It doesn't record point at all. It doesn't end up at the end of the
sexp, rather at the beginning of the insertion (which happen to be the
same place).

> This has something to do with code that runs off C-M-x, I presume.

I think it's because I forgot to call record_point when calling
record_insert. The patch I sent yesterday put that back, but did it
wrong.

The following patch (attempts) to address the issue which is,
unfortunately, a bit more extensive than the last.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-missing-point-information-in-undo.patch --]
[-- Type: text/x-diff, Size: 3185 bytes --]

From 906a5affc1fc04a9d1d3efffb7d5bfc21e6c2e44 Mon Sep 17 00:00:00 2001
From: Phillip Lord <phillip.lord@russet.org.uk>
Date: Thu, 30 Jun 2016 22:06:00 +0100
Subject: [PATCH] Fix missing point information in undo

* src/undo.c (record_insert): Use record_point instead of
  prepare_record.

Addresses Bug# 21722
---
 src/undo.c                    | 10 ++++++----
 test/automated/simple-test.el | 19 ++++++++++++++++++-
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/src/undo.c b/src/undo.c
index be5b270..d8375a9 100644
--- a/src/undo.c
+++ b/src/undo.c
@@ -60,8 +60,6 @@ record_point (ptrdiff_t pt)
   at_boundary = ! CONSP (BVAR (current_buffer, undo_list))
                 || NILP (XCAR (BVAR (current_buffer, undo_list)));
 
-  prepare_record ();
-
   /* If we are just after an undo boundary, and
      point wasn't at start of deleted range, record where it was.  */
   if (at_boundary)
@@ -85,6 +83,10 @@ record_insert (ptrdiff_t beg, ptrdiff_t length)
 
   prepare_record ();
 
+  if (point_before_last_command_or_undo != beg
+      && buffer_before_last_command_or_undo == current_buffer)
+    record_point (point_before_last_command_or_undo);
+
   /* If this is following another insertion and consecutive with it
      in the buffer, combine the two.  */
   if (CONSP (BVAR (current_buffer, undo_list)))
@@ -163,6 +165,8 @@ record_delete (ptrdiff_t beg, Lisp_Object string, bool record_markers)
   if (EQ (BVAR (current_buffer, undo_list), Qt))
     return;
 
+  prepare_record ();
+
   if (point_before_last_command_or_undo != beg
       && buffer_before_last_command_or_undo == current_buffer)
     record_point (point_before_last_command_or_undo);
@@ -170,12 +174,10 @@ record_delete (ptrdiff_t beg, Lisp_Object string, bool record_markers)
   if (PT == beg + SCHARS (string))
     {
       XSETINT (sbeg, -beg);
-      prepare_record ();
     }
   else
     {
       XSETFASTINT (sbeg, beg);
-      prepare_record ();
     }
 
   /* primitive-undo assumes marker adjustments are recorded
diff --git a/test/automated/simple-test.el b/test/automated/simple-test.el
index 40cd1d2..c41d010 100644
--- a/test/automated/simple-test.el
+++ b/test/automated/simple-test.el
@@ -311,6 +311,7 @@ undo-test-point-after-forward-kill
       (undo-test-point-after-forward-kill))))
 
 (defmacro simple-test-undo-with-switched-buffer (buffer &rest body)
+  (declare (indent 1) (debug t))
   (let ((before-buffer (make-symbol "before-buffer")))
     `(let ((,before-buffer (current-buffer)))
        (unwind-protect
@@ -340,8 +341,24 @@ simple-test-undo-with-switched-buffer
        (point-min)
        (point-max))))))
 
+(ert-deftest missing-record-point-in-undo ()
+  "Check point is being restored correctly.
 
-
+See Bug#21722."
+  (should
+   (= 5
+      (with-temp-buffer
+       (generate-new-buffer " *temp*")
+       (emacs-lisp-mode)
+       (setq buffer-undo-list nil)
+       (insert "(progn (end-of-line) (insert \"hello\"))")
+       (beginning-of-line)
+       (forward-char 4)
+       (undo-boundary)
+       (eval-defun nil)
+       (undo-boundary)
+       (undo)
+       (point)))))
 
 (provide 'simple-test)
 ;;; simple-test.el ends here
-- 
2.9.0

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

Hi Phillip,

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

> The following patch (attempts) to address the issue which is,
> unfortunately, a bit more extensive than the last.

This fixes #21722, thank you!

The current issue (#23871) still remains though. Can you reproduce this?

All the best!
Markus

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

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

> The following patch (attempts) to address the issue which is,
> unfortunately, a bit more extensive than the last.

Your patch leads to the following regression though:

1) Download bc.el from:

   https://www.metalevel.at/ei/bc.el

2) emacs -Q bc.el

3) Press: C-M-x C-/

4) Point is unexpectedly placed at the end of the buffer.
   Without your patch, point is placed at the beginning.

All the best!
Markus

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

Markus Triska <triska@metalevel.at> writes:

> Hi Phillip,
>
> phillip.lord@russet.org.uk (Phillip Lord) writes:
>
>> The following patch (attempts) to address the issue which is,
>> unfortunately, a bit more extensive than the last.
>
> This fixes #21722, thank you!

Good.

> The current issue (#23871) still remains though.

Shame. Was hoping it would magically disappear.


> Can you reproduce this?

I haven't tried yet.

Phil

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

Markus Triska <triska@metalevel.at> writes:

> phillip.lord@russet.org.uk (Phillip Lord) writes:
>
>> The following patch (attempts) to address the issue which is,
>> unfortunately, a bit more extensive than the last.
>
> Your patch leads to the following regression though:
>
> 1) Download bc.el from:
>
>    https://www.metalevel.at/ei/bc.el
>
> 2) emacs -Q bc.el
>
> 3) Press: C-M-x C-/
>
> 4) Point is unexpectedly placed at the end of the buffer.
>    Without your patch, point is placed at the beginning.



As an aside, probably would have been easier to include this in the
original message, rather than add a link.

bc.el looks like this:

(let ((p (start-process "bc" (current-buffer) "bc")))
  (process-send-string p "2^10\n")
  (goto-char (point-max)))

The strange thing here is that it's not specifically an undo problem.
Point doesn't move after eval-defun. You don't need to do undo to see
the difference.

This on the other hand:

(let ((p (start-process "bc" (current-buffer) "bc")))
  (process-send-string p "2^10\n")
  (sit-for 1)
  (goto-char (point-max)))

Works fine.

I will investigate.

Phil

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

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

> As an aside, probably would have been easier to include this in the
> original message, rather than add a link.

I only add a link if the spacing is critical, as in this case.
Otherwise, I completely agree that in-line code is preferable.

> I will investigate.

Thank you for looking into this!

All the best,
Markus

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: Eli Zaretskii <eliz@gnu.org>,  23871@debbugs.gnu.org
> Date: Fri, 01 Jul 2016 23:21:01 +0100
> 
> (let ((p (start-process "bc" (current-buffer) "bc")))
>   (process-send-string p "2^10\n")
>   (goto-char (point-max)))
> 
> The strange thing here is that it's not specifically an undo problem.
> Point doesn't move after eval-defun.

The problem, AFAIU, is in point movements _during_ eval-defun: they
seem to not be recorded in buffer-undo-list.  The undo list I get
after C-M-x is this:

  (nil (117 . 122) (t 22391 27551 0 0))

The only information about point there is position 117, which is where
undo leaves point after C-/.  The information about point position at
buffer position 1 was lost.  IOW, this fragment of elisp--eval-defun:

        ;; Read the form from the buffer, and record where it ends.
        (save-excursion
          (end-of-defun)
          (beginning-of-defun)
          (setq beg (point))
          (setq form (read (current-buffer)))
          (setq end (point)))

somehow does not reflected point movements in buffer's undo list.

By contrast, in Emacs 24.5, buffer-undo-list after C-M-x is this:

  (nil (117 . 122) 1 (t 22391 27551 0 0))

Note position 1 in it.

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

[-- Attachment #1: Type: text/plain, Size: 1253 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: phillip.lord@russet.org.uk (Phillip Lord)
>> Cc: Eli Zaretskii <eliz@gnu.org>,  23871@debbugs.gnu.org
>> Date: Fri, 01 Jul 2016 23:21:01 +0100
>> 
>> (let ((p (start-process "bc" (current-buffer) "bc")))
>>   (process-send-string p "2^10\n")
>>   (goto-char (point-max)))
>> 
>> The strange thing here is that it's not specifically an undo problem.
>> Point doesn't move after eval-defun.
>
> The problem, AFAIU, is in point movements _during_ eval-defun: they
> seem to not be recorded in buffer-undo-list.  The undo list I get
> after C-M-x is this:
>
>   (nil (117 . 122) (t 22391 27551 0 0))
>

So, I don't think this is a regression caused by my patch at all. It is
an bug that has been there since I altered undo.c last year.

The problem was caused because of undo only records point after a
boundary (or the first element). I'd changed things during slightly when
I update undo.c so that the timestamp list got added before checking
whether I was at a boundary, hence blocking addition of the point
restoration information.

This is why I found the problem so erratic to replicate; it only occurs
immediately the first change to a buffer.

I believe the following patch addresses the issue.

Phil


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-missing-point-information-in-undo.patch --]
[-- Type: text/x-diff, Size: 4295 bytes --]

From d4e9e44402fdf248ba4bc895e914d4cc5580f229 Mon Sep 17 00:00:00 2001
From: Phillip Lord <phillip.lord@russet.org.uk>
Date: Thu, 30 Jun 2016 22:06:00 +0100
Subject: [PATCH] Fix missing point information in undo

* src/undo.c (record_insert): Use record_point instead of
  prepare_record, and do so unconditionally.
  (prepare_record): Do not record first change.
  (record_point): Now conditional on state before the last command.
  (record_delete): Call record_point unconditionally.

Addresses Bug# 21722
---
 src/undo.c                    | 26 +++++++++++++-------------
 test/automated/simple-test.el | 19 ++++++++++++++++++-
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/src/undo.c b/src/undo.c
index be5b270..f5a5ea1 100644
--- a/src/undo.c
+++ b/src/undo.c
@@ -40,16 +40,13 @@ prepare_record (void)
   /* Allocate a cons cell to be the undo boundary after this command.  */
   if (NILP (pending_boundary))
     pending_boundary = Fcons (Qnil, Qnil);
-
-  if (MODIFF <= SAVE_MODIFF)
-    record_first_change ();
 }
 
 /* Record point as it was at beginning of this command.
-   PT is the position of point that will naturally occur as a result of the
+   BEG is the position of point that will naturally occur as a result of the
    undo record that will be added just after this command terminates.  */
 static void
-record_point (ptrdiff_t pt)
+record_point (ptrdiff_t beg)
 {
   /* Don't record position of pt when undo_inhibit_record_point holds.  */
   if (undo_inhibit_record_point)
@@ -60,13 +57,16 @@ record_point (ptrdiff_t pt)
   at_boundary = ! CONSP (BVAR (current_buffer, undo_list))
                 || NILP (XCAR (BVAR (current_buffer, undo_list)));
 
-  prepare_record ();
+  if (MODIFF <= SAVE_MODIFF)
+    record_first_change ();
 
   /* If we are just after an undo boundary, and
      point wasn't at start of deleted range, record where it was.  */
-  if (at_boundary)
+  if (at_boundary
+      && point_before_last_command_or_undo != beg
+      && buffer_before_last_command_or_undo == current_buffer )
     bset_undo_list (current_buffer,
-		    Fcons (make_number (pt),
+		    Fcons (make_number (point_before_last_command_or_undo),
 			   BVAR (current_buffer, undo_list)));
 }
 
@@ -85,6 +85,8 @@ record_insert (ptrdiff_t beg, ptrdiff_t length)
 
   prepare_record ();
 
+  record_point (beg);
+
   /* If this is following another insertion and consecutive with it
      in the buffer, combine the two.  */
   if (CONSP (BVAR (current_buffer, undo_list)))
@@ -163,19 +165,17 @@ record_delete (ptrdiff_t beg, Lisp_Object string, bool record_markers)
   if (EQ (BVAR (current_buffer, undo_list), Qt))
     return;
 
-  if (point_before_last_command_or_undo != beg
-      && buffer_before_last_command_or_undo == current_buffer)
-    record_point (point_before_last_command_or_undo);
+  prepare_record ();
+
+  record_point (beg);
 
   if (PT == beg + SCHARS (string))
     {
       XSETINT (sbeg, -beg);
-      prepare_record ();
     }
   else
     {
       XSETFASTINT (sbeg, beg);
-      prepare_record ();
     }
 
   /* primitive-undo assumes marker adjustments are recorded
diff --git a/test/automated/simple-test.el b/test/automated/simple-test.el
index 40cd1d2..c41d010 100644
--- a/test/automated/simple-test.el
+++ b/test/automated/simple-test.el
@@ -311,6 +311,7 @@ undo-test-point-after-forward-kill
       (undo-test-point-after-forward-kill))))
 
 (defmacro simple-test-undo-with-switched-buffer (buffer &rest body)
+  (declare (indent 1) (debug t))
   (let ((before-buffer (make-symbol "before-buffer")))
     `(let ((,before-buffer (current-buffer)))
        (unwind-protect
@@ -340,8 +341,24 @@ simple-test-undo-with-switched-buffer
        (point-min)
        (point-max))))))
 
+(ert-deftest missing-record-point-in-undo ()
+  "Check point is being restored correctly.
 
-
+See Bug#21722."
+  (should
+   (= 5
+      (with-temp-buffer
+       (generate-new-buffer " *temp*")
+       (emacs-lisp-mode)
+       (setq buffer-undo-list nil)
+       (insert "(progn (end-of-line) (insert \"hello\"))")
+       (beginning-of-line)
+       (forward-char 4)
+       (undo-boundary)
+       (eval-defun nil)
+       (undo-boundary)
+       (undo)
+       (point)))))
 
 (provide 'simple-test)
 ;;; simple-test.el ends here
-- 
2.9.0

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

Hi Phillip,

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

> I believe the following patch addresses the issue.

This patch causes a regression with the original case I posted in this
report (#23871):

Without your patch, after undo, point is *either*

(1) at the beginning of line 15 OR
(2) at a position that is outside the buffer (point 323 of 322).

Case (1) is the expected behaviour, and case (2) is the issue I reported
and which, just FYI, also still arises with your patch applied.

However, your patch also leads to the new problem that even in cases
where the point is not outside the buffer, it is unexpectedly placed at
the *end* of line 15 instead of at the beginning, where ediprolog-dwim
was invoked before the insertion is undone.

In this sense, the behaviour is worse than before for #23871 with your
patch, because point is never restored to where the interaction started.

All the best!
Markus

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: triska@metalevel.at,  23871@debbugs.gnu.org
> Date: Sat, 02 Jul 2016 21:21:28 +0100
> 
> > The problem, AFAIU, is in point movements _during_ eval-defun: they
> > seem to not be recorded in buffer-undo-list.  The undo list I get
> > after C-M-x is this:
> >
> >   (nil (117 . 122) (t 22391 27551 0 0))
> >
> 
> So, I don't think this is a regression caused by my patch at all. It is
> an bug that has been there since I altered undo.c last year.
> 
> The problem was caused because of undo only records point after a
> boundary (or the first element). I'd changed things during slightly when
> I update undo.c so that the timestamp list got added before checking
> whether I was at a boundary, hence blocking addition of the point
> restoration information.
> 
> This is why I found the problem so erratic to replicate; it only occurs
> immediately the first change to a buffer.
> 
> I believe the following patch addresses the issue.

Seems like this is still not the final patch, so I guess we will need
to wait for the right one.  It should go to the emacs-25 branch, which
probably means one more pretest, sigh.

Thanks.

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Eli Zaretskii]

> From: Markus Triska <triska@metalevel.at>
> Cc: Eli Zaretskii <eliz@gnu.org>,  23871@debbugs.gnu.org
> Date: Sat, 02 Jul 2016 22:53:37 +0200
> 
> (2) at a position that is outside the buffer (point 323 of 322).

This is not outside the buffer, this is EOB.  Try "M->" in any buffer,
and then type "C-x =".

(The above comment is meant to make the facts accurate, it doesn't
change the main issue discussed here.)

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

Markus Triska <triska@metalevel.at> writes:

> Hi Phillip,
>
> phillip.lord@russet.org.uk (Phillip Lord) writes:
>
>> I believe the following patch addresses the issue.
>
> This patch causes a regression with the original case I posted in this
> report (#23871):
>
> Without your patch, after undo, point is *either*
>
> (1) at the beginning of line 15 OR
> (2) at a position that is outside the buffer (point 323 of 322).
>
> Case (1) is the expected behaviour, and case (2) is the issue I reported
> and which, just FYI, also still arises with your patch applied.
>
> However, your patch also leads to the new problem that even in cases
> where the point is not outside the buffer, it is unexpectedly placed at
> the *end* of line 15 instead of at the beginning, where ediprolog-dwim
> was invoked before the insertion is undone.
>
> In this sense, the behaviour is worse than before for #23871 with your
> patch, because point is never restored to where the interaction started.


I haven't started to look at 23871 yet, only 21722; I would rather deal
with the simple case first. Can you tell me whether it fixes this case?

Phil

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: phillip.lord@russet.org.uk (Phillip Lord)
>> Cc: triska@metalevel.at,  23871@debbugs.gnu.org
>> Date: Sat, 02 Jul 2016 21:21:28 +0100
>> 
>> > The problem, AFAIU, is in point movements _during_ eval-defun: they
>> > seem to not be recorded in buffer-undo-list.  The undo list I get
>> > after C-M-x is this:
>> >
>> >   (nil (117 . 122) (t 22391 27551 0 0))
>> >
>> 
>> So, I don't think this is a regression caused by my patch at all. It is
>> an bug that has been there since I altered undo.c last year.
>> 
>> The problem was caused because of undo only records point after a
>> boundary (or the first element). I'd changed things during slightly when
>> I update undo.c so that the timestamp list got added before checking
>> whether I was at a boundary, hence blocking addition of the point
>> restoration information.
>> 
>> This is why I found the problem so erratic to replicate; it only occurs
>> immediately the first change to a buffer.
>> 
>> I believe the following patch addresses the issue.
>
> Seems like this is still not the final patch, so I guess we will need
> to wait for the right one.

Yes. I will try and investigate 23871 as soon as I can.

> It should go to the emacs-25 branch, which probably means one more
> pretest, sigh.

Unfortunately, yes. It's a pity that I didn't 21722 when it came in.

Phil

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

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

> I haven't started to look at 23871 yet, only 21722; I would rather deal
> with the simple case first. Can you tell me whether it fixes this
> case?

The patch fixes the concrete case reported in #21722 but leads to a new
problem with undo that is not there without this patch. I cannot regard
this as a good fix for #21722. If you want to close #21722 after
applying your patch, I can file a new report for the new problem.

Please note that #23871 is the main problem for me and has been present
in Emacs for about a decade. The other issues I filed are only my
attempts to make the issue more easily reproducible, and may in fact
even be unrelated issues that I find while trying to find the root cause
of #23871. I first filed #1095, then adapted this to #21722, and now
also filed #23871 because redisplay may also be involved.

I have tolerated the problems described in #1095 and #21722 for about a
decade and see no reason to delay Emacs 25: Neither Emacs 23 nor 24 were
delayed because of them although these issues were already reported!
#23871 was also present during this time but not yet reported.

All the best,
Markus

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

Markus Triska <triska@metalevel.at> writes:

> phillip.lord@russet.org.uk (Phillip Lord) writes:
>
>> I haven't started to look at 23871 yet, only 21722; I would rather deal
>> with the simple case first. Can you tell me whether it fixes this
>> case?
>
> The patch fixes the concrete case reported in #21722 but leads to a new
> problem with undo that is not there without this patch.

And is it there in Emacs-24.5?

> I cannot regard this as a good fix for #21722. If you want to close
> #21722 after applying your patch, I can file a new report for the new
> problem.
>
> Please note that #23871 is the main problem for me and has been present
> in Emacs for about a decade. The other issues I filed are only my
> attempts to make the issue more easily reproducible, and may in fact
> even be unrelated issues that I find while trying to find the root cause
> of #23871. I first filed #1095, then adapted this to #21722, and now
> also filed #23871 because redisplay may also be involved.
>
> I have tolerated the problems described in #1095 and #21722 for about a
> decade and see no reason to delay Emacs 25: Neither Emacs 23 nor 24 were
> delayed because of them although these issues were already reported!
> #23871 was also present during this time but not yet reported.


I've tried to reproduce #23871, but unfortunately, swiprolog crashes as
shown below.

The packaged version of swiprolog on my machine is 6.6.4, rather than >
7.2. I'm not keen on trying to update to 7.2. Is there a reproduction
that will work on 6.6.4?

Phil



% 1
% 2
% 3
% 4 almost empty lines; next line is: line 15
%?- time(ceiled_square_root(2^10000, R)).
%@ [Thread 1] pl-arith.c:1669: ar_pow: Assertion failed: 0
%@ C-stack trace labeled "crash":
%@   [0] __assert_fail+0x41
%@   [1] PL_license+0x584a
%@   [2] PL_license+0x3755
%@   [3] PL_license+0x3be0
%@   [4] PL_next_solution+0x5c3e
%@   [5] pl_skip_list3_va+0x1629
%@   [6] pl_skip_list3_va+0x1da0
%@   [7] PL_toplevel+0x1d
%@   [8] main+0x2d
%@   [9] __libc_start_main+0xf5

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Eli Zaretskii]

merge 1095 21722
thanks

> From: Markus Triska <triska@metalevel.at>
> Cc: Eli Zaretskii <eliz@gnu.org>,  23871@debbugs.gnu.org
> Date: Sun, 03 Jul 2016 12:08:01 +0200
> 
> Please note that #23871 is the main problem for me and has been present
> in Emacs for about a decade. The other issues I filed are only my
> attempts to make the issue more easily reproducible, and may in fact
> even be unrelated issues that I find while trying to find the root cause
> of #23871. I first filed #1095, then adapted this to #21722, and now
> also filed #23871 because redisplay may also be involved.

AFAICS, 1095 and 21722 report exactly the same problem; 21722 should
have never been opened as a separate bug.  So I just merged them
together.

> I have tolerated the problems described in #1095 and #21722 for about a
> decade and see no reason to delay Emacs 25: Neither Emacs 23 nor 24 were
> delayed because of them although these issues were already reported!
> #23871 was also present during this time but not yet reported.

That may be so, but my problem, which _is_ a regression from 24.5, is
that even this part of 1095:

  In "emacs -Q", when I insert into *scratch* the form:

     (progn (end-of-line) (insert "\nhi"))

  then place point on the "g" and press C-M-x C-/, then the insertion is
  undone, and point remains on the "g", as expected.

doesn't work as expected in Emacs 25.  This is a regression I'd like
to fix before 25.1 is released.

And this part of 1095/21722 _is_ solved by Phillip's patch, so I think
we should install it on the release branch.  However, I'd like Stefan
to look over the patch before we install it.

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: Eli Zaretskii <eliz@gnu.org>,  23871@debbugs.gnu.org
> Date: Sun, 03 Jul 2016 13:55:48 +0100
> 
> I've tried to reproduce #23871, but unfortunately, swiprolog crashes as
> shown below.

I very much hope that the problem could be reproduced without using
Prolog at all, by just invoking a program like 'cat' to insert some
text into the buffer.  I cannot see how Prolog can be possibly
relevant to this, as all it does is insert text into the buffer.

> The packaged version of swiprolog on my machine is 6.6.4, rather than >
> 7.2. I'm not keen on trying to update to 7.2.

Actually, the recipe asks for 7.3.

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

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

> I've tried to reproduce #23871, but unfortunately, swiprolog crashes as
> shown below.
>
> The packaged version of swiprolog on my machine is 6.6.4, rather than >
> 7.2. I'm not keen on trying to update to 7.2. Is there a reproduction
> that will work on 6.6.4?

I am trying to find a case that works without invoking Prolog at
all. However, this will definitely take me a while because ediprolog is
also involved and the buffer switching etc. that ediprolog does behind
the scenes may in fact also contribute to the regression with undo.

If you send me your SSH public key, I will create an account for you on
a machine where you can reproduce this issue (with emacs -nw).

Also for you Eli of course!

All the best,
Markus

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

Eli Zaretskii <eliz@gnu.org> writes:

> AFAICS, 1095 and 21722 report exactly the same problem; 21722 should
> have never been opened as a separate bug.  So I just merged them
> together.

You have tagged #21722 as unreproducible now too. Can you reproduce it?

All the best,
Markus

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Eli Zaretskii]

> From: Markus Triska <triska@metalevel.at>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  phillip.lord@russet.org.uk,  23871@debbugs.gnu.org
> Date: Sun, 03 Jul 2016 20:09:40 +0200
> 
> You have tagged #21722 as unreproducible now too.

Sorry, should be fixed now.  (debbugs is extremely unhelpful in these
matters, so it's all too easy to make such mistakes.)

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: phillip.lord@russet.org.uk (Phillip Lord)
>> Cc: Eli Zaretskii <eliz@gnu.org>,  23871@debbugs.gnu.org
>> Date: Sun, 03 Jul 2016 13:55:48 +0100
>> 
>> I've tried to reproduce #23871, but unfortunately, swiprolog crashes as
>> shown below.
>
> I very much hope that the problem could be reproduced without using
> Prolog at all, by just invoking a program like 'cat' to insert some
> text into the buffer.  I cannot see how Prolog can be possibly
> relevant to this, as all it does is insert text into the buffer.

That might be true, unless there is something odd about the way prolog
produces text in terms of timing. Or something happening in the other
buffers it opens. 

>
>> The packaged version of swiprolog on my machine is 6.6.4, rather than >
>> 7.2. I'm not keen on trying to update to 7.2.
>
> Actually, the recipe asks for 7.3.

Both are a long way of 6.6.4!

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

Markus Triska <triska@metalevel.at> writes:

> phillip.lord@russet.org.uk (Phillip Lord) writes:
>
>> I've tried to reproduce #23871, but unfortunately, swiprolog crashes as
>> shown below.
>>
>> The packaged version of swiprolog on my machine is 6.6.4, rather than >
>> 7.2. I'm not keen on trying to update to 7.2. Is there a reproduction
>> that will work on 6.6.4?
>
> I am trying to find a case that works without invoking Prolog at
> all. However, this will definitely take me a while because ediprolog is
> also involved and the buffer switching etc. that ediprolog does behind
> the scenes may in fact also contribute to the regression with undo.
>
> If you send me your SSH public key, I will create an account for you on
> a machine where you can reproduce this issue (with emacs -nw).

This is probably not going to help. I'll probably need to rebuild emacs
on the same machine.

How complex is the interaction with prolog? If it's just calling it
as a repl, then a shell script which dumps the same output should work.

Phil

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:
>> I have tolerated the problems described in #1095 and #21722 for about a
>> decade and see no reason to delay Emacs 25: Neither Emacs 23 nor 24 were
>> delayed because of them although these issues were already reported!
>> #23871 was also present during this time but not yet reported.
>
> That may be so, but my problem, which _is_ a regression from 24.5, is
> that even this part of 1095:
>
>   In "emacs -Q", when I insert into *scratch* the form:
>
>      (progn (end-of-line) (insert "\nhi"))
>
>   then place point on the "g" and press C-M-x C-/, then the insertion is
>   undone, and point remains on the "g", as expected.
>
> doesn't work as expected in Emacs 25.  This is a regression I'd like
> to fix before 25.1 is released.

I think we have two bugs (one which covers the other). Handling of point
after an insertion. This was hiding another, which is handling of point
as the first change in a buffer. Although the last one was being hidden,
I think this would also affect deletes.


> And this part of 1095/21722 _is_ solved by Phillip's patch, so I think
> we should install it on the release branch.  However, I'd like Stefan
> to look over the patch before we install it.

Yeah, I'd really like Stefan to look over it also. Or, alternatively,
would could wait a week till I have had chance to finish reading the
"How to Program C" book I just bought.

Phil

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Stefan Monnier]

>> From d4e9e44402fdf248ba4bc895e914d4cc5580f229 Mon Sep 17 00:00:00 2001
> From: Phillip Lord <phillip.lord@russet.org.uk>
> Date: Thu, 30 Jun 2016 22:06:00 +0100
> Subject: [PATCH] Fix missing point information in undo

> * src/undo.c (record_insert): Use record_point instead of
>   prepare_record, and do so unconditionally.
>   (prepare_record): Do not record first change.
>   (record_point): Now conditional on state before the last command.
>   (record_delete): Call record_point unconditionally.

I haven't had time to look in detail at this patch, so it's hard for me
to give a strong agreement.  Maybe some explanations would help.
Could you give a verbosish "commit log" explaining the reason behind
each hunk?

> @@ -40,16 +40,13 @@ prepare_record (void)
>    /* Allocate a cons cell to be the undo boundary after this command.  */
>    if (NILP (pending_boundary))
>      pending_boundary = Fcons (Qnil, Qnil);
> -
> -  if (MODIFF <= SAVE_MODIFF)
> -    record_first_change ();
>  }

Not sure why/how this is related to the other changes, nor why this code
was there in the first place.

But in any case, the comment before prepare_record needs to be updated,
because it seems to describe neither the current behavior, nor the
behavior after applying the above hunk.

BTW, I notice that in the current code (emacs-25), there's one other
call to record_first_change (in record_property_change), and it could be
replaced with a call to prepare_record, so it begs the question whether
the above hunk should also apply to record_property_change as well.

>  /* Record point as it was at beginning of this command.
> -   PT is the position of point that will naturally occur as a result of the
> +   BEG is the position of point that will naturally occur as a result of the
>     undo record that will be added just after this command terminates.  */
>  static void
> -record_point (ptrdiff_t pt)
> +record_point (ptrdiff_t beg)
>  {
>    /* Don't record position of pt when undo_inhibit_record_point holds.  */
>    if (undo_inhibit_record_point)
> @@ -60,13 +57,16 @@ record_point (ptrdiff_t pt)
>    at_boundary = ! CONSP (BVAR (current_buffer, undo_list))
>                  || NILP (XCAR (BVAR (current_buffer, undo_list)));

You said:

    The problem was caused because of undo only records point after a
    boundary (or the first element). I'd changed things during slightly when
    I update undo.c so that the timestamp list got added before checking
    whether I was at a boundary, hence blocking addition of the point
    restoration information.

so maybe the computation of at_boundary above should be tweaked to
ignore timestamps?

>    /* If we are just after an undo boundary, and
>       point wasn't at start of deleted range, record where it was.  */
> -  if (at_boundary)
> +  if (at_boundary
> +      && point_before_last_command_or_undo != beg
> +      && buffer_before_last_command_or_undo == current_buffer )
>      bset_undo_list (current_buffer,
> -		    Fcons (make_number (pt),
> +		    Fcons (make_number (point_before_last_command_or_undo),
>  			   BVAR (current_buffer, undo_list)));

I like this part.

> -  if (point_before_last_command_or_undo != beg
> -      && buffer_before_last_command_or_undo == current_buffer)
> -    record_point (point_before_last_command_or_undo);
> +  prepare_record ();
> +
> +  record_point (beg);

And I like this part too.


        Stefan

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

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

> This is probably not going to help. I'll probably need to rebuild emacs
> on the same machine.

First you need access to the machine, so if you need that please let me
know. You won't have to debug a ceiled root problem without root access!

> How complex is the interaction with prolog? If it's just calling it
> as a repl, then a shell script which dumps the same output should work.

OK, I have constructed a shell script that simulates SWI-Prolog for this
specific case, please download it from:

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

Place it in the current directory and change the invocation of Emacs to:

   emacs -Q ceiled.pl -fn "Bitstream Vera Sans Mono 15"   --eval \
        "(progn (load \"$PWD/ediprolog.el\") \
         (setq ediprolog-program \"$PWD/simulans.sh\"))"

Note the setting of ediprolog-program to simulans.sh. I can reproduce
the issue with this script. If it does not work for you, please try a
different window size, font, or running Emacs in the terminal.

Thank you and all the best!
Markus

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Eli Zaretskii]

> From: Markus Triska <triska@metalevel.at>
> Cc: Eli Zaretskii <eliz@gnu.org>,  23871@debbugs.gnu.org
> Date: Mon, 04 Jul 2016 00:03:31 +0200
> 
> OK, I have constructed a shell script that simulates SWI-Prolog for this
> specific case, please download it from:
> 
>     https://www.metalevel.at/ei/simulans.sh
> 
> Place it in the current directory and change the invocation of Emacs to:
> 
>    emacs -Q ceiled.pl -fn "Bitstream Vera Sans Mono 15"   --eval \
>         "(progn (load \"$PWD/ediprolog.el\") \
>          (setq ediprolog-program \"$PWD/simulans.sh\"))"
> 
> Note the setting of ediprolog-program to simulans.sh. I can reproduce
> the issue with this script. If it does not work for you, please try a
> different window size, font, or running Emacs in the terminal.

Thanks, I reproduced the problem, and it definitely involves some
anomaly in redisplay.  I will look into that aspect of the problem.

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

[-- Attachment #1: Type: text/plain, Size: 10479 bytes --]

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

>>> From d4e9e44402fdf248ba4bc895e914d4cc5580f229 Mon Sep 17 00:00:00 2001
>> From: Phillip Lord <phillip.lord@russet.org.uk>
>> Date: Thu, 30 Jun 2016 22:06:00 +0100
>> Subject: [PATCH] Fix missing point information in undo
>
>> * src/undo.c (record_insert): Use record_point instead of
>>   prepare_record, and do so unconditionally.
>>   (prepare_record): Do not record first change.
>>   (record_point): Now conditional on state before the last command.
>>   (record_delete): Call record_point unconditionally.
>
> I haven't had time to look in detail at this patch, so it's hard for me
> to give a strong agreement.  Maybe some explanations would help.


This stems from two reported bugs: one that point was not being
correctly restored after an insertion away from point, and another than
point was not being correctly restored after the first change in a
buffer.


> Could you give a verbosish "commit log" explaining the reason behind
> each hunk?

Yes. That is below with the final patch (including changes you have
requested).


>> @@ -40,16 +40,13 @@ prepare_record (void)
>>    /* Allocate a cons cell to be the undo boundary after this command.  */
>>    if (NILP (pending_boundary))
>>      pending_boundary = Fcons (Qnil, Qnil);
>> -
>> -  if (MODIFF <= SAVE_MODIFF)
>> -    record_first_change ();
>>  }
>
> Not sure why/how this is related to the other changes, nor why this code
> was there in the first place.

It was a refactor since these two conditinals were always called
together. A mistake unfortunately (see below).

> But in any case, the comment before prepare_record needs to be updated,
> because it seems to describe neither the current behavior, nor the
> behavior after applying the above hunk.

Done.

> BTW, I notice that in the current code (emacs-25), there's one other
> call to record_first_change (in record_property_change), and it could be
> replaced with a call to prepare_record, so it begs the question whether
> the above hunk should also apply to record_property_change as well.

Don't understand. Think that the call to record_first_change is necessary.


>>  /* Record point as it was at beginning of this command.
>> -   PT is the position of point that will naturally occur as a result of the
>> +   BEG is the position of point that will naturally occur as a result of the
>>     undo record that will be added just after this command terminates.  */
>>  static void
>> -record_point (ptrdiff_t pt)
>> +record_point (ptrdiff_t beg)
>>  {
>>    /* Don't record position of pt when undo_inhibit_record_point holds.  */
>>    if (undo_inhibit_record_point)
>> @@ -60,13 +57,16 @@ record_point (ptrdiff_t pt)
>>    at_boundary = ! CONSP (BVAR (current_buffer, undo_list))
>>                  || NILP (XCAR (BVAR (current_buffer, undo_list)));
>
> You said:
>
>     The problem was caused because of undo only records point after a
>     boundary (or the first element). I'd changed things during slightly when
>     I update undo.c so that the timestamp list got added before checking
>     whether I was at a boundary, hence blocking addition of the point
>     restoration information.
>
> so maybe the computation of at_boundary above should be tweaked to
> ignore timestamps?


That's a possibility, but it appears more complex that re-ordering the
methods.

>>    /* If we are just after an undo boundary, and
>>       point wasn't at start of deleted range, record where it was.  */
>> -  if (at_boundary)
>> +  if (at_boundary
>> +      && point_before_last_command_or_undo != beg
>> +      && buffer_before_last_command_or_undo == current_buffer )
>>      bset_undo_list (current_buffer,
>> -		    Fcons (make_number (pt),
>> +		    Fcons (make_number (point_before_last_command_or_undo),
>>  			   BVAR (current_buffer, undo_list)));
>
> I like this part.


Good!

>> -  if (point_before_last_command_or_undo != beg
>> -      && buffer_before_last_command_or_undo == current_buffer)
>> -    record_point (point_before_last_command_or_undo);
>> +  prepare_record ();
>> +
>> +  record_point (beg);
>
> And I like this part too.


Good!

Here is a hunk-by-hunk description. Comments come before the hunk they
apply to, and I've split some hunks where necessary. The final patch is
attached (it has some more doc, and another call to "prepare_record"
added) also.

Having read it though again, I cannot understand why the boundary is
pre-allocated at all, but that is a discussion for another time.

Sorry for the hassle this late in the development process.

Phil





`prepare_record` had been refactored out when undo.c was re-written. as the
two conditionals were being called together in many places. This turns out to
have been been a mistake, as we need to call something between these two
conditionals in one place (`record_point`).


#+begin_src diff
@@ -31,25 +31,21 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
    an undo-boundary.  */
 static Lisp_Object pending_boundary;
 
-/* Record point as it was at beginning of this command (if necessary)
-   and prepare the undo info for recording a change.
-   Prepare the undo info for recording a change. */
+/* Prepare the undo info for recording a change. */
 static void
 prepare_record (void)
 {
   /* Allocate a cons cell to be the undo boundary after this command.  */
   if (NILP (pending_boundary))
     pending_boundary = Fcons (Qnil, Qnil);
-
-  if (MODIFF <= SAVE_MODIFF)
-    record_first_change ();
 }
#+end_src


The semantics of record_point had been changed also, so that pt became the
point that was to be recorded, rather than the point that we shouldn't record.
The point we should record is available as global state
(`point_before_last_command_or_undo`) anyway.

#+begin_src diff
-/* Record point as it was at beginning of this command.
-   PT is the position of point that will naturally occur as a result of the
-   undo record that will be added just after this command terminates.  */
+/* Record point, if necessary, as it was at beginning of this command.
+   BEG is the position of point that will naturally occur as a result
+   of the undo record that will be added just after this command
+   terminates.  */
 static void
-record_point (ptrdiff_t pt)
+record_point (ptrdiff_t beg)
 {
   /* Don't record position of pt when undo_inhibit_record_point holds.  */
   if (undo_inhibit_record_point)
#+end_src

Doc added


#+begin_src diff
@@ -57,16 +53,23 @@ record_point (ptrdiff_t pt)
 
   bool at_boundary;
 
+  /* Check whether we are at a boundary now, in case we record the
+  first change. */
   at_boundary = ! CONSP (BVAR (current_buffer, undo_list))
                 || NILP (XCAR (BVAR (current_buffer, undo_list)));
#+end_src

Record point is called from only two places, in both cases before immediately
after `prepare_record`, so we no longer call it here.

However, we must bring back the conditional statement because this is no
longer called in prepare_record, as in this case, it must be called after the
`at_boundary` check. The alternative, which is making the `at_boundary` check
accept "nil" then a timestamp as a boundary sounds more complex.

#+begin_src diff 
-  prepare_record ();
+  /* If this is the first change since save, then record this.*/
+  if (MODIFF <= SAVE_MODIFF)
+    record_first_change ();
#+end_src 

`record_point` is now called unconditionally in the places where it is called,
so we more incorporate the conditional checks here. Take the point to be
recorded from global state.


#+begin_src diff
-  /* If we are just after an undo boundary, and
-     point wasn't at start of deleted range, record where it was.  */
-  if (at_boundary)
+  /* If we are just after an undo boundary, and point was not at start
+     of deleted range, and the recorded point was in this buffer, then
+     record where it was.  */
+  if (at_boundary
+      && point_before_last_command_or_undo != beg
+      && buffer_before_last_command_or_undo == current_buffer )
     bset_undo_list (current_buffer,
-		    Fcons (make_number (pt),
+		    Fcons (make_number (point_before_last_command_or_undo),
 			   BVAR (current_buffer, undo_list)));
 }
#+end_src
 

This record_point was simply missing -- point was never recorded after an
insertion, and this has been directly added to address the bug.

#+begin_src diff
@@ -85,6 +88,8 @@ record_insert (ptrdiff_t beg, ptrdiff_t length)
 
   prepare_record ();
 
+  record_point (beg);
+
   /* If this is following another insertion and consecutive with it
      in the buffer, combine the two.  */
   if (CONSP (BVAR (current_buffer, undo_list)))
#+end_src

Refactor for completeness.

#+begin_src diff
@@ -120,9 +125,7 @@ record_marker_adjustments (ptrdiff_t from, ptrdiff_t to)
   register struct Lisp_Marker *m;
   register ptrdiff_t charpos, adjustment;
 
-  /* Allocate a cons cell to be the undo boundary after this command.  */
-  if (NILP (pending_boundary))
-    pending_boundary = Fcons (Qnil, Qnil);
+  prepare_record();
 
   for (m = BUF_MARKERS (current_buffer); m; m = m->next)
     {
#+end_src


Remove the conditionality, as this is now inside record_point. Change the
parameter to `record_point` to reflect the changed semantics of that method.

Move `prepare_record` call out of both sides of a conditional. The re-ordering
of `prepare_record` and `record_point` should not matter (famous last words).

#+begin_src diff
@@ -163,19 +168,17 @@ record_delete (ptrdiff_t beg, Lisp_Object string, bool record_markers)
   if (EQ (BVAR (current_buffer, undo_list), Qt))
     return;
 
-  if (point_before_last_command_or_undo != beg
-      && buffer_before_last_command_or_undo == current_buffer)
-    record_point (point_before_last_command_or_undo);
+  prepare_record ();
+
+  record_point (beg);
 
   if (PT == beg + SCHARS (string))
     {
       XSETINT (sbeg, -beg);
-      prepare_record ();
     }
   else
     {
       XSETFASTINT (sbeg, beg);
-      prepare_record ();
     }
 
   /* primitive-undo assumes marker adjustments are recorded
#+end_src

Refactor for completeness.

#+begin_src diff
@@ -234,9 +237,7 @@ record_property_change (ptrdiff_t beg, ptrdiff_t length,
   if (EQ (BVAR (buf, undo_list), Qt))
     return;
 
-  /* Allocate a cons cell to be the undo boundary after this command.  */
-  if (NILP (pending_boundary))
-    pending_boundary = Fcons (Qnil, Qnil);
+  prepare_record();
 
   if (MODIFF <= SAVE_MODIFF)
     record_first_change ();
+end_src



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-missing-point-information-in-undo.patch --]
[-- Type: text/x-diff, Size: 6058 bytes --]

From c7e116d16299dfe3bb13a6574f9b03542cb72099 Mon Sep 17 00:00:00 2001
From: Phillip Lord <phillip.lord@russet.org.uk>
Date: Thu, 30 Jun 2016 22:06:00 +0100
Subject: [PATCH] Fix missing point information in undo

* src/undo.c (record_insert): Use record_point instead of
  prepare_record, and do so unconditionally.
  (prepare_record): Do not record first change.
  (record_point): Now conditional on state before the last command.
  (record_delete): Call record_point unconditionally.
  (record_property_change): Use prepare_record.
  (record_marker_adjustments): Use prepare_record.

Addresses Bug# 21722
---
 src/undo.c                    | 51 +++++++++++++++++++++----------------------
 test/automated/simple-test.el | 19 +++++++++++++++-
 2 files changed, 43 insertions(+), 27 deletions(-)

diff --git a/src/undo.c b/src/undo.c
index be5b270..2676c3d 100644
--- a/src/undo.c
+++ b/src/undo.c
@@ -31,25 +31,21 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
    an undo-boundary.  */
 static Lisp_Object pending_boundary;
 
-/* Record point as it was at beginning of this command (if necessary)
-   and prepare the undo info for recording a change.
-   Prepare the undo info for recording a change. */
+/* Prepare the undo info for recording a change. */
 static void
 prepare_record (void)
 {
   /* Allocate a cons cell to be the undo boundary after this command.  */
   if (NILP (pending_boundary))
     pending_boundary = Fcons (Qnil, Qnil);
-
-  if (MODIFF <= SAVE_MODIFF)
-    record_first_change ();
 }
 
-/* Record point as it was at beginning of this command.
-   PT is the position of point that will naturally occur as a result of the
-   undo record that will be added just after this command terminates.  */
+/* Record point, if necessary, as it was at beginning of this command.
+   BEG is the position of point that will naturally occur as a result
+   of the undo record that will be added just after this command
+   terminates.  */
 static void
-record_point (ptrdiff_t pt)
+record_point (ptrdiff_t beg)
 {
   /* Don't record position of pt when undo_inhibit_record_point holds.  */
   if (undo_inhibit_record_point)
@@ -57,16 +53,23 @@ record_point (ptrdiff_t pt)
 
   bool at_boundary;
 
+  /* Check whether we are at a boundary now, in case we record the
+  first change. */
   at_boundary = ! CONSP (BVAR (current_buffer, undo_list))
                 || NILP (XCAR (BVAR (current_buffer, undo_list)));
 
-  prepare_record ();
+  /* If this is the first change since save, then record this.*/
+  if (MODIFF <= SAVE_MODIFF)
+    record_first_change ();
 
-  /* If we are just after an undo boundary, and
-     point wasn't at start of deleted range, record where it was.  */
-  if (at_boundary)
+  /* If we are just after an undo boundary, and point was not at start
+     of deleted range, and the recorded point was in this buffer, then
+     record where it was.  */
+  if (at_boundary
+      && point_before_last_command_or_undo != beg
+      && buffer_before_last_command_or_undo == current_buffer )
     bset_undo_list (current_buffer,
-		    Fcons (make_number (pt),
+		    Fcons (make_number (point_before_last_command_or_undo),
 			   BVAR (current_buffer, undo_list)));
 }
 
@@ -85,6 +88,8 @@ record_insert (ptrdiff_t beg, ptrdiff_t length)
 
   prepare_record ();
 
+  record_point (beg);
+
   /* If this is following another insertion and consecutive with it
      in the buffer, combine the two.  */
   if (CONSP (BVAR (current_buffer, undo_list)))
@@ -120,9 +125,7 @@ record_marker_adjustments (ptrdiff_t from, ptrdiff_t to)
   register struct Lisp_Marker *m;
   register ptrdiff_t charpos, adjustment;
 
-  /* Allocate a cons cell to be the undo boundary after this command.  */
-  if (NILP (pending_boundary))
-    pending_boundary = Fcons (Qnil, Qnil);
+  prepare_record();
 
   for (m = BUF_MARKERS (current_buffer); m; m = m->next)
     {
@@ -163,19 +166,17 @@ record_delete (ptrdiff_t beg, Lisp_Object string, bool record_markers)
   if (EQ (BVAR (current_buffer, undo_list), Qt))
     return;
 
-  if (point_before_last_command_or_undo != beg
-      && buffer_before_last_command_or_undo == current_buffer)
-    record_point (point_before_last_command_or_undo);
+  prepare_record ();
+
+  record_point (beg);
 
   if (PT == beg + SCHARS (string))
     {
       XSETINT (sbeg, -beg);
-      prepare_record ();
     }
   else
     {
       XSETFASTINT (sbeg, beg);
-      prepare_record ();
     }
 
   /* primitive-undo assumes marker adjustments are recorded
@@ -234,9 +235,7 @@ record_property_change (ptrdiff_t beg, ptrdiff_t length,
   if (EQ (BVAR (buf, undo_list), Qt))
     return;
 
-  /* Allocate a cons cell to be the undo boundary after this command.  */
-  if (NILP (pending_boundary))
-    pending_boundary = Fcons (Qnil, Qnil);
+  prepare_record();
 
   if (MODIFF <= SAVE_MODIFF)
     record_first_change ();
diff --git a/test/automated/simple-test.el b/test/automated/simple-test.el
index 40cd1d2..c41d010 100644
--- a/test/automated/simple-test.el
+++ b/test/automated/simple-test.el
@@ -311,6 +311,7 @@ undo-test-point-after-forward-kill
       (undo-test-point-after-forward-kill))))
 
 (defmacro simple-test-undo-with-switched-buffer (buffer &rest body)
+  (declare (indent 1) (debug t))
   (let ((before-buffer (make-symbol "before-buffer")))
     `(let ((,before-buffer (current-buffer)))
        (unwind-protect
@@ -340,8 +341,24 @@ simple-test-undo-with-switched-buffer
        (point-min)
        (point-max))))))
 
+(ert-deftest missing-record-point-in-undo ()
+  "Check point is being restored correctly.
 
-
+See Bug#21722."
+  (should
+   (= 5
+      (with-temp-buffer
+       (generate-new-buffer " *temp*")
+       (emacs-lisp-mode)
+       (setq buffer-undo-list nil)
+       (insert "(progn (end-of-line) (insert \"hello\"))")
+       (beginning-of-line)
+       (forward-char 4)
+       (undo-boundary)
+       (eval-defun nil)
+       (undo-boundary)
+       (undo)
+       (point)))))
 
 (provide 'simple-test)
 ;;; simple-test.el ends here
-- 
2.9.0

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Stefan Monnier]

>> BTW, I notice that in the current code (emacs-25), there's one other
>> call to record_first_change (in record_property_change), and it could be
>> replaced with a call to prepare_record, so it begs the question whether
>> the above hunk should also apply to record_property_change as well.

> Don't understand.

In record_property_change we have (among other things) the exact same
code as in prepare_record (i.e. there's code duplication).  So we
could/should replace those 4 lines of code with a call to
prepare_record.  But if we do, then your (previous) patch changes the
behavior of record_property_change, whereas if we don't, your (previous)
patch doesn't change its behavior.
Anyway, your new patch addresses this.

> That's a possibility, but it appears more complex than re-ordering the
> methods.
[...]
> However, we must bring back the conditional statement because this is no
> longer called in prepare_record, as in this case, it must be called after the
> `at_boundary` check. The alternative, which is making the `at_boundary` check
> accept "nil" then a timestamp as a boundary sounds more complex.

More complex but more robust.  I think it'd be worthwhile to put a FIXME
comment in there, at least.  E.g. the above explanation should be put
inside the code.

> +  /* If we are just after an undo boundary, and point was not at start
> +     of deleted range, and the recorded point was in this buffer, then
> +     record where it was.  */
> +  if (at_boundary
> +      && point_before_last_command_or_undo != beg
> +      && buffer_before_last_command_or_undo == current_buffer )
>      bset_undo_list (current_buffer,
> -		    Fcons (make_number (pt),
> +		    Fcons (make_number (point_before_last_command_or_undo),
>  			   BVAR (current_buffer, undo_list)));
>  }

I think the comment should explain the intention better.
It is currently too close to a simple paraphrase of the code.
I suggest "If it's the first change since the last boundary, and the
upcoming undo record wouldn't restore point correctly, then record where
it was".

Other than that it looks good, thank you for the detailed explanation.


        Stefan

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

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

>>> BTW, I notice that in the current code (emacs-25), there's one other
>>> call to record_first_change (in record_property_change), and it could be
>>> replaced with a call to prepare_record, so it begs the question whether
>>> the above hunk should also apply to record_property_change as well.
>
>> Don't understand.
>
> In record_property_change we have (among other things) the exact same
> code as in prepare_record (i.e. there's code duplication).  So we
> could/should replace those 4 lines of code with a call to
> prepare_record.  But if we do, then your (previous) patch changes the
> behavior of record_property_change, whereas if we don't, your (previous)
> patch doesn't change its behavior.
> Anyway, your new patch addresses this.

Good!

>
> More complex but more robust.  I think it'd be worthwhile to put a FIXME
> comment in there, at least.  E.g. the above explanation should be put
> inside the code.

Done.

> I think the comment should explain the intention better.
> It is currently too close to a simple paraphrase of the code.

Yeah, I tell my students off for doing that.

> I suggest "If it's the first change since the last boundary, and the
> upcoming undo record wouldn't restore point correctly, then record where
> it was".

I've updated it (with a something a bit longer).


> Other than that it looks good, thank you for the detailed explanation.

I have pushed this to emacs-25, since Eli was happy with the last
version, and you're happy with this.

Phil

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Eli Zaretskii]

> Date: Mon, 04 Jul 2016 17:38:52 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 23871@debbugs.gnu.org, phillip.lord@russet.org.uk
> 
> > From: Markus Triska <triska@metalevel.at>
> > Cc: Eli Zaretskii <eliz@gnu.org>,  23871@debbugs.gnu.org
> > Date: Mon, 04 Jul 2016 00:03:31 +0200
> > 
> > OK, I have constructed a shell script that simulates SWI-Prolog for this
> > specific case, please download it from:
> > 
> >     https://www.metalevel.at/ei/simulans.sh
> > 
> > Place it in the current directory and change the invocation of Emacs to:
> > 
> >    emacs -Q ceiled.pl -fn "Bitstream Vera Sans Mono 15"   --eval \
> >         "(progn (load \"$PWD/ediprolog.el\") \
> >          (setq ediprolog-program \"$PWD/simulans.sh\"))"
> > 
> > Note the setting of ediprolog-program to simulans.sh. I can reproduce
> > the issue with this script. If it does not work for you, please try a
> > different window size, font, or running Emacs in the terminal.
> 
> Thanks, I reproduced the problem, and it definitely involves some
> anomaly in redisplay.  I will look into that aspect of the problem.

This is now fixed on the master branch.  The situation where buffer
changes happen in a window whose start point is on a continuation line
was mishandled by code that never expected to find itself in this
situation.  After the fix, the recipe leaves point at the end of line
15, in a window that displays that single line at its top.

Thanks for the easy to reproduce recipe.

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

On Tue, July 5, 2016 5:36 pm, Eli Zaretskii wrote:

>>> Note the setting of ediprolog-program to simulans.sh. I can reproduce
>>>  the issue with this script. If it does not work for you, please try
>>> a different window size, font, or running Emacs in the terminal.
>>
>> Thanks, I reproduced the problem, and it definitely involves some
>> anomaly in redisplay.  I will look into that aspect of the problem.
>
> This is now fixed on the master branch.  The situation where buffer
> changes happen in a window whose start point is on a continuation line was
> mishandled by code that never expected to find itself in this situation.
> After the fix, the recipe leaves point at the end of line
> 15, in a window that displays that single line at its top.
>
>
> Thanks for the easy to reproduce recipe.

Also, thanks for helping to uncover issues in the undo system, with some
very nice bug reports.

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

Eli Zaretskii <eliz@gnu.org> writes:

> This is now fixed on the master branch.  The situation where buffer
> changes happen in a window whose start point is on a continuation line
> was mishandled by code that never expected to find itself in this
> situation.  After the fix, the recipe leaves point at the end of line
> 15, in a window that displays that single line at its top.

Thank you very much for this great improvement! I really appreciate it!

I have run a few more test cases I had collected with similar issues,
and they all run much better now. In some cases, I would prefer if the
window displayed said line at its center (instead of at its top). That's
a very minor issue though and I am very glad with the current behaviour!

Thank you and all the best,
Markus

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Eli Zaretskii]

> From: Markus Triska <triska@metalevel.at>
> Cc: 23871@debbugs.gnu.org,  phillip.lord@russet.org.uk
> Date: Tue, 05 Jul 2016 21:47:51 +0200
> 
> In some cases, I would prefer if the window displayed said line at
> its center (instead of at its top).

I thought about this, and doing so is very problematic.  Many of these
cases are when a large change happens in a continuation line which
originally contained the window-start.  When such a change happens,
the display engine cannot know whether the result will be a continued
line or a short line that doesn't need to be continued.  The current
code plays it safe and assumes it will still be continued, and in that
case you don't want the recentering, because it would be unexpected.

Detecting the special case where the remaining line is short enough to
nod need continuation would need significantly more code, with special
cases for invisible text, images, large fonts etc., all of which could
take a line that has just a few character positions and make it need
continuation lines.  So I decided this minor issue isn't worth the
hassle.

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

"Phillip Lord" <phillip.lord@russet.org.uk> writes:

> Also, thanks for helping to uncover issues in the undo system, with some
> very nice bug reports.

Thank you Phillip for your great work on this! I really greatly
appreciate your effort because these are some subtle details that are
hard to pin down yet extremely nice to have if they work reliably.

Thank you and all the best,
Markus

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

Hi Phillip,

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

> I have pushed this to emacs-25, since Eli was happy with the last
> version, and you're happy with this.

One issue I noticed with the current undo system is that it behaves
differently if the command that inserts text is invoked via a keyboard
shortcut instead of via M-x ... RET.

For example, in the recipe I gave, if you invoke Emacs via:

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

(note the use of F9 as a shortcut for ediprolog-dwim), and then press F9
instead of M-x ediprolog-dwim RET to invoke the query, then, after undo,
point is placed at the *beginning* of line 15 instead of at the end.

In fact, I find the former behaviour desirable since it puts point
exactly where it was before the whole insertion started, and I hope that
this can also be adopted if the command is invoked with M-x ... RET.

All the best,
Markus

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Stefan Monnier]

> One issue I noticed with the current undo system is that it behaves
> differently if the command that inserts text is invoked via a keyboard
> shortcut instead of via M-x ... RET.

This is probably because of the

    buffer_before_last_command_or_undo == current_buffer

test, which is a naive/conservative test that just punts if there's
a buffer switch (in which case point_before_last_command_or_undo is
simply meaningless).  And in this case there is, since the last command
was in another buffer (the last command was the RET you executed in the
minibuffer).

We could probably make it work by saving&restoring
buffer_before_last_command_or_undo and point_before_last_command_or_undo
around the minibuffer thingy.  Or, making
point_before_last_command_or_undo into a buffer-local variable and get
rid of buffer_before_last_command_or_undo.


        Stefan

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

Markus Triska <triska@metalevel.at> writes:

>> I have pushed this to emacs-25, since Eli was happy with the last
>> version, and you're happy with this.
>
> One issue I noticed with the current undo system is that it behaves
> differently if the command that inserts text is invoked via a keyboard
> shortcut instead of via M-x ... RET.
>
> For example, in the recipe I gave, if you invoke Emacs via:
>
>    $ emacs -Q ceiled.pl -fn "Bitstream Vera Sans Mono 15" \
>        --eval "(progn (load \"$PWD/ediprolog.el\") \
>                       (global-set-key [f9] 'ediprolog-dwim))"
>
> (note the use of F9 as a shortcut for ediprolog-dwim), and then press F9
> instead of M-x ediprolog-dwim RET to invoke the query, then, after undo,
> point is placed at the *beginning* of line 15 instead of at the end.
>
> In fact, I find the former behaviour desirable since it puts point
> exactly where it was before the whole insertion started, and I hope that
> this can also be adopted if the command is invoked with M-x ... RET.

I'm amazed at how you spot all of these!

Can we start a new bug for this? We've been discussing about 4 different
bugs on this thread, and IIUC, Eli's fixed this one.

Phil

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

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

>> One issue I noticed with the current undo system is that it behaves
>> differently if the command that inserts text is invoked via a keyboard
>> shortcut instead of via M-x ... RET.
>
> This is probably because of the
>
>     buffer_before_last_command_or_undo == current_buffer
>
> test, which is a naive/conservative test that just punts if there's
> a buffer switch (in which case point_before_last_command_or_undo is
> simply meaningless).  And in this case there is, since the last command
> was in another buffer (the last command was the RET you executed in the
> minibuffer).

It's going to be the buffer switch for sure, and yes, I'd guess it's
this part. Bit surprised this is a regression though. This check is
different from before, but the old system also used a global buffer
which should have been affected by the minibuffer changes.


> We could probably make it work by saving&restoring
> buffer_before_last_command_or_undo and
> point_before_last_command_or_undo around the minibuffer thingy.

Or just not set buffer_before_last_command_or_undo to the minibuffer.

> Or, making point_before_last_command_or_undo into a buffer-local
> variable and get rid of buffer_before_last_command_or_undo.

This seems the nicest to me. It should also cope with command completion
frameworks for like helm which use buffers as well as the minibuffer.

Phil

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

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

> I'm amazed at how you spot all of these!

Thanks to your extremely nice work on these issues, I am encouraged to
file more of them. I am very glad that these issues are being corrected!

I have found more issues with undo for which I don't have reproducible
cases yet, and I can tell you more about them if you are interested.
They may depend on the exact timing of process output and user input.

> Can we start a new bug for this? We've been discussing about 4 different
> bugs on this thread, and IIUC, Eli's fixed this one.

I have filed #23903 for this, let us continue the discussion there.
It's also great that you are including these snippets as actual test
cases that are run automatically!

Thank you and all the best,
Markus

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Phillip Lord]

Markus Triska <triska@metalevel.at> writes:

> phillip.lord@russet.org.uk (Phillip Lord) writes:
>
>> I'm amazed at how you spot all of these!
>
> Thanks to your extremely nice work on these issues, I am encouraged to
> file more of them. I am very glad that these issues are being corrected!
>
> I have found more issues with undo for which I don't have reproducible
> cases yet, and I can tell you more about them if you are interested.
> They may depend on the exact timing of process output and user input.
>
>> Can we start a new bug for this? We've been discussing about 4 different
>> bugs on this thread, and IIUC, Eli's fixed this one.
>
> I have filed #23903 for this, let us continue the discussion there.
> It's also great that you are including these snippets as actual test
> cases that are run automatically!


Yeah, it's not the best form of testing, but it works whether nothing
else does, and quite a few of your bugs have been subtle. While I like
these, I need to try and make some simpler more unit test like ones as
well. These functional tests are good for telling you something is
wrong, but not good for telling you what is wrong.

Please feel free to submit bugs in the same form.

Phil

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

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

> Please feel free to submit bugs in the same form.

Great, I have now filed #23906 which is also related to undo. I hope you
can reproduce it.

All the best!
Markus

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[npostavs]

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

> Can we start a new bug for this? We've been discussing about 4 different
> bugs on this thread, and IIUC, Eli's fixed this one.

So should this bug be closed now?  It's difficult to tell from reading
the thread.

bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer

[Markus Triska]

npostavs@users.sourceforge.net writes:

> So should this bug be closed now?

Yes, Eli and Phillip fixed this issue, thank you very much!

#23903 describes the mentioned remaining issue, and still applies.

All the best,
Markus