bug-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#17235: Undo in region adjusts past positions incorrectly


From: Barry OReilly
Subject: bug#17235: Undo in region adjusts past positions incorrectly
Date: Sat, 26 Apr 2014 17:00:04 -0400

>> +;; More interesting is how to adjust the "ddd" insertion due to the
>> +;; next undo-delta: (6 . -2). If the reinsertion of "ad" was an undo,
>> +;; it is most sensical that the total "ddd" insertion adjustment be (7
>> +;; . 10) -> (6 . 8) -> (7 . 10). However, if the reinsertion was a
>> +;; normal user edit, then most sensical is: (7 . 10) -> (6 . 8) -> (8
>> +;; . 10). The undo history is ambiguous about which.

> I was not able to understand the above comment.

By "normal user edit", I mean manually typing "ad" again, in which
case, the "ad" is new. If "ad" was reintroduced using undo, then it is
conceptually the same "ad" as before, in which case one might hope the
"ddd" insertion starts at position 7 rather than 8.

By "undo history is ambiguous", I mean buffer-undo-list is.
undo-equiv-table can disambiguate the two, albeit not usefully.

I revised the comments in the hopes it is clearer. Here's the diff of
comments changes since the last patch:

 ;; The adjustment of the (7 . 10) insertion of "ddd" shows an edge
-;; case. Normally an undo-delta of (6 . 2) would cause positions after
-;; 6 to adjust by 2. However, they shouldn't adjust to less than 6, so
-;; (7 . 10) adjusts to (6 . 8) due to this particular undo delta.
+;; case. It is adjusted through the undo-deltas: ((6 . 2) (6
+;; . -2)). Normally an undo-delta of (6 . 2) would cause positions
+;; after 6 to adjust by 2. However, they shouldn't adjust to less than
+;; 6, so (7 . 10) adjusts to (6 . 8) due to the first undo delta.
 ;;
 ;; More interesting is how to adjust the "ddd" insertion due to the
-;; next undo-delta: (6 . -2). If the reinsertion of "ad" was an undo,
-;; it is most sensical that the total "ddd" insertion adjustment be (7
-;; . 10) -> (6 . 8) -> (7 . 10). However, if the reinsertion was a
-;; normal user edit, then most sensical is: (7 . 10) -> (6 . 8) -> (8
-;; . 10). The undo history is ambiguous about which.
+;; next undo-delta: (6 . -2), corresponding to reinsertion of "ad". If
+;; the reinsertion was a manual retyping of "ad", then the total
+;; adjustment should be (7 . 10) -> (6 . 8) -> (8 . 10). However, if
+;; the reinsertion was due to undo, one might expect the first "d"
+;; character would again be a part of the "ddd" text, meaning its
+;; total adjustment would be (7 . 10) -> (6 . 8) -> (7 . 10).
 ;;
 ;; undo-make-selective-list assumes in this situation that "ad" was a
-;; new edit. This means the undo system considers there to be a
-;; deleted "ad" at position 8 of buffer content "ccaabaddd". If the
-;; user undos in region "7 to 9", they could be surprised to get
-;; buffer content: "ccaabadaddd" . This is a FIXME. Bug 16411
-;; describes the possibility of undo elements referencing what they
-;; undid, and so resolving the problematic ambiguity.
+;; new edit, even if it was inserted because of an undo. Consequently,
+;; if the user undos in region "8 to 10" of the "ccaabaddd" buffer,
+;; they could be surprised that it becomes "ccaabad", as though the
+;; first "d" became detached from the original "ddd" insertion. This
+;; quirk is a FIXME.

Whether it's a quirk or bug is debatable. Either way, I think an
approach is to effectively cancel out undos, using ideas described in
bug 16411.

Here's the other substantive changes since the last patch:

@@ -2523,19 +2524,50 @@ list."
      (undo-adjust-pos elt deltas))
     ;; (BEG . END)
     (`(,(and beg (pred integerp)) . ,(and end (pred integerp)))
-     (cons (undo-adjust-pos beg deltas)
-           (undo-adjust-pos end deltas t)))
+     (undo-adjust-beg-end beg end deltas))
     ;; (TEXT . POSITION)
     (`(,(and text (pred stringp)) . ,(and pos (pred integerp)))
-     (cons text (undo-adjust-pos pos deltas)))
+     (cons text (* (if (< pos 0) -1 1)
+                   (undo-adjust-pos (abs pos) deltas))))
     ;; (nil PROPERTY VALUE BEG . END)
     (`(nil . ,(or `(,prop ,val ,beg . ,end) pcase--dontcare))
-     `(nil ,prop ,val ,(undo-adjust-pos beg deltas) . ,(undo-adjust-pos end deltas t)))
+     `(nil ,prop ,val . ,(undo-adjust-beg-end beg end deltas)))
     ;; (apply DELTA START END FUN . ARGS)
     ;; FIXME: (Prior undo in region code didn't implement this.)
     ;; All others return same elt
     (_ elt)))
 
+;; (BEG . END) can adjust to the same positions, commonly when an
+;; insertion was undone and they are out of region, for example:
+;;
+;; buf pos:
+;; 123456789 buffer-undo-list undo-deltas
+;; --------- ---------------- -----------
+;; [...]
+;; abbaa     (2 . 4)          (2 . -2)
+;; aaa       ("bb" . 2)       (2 . 2)
+;; [...]
+;;
+;; "bb" insertion (2 . 4) adjusts to (2 . 2) because of the subsequent
+;; undo. Further adjustments to such an element should be the same as
+;; for (TEXT . POSITION) elements. The options are:
+;;
+;;   1: POSITION adjusts using <= (use-< nil), resulting in behavior
+;;      analogous to marker insertion-type t.
+;;
+;;   2: POSITION adjusts using <, resulting in behavior analogous to
+;;      marker insertion-type nil.
+;;
+;; There was no strong reason to prefer one or the other, except that
+;; the first is more consistent with prior undo in region behavior.
+(defun undo-adjust-beg-end (beg end deltas)
+  "Return cons of adjustments to BEG and END by the undo DELTAS
+list."
+  (let ((adj-beg (undo-adjust-pos beg deltas)))
+    ;; Note: option 2 above would be like (cons (min ...) adj-end)
+    (cons adj-beg
+          (max adj-beg (undo-adjust-pos end deltas t)))))
+
 (defun undo-adjust-pos (pos deltas &optional use-<)
   "Return adjustment of POS by the undo DELTAS list, comparing
 with < or <= based on USE-<."

Attached is the latest revision, which I'll install soon.

Attachment: undo-in-region.2.diff
Description: Text document


reply via email to

[Prev in Thread] Current Thread [Next in Thread]