[Bf-blender-cvs] [d0bb6d372c8] temp-graph-select-changes: Cleanup: Detailed comments on selection helpers & simplify logic

[Julian Eisel]

Commit: d0bb6d372c85137c526b1ddedc19b75c5a9247f7
Author: Julian Eisel
Date:   Fri Nov 15 17:01:15 2019 +0100
Branches: temp-graph-select-changes
https://developer.blender.org/rBd0bb6d372c85137c526b1ddedc19b75c5a9247f7

Cleanup: Detailed comments on selection helpers & simplify logic

===================================================================

M	source/blender/editors/space_graph/graph_select.c
M	source/blender/windowmanager/intern/wm_operator_props.c
M	source/blender/windowmanager/intern/wm_operators.c

===================================================================

diff --git a/source/blender/editors/space_graph/graph_select.c b/source/blender/editors/space_graph/graph_select.c
index 25438ab4b57..9c1d63ef68a 100644
--- a/source/blender/editors/space_graph/graph_select.c
+++ b/source/blender/editors/space_graph/graph_select.c
@@ -1437,6 +1437,8 @@ static int mouse_graph_keys(bAnimContext *ac,
   nvi = find_nearest_fcurve_vert(ac, mval);
 
   if (select_mode != SELECT_REPLACE) {
+    /* The modal execution to delay deselecting other items is only needed for normal click
+     * selection, i.e. for SELECT_REPLACE. */
     wait_to_deselect_others = false;
   }
 
@@ -1604,14 +1606,14 @@ static int graphkeys_mselect_column(bAnimContext *ac,
   selx = nvi->frame;
 
   if (select_mode != SELECT_REPLACE) {
-    wait_to_deselect_others = false;
+    /* Doesn't need to deselect anything -> Pass. */
   }
-
-  if (wait_to_deselect_others && (nvi->bezt->f2 & SELECT)) {
+  else if (wait_to_deselect_others && (nvi->bezt->f2 & SELECT)) {
     run_modal = true;
   }
-  /* if select mode is replace, deselect all keyframes first */
-  else if (select_mode == SELECT_REPLACE) {
+  /* If select mode is replace (and we don't do delayed deselection on mouse release), deselect all
+   * keyframes first. */
+  else {
     /* reset selection mode to add to selection */
     select_mode = SELECT_ADD;
 
@@ -1672,6 +1674,8 @@ static int graphkeys_clickselect_exec(bContext *C, wmOperator *op)
   /* select mode is either replace (deselect all, then add) or add/extend */
   const short selectmode = RNA_boolean_get(op->ptr, "extend") ? SELECT_INVERT : SELECT_REPLACE;
   const bool deselect_all = RNA_boolean_get(op->ptr, "deselect_all");
+  /* See #WM_operator_properties_generic_select() for a detailed description of the how and why of
+   * this. */
   const bool wait_to_deselect_others = RNA_boolean_get(op->ptr, "wait_to_deselect_others");
   int mval[2];
   int ret_val;
diff --git a/source/blender/windowmanager/intern/wm_operator_props.c b/source/blender/windowmanager/intern/wm_operator_props.c
index 21636153904..7743b8856af 100644
--- a/source/blender/windowmanager/intern/wm_operator_props.c
+++ b/source/blender/windowmanager/intern/wm_operator_props.c
@@ -396,8 +396,38 @@ void WM_operator_properties_select_operation_simple(wmOperatorType *ot)
   RNA_def_property_flag(prop, PROP_SKIP_SAVE);
 }
 
+/**
+ * Selecting and tweaking items are overlapping operations. Getting both to work without conflicts
+ * requires special care. See
+ * https://wiki.blender.org/wiki/Human_Interface_Guidelines/Selection#Select-tweaking for the
+ * desired behavior.
+ *
+ * For default click selection (with no modifier keys held), the select operators can do the
+ * following:
+ * * On a select-mouse press on an unselected item, change selection and finish immidiately after.
+ *   This sends an undo push and allows transform to take over should a tweak event be caught now.
+ * * On a select-mouse press on a selected item, don't change selection state, but start modal
+ *   execution of the operator. Idea is that we wait with deselecting other items until we know
+ *   that the intention wasn't to tweak-move (mouse click+drag) all selected items.
+ * * If a tweak is recognized before the release event happens, cancel the operator, so that
+ *   transform can take over and no undo-push is sent.
+ * * If the release event occurs rather than a tweak one, deselect all items but the one under the
+ *   cursor, and finish the modal operator.
+ *
+ * This utility, together with #WM_generic_select_invoke() and #WM_generic_select_modal() should
+ * help getting the wanted behavior to work. Most generic logic should be handled in these, so that
+ * the select operators only have to care for the case dependent handling.
+ *
+ * Every select operator has slightly diferent requirements, e.g. VSE strip selection also needs to
+ * account for handle selection. This should be the baseline behavior though.
+ */
 void WM_operator_properties_generic_select(wmOperatorType *ot)
 {
+  /* On the initial mouse press, this is set by #WM_generic_select_modal() to let the select
+   * operator exec callback know that it should not __yet__ deselect other items when clicking on
+   * an already selected one. Instead should make sure the operator executes modal then (see
+   * #WM_generic_select_modal()), so that the exec callback can be called a second time on the
+   * mouse release event to do this part. */
   PropertyRNA *prop = RNA_def_boolean(
       ot->srna, "wait_to_deselect_others", false, "Wait to Deselect Others", "");
   RNA_def_property_flag(prop, PROP_HIDDEN | PROP_SKIP_SAVE);
diff --git a/source/blender/windowmanager/intern/wm_operators.c b/source/blender/windowmanager/intern/wm_operators.c
index 83551a86211..9c3c433d675 100644
--- a/source/blender/windowmanager/intern/wm_operators.c
+++ b/source/blender/windowmanager/intern/wm_operators.c
@@ -721,6 +721,13 @@ void WM_operator_properties_free(PointerRNA *ptr)
 /** \name Default Operator Callbacks
  * \{ */
 
+/**
+ * Helper to get select and tweak-transform to work conflict free and as desired. See
+ * #WM_operator_properties_generic_select() for details.
+ *
+ * To be used together with #WM_generic_select_invoke() and
+ * #WM_operator_properties_generic_select().
+ */
 int WM_generic_select_modal(bContext *C, wmOperator *op, const wmEvent *event)
 {
   PropertyRNA *wait_to_deselect_prop = RNA_struct_find_property(op->ptr,
@@ -787,6 +794,13 @@ int WM_generic_select_modal(bContext *C, wmOperator *op, const wmEvent *event)
   return OPERATOR_RUNNING_MODAL | OPERATOR_PASS_THROUGH;
 }
 
+/**
+ * Helper to get select and tweak-transform to work conflict free and as desired. See
+ * #WM_operator_properties_generic_select() for details.
+ *
+ * To be used together with #WM_generic_select_modal() and
+ * #WM_operator_properties_generic_select().
+ */
 int WM_generic_select_invoke(bContext *C, wmOperator *op, const wmEvent *event)
 {
   RNA_int_set(op->ptr, "mouse_x", event->mval[0]);