[Emacs-diffs] fix/undo-point-in-wrong-place d4b252c 1/2: Working on storing point.

[Phillip Lord]

branch: fix/undo-point-in-wrong-place
commit d4b252c84279fb5c1b9d1431763fa1ea9d011d95
Author: Phillip Lord <address@hidden>
Commit: Phillip Lord <address@hidden>

    Working on storing point.
    
    I think we already have these stored in keyboard.c, so we can probably
    reuse these.
---
 problem.org                   |   56 +++++++++++++++++++++++++++++++++++++++++
 src/undo.c                    |   13 +++++++--
 test/automated/simple-test.el |   21 ++++++++++++++-
 3 files changed, 86 insertions(+), 4 deletions(-)

diff --git a/problem.org b/problem.org
index f75df0c..d4c9aee 100644
--- a/problem.org
+++ b/problem.org
@@ -276,3 +276,59 @@ So, we only actually record point in this way before a 
delete, and
 only from one of the two calls from record_delete
 
 So, we can rewrite record_point into two functions
+
+
+* Stefan
+
+>     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.  */
+
+This comment is invalidated by your change.
+
+> +      record_point (beg + SCHARS (string));
+
+Hmm I thought the sign on sbeg took care of this case already (i.e. the
+record_point should only record something when point was neither at the
+beginning nor at the end of the deleted string).
+
+As for the source of the bug (i.e. what change caused the new behavior):
+in the old code, undo-boundary was called right before every command
+(whether there was a need to push a boundary or not), so contrary to the
+comment in the code, last_boundary_position was actually recording
+"position of point at beginning of the command" rather than "position of
+point last time we inserted a boundary".
+
+So the hunk below should recover the old behavior (well, more or less:
+it wouldn't compile as is, but I hope you get the idea).  But to fix it
+right, we should rename these vars and adjust their comment to better
+reflect the way they're really used.
+
+
+        Stefan
+
+
+diff --git a/src/keyboard.c b/src/keyboard.c
+index 849066c..125091e 100644
+--- a/src/keyboard.c
++++ b/src/keyboard.c
+@@ -1448,6 +1448,8 @@ command_loop_1 (void)
+             /* Ensure that we have added appropriate undo-boundaries as a
+                result of changes from the last command. */
+             call0 (Qundo_auto__add_boundary);
++            last_boundary_position = PT;
++            last_boundary_buffer = current_buffer;
+
+             call1 (Qcommand_execute, Vthis_command);
+
+
+
+But how does this solve the problem where things change in other bufers?
+
+It doesn't -- but that is more complicated, so don't worry.
+
+Are these things not already available here?
+
+
+      prev_buffer = current_buffer;
+      prev_modiff = MODIFF;
+      last_point_position = PT;
diff --git a/src/undo.c b/src/undo.c
index ade47d1..fbaa066 100644
--- a/src/undo.c
+++ b/src/undo.c
@@ -22,6 +22,7 @@ along with GNU Emacs.  If not, see 
<http://www.gnu.org/licenses/>.  */
 
 #include "lisp.h"
 #include "buffer.h"
+#include <stdio.h>
 
 /* The first time a command records something for undo.
    it also allocates the undo-boundary object
@@ -69,10 +70,13 @@ record_point (ptrdiff_t pt)
 
   /* 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
+      && current_buffer == last_boundary_buffer
+      && last_boundary_position != pt){
     bset_undo_list (current_buffer,
                    Fcons (make_number (pt),
                           BVAR (current_buffer, undo_list)));
+  }
 }
 
 /* Record an insertion that just happened or is about to happen,
@@ -162,7 +166,6 @@ record_marker_adjustments (ptrdiff_t from, ptrdiff_t to)
 /* Record that a deletion is about to take place, of the characters in
    STRING, at location BEG.  Optionally record adjustments for markers
    in the region STRING occupies in the current buffer.  */
-
 void
 record_delete (ptrdiff_t beg, Lisp_Object string, bool record_markers)
 {
@@ -171,6 +174,10 @@ record_delete (ptrdiff_t beg, Lisp_Object string, bool 
record_markers)
   if (EQ (BVAR (current_buffer, undo_list), Qt))
     return;
 
+  if (PT != beg ){
+    record_point (PT);
+  }
+
   if (PT == beg + SCHARS (string))
     {
       XSETINT (sbeg, -beg);
@@ -179,7 +186,7 @@ record_delete (ptrdiff_t beg, Lisp_Object string, bool 
record_markers)
   else
     {
       XSETFASTINT (sbeg, beg);
-      record_point (beg + SCHARS (string));
+      prepare_record ();
     }
 
   /* primitive-undo assumes marker adjustments are recorded
diff --git a/test/automated/simple-test.el b/test/automated/simple-test.el
index 2fd68a1..5f035c0 100644
--- a/test/automated/simple-test.el
+++ b/test/automated/simple-test.el
@@ -274,11 +274,30 @@
                         ])
     (point)))
 
+(defun undo-test-point-after-forward-kill ()
+  (with-temp-buffer
+    (switch-to-buffer (current-buffer))
+    (setq buffer-undo-list nil)
+    (insert "kill word forward")
+    ;; Move to word "word".
+    (goto-char 6)
+    (kmacro-call-macro nil nil nil
+                       [
+                        ;; kill-word
+                        C-delete
+                        ;; undo
+                        67108911
+                        ])
+    (point)))
+
 (ert-deftest undo-point-in-wrong-place ()
   (should
    ;; returns 5 with the bug
    (= 2
-      (undo-test-kill-c-a-then-undo))))
+      (undo-test-kill-c-a-then-undo)))
+  (should
+   (= 6
+      (undo-test-point-after-forward-kill))))
 
 
 (provide 'simple-test)