VSE: Add effect/transition UI/code cleanup

Part of the Jan 2025 Code Quality project described in #130975.

This patch aims to improve user-facing messaging when adding effects or
transitions, properly polling them out based on context, and avoiding
unnecessary error messages when possible.

- Rearrange "add Effect Strip" UI in order of required number of
  selected strips to create the effect strip, from 0 -> 1 -> 2
- Properly poll out these operators if not enough non-sound strips (i.e.
  any strips with video content) are selected.
	- Note that this does not require any extra iterations over the
	  entire seqbase.
- Gracefully avoid errors with trying to add effect/transition strips
  when sound strips are part of the selection: for example, when the
  user has selected connected strips.
	- In these cases, it is clear that the user wishes to operate on
	  the strips with video content.
- Refactor `seq_effect_find_selected` to fix bugs and account for all
  cases, removing TODOs in place. Rename it `seq_effect_get_new_inputs`
  to more accurately express its purpose.
- Rename various `last_seq` to `active_strip` to adhere to new conventions
  laid out in #132736
- Update UI tooltips for effect, transition, and fades to make their use
  clearer.

Pull Request: https://projects.blender.org/blender/blender/pulls/132672

scripts/startup/bl_ui/space_sequencer.py

@@ -30,11 +30,15 @@ def _space_view_types(st):
     )
 
 
-def selected_sequences_len(context):
+def selected_strips_count(context):
-    selected_sequences = getattr(context, "selected_sequences", None)
+    selected_strips = getattr(context, "selected_sequences", None)
-    if selected_sequences is None:
+    if selected_strips is None:
-        return 0
+        return 0, 0
-    return len(selected_sequences)
+
+    total_count = len(selected_strips)
+    nonsound_count = sum(1 for strip in selected_strips if strip.type != 'SOUND')
+
+    return total_count, nonsound_count
 
 
 def draw_color_balance(layout, color_balance):
@@ -424,12 +428,13 @@ class SEQUENCER_MT_proxy(Menu):
 
     def draw(self, context):
         layout = self.layout
-
         st = context.space_data
+        _, nonsound = selected_strips_count(context)
+
         col = layout.column()
         col.operator("sequencer.enable_proxies", text="Setup")
         col.operator("sequencer.rebuild_proxy", text="Rebuild")
-        col.enabled = selected_sequences_len(context) >= 1
+        col.enabled = nonsound >= 1
         layout.prop(st, "proxy_render_size", text="")
 
 
@@ -742,13 +747,16 @@ class SEQUENCER_MT_add(Menu):
         layout.operator_context = 'INVOKE_DEFAULT'
         layout.menu("SEQUENCER_MT_add_effect", icon='SHADERFX')
 
+        total, nonsound = selected_strips_count(context)
+
         col = layout.column()
         col.menu("SEQUENCER_MT_add_transitions", icon='ARROW_LEFTRIGHT')
-        col.enabled = selected_sequences_len(context) >= 2
+        # Enable for video transitions or sound crossfade.
+        col.enabled = nonsound == 2 or (nonsound == 0 and total == 2)
 
         col = layout.column()
         col.operator_menu_enum("sequencer.fades_add", "type", text="Fade", icon='IPO_EASE_IN_OUT')
-        col.enabled = selected_sequences_len(context) >= 1
+        col.enabled = total >= 1
 
 
 class SEQUENCER_MT_add_scene(Menu):
@@ -792,22 +800,24 @@ class SEQUENCER_MT_add_transitions(Menu):
     bl_label = "Transition"
 
     def draw(self, context):
+        total, nonsound = selected_strips_count(context)
 
         layout = self.layout
 
         col = layout.column()
-
         col.operator("sequencer.crossfade_sounds", text="Sound Crossfade")
+        col.enabled = (nonsound == 0 and total == 2)
 
-        col.separator()
+        layout.separator()
 
+        col = layout.column()
         col.operator("sequencer.effect_strip_add", text="Cross").type = 'CROSS'
         col.operator("sequencer.effect_strip_add", text="Gamma Cross").type = 'GAMMA_CROSS'
 
         col.separator()
 
         col.operator("sequencer.effect_strip_add", text="Wipe").type = 'WIPE'
-        col.enabled = selected_sequences_len(context) >= 2
+        col.enabled = nonsound == 2
 
 
 class SEQUENCER_MT_add_effect(Menu):
