ed_undo refactor: split/remove `ed_undo_step_impl`.

This function was doing too many things, with behaviors fairly different
depending on its input parameters. This was making the code fragile and
hard to follow.

Split it in three:
* `ed_undo_step_pre` does the common actions before we actually undo
  data.
* `ed_undo_step_post` does the common actions after we have undone/redone
  data.

Then, `ed_undo_step_direction`, `ed_undo_step_by_name` and
`ed_undo_step_by_index` do their actual specific actions, with their own
logic.

Note: Since the actual behavior of those three funtions is fairly
different (the first only undo/redo one effective step, the second is only
supposed to **undo** //before// given named step, and the third actually
undo/redo until given indexed step become active), we could also find
better names for those. right now, it sounds like they are doing the
same thing, with just different ways to specify the target step.

Note: This is part of on-going refactor work on undo system, see T83806.

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

1 files changed, 125 insertions, 94 deletions

diff --git a/source/blender/editors/undo/ed_undo.c b/source/blender/editors/undo/ed_undo.c
index 723e92b9d09..883c13f20db 100644
--- a/source/blender/editors/undo/ed_undo.c
+++ b/source/blender/editors/undo/ed_undo.c
@@ -77,8 +77,6 @@ static CLG_LogRef LOG = {"ed.undo"};
 enum eUndoStepDir {
   STEP_REDO = 1,
   STEP_UNDO = -1,
-  /** Only used when the undo step name or index is passed to #ed_undo_step_impl. */
-  STEP_NONE = 0,
 };
 
 /* -------------------------------------------------------------------- */
@@ -180,19 +178,16 @@ void ED_undo_push(bContext *C, const char *str)
 }
 
 /**
- * \note Also check #undo_history_exec in bottom if you change notifiers.
+ * Common pre management of undo/redo (killing all running jobs, calling pre handlers, etc.).
  */
