Merge nab-mark-undo (mark undo lifecycle fixes)

Author: sl1200mk2

generic/tkText.c

@@ -5594,7 +5594,10 @@ TextUndoRedoCallback(
 		 */
 		((TkTextUndoSubAtom *) subAtom)->item = NULL;
 	    }
-	    TkTextPushUndoToken(sharedTextPtr, redoInfo.token, redoInfo.byteSize);
+	    if (redoInfo.token) {
+		/* A replay may be a no-op (the mark it acts on is gone). */
+		TkTextPushUndoToken(sharedTextPtr, redoInfo.token, redoInfo.byteSize);
+	    }
 	}
 	if (!isDelete && sharedTextPtr->triggerWatchCmd) {
 	    TriggerWatchUndoRedo(sharedTextPtr, token, subAtom->redo, i == 0, peers, countPeers);

generic/tkText.h

@@ -1694,6 +1694,7 @@ typedef enum {
 #define DELETE_INCLUSIVE	(1 << 3)
 #define DELETE_CLEANUP		(1 << 4)
 #define DELETE_LASTLINE		(1 << 5)
+#define DELETE_RELEASE		(1 << 6)
 
 /*
  * Flags for sorting.

generic/tkTextBTree.c

@@ -1768,7 +1768,14 @@ UndoDeleteDestroy(
 		    assert(segPtr->typePtr->deleteProc);
 
 		    nextPtr = (segPtr->nextPtr && !segPtr->sectionPtr) ? segPtr->nextPtr : NULL;
-		    segPtr->typePtr->deleteProc(sharedTextPtr, segPtr, DELETE_BRANCHES | DELETE_MARKS);
+		    /*
+		     * The chain may hold references to segments still living
+		     * inside the tree (relocated marks): DELETE_RELEASE makes
+		     * the delete procs release the reference instead of
+		     * deleting the segment behind the back of the tree.
+		     */
+		    segPtr->typePtr->deleteProc(sharedTextPtr, segPtr,
+			    DELETE_BRANCHES | DELETE_MARKS | DELETE_RELEASE);
 		    segPtr = nextPtr;
 		}
 	    }
@@ -5296,38 +5303,28 @@ TkBTreeInsertChars(
 
 static void
 MakeUndoIndex(
-    const TkSharedText *sharedTextPtr,
+    TCL_UNUSED(const TkSharedText *),
     const TkTextIndex *indexPtr,	/* Convert this index. */
     TkTextUndoIndex *undoIndexPtr,	/* Pointer to resulting index. */
-    int gravity)			/* +1 = right gravity, -1 = left gravity */
+    TCL_UNUSED(int))			/* +1 = right gravity, -1 = left gravity */
 {
-    TkTextSegment *segPtr;
-
     assert(indexPtr);
-    assert(gravity == GRAVITY_LEFT || gravity == GRAVITY_RIGHT);
 
     /*
-     * At first, try to find a neighboring mark segment at the same byte
-     * index, but we cannot use the special marks "insert" and "current",
-     * and we cannot not use private marks.
+     * A neighboring mark used to be recorded as the anchor of this index
+     * ("right behind this mark"), because a mark is stable enough to act
+     * as a predecessor. But the anchor was a bare pointer: nothing kept
+     * the mark alive nor linked, and any interleaving which unlinks it
+     * (deletion of the marks of a range, unset, an homonym superseding
+     * it) left the index dangling - the resolution then answered with a
+     * wrong position, or dereferenced an unlinked segment. The positional
+     * form below is the one the widget uses without steady marks, and it
+     * is valid at replay time as well: the stack is replayed in reverse
+     * order, so the line and byte index recorded here still designate the
+     * same place. Only the order among zero-sized segments sharing that
+     * position is not preserved anymore.
      */
 
-    if (sharedTextPtr->steadyMarks
-	    && (segPtr = TkTextIndexGetSegment(indexPtr))
-	    && segPtr->typePtr->group == SEG_GROUP_MARK) {
-	TkTextSegment *searchPtr = (gravity == GRAVITY_LEFT) ? segPtr->prevPtr : segPtr->nextPtr;
-
-	while (searchPtr && TkTextIsSpecialOrPrivateMark(searchPtr)) {
-	    searchPtr = (gravity == GRAVITY_LEFT) ? searchPtr->prevPtr : searchPtr->nextPtr;
-	}
-
-	if (searchPtr && TkTextIsStableMark(searchPtr)) {
-	    undoIndexPtr->u.markPtr = searchPtr;
-	    undoIndexPtr->lineIndex = -1;
-	    return;
-	}
-    }
-
     undoIndexPtr->lineIndex = TkTextIndexGetLineNumber(indexPtr, NULL);
     undoIndexPtr->u.byteIndex = TkTextIndexGetByteIndex(indexPtr);
 }