@@ -817,6 +827,23 @@ class SEQUENCER_MT_add_effect(Menu):
 
         layout = self.layout
         layout.operator_context = 'INVOKE_REGION_WIN'
+        _, nonsound = selected_strips_count(context)
+
+        layout.operator("sequencer.effect_strip_add", text="Multicam Selector").type = 'MULTICAM'
+
+        layout.separator()
+
+        col = layout.column()
+        col.operator("sequencer.effect_strip_add", text="Transform").type = 'TRANSFORM'
+        col.operator("sequencer.effect_strip_add", text="Speed Control").type = 'SPEED'
+
+        col.separator()
+
+        col.operator("sequencer.effect_strip_add", text="Glow").type = 'GLOW'
+        col.operator("sequencer.effect_strip_add", text="Gaussian Blur").type = 'GAUSSIAN_BLUR'
+        col.enabled = nonsound == 1
+
+        layout.separator()
 
         col = layout.column()
         col.operator(
@@ -854,23 +881,7 @@ class SEQUENCER_MT_add_effect(Menu):
             text="Color Mix",
             text_ctxt=i18n_contexts.id_sequence,
         ).type = 'COLORMIX'
-        col.enabled = selected_sequences_len(context) >= 2
+        col.enabled = nonsound == 2
-
-        layout.separator()
-
-        layout.operator("sequencer.effect_strip_add", text="Multicam Selector").type = 'MULTICAM'
-
-        layout.separator()
-
-        col = layout.column()
-        col.operator("sequencer.effect_strip_add", text="Transform").type = 'TRANSFORM'
-        col.operator("sequencer.effect_strip_add", text="Speed Control").type = 'SPEED'
-
-        col.separator()
-
-        col.operator("sequencer.effect_strip_add", text="Glow").type = 'GLOW'
-        col.operator("sequencer.effect_strip_add", text="Gaussian Blur").type = 'GAUSSIAN_BLUR'
-        col.enabled = selected_sequences_len(context) != 0
 
 
 class SEQUENCER_MT_strip_transform(Menu):
@@ -1247,23 +1258,22 @@ class SEQUENCER_MT_context_menu(Menu):
 
         if strip:
             strip_type = strip.type
-            selected_sequences_count = selected_sequences_len(context)
+            total, nonsound = selected_strips_count(context)
 
             layout.separator()
             layout.operator_menu_enum("sequencer.strip_modifier_add", "type", text="Add Modifier")
             layout.operator("sequencer.strip_modifier_copy", text="Copy Modifiers to Selection")
 
-            if strip_type != 'SOUND':
+            if total == 2:
-                if selected_sequences_count >= 2:
+                if nonsound == 2:
                     layout.separator()
                     col = layout.column()
                     col.menu("SEQUENCER_MT_add_transitions", text="Add Transition")
-            else:
+                elif nonsound == 0:
-                if selected_sequences_count >= 2:
                     layout.separator()
                     layout.operator("sequencer.crossfade_sounds", text="Crossfade Sounds")
 
-            if selected_sequences_count >= 1:
+            if total >= 1:
                 col = layout.column()
                 col.operator_menu_enum("sequencer.fades_add", "type", text="Fade")
                 layout.operator("sequencer.fades_clear", text="Clear Fade")

source/blender/editors/space_sequencer/sequencer_add.cc

@@ -1467,10 +1467,10 @@ static int sequencer_add_effect_strip_exec(bContext *C, wmOperator *op)
   SeqLoadData load_data;
   load_data_init_from_operator(&load_data, C, op);
   load_data.effect.type = RNA_enum_get(op->ptr, "type");
+  const int num_inputs = SEQ_effect_get_num_inputs(load_data.effect.type);
 
   Strip *seq1, *seq2;