-static int ed_undo_step_impl(bContext *C,
-                             enum eUndoStepDir step,
-                             const char *undo_name,
-                             const int undo_index,
+static void ed_undo_step_pre(bContext *C,
+                             wmWindowManager *wm,
+                             const enum eUndoStepDir undo_dir,
                              ReportList *reports)
 {
-  /* Mutually exclusives, ensure correct input. */
-  BLI_assert(((undo_name || undo_index != -1) && (step == STEP_NONE)) ||
-             (!(undo_name || undo_index != -1) && (step != STEP_NONE)));
-  CLOG_INFO(&LOG, 1, "name='%s', index=%d, step=%d", undo_name, undo_index, step);
-  wmWindowManager *wm = CTX_wm_manager(C);
+  BLI_assert(ELEM(undo_dir, STEP_UNDO, STEP_REDO));
+
+  Main *bmain = CTX_data_main(C);
   Scene *scene = CTX_data_scene(C);
   ScrArea *area = CTX_wm_area(C);
 
@@ -201,22 +196,12 @@ static int ed_undo_step_impl(bContext *C,
   WM_jobs_kill_all(wm);
 
   if (G.debug & G_DEBUG_IO) {
-    Main *bmain = CTX_data_main(C);
     if (bmain->lock != NULL) {
       BKE_report(reports, RPT_INFO, "Checking sanity of current .blend file *BEFORE* undo step");
       BLO_main_validate_libraries(bmain, reports);
     }
   }
 
-  /* TODO(campbell): undo_system: use undo system */
-  /* grease pencil can be can be used in plenty of spaces, so check it first */
-  /* FIXME: This gpencil undo effectively only supports the one step undo/redo, undo based on name
-   * or index is fully not implemented.
-   * FIXME: However, it seems to never be used in current code (`ED_gpencil_session_active` seems
-   * to always return false). */
-  if (ED_gpencil_session_active()) {
-    return ED_undo_gpencil_step(C, (int)step);
-  }
   if (area && (area->spacetype == SPACE_VIEW3D)) {
     Object *obact = CTX_data_active_object(C);
     if (obact && (obact->type == OB_GPENCIL)) {
@@ -224,89 +209,40 @@ static int ed_undo_step_impl(bContext *C,
     }
   }
 
-  UndoStep *step_data_from_name = NULL;
-  enum eUndoStepDir step_for_callback = step;
-  if (undo_name != NULL) {
-    step_data_from_name = BKE_undosys_step_find_by_name(wm->undo_stack, undo_name);
-    if (step_data_from_name == NULL) {
-      return OPERATOR_CANCELLED;
-    }
-
-    /* TODO(campbell), could use simple optimization. */
-    /* Pointers match on redo. */
-    step_for_callback = (BLI_findindex(&wm->undo_stack->steps, step_data_from_name) <
-                         BLI_findindex(&wm->undo_stack->steps, wm->undo_stack->step_active)) ?
-                            STEP_UNDO :
-                            STEP_REDO;
-  }
-  else if (undo_index != -1) {
-    step_for_callback = (undo_index <
-                         BLI_findindex(&wm->undo_stack->steps, wm->undo_stack->step_active)) ?
-                            STEP_UNDO :
-                            STEP_REDO;
-  }
-
   /* App-Handlers (pre). */
   {
     /* Note: ignore grease pencil for now. */
-    Main *bmain = CTX_data_main(C);
     wm->op_undo_depth++;
     BKE_callback_exec_id(
-        bmain, &scene->id, (step_for_callback == STEP_UNDO) ? BKE_CB_EVT_UNDO_PRE : BKE_CB_EVT_REDO_PRE);
+        bmain, &scene->id, (undo_dir == STEP_UNDO) ? BKE_CB_EVT_UNDO_PRE : BKE_CB_EVT_REDO_PRE);
     wm->op_undo_depth--;
   }
+}
 
-  /* Undo System */
-  {
-    if (undo_name) {
-      BKE_undosys_step_undo_with_data(wm->undo_stack, C, step_data_from_name);
-    }
-    else if (undo_index != -1) {
-      BKE_undosys_step_undo_from_index(wm->undo_stack, C, undo_index);
-    }
-    else {
-      if (step == STEP_UNDO) {
-        BKE_undosys_step_undo(wm->undo_stack, C);
-      }
-      else {
-        BKE_undosys_step_redo(wm->undo_stack, C);
-      }
-    }
+/**
+ * Common post management of undo/redo (calling post handlers, adding notifiers etc.).
+ *
+ * \note Also check #undo_history_exec in bottom if you change notifiers.
+ */
+static void ed_undo_step_post(bContext *C,
+                              wmWindowManager *wm,
+                              const enum eUndoStepDir undo_dir,
+                              ReportList *reports)
+{
+  BLI_assert(ELEM(undo_dir, STEP_UNDO, STEP_REDO));
 
-    /* Set special modes for grease pencil */
-    if (area && (area->spacetype == SPACE_VIEW3D)) {
-      Object *obact = CTX_data_active_object(C);
-      if (obact && (obact->type == OB_GPENCIL)) {
-        /* set cursor */
-        if (ELEM(obact->mode,
-                 OB_MODE_PAINT_GPENCIL,
-                 OB_MODE_SCULPT_GPENCIL,
-                 OB_MODE_WEIGHT_GPENCIL,
-                 OB_MODE_VERTEX_GPENCIL)) {
-          ED_gpencil_toggle_brush_cursor(C, true, NULL);
-        }
-        else {
-          ED_gpencil_toggle_brush_cursor(C, false, NULL);
-        }
-        /* set workspace mode */
-        Base *basact = CTX_data_active_base(C);
-        ED_object_base_activate(C, basact);
-      }
-    }
-  }
+  Main *bmain = CTX_data_main(C);
+  Scene *scene = CTX_data_scene(C);
 
   /* App-Handlers (post). */
   {
-    Main *bmain = CTX_data_main(C);
-    scene = CTX_data_scene(C);
     wm->op_undo_depth++;
     BKE_callback_exec_id(
-        bmain, &scene->id, (step_for_callback == STEP_UNDO) ? BKE_CB_EVT_UNDO_POST : BKE_CB_EVT_REDO_POST);
+        bmain, &scene->id, (undo_dir == STEP_UNDO) ? BKE_CB_EVT_UNDO_POST : BKE_CB_EVT_REDO_POST);
     wm->op_undo_depth--;
   }
 
   if (G.debug & G_DEBUG_IO) {
-    Main *bmain = CTX_data_main(C);
     if (bmain->lock != NULL) {
       BKE_report(reports, RPT_INFO, "Checking sanity of current .blend file *AFTER* undo step");
       BLO_main_validate_libraries(bmain, reports);
@@ -317,30 +253,125 @@ static int ed_undo_step_impl(bContext *C,
   WM_event_add_notifier(C, NC_WM | ND_UNDO, NULL);
 
   WM_toolsystem_refresh_active(C);
-
-  Main *bmain = CTX_data_main(C);
   WM_toolsystem_refresh_screen_all(bmain);
 
   if (CLOG_CHECK(&LOG, 1)) {
     BKE_undosys_print(wm->undo_stack);
   }
-
-  return OPERATOR_FINISHED;
 }
 
+/** Undo or redo one step from current active one.
+ *  May undo or redo several steps at once only if the target step is a 'skipped' one.
+ *  The target step will be the one immediately before or after the active one. */
 static int ed_undo_step_direction(bContext *C, enum eUndoStepDir step, ReportList *reports)
 {
-  return ed_undo_step_impl(C, step, NULL, -1, reports);
+  BLI_assert(ELEM(step, STEP_UNDO, STEP_REDO));
+
+  CLOG_INFO(&LOG, 1, "direction=%s", (step == STEP_UNDO) ? "STEP_UNDO" : "STEP_REDO");
+
+  /* TODO(campbell): undo_system: use undo system */
+  /* grease pencil can be can be used in plenty of spaces, so check it first */
+  /* FIXME: This gpencil undo effectively only supports the one step undo/redo, undo based on name
+   * or index is fully not implemented.
+   * FIXME: However, it seems to never be used in current code (`ED_gpencil_session_active` seems
+   * to always return false). */
+  if (ED_gpencil_session_active()) {
+    return ED_undo_gpencil_step(C, (int)step);
+  }
+
+  wmWindowManager *wm = CTX_wm_manager(C);
+
+  ed_undo_step_pre(C, wm, step, reports);
+
+  if (step == STEP_UNDO) {
+    BKE_undosys_step_undo(wm->undo_stack, C);
+  }
+  else {
+    BKE_undosys_step_redo(wm->undo_stack, C);
+  }
+
+  ed_undo_step_post(C, wm, step, reports);
+
+  return OPERATOR_FINISHED;
 }
 
+/** Undo the step matching given name.
+ *  May undo several steps at once.
+ *  The target step will be the one immediately before given named one.
+ *  Only supposed to undo (will assert in case given named step is after current active one). */
 static int ed_undo_step_by_name(bContext *C, const char *undo_name, ReportList *reports)
 {
-  return ed_undo_step_impl(C, STEP_NONE, undo_name, -1, reports);
+  BLI_assert(undo_name != NULL);
+
+  /* FIXME: See comments in `ed_undo_step_direction`. */
+  if (ED_gpencil_session_active()) {
+    BLI_assert(!"Not implemented currently.");
+  }
+
+  wmWindowManager *wm = CTX_wm_manager(C);
+  UndoStep *undo_step_from_name = BKE_undosys_step_find_by_name(wm->undo_stack, undo_name);
+  if (undo_step_from_name == NULL) {
+    CLOG_ERROR(&LOG, "Step name='%s' not found in current undo stack", undo_name);
+
+    return OPERATOR_CANCELLED;
+  }
+
+  /* TODO(campbell), could use simple optimization. */
+  /* Pointers match on redo. */
+  const int target_step_index = BLI_findindex(&wm->undo_stack->steps, undo_step_from_name);
+  const int active_step_index = BLI_findindex(&wm->undo_stack->steps, wm->undo_stack->step_active);
+  const enum eUndoStepDir undo_dir = (target_step_index < active_step_index) ? STEP_UNDO :
+                                                                               STEP_REDO;
+
+  CLOG_INFO(&LOG,
+            1,
+            "name='%s', found direction=%s, index=%d",
+            undo_name,
+            (undo_dir == STEP_UNDO) ? "STEP_UNDO" : "STEP_REDO",
+            target_step_index);
+
+  /* This function is currently not supposed to redo ever.
+   * TODO: Will be fixed in future in continuing undo code refactor effort. */
+  BLI_assert(undo_dir == STEP_UNDO);
+
+  ed_undo_step_pre(C, wm, undo_dir, reports);
+
+  BKE_undosys_step_undo_with_data(wm->undo_stack, C, undo_step_from_name);
+
+  ed_undo_step_post(C, wm, undo_dir, reports);
+
+  return OPERATOR_FINISHED;
 }
 
+/** Load the step matching given index in the stack.
+ *  May undo or redo several steps at once.
+ *  The target step will be the one indicated by the given index. */
 static int ed_undo_step_by_index(bContext *C, const int undo_index, ReportList *reports)
 {
-  return ed_undo_step_impl(C, STEP_NONE, NULL, undo_index, reports);
+  BLI_assert(undo_index >= 0);
+
+  /* FIXME: See comments in `ed_undo_step_direction`. */
+  if (ED_gpencil_session_active()) {
+    BLI_assert(!"Not implemented currently.");
+  }
+
+  wmWindowManager *wm = CTX_wm_manager(C);
+  const int active_step_index = BLI_findindex(&wm->undo_stack->steps, wm->undo_stack->step_active);
+  const enum eUndoStepDir undo_dir = (undo_index < active_step_index) ? STEP_UNDO : STEP_REDO;
+
+  CLOG_INFO(&LOG,
+            1,
+            "index='%d', found direction=%s",
+            undo_index,
+            (undo_dir == STEP_UNDO) ? "STEP_UNDO" : "STEP_REDO");
+
+  ed_undo_step_pre(C, wm, undo_dir, reports);
+
+  BKE_undosys_step_undo_from_index(wm->undo_stack, C, undo_index);
+
+  ed_undo_step_post(C, wm, undo_dir, reports);
+
+  return OPERATOR_FINISHED;
 }
 
 void ED_undo_grouped_push(bContext *C, const char *str)