@@ -8527,28 +8524,19 @@ DeleteIndexRange(
 	    redoToken->startIndex = undoToken->startIndex;
 	    redoToken->endIndex = undoToken->endIndex;
 	} else {
-	    if (sharedTextPtr->steadyMarks
-		    && segPtr1
-		    && TkTextIsStableMark(segPtr1)
-		    && !(flags & DELETE_MARKS)) {
-		redoToken->startIndex.u.markPtr = segPtr1;
-		redoToken->startIndex.lineIndex = -1;
-	    } else {
-		TkTextIndex index = *indexPtr1;
-		TkTextIndexSetSegment(&index, firstPtr);
-		MakeUndoIndex(sharedTextPtr, &index, &redoToken->startIndex, GRAVITY_LEFT);
-	    }
-	    if (sharedTextPtr->steadyMarks
-		    && segPtr2
-		    && TkTextIsStableMark(segPtr2)
-		    && !(flags & DELETE_MARKS)) {
-		redoToken->endIndex.u.markPtr = segPtr2;
-		redoToken->endIndex.lineIndex = -1;
-	    } else {
-		TkTextIndex index = *indexPtr2;
-		TkTextIndexSetSegment(&index, lastPtr);
-		MakeUndoIndex(sharedTextPtr, &index, &redoToken->endIndex, GRAVITY_RIGHT);
-	    }
+	    TkTextIndex index = *indexPtr1;
+
+	    /*
+	     * The boundary marks were recorded as anchors here, see the
+	     * remarks in MakeUndoIndex: a bare pointer to a mark does not
+	     * survive the interleavings of the undo stack.
+	     */
+
+	    TkTextIndexSetSegment(&index, firstPtr);
+	    MakeUndoIndex(sharedTextPtr, &index, &redoToken->startIndex, GRAVITY_LEFT);
+	    index = *indexPtr2;
+	    TkTextIndexSetSegment(&index, lastPtr);
+	    MakeUndoIndex(sharedTextPtr, &index, &redoToken->endIndex, GRAVITY_RIGHT);
 	}
 	redoInfo->token = (TkTextUndoToken *) redoToken;
 	redoInfo->byteSize = 0;

generic/tkTextMark.c

@@ -289,6 +289,50 @@ UndoToggleGravityInspect(
     return AppendName(UndoToggleGravityGetCommand(sharedTextPtr, item), sharedTextPtr, token->markPtr);
 }
 
