UndoSys: Refactor step 1: cleanup and simplify logic in main undo/redo handlers.

* Simplify and clarify logic in `BKE_undosys_step_undo/redo_with_data_ex`,
  by adding early return on invalid situations, renaming some variables,
  and adding comments.
* Add more sanity checks in those functions.

No behavioral change are expected here, besides in potential edge-case,
invalid situations.

This is a preliminary change, before some deeper modifications of
`BKE_undosys` undo/redo API.

Differential Revision: https://developer.blender.org/D10033

1 files changed, 153 insertions, 67 deletions

diff --git a/source/blender/blenkernel/intern/undo_system.c b/source/blender/blenkernel/intern/undo_system.c
index e5d2d4a9f0b..e78576206de 100644
--- a/source/blender/blenkernel/intern/undo_system.c
+++ b/source/blender/blenkernel/intern/undo_system.c
@@ -251,10 +251,42 @@ static void undosys_stack_validate(UndoStack *ustack, bool expect_non_empty)
     BLI_assert(!BLI_listbase_is_empty(&ustack->steps));
   }
 }
+
+/* Return whether `us_item` is before (-1), after (1) or same as (0) `us_anchor` step. */
+static int undosys_stack_order(const UndoStack *ustack,
+                               const UndoStep *us_anchor,
+                               const UndoStep *us_item)
+{
+  const int index_anchor = BLI_findindex(&ustack->steps, us_anchor);
+  const int index_item = BLI_findindex(&ustack->steps, us_item);
+  BLI_assert(index_anchor >= 0);
+  BLI_assert(index_item >= 0);
+
+  return (index_item == index_anchor) ? 0 : (index_item < index_anchor) ? -1 : 1;
+}
+
+#  define ASSERT_VALID_UNDO_STEP(_ustack, _us_undo) \
+    { \
+      const UndoStep *_us_anchor = (_ustack)->step_active; \
+      BLI_assert(_us_anchor == NULL || \
+                 (undosys_stack_order((_ustack), _us_anchor, (_us_undo)) <= 0)); \
+    } \
+    (void)0
+
+#  define ASSERT_VALID_REDO_STEP(_ustack, _us_redo) \
+    { \
+      const UndoStep *_us_anchor = (_ustack)->step_active; \
+      BLI_assert(_us_anchor == NULL || \
+                 (undosys_stack_order((_ustack), _us_anchor, (_us_redo)) >= 0)); \
+    } \
+    (void)0
+
 #else
 static void undosys_stack_validate(UndoStack *UNUSED(ustack), bool UNUSED(expect_non_empty))
 {
 }
+#  define ASSERT_VALID_UNDO_STEP(_ustack, _us_undo)
+#  define ASSERT_VALID_REDO_STEP(_ustack, _us_redo)
 #endif
 
 UndoStack *BKE_undosys_stack_create(void)