-  if (!strip_effect_find_selected(scene, nullptr, load_data.effect.type, &seq1, &seq2, &error_msg))
+  if (!strip_effect_get_new_inputs(scene, false, num_inputs, &seq1, &seq2, &error_msg)) {
-  {
     BKE_report(op->reports, RPT_ERROR, error_msg);
     return OPERATOR_CANCELLED;
   }
@@ -1537,37 +1537,44 @@ static std::string sequencer_add_effect_strip_get_description(bContext * /*C*/,
 
   switch (type) {
     case STRIP_TYPE_CROSS:
-      return TIP_("Add a crossfade transition to the sequencer");
+      return TIP_("Add a crossfade transition strip for two selected strips with video content");
     case STRIP_TYPE_ADD:
-      return TIP_("Add an add effect strip to the sequencer");
+      return TIP_("Add an add blend mode effect strip for two selected strips with video content");
     case STRIP_TYPE_SUB:
-      return TIP_("Add a subtract effect strip to the sequencer");
+      return TIP_(
+          "Add a subtract blend mode effect strip for two selected strips with video content");
     case STRIP_TYPE_ALPHAOVER:
-      return TIP_("Add an alpha over effect strip to the sequencer");
+      return TIP_(
+          "Add an alpha over blend mode effect strip for two selected strips with video content");
     case STRIP_TYPE_ALPHAUNDER:
-      return TIP_("Add an alpha under effect strip to the sequencer");
+      return TIP_(
+          "Add an alpha under blend mode effect strip for two selected strips with video content");
     case STRIP_TYPE_GAMCROSS:
-      return TIP_("Add a gamma cross transition to the sequencer");
+      return TIP_("Add a gamma cross transition strip for two selected strips with video content");
     case STRIP_TYPE_MUL:
-      return TIP_("Add a multiply effect strip to the sequencer");
+      return TIP_(
+          "Add a multiply blend mode effect strip for two selected strips with video content");
     case STRIP_TYPE_OVERDROP:
-      return TIP_("Add an alpha over drop effect strip to the sequencer");
+      return TIP_(
+          "Add an alpha over drop blend mode effect strip for two selected strips with video "
+          "content");
     case STRIP_TYPE_WIPE:
-      return TIP_("Add a wipe transition to the sequencer");
+      return TIP_("Add a wipe transition strip for two selected strips with video content");
     case STRIP_TYPE_GLOW:
-      return TIP_("Add a glow effect strip to the sequencer");
+      return TIP_("Add a glow effect strip for a single selected strip with video content");
     case STRIP_TYPE_TRANSFORM:
-      return TIP_("Add a transform effect strip to the sequencer");
+      return TIP_("Add a transform effect strip for a single selected strip with video content");
     case STRIP_TYPE_COLOR:
       return TIP_("Add a color strip to the sequencer");
     case STRIP_TYPE_SPEED:
-      return TIP_("Add a speed effect strip to the sequencer");
+      return TIP_("Add a video speed effect strip for a single selected strip with video content");
     case STRIP_TYPE_MULTICAM:
       return TIP_("Add a multicam selector effect strip to the sequencer");
     case STRIP_TYPE_ADJUSTMENT:
       return TIP_("Add an adjustment layer effect strip to the sequencer");
     case STRIP_TYPE_GAUSSIAN_BLUR:
-      return TIP_("Add a gaussian blur effect strip to the sequencer");
+      return TIP_(
+          "Add a gaussian blur effect strip for a single selected strip with video content");
     case STRIP_TYPE_TEXT:
       return TIP_("Add a text strip to the sequencer");
     case STRIP_TYPE_COLORMIX:

source/blender/editors/space_sequencer/sequencer_edit.cc

@@ -1200,111 +1200,91 @@ void SEQUENCER_OT_refresh_all(wmOperatorType *ot)
 /** \name Reassign Inputs Operator
  * \{ */
 
-int strip_effect_find_selected(Scene *scene,
+bool strip_effect_get_new_inputs(Scene *scene,
-                               Strip *activeseq,
+                                 bool ignore_active,
-                               int type,
+                                 int num_inputs,
-                               Strip **r_selseq1,
+                                 Strip **r_seq1,
-                               Strip **r_selseq2,
+                                 Strip **r_seq2,
-                               const char **r_error_str)
+                                 const char **r_error_str)
 {
   Editing *ed = SEQ_editing_get(scene);
   Strip *seq1 = nullptr, *seq2 = nullptr;
 
   *r_error_str = nullptr;
 
-  if (!activeseq) {
+  if (num_inputs == 0) {
-    seq2 = SEQ_select_active_get(scene);
+    *r_seq1 = *r_seq2 = nullptr;
+    return true;
   }
 
-  if (SEQ_effect_get_num_inputs(type) == 0) {
+  blender::VectorSet<Strip *> new_inputs = SEQ_query_selected_strips(ed->seqbasep);
-    *r_selseq1 = *r_selseq2 = nullptr;
+  // Ignore sound strips for now (avoids unnecessary errors when connected strips are
-    return 1;
+  // selected together, and the intent to operate on strips with video content is clear).
+  new_inputs.remove_if([&](Strip *strip) { return strip->type == STRIP_TYPE_SOUND_RAM; });
+
+  if (ignore_active) {
+    // If `ignore_active` is true, this function is being called from the reassign inputs
+    // operator, meaning the active strip must be the effect strip to reassign.
+    Strip *active_strip = SEQ_select_active_get(scene);
+    new_inputs.remove_if([&](Strip *strip) { return strip == active_strip; });
   }
 
-  LISTBASE_FOREACH (Strip *, strip, ed->seqbasep) {
+  if (new_inputs.size() > 2) {
-    if (strip->flag & SELECT) {
+    *r_error_str = N_("Cannot apply effect to more than 2 sequence strips with video content");
-      if (strip->type == STRIP_TYPE_SOUND_RAM) {
+    return false;
-        *r_error_str = N_("Cannot apply effects to audio sequence strips");
+  }
-        return 0;
+
-      }
+  if (num_inputs == 2) {
-      if (!ELEM(strip, activeseq, seq2)) {
+    if (new_inputs.size() != 2) {
-        if (seq2 == nullptr) {
+      *r_error_str = N_("Exactly 2 selected sequence strips with video content are needed");
-          seq2 = strip;
+      return false;
-        }
-        else if (seq1 == nullptr) {
-          seq1 = strip;
-        }
-        else {
-          *r_error_str = N_("Cannot apply effect to more than 2 sequence strips");
-          return 0;
-        }
-      }
     }
+    seq1 = new_inputs[0];
+    seq2 = new_inputs[1];
+  }
+  else if (num_inputs == 1) {
+    if (new_inputs.size() != 1) {
+      *r_error_str = N_("Exactly one selected sequence strip with video content is needed");
+      return false;
+    }
+    seq1 = new_inputs[0];
   }
 
-  switch (SEQ_effect_get_num_inputs(type)) {
+  *r_seq1 = seq1;
-    case 1:
+  *r_seq2 = seq2;
-      if (seq2 == nullptr) {
-        *r_error_str = N_("At least one selected sequence strip is needed");
-        return 0;
-      }
-      if (seq1 == nullptr) {
-        seq1 = seq2;
-      }
-      ATTR_FALLTHROUGH;
-    case 2:
-      if (seq1 == nullptr || seq2 == nullptr) {
-        *r_error_str = N_("2 selected sequence strips are needed");
-        return 0;
-      }
-      break;
-  }
 
-  if (seq1 == nullptr && seq2 == nullptr) {
+  return true;
-    *r_error_str = N_("TODO: in what cases does this happen?");
-    return 0;
-  }
-
-  *r_selseq1 = seq1;
-  *r_selseq2 = seq2;
-
-  /* TODO(Richard): This function needs some refactoring, this is just quick hack for #73828. */
-  if (SEQ_effect_get_num_inputs(type) < 2) {
-    *r_selseq2 = nullptr;
-  }
-
-  return 1;
 }
 
 static int sequencer_reassign_inputs_exec(bContext *C, wmOperator *op)
 {
   Scene *scene = CTX_data_scene(C);
-  Strip *seq1, *seq2, *last_seq = SEQ_select_active_get(scene);
+  Strip *seq1, *seq2;
+  Strip *active_strip = SEQ_select_active_get(scene);
   const char *error_msg;
+  const int num_inputs = SEQ_effect_get_num_inputs(active_strip->type);
 
-  if (SEQ_effect_get_num_inputs(last_seq->type) == 0) {
+  if (num_inputs == 0) {
     BKE_report(op->reports, RPT_ERROR, "Cannot reassign inputs: strip has no inputs");
     return OPERATOR_CANCELLED;
   }
 
-  if (!strip_effect_find_selected(scene, last_seq, last_seq->type, &seq1, &seq2, &error_msg) ||
+  if (!strip_effect_get_new_inputs(scene, true, num_inputs, &seq1, &seq2, &error_msg)) {
-      SEQ_effect_get_num_inputs(last_seq->type) == 0)
-  {
     BKE_report(op->reports, RPT_ERROR, error_msg);
     return OPERATOR_CANCELLED;
   }
   /* Check if reassigning would create recursivity. */
-  if (SEQ_relations_render_loop_check(seq1, last_seq) ||
+  if (SEQ_relations_render_loop_check(seq1, active_strip) ||
-      SEQ_relations_render_loop_check(seq2, last_seq))
+      SEQ_relations_render_loop_check(seq2, active_strip))
   {
     BKE_report(op->reports, RPT_ERROR, "Cannot reassign inputs: recursion detected");
     return OPERATOR_CANCELLED;
   }
 
-  last_seq->seq1 = seq1;
+  active_strip->seq1 = seq1;
-  last_seq->seq2 = seq2;
+  active_strip->seq2 = seq2;
 
-  int old_start = last_seq->start;
+  int old_start = active_strip->start;
 
   /* Force time position update for reassigned effects.
    * TODO(Richard): This is because internally startdisp is still used, due to poor performance of
@@ -1312,8 +1292,8 @@ static int sequencer_reassign_inputs_exec(bContext *C, wmOperator *op)
   SEQ_strip_lookup_invalidate(scene);
   SEQ_time_left_handle_frame_set(scene, seq1, SEQ_time_left_handle_frame_get(scene, seq1));
 
-  SEQ_relations_invalidate_cache_preprocessed(scene, last_seq);
+  SEQ_relations_invalidate_cache_preprocessed(scene, active_strip);
-  SEQ_offset_animdata(scene, last_seq, (last_seq->start - old_start));
+  SEQ_offset_animdata(scene, active_strip, (active_strip->start - old_start));
 
   WM_event_add_notifier(C, NC_SCENE | ND_SEQUENCER, scene);
 
@@ -1326,8 +1306,8 @@ static bool sequencer_effect_poll(bContext *C)
   Editing *ed = SEQ_editing_get(scene);
 
   if (ed) {
-    Strip *last_seq = SEQ_select_active_get(scene);
+    Strip *active_strip = SEQ_select_active_get(scene);
-    if (last_seq && (last_seq->type & STRIP_TYPE_EFFECT)) {
+    if (active_strip && (active_strip->type & STRIP_TYPE_EFFECT)) {
       return true;
     }
   }
@@ -1359,18 +1339,18 @@ void SEQUENCER_OT_reassign_inputs(wmOperatorType *ot)
 static int sequencer_swap_inputs_exec(bContext *C, wmOperator *op)
 {
   Scene *scene = CTX_data_scene(C);
-  Strip *strip, *last_seq = SEQ_select_active_get(scene);
+  Strip *active_strip = SEQ_select_active_get(scene);
 
-  if (last_seq->seq1 == nullptr || last_seq->seq2 == nullptr) {
+  if (active_strip->seq1 == nullptr || active_strip->seq2 == nullptr) {
     BKE_report(op->reports, RPT_ERROR, "No valid inputs to swap");
     return OPERATOR_CANCELLED;
   }
 
-  strip = last_seq->seq1;
+  Strip *strip = active_strip->seq1;
-  last_seq->seq1 = last_seq->seq2;
+  active_strip->seq1 = active_strip->seq2;
-  last_seq->seq2 = strip;
+  active_strip->seq2 = strip;
 
-  SEQ_relations_invalidate_cache_preprocessed(scene, last_seq);
+  SEQ_relations_invalidate_cache_preprocessed(scene, active_strip);
 
   WM_event_add_notifier(C, NC_SCENE | ND_SEQUENCER, scene);
 

source/blender/editors/space_sequencer/sequencer_intern.hh

@@ -177,12 +177,12 @@ void channel_draw_context_init(const bContext *C,
 void strip_rectf(const Scene *scene, const Strip *strip, rctf *r_rect);
 Strip *find_neighboring_sequence(Scene *scene, Strip *test, int lr, int sel);
 void recurs_sel_seq(Strip *strip_meta);
-int strip_effect_find_selected(Scene *scene,
+bool strip_effect_get_new_inputs(Scene *scene,
-                               Strip *activeseq,
+                                 bool ignore_active,
-                               int type,
+                                 int strip_type,
-                               Strip **r_selseq1,
+                                 Strip **r_seq1,
-                               Strip **r_selseq2,
+                                 Strip **r_seq2,
-                               const char **r_error_str);
+                                 const char **r_error_str);
 
 /* Operator helpers. */
 bool sequencer_edit_poll(bContext *C);