+/*
+ *----------------------------------------------------------------------
+ *
+ * ReplayTarget --
+ *
+ *	Find the mark a token has to act on. The referenced mark may have
+ *	been unlinked meanwhile (deleted by [delete -marks], unset, or
+ *	superseded by a living homonym): in this case the living mark of
+ *	the same name is the target, replaying the operation on it - this
+ *	is the same adoption the redo of a [mark set] performs.
+ *
+ * Results:
+ *	The mark to act on, or NULL if the name is not alive anymore; in
+ *	the latter case the token has to be a no-op.
+ *
+ * Side effects:
+ *	None.
+ *
+ *----------------------------------------------------------------------
+ */
+
+static TkTextSegment *
+ReplayTarget(
+    const TkSharedText *sharedTextPtr,	/* Handle to shared text resource. */
+    TkTextSegment *markPtr)		/* The mark referenced by the token. */
+{
+    Tcl_HashEntry *hPtr;
+
+    assert(markPtr);
+    assert(TkTextIsNormalMark(markPtr));
+
+    if (markPtr->sectionPtr) {
+	return markPtr;
+    }
+    if (!IS_PRESERVED(markPtr)) {
+	return NULL;
+    }
+    if (!(hPtr = Tcl_FindHashEntry(&((TkSharedText *) sharedTextPtr)->markTable,
+	    GET_NAME(markPtr)))) {
+	return NULL;
+    }
+    return (TkTextSegment *)Tcl_GetHashValue(hPtr);
+}
+
 static void
 UndoToggleGravityPerform(
     TkSharedText *sharedTextPtr,
@@ -299,12 +343,17 @@ UndoToggleGravityPerform(
     UndoTokenToggleGravity *token = (UndoTokenToggleGravity *) undoInfo->token;
     const Tk_SegType *newTypePtr;
     const Tk_SegType *oldTypePtr;
+    TkTextSegment *markPtr;
 
     assert(!token->markPtr->body.mark.changePtr);
 
-    oldTypePtr = token->markPtr->typePtr;
+    if (!(markPtr = ReplayTarget(sharedTextPtr, token->markPtr))) {
+	return; /* the mark is gone, nothing to toggle */
+    }
+
+    oldTypePtr = markPtr->typePtr;
     newTypePtr = (oldTypePtr == &tkTextRightMarkType) ? &tkTextLeftMarkType : &tkTextRightMarkType;
-    ChangeGravity(sharedTextPtr, NULL, token->markPtr, newTypePtr, NULL);
+    ChangeGravity(sharedTextPtr, NULL, markPtr, newTypePtr, NULL);
 
     if (redoInfo) {
 	redoInfo->token = undoInfo->token;
@@ -335,24 +384,29 @@ UndoMoveMarkPerform(
 {
     UndoTokenMoveMark *token = (UndoTokenMoveMark *) undoInfo->token;
     TkTextUndoIndex index = token->index;
+    TkTextSegment *markPtr;
 
     assert(!token->markPtr->body.mark.changePtr);
 
+    if (!(markPtr = ReplayTarget(sharedTextPtr, token->markPtr))) {
+	return; /* the mark is gone, nothing to move */
+    }
+
     if (redoInfo) {
 	TkTextUndoIndex redoIndex;
 
 	/*
 	 * Don't clobber 'index': the mark must be re-inserted at the saved
 	 * position, whereas the redo token receives the current one.
 	 */
-	TkBTreeMakeUndoIndex(sharedTextPtr, token->markPtr, &redoIndex);
+	TkBTreeMakeUndoIndex(sharedTextPtr, markPtr, &redoIndex);
 	token->index = redoIndex;
 	redoInfo->token = undoInfo->token;
 	redoInfo->token->undoType = isRedo ? &undoTokenMoveMarkType : &redoTokenMoveMarkType;
     }
 
-    TkBTreeUnlinkSegment(sharedTextPtr, token->markPtr);
-    TkBTreeReInsertSegment(sharedTextPtr, &index, token->markPtr);
+    TkBTreeUnlinkSegment(sharedTextPtr, markPtr);
+    TkBTreeReInsertSegment(sharedTextPtr, &index, markPtr);
 }
 
 static void
@@ -412,6 +466,10 @@ UndoSetMarkPerform(
     const UndoTokenSetMark *token = (const UndoTokenSetMark *) undoInfo->token;
     TkTextSegment *markPtr = (TkTextSegment *)GET_POINTER(token->markPtr);
 
+    if (!(markPtr = ReplayTarget(sharedTextPtr, markPtr))) {
+	return; /* the name is gone, nothing to unset */
+    }
+
     assert(!markPtr->body.mark.changePtr);
     UnsetMark(sharedTextPtr, markPtr, redoInfo);
     if (redoInfo && !isRedo) {
@@ -447,8 +505,35 @@ RedoSetMarkPerform(
     assert(TkTextIsNormalMark(markPtr));
 
     if (IS_PRESERVED(markPtr)) {
+	Tcl_HashEntry *hPtr = Tcl_FindHashEntry(&sharedTextPtr->markTable, GET_NAME(markPtr));
+
+	if (hPtr) {
+	    /*
+	     * The name is alive again, undo/redo interleavings can revive an
+	     * homonym (the redo of a deletion does not delete the marks
+	     * again). The mark of the token owns the name back, with its
+	     * recorded gravity and position: unset the homonym, without
+	     * recording - the further undoes revive it through the chain of
+	     * the deletion which preserved it, and its restoration then
+	     * finds the name taken (MarkRestoreProc drops the duplicate).
+	     */
+
+	    TkTextSegment *livingPtr = (TkTextSegment *)Tcl_GetHashValue(hPtr);
+
+	    assert(livingPtr != markPtr);
+	    assert(TkTextIsNormalMark(livingPtr));
+	    UnsetMark(sharedTextPtr, livingPtr, NULL);
+	}
+
 	ReactivateMark(sharedTextPtr, markPtr);
 	sharedTextPtr->numMarks += 1;
+    } else if (markPtr->sectionPtr) {
+	/*
+	 * The mark is alive and linked again (an undone deletion has
+	 * restored it): setting an existing mark is a move, so unlink it
+	 * before it is re-inserted at the recorded position.
+	 */
+	TkBTreeUnlinkSegment(sharedTextPtr, markPtr);
     }
 
     TkBTreeReInsertSegment(sharedTextPtr, &token->index, markPtr);
@@ -1834,6 +1919,24 @@ SetMark(
 	}
 
 	if ((segPtr = TkTextIndexGetSegment(indexPtr)) == markPtr) {
+	    if (typePtr && typePtr != markPtr->typePtr) {
+		/*
+		 * The index resolves to the mark itself (e.g. [mark set m
+		 * end right] with the mark heading the last line, or [mark
+		 * set m m right]), but an explicit direction still has to
+		 * be applied - it used to be skipped on this path.
+		 */
+
+		TkTextUndoInfo undoInfo;
+		TkTextUndoInfo *undoInfoPtr = NULL;
+
+		if (sharedTextPtr->steadyMarks
+			&& TkTextIsNormalMark(markPtr)
+			&& !TkTextUndoUndoStackIsFull(sharedTextPtr->undoStack)) {
+		    undoInfoPtr = &undoInfo;
+		}
+		ChangeGravity(sharedTextPtr, textPtr, markPtr, typePtr, undoInfoPtr);
+	    }
 	    return markPtr;
 	}
 
@@ -2281,6 +2384,18 @@ MarkDeleteProc(
 	return 0;
     }
 
+    if ((flags & DELETE_RELEASE) && segPtr->sectionPtr) {
+	/*
+	 * The caller destroys a token whose chain referenced this mark, but
+	 * the mark is still alive inside the tree: release the reference,
+	 * the mark is not ours to delete - preserving it here would remove
+	 * its hash entry and leave a preserved segment inside the tree.
+	 */
+	assert(segPtr->refCount > 1);
+	segPtr->refCount -= 1;
+	return 1;
+    }
+
     assert(segPtr->body.mark.ptr);
 
     if (segPtr->body.mark.changePtr) {
@@ -2446,7 +2561,15 @@ MarkRestoreProc(
 	}
 
 	hPtr = Tcl_CreateHashEntry(&sharedTextPtr->markTable, GET_NAME(segPtr), &isNew);
-	assert(isNew);
+	if (!isNew) {
+	    /*
+	     * A mark of this name is alive again (undo/redo interleavings
+	     * can revive an homonym): the living one wins, discard the
+	     * preserved duplicate like the branch above does.
+	     */
+	    MarkDeleteProc(sharedTextPtr, segPtr, DELETE_CLEANUP);
+	    return 0;
+	}
 	Tcl_SetHashValue(hPtr, segPtr);
 	sharedTextPtr->numMarks += 1;
 	Tcl_Free(GET_NAME(segPtr));