[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Emacs-diffs] fix/undo-point-in-wrong-place 0ab5e01 3/4: Further investi
From: |
Phillip Lord |
Subject: |
[Emacs-diffs] fix/undo-point-in-wrong-place 0ab5e01 3/4: Further investigation. |
Date: |
Fri, 20 Nov 2015 22:13:23 +0000 |
branch: fix/undo-point-in-wrong-place
commit 0ab5e01a090fede6f26d065e756329396895e376
Author: Phillip Lord <address@hidden>
Commit: Phillip Lord <address@hidden>
Further investigation.
---
lisp/simple.el | 18 ++++-
problem.org | 144 +++++++++++++++++++++++++++++++++++------
test/automated/simple-test.el | 16 +++--
3 files changed, 150 insertions(+), 28 deletions(-)
diff --git a/lisp/simple.el b/lisp/simple.el
index deb5c88..24a712a 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2792,6 +2792,14 @@ with < or <= based on USE-<."
;; This section adds a new undo-boundary at either after a command is
;; called or in some cases on a timer called after a change is made in
;; any buffer.
+(defvar undo-test nil)
+
+(defun undo-test-change ()
+ (when undo-test
+ (with-current-buffer
+ (get-buffer-create "*undo-log*")
+ (insert "a"))))
+
(defvar-local undo-auto--last-boundary-cause nil
"Describe the cause of the last undo-boundary.
@@ -2852,6 +2860,7 @@ REASON describes the reason that the boundary is being
added; see
"Check recently changed buffers and add a boundary if necessary.
REASON describes the reason that the boundary is being added; see
`undo-last-boundary' for more information."
+ (undo-test-change)
(dolist (b undo-auto--undoably-changed-buffers)
(when (buffer-live-p b)
(with-current-buffer b
@@ -2881,10 +2890,11 @@ See also `undo-auto--buffer-undoably-changed'.")
(defun undo-auto--add-boundary ()
"Add an `undo-boundary' in appropriate buffers."
(undo-auto--boundaries
- (if undo-auto--this-command-amalgamating
- 'amalgamate
- 'command))
- (setq undo-auto--this-command-amalgamating nil))
+ (let ((amal undo-auto--this-command-amalgamating))
+ (setq undo-auto--this-command-amalgamating nil)
+ (if amal
+ 'amalgamate
+ 'command))))
(defun undo-auto--amalgamate ()
"Amalgamate undo if necessary.
diff --git a/problem.org b/problem.org
index c56428c..63105c5 100644
--- a/problem.org
+++ b/problem.org
@@ -29,7 +29,8 @@ Value: (nil
(#<marker at 39 in *scratch*> . -1)
(#<marker in no buffer> . -4)
(#<marker in no buffer> . -4)
- 43 nil
+ 43
+ nil
(#("buffer" 0 6
(fontified t face font-lock-comment-face))
. 183)
@@ -76,30 +77,30 @@ Value: (nil
* After Change
-
-
** Without Undo
- (nil
+#+begin_src emacs-lisp
+ (nil ;; boundary
(#("want" 0 4
(fontified t face font-lock-comment-face))
- . 39)
- (#<marker at 39 in *scratch*> . -4)
- (#<marker at 39 in *scratch*> . -2)
- (#<marker in no buffer> . -4)
- 183 nil
+ . 39) ;; text that was deleted
+ (#<marker at 39 in *scratch*> . -4) ;; marker was moved
+ (#<marker at 39 in *scratch*> . -2) ;; marker was moved
+ (#<marker in no buffer> . -4) ;; marker was moved
+ 183 ;; position of point
+ nil ;; boundary
(#("buffer" 0 6
(fontified t face font-lock-comment-face))
- . 183)
- (#<marker at 39 in *scratch*> . -6)
- (#<marker at 179 in *scratch*> . -2)
- (#<marker at 179 in *scratch*> . -2)
- (#<marker in no buffer> . -6)
- (t . 0)
- nil
- (1 . 192)
- (t . 0))
-
+ . 183) ;; text that was deleted
+ (#<marker at 39 in *scratch*> . -6) ;; marker moved
+ (#<marker at 179 in *scratch*> . -2) ;; marker moved
+ (#<marker at 179 in *scratch*> . -2) ;; marker moved
+ (#<marker in no buffer> . -6) ;; marker moved
+ (t . 0) ;; buffer become modified
+ nil ;; boundary
+ (1 . 192) ;; text inserted (initial message)
+ (t . 0)) ;; buffer become modified
+#+end_src
** With Undo
@@ -125,3 +126,108 @@ Value: (nil
nil
(1 . 192)
(t . 0))
+
+
+* Investigation
+
+I've added a test case to simple-test.el.
+
+I have tried added an undo-log command. Bizarrely, though the log command
+changes the behaviour.
+
+
+(defvar undo-log nil)
+;;(setq undo-log t)
+
+(defun undo-log (&rest args)
+ (when
+ undo-log
+ (with-current-buffer
+ (get-buffer-create "*undo-log*")
+ (goto-char (point-max))
+ (insert (apply 'format args))
+ (insert "\n"))))
+
+With undo-log set to t, my unit test runs correctly. Otherwise it
+fails. So, something is very wrong!
+
+
+Tested further -- adding to a temp-buffer doesn't have the same
+effect. So this only works because a undoable change has happened in
+*undo-log*.
+
+So, if we move this code up the stack till it stops working, we should
+find the error.
+
+(defvar undo-test nil)
+
+(defun undo-test-change()
+ (when undo-test
+ (with-current-buffer
+ (get-buffer-create "*undo-log*")
+ (insert "a"))))
+
+Test undo-auto--boundaries -- works
+
+
+
+So this code is worrying...
+
+(defun undo-auto--add-boundary ()
+ "Add an `undo-boundary' in appropriate buffers."
+ (undo-auto--boundaries
+ (if undo-auto--this-command-amalgamating
+ 'amalgamate
+ 'command))
+ (setq undo-auto--this-command-amalgamating nil))
+
+If another --add-boundary is signallyed as a result of this command,
+the --this-command-amalgamating will not have been reset to nil, so
+that command may be amagamating also? Perhaps we should set to nil
+before we call
+
+(defun undo-auto--add-boundary ()
+ "Add an `undo-boundary' in appropriate buffers."
+ (undo-auto--boundaries
+ (let ((amal undo-auto--this-command-amalgamating))
+ (setq undo-auto--this-command-amalgamating nil)
+ (if amal
+ 'amalgamate
+ 'command))))
+
+
+Tried that -- nicer perhaps, but makes no difference.
+
+
+** buffer-undo-list
+
+It's this line that is wrong -- the 183 should be 43. So, we have a
+problem with record_point I think.
+
+ (#<marker in no buffer> . -4) ;; marker was moved
+ 183 ;; position of point
+ nil ;; boundary
+
+
+The value comes from last_boundary_position.
+
+This is set in undo-boundary
+
+ last_boundary_position = PT;
+ last_boundary_buffer = current_buffer;
+
+So, if a boundary is added in another buffer this all changes.
+
+ if (at_boundary
+ && current_buffer == last_boundary_buffer
+ && last_boundary_position != pt)
+ bset_undo_list (current_buffer,
+ Fcons (make_number (last_boundary_position),
+ BVAR (current_buffer, undo_list)));
+
+
+Hence the bug. Not quite sure, why the bug fixes things. But, either
+way, we need a new way of storing this data which is not dependent on
+the buffer before the last.
+
+last_boundary_position should be local?
diff --git a/test/automated/simple-test.el b/test/automated/simple-test.el
index 4e581ba..2fd68a1 100644
--- a/test/automated/simple-test.el
+++ b/test/automated/simple-test.el
@@ -260,12 +260,18 @@
(setq buffer-undo-list nil)
(insert "a\nb\n\c\n")
(goto-char (point-max))
- ;; delete c, then a, then undo
+ ;; We use a keyboard macro because it adds undo events in the same
+ ;; way as if a user were involved.
(kmacro-call-macro nil nil nil
- [left backspace left left left
- backspace
- 67108911
- ])
+ [left
+ ;; Delete "c"
+ backspace
+ left left left
+ ;; Delete "a"
+ backspace
+ ;; C-/ or undo
+ 67108911
+ ])
(point)))
(ert-deftest undo-point-in-wrong-place ()