@@ -676,62 +708,91 @@ bool BKE_undosys_step_undo_with_data_ex(UndoStack *ustack,
 {
   UNDO_NESTED_ASSERT(false);
   if (us == NULL) {
+    CLOG_ERROR(&LOG, "called with a NULL step");
     return false;
   }
   undosys_stack_validate(ustack, true);
 
-  UndoStep *us_prev = us ? us->prev : NULL;
-  if (us) {
-    /* The current state is a copy, we need to load the previous state. */
-    us = us_prev;
+  /* We expect to get next-from-actual-target step here (i.e. active step in case we only undo
+   * once)?
+   * FIXME: this is very confusing now that we may have to undo several steps anyway, this function
+   * should just get the target final step, not assume that it is getting the active one by default
+   * (or the step after the target one when undoing more than one step). */
+  UndoStep *us_target = us->prev;
+  if (us_target == NULL) {
+    CLOG_ERROR(&LOG, "could not find a valid target step");
+    return false;
   }
+  ASSERT_VALID_UNDO_STEP(ustack, us_target);
 
   /* This will be active once complete. */
-  UndoStep *us_active = us_prev;
+  UndoStep *us_active = us_target;
   if (use_skip) {
     while (us_active && us_active->skip) {
       us_active = us_active->prev;
     }
   }
 
-  if ((us != NULL) && (us_active != NULL)) {
-    CLOG_INFO(&LOG, 1, "addr=%p, name='%s', type='%s'", us, us->name, us->type->name);
+  /* This will be the active step once the undo process is complete.
+   *
+   * In case we do skip 'skipped' steps, the final active step may be several steps backward from
+   * the one passed as parameter. */
+  UndoStep *us_target_active = us_target;
+  if (use_skip) {
+    while (us_target_active != NULL && us_target_active->skip) {
+      us_target_active = us_target_active->prev;
+    }
+  }
+  if (us_target_active == NULL) {
+    CLOG_ERROR(&LOG, "could not find a valid final active target step");
+    return false;
+  }
 
-    /* Handle accumulate steps. */
-    if (ustack->step_active) {
-      UndoStep *us_iter = ustack->step_active;
-      while (us_iter != us) {
-        /* TODO:
-         * - skip successive steps that store the same data, eg: memfile steps.
-         * - or steps that include another steps data, eg: a memfile step includes text undo data.
-         */
-        undosys_step_decode(C, G_MAIN, ustack, us_iter, -1, false);
-
-        us_iter = us_iter->prev;
-      }
+  CLOG_INFO(
+      &LOG, 1, "addr=%p, name='%s', type='%s'", us_target, us_target->name, us_target->type->name);
+
+  /* Undo steps until we reach original given target, if we do have a current active step.
+   *
+   * NOTE: Unlike with redo case, where we can expect current active step to fully reflect current
+   * data status, in undo case we also do reload the active step.
+   * FIXME: this feels weak, and should probably not be actually needed? Or should also be done in
+   * redo case? */
+  if (ustack->step_active != NULL) {
+    for (UndoStep *us_iter = ustack->step_active; us_iter != us_target; us_iter = us_iter->prev) {
+      BLI_assert(us_iter != NULL);
+      undosys_step_decode(C, G_MAIN, ustack, us_iter, -1, false);
+      ustack->step_active = us_iter;
     }
+  }
 
-    {
-      UndoStep *us_iter = us_prev;
-      do {
-        const bool is_final = (us_iter == us_active);
-        if (is_final == false) {
-          CLOG_INFO(&LOG,
-                    2,
-                    "undo continue with skip %p '%s', type='%s'",
-                    us_iter,
-                    us_iter->name,
-                    us_iter->type->name);
-        }
-        undosys_step_decode(C, G_MAIN, ustack, us_iter, -1, is_final);
-        ustack->step_active = us_iter;
-      } while ((us_active != us_iter) && (us_iter = us_iter->prev));
+  /* Undo target step, and all potential extra ones if some steps have to be 'skipped'. */
+  for (UndoStep *us_iter = us_target; us_iter != NULL; us_iter = us_iter->prev) {
+    const bool is_final = (us_iter == us_target_active);
+
+    if (!is_final) {
+      BLI_assert(us_iter->skip == true);
+      CLOG_INFO(&LOG,
+                2,
+                "undo continue with skip addr=%p, name='%s', type='%s'",
+                us_iter,
+                us_iter->name,
+                us_iter->type->name);
     }
 
-    return true;
+    undosys_step_decode(C, G_MAIN, ustack, us_iter, -1, is_final);
+    ustack->step_active = us_iter;
+
+    if (is_final) {
+      /* Undo process is finished and successful. */
+      return true;
+    }
   }
+
+  BLI_assert(
+      !"This should never be reached, either undo stack is corrupted, or code above is buggy");
   return false;
 }
+
 bool BKE_undosys_step_undo_with_data(UndoStack *ustack, bContext *C, UndoStep *us)
 {
   return BKE_undosys_step_undo_with_data_ex(ustack, C, us, true);
@@ -756,54 +817,79 @@ bool BKE_undosys_step_redo_with_data_ex(UndoStack *ustack,
 {
   UNDO_NESTED_ASSERT(false);
   if (us == NULL) {
+    CLOG_ERROR(&LOG, "called with a NULL step");
     return false;
   }
   undosys_stack_validate(ustack, true);
 
-  UndoStep *us_next = us ? us->next : NULL;
-  /* Unlike undo accumulate, we always use the next. */
-  us = us_next;
+  /* We expect to get previous-from-actual-target step here (i.e. active step in case we only redo
+   * once)?
+   * FIXME: this is very confusing now that we may have to redo several steps anyway, this function
+   * should just get the target final step, not assume that it is getting the active one by default
+   * (or the step before the target one when redoing more than one step). */
+  UndoStep *us_target = us->next;
+  if (us_target == NULL) {
+    CLOG_ERROR(&LOG, "could not find a valid target step");
+    return false;
+  }
+  ASSERT_VALID_REDO_STEP(ustack, us_target);
 
-  /* This will be active once complete. */
-  UndoStep *us_active = us_next;
+  /* This will be the active step once the redo process is complete.
+   *
+   * In case we do skip 'skipped' steps, the final active step may be several steps forward the one
+   * passed as parameter. */
+  UndoStep *us_target_active = us_target;
   if (use_skip) {
-    while (us_active && us_active->skip) {
-      us_active = us_active->next;
+    while (us_target_active != NULL && us_target_active->skip) {
+      us_target_active = us_target_active->next;
     }
   }
+  if (us_target_active == NULL) {
+    CLOG_ERROR(&LOG, "could not find a valid final active target step");
+    return false;
+  }
 
-  if ((us != NULL) && (us_active != NULL)) {
-    CLOG_INFO(&LOG, 1, "addr=%p, name='%s', type='%s'", us, us->name, us->type->name);
+  CLOG_INFO(
+      &LOG, 1, "addr=%p, name='%s', type='%s'", us_target, us_target->name, us_target->type->name);
 
-    /* Handle accumulate steps. */
-    if (ustack->step_active && ustack->step_active->next) {
-      UndoStep *us_iter = ustack->step_active->next;
-      while (us_iter != us) {
-        undosys_step_decode(C, G_MAIN, ustack, us_iter, 1, false);
-        us_iter = us_iter->next;
-      }
+  /* Redo steps until we reach original given target, if we do have a current active step. */
+  if (ustack->step_active != NULL) {
+    for (UndoStep *us_iter = ustack->step_active->next; us_iter != us_target;
+         us_iter = us_iter->next) {
+      BLI_assert(us_iter != NULL);
+      undosys_step_decode(C, G_MAIN, ustack, us_iter, 1, false);
+      ustack->step_active = us_iter;
     }
+  }
 
-    {
-      UndoStep *us_iter = us_next;
-      do {
-        const bool is_final = (us_iter == us_active);
-        if (is_final == false) {
-          CLOG_INFO(&LOG,
-                    2,
-                    "redo continue with skip %p '%s', type='%s'",
-                    us_iter,
-                    us_iter->name,
-                    us_iter->type->name);
-        }
-        undosys_step_decode(C, G_MAIN, ustack, us_iter, 1, is_final);
-        ustack->step_active = us_iter;
-      } while ((us_active != us_iter) && (us_iter = us_iter->next));
+  /* Redo target step, and all potential extra ones if some steps have to be 'skipped'. */
+  for (UndoStep *us_iter = us_target; us_iter != NULL; us_iter = us_iter->next) {
+    const bool is_final = (us_iter == us_target_active);
+
+    if (!is_final) {
+      BLI_assert(us_iter->skip == true);
+      CLOG_INFO(&LOG,
+                2,
+                "redo continue with skip addr=%p, name='%s', type='%s'",
+                us_iter,
+                us_iter->name,
+                us_iter->type->name);
+    }
+
+    undosys_step_decode(C, G_MAIN, ustack, us_iter, 1, is_final);
+    ustack->step_active = us_iter;
+
+    if (is_final) {
+      /* Redo process is finished and successful. */
+      return true;
     }
-    return true;
   }
+
+  BLI_assert(
+      !"This should never be reached, either undo stack is corrupted, or code above is buggy");
   return false;
 }
+
 bool BKE_undosys_step_redo_with_data(UndoStack *ustack, bContext *C, UndoStep *us)
 {
   return BKE_undosys_step_redo_with_data_ex(ustack, C, us, true);