Page MenuHome

Node Editor: Move All Selected Nodes when dragging
Closed, ResolvedPublic

Tokens
"Yellow Medal" token, awarded by Alrob."Like" token, awarded by EitanSomething."Love" token, awarded by monio."Love" token, awarded by Draise."Love" token, awarded by duarteframos."Like" token, awarded by aliasguru.
Authored By

Description

In the Node Editor, it should be possible to move more than just one node by dragging, if more are selected.
Just like files on a desktop, all selected items should move along when dragging on one of them.

Event Timeline

William Reynish (billreynish) lowered the priority of this task from Needs Triage by Developer to Confirmed, High.Apr 29 2019, 8:14 PM

Problem here is that the 'click' PRESS event will always be caught before the TWEAK one, so selection will always happen before we start translation.

Only simple solution I can see so far is to trigger selection on RELEASE (or CLICK), not on PRESS (tried, seems to work fine, but gives an annoying delay between click and actual selection, especially on laptops with touchpad fancy 'click' handling :( ).

Other possibility, much more radical, would be to 'eat' the click event altogether when we detect a tweak one. Am quiet unsure that is even possible though, at the very least not without a noticeable delay for the user.

Need some thoughts, also probably inputs from @Campbell Barton (campbellbarton) on that one.

Would be interested to first consider where else this logic would apply
(where LMB tweak is used too).

  • Mask points.
  • Paint 'Curve' stroke method.
  • (Possibly Movie-Clip editor in the future?)

If we want this to work as many file-selectors do, we'd need to do as @Bastien Montagne (mont29) suggests and run this on click instead of press.

We could make it feel more responsive we could.

  • Press sets the node active, then either.
    • Click de-selects all others.
    • Tweak moves everything.

... Even this could back-fire (with undo steps and having a click sometimes performing two operations, and the first press event having to pass-through so the click event is registered).

All things considered we're probably best off making select happen on release (exactly like how a file manager works), because supporting press events which pass-through complicates event handling.


Other possibility, much more radical, would be to 'eat' the click event altogether when we detect a tweak one.

Not sure what's meant here., when a tweak event is registered a click event wont be sent.

Testing how it works with something like document files in the Mac Finder:

As you can see, when you click on an item that is already selected, it doesn't deselect other selections. This way they are still able to register the click event immediately (mousedown) and don't have to wait for the mouseup event.

@William Reynish (billreynish) it does not really help to compare a filebrowser with a node editor (and Blender in general), the later is much more complex and allows for many different actions…

I’d agree with @Campbell Barton (campbellbarton) here then, if we want that behavior then we just select on CLICK (or RELEASE) event.

Would not go into handling two different kind of events, this would make code even more tricky to follow, and more fragile too. Unless maybe we had a specific 'set active' operator for the 'PRESS' event, that would not register any undo steps? Still sounds cumbersome for little gain… :/

Btw, just realized that in 3DView at least, we do select on click, and not on PRESS event…

It actually depends if you use left or right click select. On press it feels more responsive so we kept it for right click, but it's incompatible with tools on left click select.

I'm not sure how we can put select on click instead of press for the node editor though? You want to be able to press to select and drag on a node immediately to move it?

@Bastien Montagne (mont29): Well, the issue is identical: How can we handle click input so that you can both select and drag the selected items, without having to use slower mouse-up events? It's the exact same thing as selecting and moving files on your desktop.

They solve it that way, by making it not deselect others if you click on an item that was already selected. The same solution could work in Blender too.

@Brecht Van Lommel (brecht) You want to be able to press to select and drag on a node immediately to move it? that’s kind of incompatible with behavior described in that task? Either we select, and then move on drag (what happens currently), or we give priority to drag, and only select on CLICK (i.e. when there was no drag detected).

Or we try to implement what @Campbell Barton (campbellbarton) described (some kind of two-steps selection, with PRESS adding to current selection, and then RELEASE removing all selected items and selecting again the active one). With all the PASSTHROUGH and double-undo history issues… yuck. :/

Btw, T63996 looks like a very similar can of worms.

@William Reynish (billreynish) Sure thing, all problems can be solved. But if we have to rethink/rewrite half of our event/operator handling code to support that, this is not a task for me, and this is not a task for 2.80. I (again) have the feeling that a tremendous amount of time and energy needs to be put to fix that kind of issues (and am sorry, but those still look pretty low-priority to me, things that are 'nice to have', but by no mean critical issues). Nice if we can fix them simply and quickly, but otherwise I would not really bother too much.

And as a reminder, I was not even involved in that left-click-select project/decision, not at all. So I do not really feel like spending too much of my time trying to fix the cans of worms it unveils. I have other priorities and projects to tackle too. For me, either we go with the cheap 'CLICK' to select things, or we live with current behavior.

@William Reynish (billreynish): ...without having to use slower mouse-up events? It's the exact same thing as selecting and moving files on your desktop...

The trick with our desktops: The timing of the deselection of other items changes depending on whether the new item is selected or not.

If the new item is not already selected then the other items are deselected on mouse-down. But if the item is already selected then peers are only unselected on mouse-up. This is what allows selecting and dragging singles or groups without having to wait for mouse up.

Ok, sure, it may be a bigger project to make selection more fundamentally different. I'm fine with any solution that works, even if it means using mouse-up events, if it makes it possible to move multiple nodes easily by simply dragging on your selection.

I think the way to solve it is actually to make changes to the select operator, so that clicking on selected items doesn't deselect others. Moving all selected items already works, so the issue is only that the select operator is first deselecting everything each time you click and drag.

It still needs to deselect others when releasing the mouse button, if you haven't started dragging already. One way of doing that would be to make the select operator a modal operator that finishes as soon as you release the mouse button (deselecting others), or a tweak event is started (doing nothing).

To me this seems possible in a way that's isolated to the select operator and not interfering with other operators that need to run right after. But I'm not sure how much work it would be.

@Brecht Van Lommel (brecht) Yes, that would be idea' - that it deselects others on mouse-up if the user didn't drag.

But IMO it would also be acceptable to do it in a simpler way so it just doesn't deselect others if you click on a selected item. Then you would have to click away to deselect first, or use the deselect operator. Not as nice, but eg the macOS Finder works this way and I never noticed it as being an issue.

One way of doing that would be to make the select operator a modal operator that finishes as soon as you release the mouse button (deselecting others), or a tweak event is started (doing nothing).
To me this seems possible in a way that's isolated to the select operator and not interfering with other operators that need to run right after. But I'm not sure how much work it would be.

Whats the advantage of this compared to having click for select, tweak for border select?

Both use the same threshold so it shouldn't be possible to move the cursor so it's moved too much for a click but not enough for a drag.

Whats the advantage of this compared to having click for select, tweak for border select?
Both use the same threshold so it shouldn't be possible to move the cursor so it's moved too much for a click but not enough for a drag.

The advantage is that you select and transform the node immediately. Otherwise you would have to click once to select, and then click again to transform the node.

P966 Is an attempt to implement @Brecht Van Lommel (brecht)'s suggestion, seems to work OK-ish.

Note that it relies on press/release events being the two triggers, think we can accept that hard-coded behavior here?

I remain pretty unhappy about that hack though. To have to add such complexity to a simple operator, just to work around (imho very bad) design decision to merge action and select buttons together… :(

@Bastien Montagne (mont29) Maybe I should have asked in here and not in the diff snippet:

This seems to work well!

Question: How generic is it? Could we re-use same logic for sequencer strips or NLA clips etc?

Lol… copy-pasting my answer from P966 here then. :P

Copy-pasting similar logic should work, yes, at least for similar selection (like in most 2D editors), would expect some complications if we tried to do so with e.g. 3DView selection (also we might have to do similar things there, cf. can of worms II, T63996). This is not a small change though, as pretty much all select ops do things a bit differently. Kind of like what I did for the 'deselect on nothing' change, but several times bigger every time…

My concern with that change is that it adds a whole other level of complexity over selection logic, which already has to deal with extend, select_all, etc. It also makes behavior in different kind of scenarii more complicated to predict (like, what if user adds a keymap using a totally different shortcut to select nodes? What about macros [e.g. just realized that current patch breaks the 'link to viewer node' macro, the one using sockets selection]? etc. etc.). That code can probably be refactored further to make it more easy to follow, but still… am not so happy with it. This adds technical debt for a bad reason imho. :|

@Bastien Montagne (mont29)

Well, obviously I don't agree with "for a bad reason". So many users are already finding Blender 2.8 much easier to use, and the ability to select and manipulate things more easily is a core reason why.

As for code complexity, I guess ideally we could somehow share this logic so it's not duplicated several places. We could also just start by adding this to the Node Editor only for 2.80, and then later we could refactor and generalize the logic to make it easier to re-use in other places.

Hello, I'm new here, but I wonder if this might be a helpful way to approach this:

Since dragging multiple nodes works as expected when they are contained within a frame, would it be possible to create an invisible frame around all selected nodes on mouse press, tweak to move it, then mouse release destroys the frame? (which the user never saw to begin with)

I did an experiment and replaced the Join Nodes - Keymapping (Alt+J by default, I think, I'm using my own keymap) with a Left Click, and while it didn't work if you selected a bunch of nodes and then clicked on one of them to drag, it DID work when you selected a bunch of nodes and clicked anywhere OTHER than a node.

So any misclicks ended up creating empty frames, perhaps that could be fixed with a check on mouse press, but that might then be back in the problem of adding latency.

Or, if they're invisible, creating a million nodes in the background that you never see.

Then again, maybe you could map a function that deletes all empty frames in addition to the select box frame on mouse release?

Just a thought.

@Rob (SpectralVectors) That sounds even more complicated than the modal selection op solution?

@Brecht Van Lommel (brecht) @Campbell Barton (campbellbarton) We should decide now what we do here. Afaics, we have two options:

  1. Select on RELEASE, which solves present issue, is simple and safe, but makes selection experience a tad less nice for user, and prevents the 'click-drag' behavior to click on a node and move it in a single action.
  2. Modal selection performing in two steps, which does everything user might expect, but makes selection code much more hairy than it already is (and hence prone to bugs).

I am a bit torn here, for me solution 1 is far better from a safety/'code quality'/maintainability point of view, while solution 2 is ideal from user point of view.

If we decide to take the risk of modal op solution, I would then like to do it as a separate operator, explicitly only modal, and explicitly not handling all extra cases (like extend selection, selection of sockets, etc.). That should keep code complexity to a more reasonable/maintainable level.

I think we should do 2), it's a nice improvement.

But there are some issues with the implementation that I found:

  • When the node is not initially selected, then dragging on it should not drag all other nodes. It should only do that when the node is already selected. File browsers work similar, and it keeps tweaking individual node positions quick (which I do much more often than moving multiple nodes). In this case the operator could finish immediately and not start the running modal.
  • When clicking and dragging a small distance (by accident), it seems to not deselect other nodes. The threshold should be the same as starting the transform. It may help to listen for EVT_TWEAK_L/M/R events instead of doing our own distance calculation with mouse events.

I'm not entirely sure making it a separate operator will reduce code complexity? We would have to see how that works out, but I fear it would complicate the keymaps. I do think that the socket selection should be handled separately, that could be in its own function, and if a socket is found it can skip running modal and finish immediately.

For the implementation, I guess storing active_item is not actually needed? As far as I can tell it's only used for asserts, so removing it could simplify the implementation.

I think we should do 2), it's a nice improvement.

OK… Still not totally happy with it, but am happier with new version of patch (P966), so guess it’s OK. ;)

But there are some issues with the implementation that I found:

  • When the node is not initially selected, then dragging on it should not drag all other nodes. It should only do that when the node is already selected. File browsers work similar, and it keeps tweaking individual node positions quick (which I do much more often than moving multiple nodes). In this case the operator could finish immediately and not start the running modal.

Good point, fixed in new patch.

  • When clicking and dragging a small distance (by accident), it seems to not deselect other nodes. The threshold should be the same as starting the transform. It may help to listen for EVT_TWEAK_L/M/R events instead of doing our own distance calculation with mouse events.

Threshold is the same as starting tweak (see WM_EVENT_CLICK_TWEAK_THRESHOLD in wm_event_system.c). Also, EVT_TWEAK_L/M/R never reach our modal handler, not sure why (would assume WM event code does not always trigger them?). But previous code was buggy (using wrong coordinate values), new version of patch seems to work as expected here as well.

I'm not entirely sure making it a separate operator will reduce code complexity? We would have to see how that works out, but I fear it would complicate the keymaps. I do think that the socket selection should be handled separately, that could be in its own function, and if a socket is found it can skip running modal and finish immediately.
For the implementation, I guess storing active_item is not actually needed? As far as I can tell it's only used for asserts, so removing it could simplify the implementation.

Good points, new patch is much more satisfying on simplicity side imho (also made it more strict to only accept regular selection in modal 'two steps' mode, other options just get always executed in basic exec mode.

1diff --git a/source/blender/editors/space_node/node_select.c b/source/blender/editors/space_node/node_select.c
2index 07b01e576bc..681bf693246 100644
3--- a/source/blender/editors/space_node/node_select.c
4+++ b/source/blender/editors/space_node/node_select.c
5@@ -435,13 +435,21 @@ static int node_mouse_select(Main *bmain,
6 const int mval[2],
7 const bool extend,
8 const bool socket_select,
9- const bool deselect_all)
10+ const bool deselect_all,
11+ const bool do_first_step,
12+ const bool do_second_step)
13 {
14 bNode *node, *tnode;
15 bNodeSocket *sock = NULL;
16 bNodeSocket *tsock;
17 float cursor[2];
18- bool selected = false;
19+ int ret_value = OPERATOR_CANCELLED;
20+
21+ /* Handle two-steps selection (with deselection of already selected items as second step). */
22+ const bool use_two_steps = do_first_step || do_second_step;
23+
24+ /* Two-steps modal selection is only allowed for basic selection. */
25+ BLI_assert(!(extend || socket_select) || !use_two_steps);
26
27 /* get mouse coordinates in view2d space */
28 UI_view2d_region_to_view(&ar->v2d, mval[0], mval[1], &cursor[0], &cursor[1]);
29@@ -452,7 +460,7 @@ static int node_mouse_select(Main *bmain,
30 /* NOTE: SOCK_IN does not take into account the extend case...
31 * This feature is not really used anyway currently? */
32 node_socket_toggle(node, sock, true);
33- selected = true;
34+ ret_value = OPERATOR_FINISHED;
35 }
36 else if (node_find_indicated_socket(snode, &node, &sock, cursor, SOCK_OUT)) {
37 if (sock->flag & SELECT) {
38@@ -460,7 +468,7 @@ static int node_mouse_select(Main *bmain,
39 node_socket_deselect(node, sock, true);
40 }
41 else {
42- selected = true;
43+ ret_value = OPERATOR_FINISHED;
44 }
45 }
46 else {
47@@ -485,7 +493,7 @@ static int node_mouse_select(Main *bmain,
48 }
49 }
50 node_socket_select(node, sock);
51- selected = true;
52+ ret_value = OPERATOR_FINISHED;
53 }
54 }
55 }
56@@ -501,24 +509,43 @@ static int node_mouse_select(Main *bmain,
57 if (!((node->flag & SELECT) && (node->flag & NODE_ACTIVE) == 0)) {
58 node_toggle(node);
59 }
60- selected = true;
61+ ret_value = OPERATOR_FINISHED;
62 }
63 }
64 else {
65 if (node != NULL || deselect_all) {
66- for (tnode = snode->edittree->nodes.first; tnode; tnode = tnode->next) {
67- nodeSetSelected(tnode, false);
68+ /* We want to deselect all other nodes in three cases:
69+ * - If 'deselect on nothing' is enabled, and we clicked on nothing.
70+ * - If we are in single-step selection, or in second step of two-steps selection.
71+ * - If we are in first step of two-steps selection, and node on which we clicked
72+ * was not selected (that way, click-drag on non-selected node will only move that one,
73+ * not also all other previously selected nodes).
74+ */
75+ if ((deselect_all && node == NULL) || (!use_two_steps || do_second_step) ||
76+ (do_first_step && node != NULL && (node->flag & SELECT) == 0)) {
77+ for (tnode = snode->edittree->nodes.first; tnode; tnode = tnode->next) {
78+ if (tnode == node) {
79+ continue;
80+ }
81+ nodeSetSelected(tnode, false);
82+ }
83+ if (node != NULL) {
84+ nodeSetSelected(node, true);
85+ }
86+ /* In case we also do de-selection of previously selected nodes, we are done,
87+ * no need to keep modal running. */
88+ ret_value = OPERATOR_FINISHED;
89 }
90- selected = true;
91- if (node != NULL) {
92+ else if (node != NULL) {
93 nodeSetSelected(node, true);
94+ ret_value = do_first_step ? OPERATOR_RUNNING_MODAL : OPERATOR_FINISHED;
95 }
96 }
97 }
98 }
99
100 /* update node order */
101- if (selected) {
102+ if (ret_value != OPERATOR_CANCELLED) {
103 if (node != NULL) {
104 ED_node_set_active(bmain, snode->edittree, node);
105 }
106@@ -526,7 +553,7 @@ static int node_mouse_select(Main *bmain,
107 ED_node_sort(snode->edittree);
108 }
109
110- return selected;
111+ return ret_value;
112 }
113
114 static int node_select_exec(bContext *C, wmOperator *op)
115@@ -546,17 +573,90 @@ static int node_select_exec(bContext *C, wmOperator *op)
116 const bool deselect_all = RNA_boolean_get(op->ptr, "deselect_all");
117
118 /* perform the select */
119- if (node_mouse_select(bmain, snode, ar, mval, extend, socket_select, deselect_all)) {
120+ const int ret_value = node_mouse_select(
121+ bmain, snode, ar, mval, extend, socket_select, deselect_all, false, false);
122+ if (ret_value & OPERATOR_FINISHED) {
123 /* send notifiers */
124 WM_event_add_notifier(C, NC_NODE | NA_SELECTED, NULL);
125+ }
126+ /* allow tweak event to work too */
127+ return ret_value | OPERATOR_PASS_THROUGH;
128+}
129
130- /* allow tweak event to work too */
131- return OPERATOR_FINISHED | OPERATOR_PASS_THROUGH;
132+static int node_select_modal(bContext *C, wmOperator *op, const wmEvent *event)
133+{
134+ Main *bmain = CTX_data_main(C);
135+ SpaceNode *snode = CTX_wm_space_node(C);
136+ ARegion *ar = CTX_wm_region(C);
137+
138+ const short init_event_type = (short)POINTER_AS_INT(op->customdata);
139+
140+ /* get settings from RNA properties for operator */
141+ int mval[2];
142+ mval[0] = RNA_int_get(op->ptr, "mouse_x");
143+ mval[1] = RNA_int_get(op->ptr, "mouse_y");
144+
145+ const bool extend = RNA_boolean_get(op->ptr, "extend");
146+ /* always do socket_select when extending selection. */
147+ const bool socket_select = extend || RNA_boolean_get(op->ptr, "socket_select");
148+ const bool deselect_all = RNA_boolean_get(op->ptr, "deselect_all");
149+
150+ /* These cases are never modal. */
151+ if (extend || socket_select) {
152+ return node_select_exec(C, op);
153 }
154- else {
155- /* allow tweak event to work too */
156- return OPERATOR_CANCELLED | OPERATOR_PASS_THROUGH;
157+
158+ if (init_event_type == 0) {
159+ if (event->val == KM_PRESS) {
160+ const int ret_value = node_mouse_select(
161+ bmain, snode, ar, mval, extend, socket_select, deselect_all, true, false);
162+
163+ op->customdata = POINTER_FROM_INT((int)event->type);
164+ if (ret_value & OPERATOR_RUNNING_MODAL) {
165+ WM_event_add_modal_handler(C, op);
166+ }
167+ if (ret_value & (OPERATOR_RUNNING_MODAL | OPERATOR_FINISHED)) {
168+ /* send notifiers */
169+ WM_event_add_notifier(C, NC_NODE | NA_SELECTED, NULL);
170+ }
171+ return ret_value | OPERATOR_PASS_THROUGH;
172+ }
173+ else {
174+ /* If we are in init phase, and cannot validate init of modal operations,
175+ * just fall back to basic exec.
176+ */
177+ return node_select_exec(C, op);
178+ }
179+ }
180+ else if (event->type == init_event_type && event->val == KM_RELEASE) {
181+ const int ret_value = node_mouse_select(
182+ bmain, snode, ar, mval, extend, socket_select, deselect_all, false, true);
183+
184+ if (ret_value & (OPERATOR_RUNNING_MODAL | OPERATOR_FINISHED)) {
185+ /* send notifiers */
186+ WM_event_add_notifier(C, NC_NODE | NA_SELECTED, NULL);
187+ }
188+
189+ return ret_value | OPERATOR_PASS_THROUGH;
190 }
191+ else if (ELEM(event->type, MOUSEMOVE, INBETWEEN_MOUSEMOVE)) {
192+ const int dx = mval[0] - event->mval[0];
193+ const int dy = mval[1] - event->mval[1];
194+ const float tweak_threshold = U.tweak_threshold * U.dpi_fac;
195+ /* If user moves mouse more than defined threshold, we consider select operator as
196+ * finished. Otherwise, it is still running until we get an 'release' event. In any
197+ * case, we pass through event, but select op is not finished yet. */
198+ if (abs(dx) + abs(dy) > tweak_threshold) {
199+ return OPERATOR_FINISHED | OPERATOR_PASS_THROUGH;
200+ }
201+ else {
202+ /* Important not to return anything other than PASS_THROUGH here,
203+ * otherwise it prevents underlying tweak detection code to work properly. */
204+ return OPERATOR_PASS_THROUGH;
205+ }
206+ }
207+
208+ return OPERATOR_FINISHED | OPERATOR_PASS_THROUGH;
209 }
210
211 static int node_select_invoke(bContext *C, wmOperator *op, const wmEvent *event)
212@@ -564,7 +664,9 @@ static int node_select_invoke(bContext *C, wmOperator *op, const wmEvent *event)
213 RNA_int_set(op->ptr, "mouse_x", event->mval[0]);
214 RNA_int_set(op->ptr, "mouse_y", event->mval[1]);
215
216- return node_select_exec(C, op);
217+ op->customdata = POINTER_FROM_INT(0);
218+
219+ return node_select_modal(C, op, event);
220 }
221
222 void NODE_OT_select(wmOperatorType *ot)
223@@ -577,6 +679,7 @@ void NODE_OT_select(wmOperatorType *ot)
224 /* api callbacks */
225 ot->invoke = node_select_invoke;
226 ot->exec = node_select_exec;
227+ ot->modal = node_select_modal;
228 ot->poll = ED_operator_node_active;
229
230 /* flags */

I think the patch works very well.

The code can be simplified further:

Seems ok to commit.

Ah yes, nice improvements, thanks.

Digged into this today for T57918, but wondered if we wouldn't simplify this a lot by doing things the other way around:

  • Rather than making the select operators modal and letting them pass events through to the transform operator, make the transform operator invoke immediately, but letting it early exit and pass through events to the select operators.
  • If a tweak event is detected, the transform operator starts applying its changes and swallows all further events
  • If a release event is detected prior to a tweak one, transform is cancelled
  • Call selection once on mouse press event. If the click is on an unselected item, select it and deselect others, otherwise, do nothing.
  • On mouse release, call select operator again (or a different operator), letting it deselect items not under the cursor - note that this point is not reached if a tweak event triggered proper transform

This seemed almost too simple to be true, so I gave it a go. And: It appears to work just fine, with little changes in total, and the select operator changes are tiny.

P1121 adds drag-all-selected support to the sequencer. Note how few changes were needed in the select operator.

There a some small glitches to fix still (e.g. cursor changing on select), and we may dig deeper to make sure the transform code stays in valid states. But it seems like a very promising direction.
Am I missing something?

A problem in this patch is that the select operator is registered twice and there are two undo pushes.

Ideally we would also not complicate the keymap with this, since editing keymaps is already complicated enough and this adds more obscure properties. The way it works in the node editor requires no keymap changes,

It also now actually starts the transform whenever you click to select. This shows up in the status bar, is potentially slow or might report some kind of warning. In the sequence editor, it's now clearing the sequencer cache when just selecting strips.

So I'm not sure about this approach, is it not possible to make the modal select thing more generic and reusable?

Checking again on this, these issues seem to be solvable without too much hassle.

A problem in this patch is that the select operator is registered twice and there are two undo pushes.

This can be addressed by making sure the operator returns OPERATOR_CANCELLED when it doesn't perform any operation. I think we'd have to do that with the modal select operators too.

Ideally we would also not complicate the keymap with this, since editing keymaps is already complicated enough and this adds more obscure properties. The way it works in the node editor requires no keymap changes,

Agree it's nice to avoid changes to the keymap for such things. But in the node editor implementation, there are hardcoded event checks, which are nice to avoid too.
I think we could also trigger the potential deselecting on release by making transform + deselecting a macro operator. Alternatively it might be a good idea to optionally let the transform operator trigger item deselection if it cancels before having applied any transformation. Again, not the nicest solution but solves this with less tweaks hacks in the keymap.

It also now actually starts the transform whenever you click to select. This shows up in the status bar, is potentially slow or might report some kind of warning. In the sequence editor, it's now clearing the sequencer cache when just selecting strips.

Although I'm not sure how well my previous patch handled this, the idea is that exactly this is prevented. The transform operator should just return OPERATOR_PASS_THROUGH until a tweak event it detected, without applying any change.

So I'm not sure about this approach, is it not possible to make the modal select thing more generic and reusable?

Well, the complaints about this approach from Bastien are still valid. The modal selection stuff makes selection even more complicated and contains hard coded event checks. I'm not sure if we can make this generic/reusable enough to work well. There's quite a difference between select operators.

This new (experimental!) patch fixes the issues mentioned:

1diff --git a/release/scripts/presets/keyconfig/keymap_data/blender_default.py b/release/scripts/presets/keyconfig/keymap_data/blender_default.py
2index 1839dd5f322..2a8231ed1d2 100644
3--- a/release/scripts/presets/keyconfig/keymap_data/blender_default.py
4+++ b/release/scripts/presets/keyconfig/keymap_data/blender_default.py
5@@ -2394,7 +2394,10 @@ def km_sequencer(params):
6 ),
7 ("sequencer.select", {"type": params.select_mouse, "value": 'PRESS'},
8 {"properties": [("extend", False), ("deselect_all", True),
9- ("linked_handle", False), ("left_right", 'NONE'), ("linked_time", False)]}),
10+ ("linked_handle", False), ("left_right", 'NONE'), ("linked_time", False), ("exit_if_selected", True)]}),
11+ ("sequencer.select", {"type": params.select_mouse, "value": 'RELEASE'},
12+ {"properties": [("extend", False), ("deselect_all", True),
13+ ("linked_handle", False), ("left_right", 'NONE'), ("linked_time", False), ("exit_if_selected", False)]}),
14 ("sequencer.select", {"type": params.select_mouse, "value": 'PRESS', "shift": True},
15 {"properties": [("extend", True), ("linked_handle", False), ("left_right", 'NONE'), ("linked_time", False)]}),
16 ("sequencer.select", {"type": params.select_mouse, "value": 'PRESS', "alt": True},
17@@ -2429,7 +2432,8 @@ def km_sequencer(params):
18 ("wm.context_set_int", {"type": 'O', "value": 'PRESS'},
19 {"properties": [("data_path", 'scene.sequence_editor.overlay_frame'), ("value", 0)]}),
20 ("transform.seq_slide", {"type": 'G', "value": 'PRESS'}, None),
21- ("transform.seq_slide", {"type": params.select_tweak, "value": 'ANY'}, None),
22+ ("transform.seq_slide", {"type": params.select_mouse, "value": 'PRESS'},
23+ {"properties": [("wait_for_tweak", True)]}),
24 ("transform.transform", {"type": 'E', "value": 'PRESS'},
25 {"properties": [("mode", 'TIME_EXTEND')]}),
26 ("marker.add", {"type": 'M', "value": 'PRESS'}, None),
27diff --git a/source/blender/editors/space_sequencer/sequencer_select.c b/source/blender/editors/space_sequencer/sequencer_select.c
28index affb6d3fd88..39cc353e197 100644
29--- a/source/blender/editors/space_sequencer/sequencer_select.c
30+++ b/source/blender/editors/space_sequencer/sequencer_select.c
31@@ -328,6 +328,7 @@ static int sequencer_select_invoke(bContext *C, wmOperator *op, const wmEvent *e
32 const bool deselect_all = RNA_boolean_get(op->ptr, "deselect_all");
33 const bool linked_handle = RNA_boolean_get(op->ptr, "linked_handle");
34 const bool linked_time = RNA_boolean_get(op->ptr, "linked_time");
35+ const bool exit_if_selected = RNA_boolean_get(op->ptr, "exit_if_selected");
36 int left_right = RNA_enum_get(op->ptr, "left_right");
37
38 Sequence *seq, *neighbor, *act_orig;
39@@ -342,6 +343,44 @@ static int sequencer_select_invoke(bContext *C, wmOperator *op, const wmEvent *e
40
41 seq = find_nearest_seq(scene, v2d, &hand, event->mval);
42
43+ if (exit_if_selected && seq && (seq->flag & SELECT)) {
44+ /* Allow tweaks. */
45+ return OPERATOR_CANCELLED | OPERATOR_PASS_THROUGH;
46+ }
47+ if (seq && (seq->flag & SELECT)) {
48+ Sequence *iter_seq;
49+ bool any_other_selected = false;
50+
51+ SEQP_BEGIN (ed, iter_seq) {
52+ if ((iter_seq->flag & SELECT) && (iter_seq != seq)) {
53+ any_other_selected = true;
54+ break;
55+ }
56+ }
57+ SEQ_END;
58+
59+ /* All we would do is selecting an item that is already selected. Exit. */
60+ if (any_other_selected == false) {
61+ return OPERATOR_CANCELLED | OPERATOR_PASS_THROUGH;
62+ }
63+ }
64+ if (!seq) {
65+ Sequence *iter_seq;
66+ bool any_selected = false;
67+
68+ SEQP_BEGIN (ed, iter_seq) {
69+ if (iter_seq->flag & SELECT) {
70+ any_selected = true;
71+ break;
72+ }
73+ }
74+ SEQ_END;
75+
76+ if (any_selected == false) {
77+ return OPERATOR_CANCELLED | OPERATOR_PASS_THROUGH;
78+ }
79+ }
80+
81 // XXX - not nice, Ctrl+RMB needs to do left_right only when not over a strip
82 if (seq && linked_time && (left_right == SEQ_SELECT_LR_MOUSE)) {
83 left_right = SEQ_SELECT_LR_NONE;
84@@ -598,6 +637,12 @@ void SEQUENCER_OT_select(wmOperatorType *ot)
85 "Select based on the current frame side the cursor is on");
86 RNA_def_boolean(
87 ot->srna, "linked_time", 0, "Linked Time", "Select other strips at the same time");
88+ RNA_def_boolean(
89+ ot->srna,
90+ "exit_if_selected",
91+ false,
92+ "Exit if selected",
93+ "Do not perform any selection change if the strip to be selected already is selected");
94 }
95
96 /* run recursively to select linked */
97@@ -1038,7 +1083,8 @@ static const EnumPropertyItem sequencer_prop_select_grouped_types[] = {
98 "EFFECT_LINK",
99 0,
100 "Effect/Linked",
101- "Other strips affected by the active one (sharing some time, and below or effect-assigned)"},
102+ "Other strips affected by the active one (sharing some time, and below or "
103+ "effect-assigned)"},
104 {SEQ_SELECT_GROUP_OVERLAP, "OVERLAP", 0, "Overlap", "Overlapping time"},
105 {0, NULL, 0, NULL, NULL},
106 };
107diff --git a/source/blender/editors/transform/transform.c b/source/blender/editors/transform/transform.c
108index 67ea0f255fc..bff4d6fe71a 100644
109--- a/source/blender/editors/transform/transform.c
110+++ b/source/blender/editors/transform/transform.c
111@@ -54,6 +54,7 @@
112 #include "BKE_editmesh_bvh.h"
113 #include "BKE_context.h"
114 #include "BKE_constraint.h"
115+#include "BKE_global.h"
116 #include "BKE_particle.h"
117 #include "BKE_unit.h"
118 #include "BKE_scene.h"
119@@ -832,13 +833,18 @@ enum {
120 TFM_MODAL_INSERTOFS_TOGGLE_DIR = 27,
121 };
122
123-static bool transform_modal_item_poll(const wmOperator *op, int value)
124+static int transform_modal_item_poll(const wmOperator *op, int value)
125 {
126 const TransInfo *t = op->customdata;
127+
128+ if (t->state == TRANS_WAITING) {
129+ return KEYMAP_MODAL_RUN_ONLY;
130+ }
131+
132 switch (value) {
133 case TFM_MODAL_CANCEL: {
134 if ((t->flag & T_RELEASE_CONFIRM) && ISMOUSE(t->launch_event)) {
135- return false;
136+ return KEYMAP_MODAL_SKIP;
137 }
138 break;
139 }
140@@ -846,20 +852,20 @@ static bool transform_modal_item_poll(const wmOperator *op, int value)
141 case TFM_MODAL_PROPSIZE_UP:
142 case TFM_MODAL_PROPSIZE_DOWN: {
143 if ((t->flag & T_PROP_EDIT) == 0) {
144- return false;
145+ return KEYMAP_MODAL_SKIP;
146 }
147 break;
148 }
149 case TFM_MODAL_ADD_SNAP:
150 case TFM_MODAL_REMOVE_SNAP: {
151 if (t->spacetype != SPACE_VIEW3D) {
152- return false;
153+ return KEYMAP_MODAL_SKIP;
154 }
155 else if (t->tsnap.mode & (SCE_SNAP_MODE_INCREMENT | SCE_SNAP_MODE_GRID)) {
156- return false;
157+ return KEYMAP_MODAL_SKIP;
158 }
159 else if (!validSnap(t)) {
160- return false;
161+ return KEYMAP_MODAL_SKIP;
162 }
163 break;
164 }
165@@ -870,43 +876,43 @@ static bool transform_modal_item_poll(const wmOperator *op, int value)
166 case TFM_MODAL_PLANE_Y:
167 case TFM_MODAL_PLANE_Z: {
168 if (t->flag & T_NO_CONSTRAINT) {
169- return false;
170+ return KEYMAP_MODAL_SKIP;
171 }
172 if (!ELEM(value, TFM_MODAL_AXIS_X, TFM_MODAL_AXIS_Y)) {
173 if (t->flag & T_2D_EDIT) {
174- return false;
175+ return KEYMAP_MODAL_SKIP;
176 }
177 }
178 break;
179 }
180 case TFM_MODAL_CONS_OFF: {
181 if ((t->con.mode & CON_APPLY) == 0) {
182- return false;
183+ return KEYMAP_MODAL_SKIP;
184 }
185 break;
186 }
187 case TFM_MODAL_EDGESLIDE_UP:
188 case TFM_MODAL_EDGESLIDE_DOWN: {
189 if (t->mode != TFM_EDGE_SLIDE) {
190- return false;
191+ return KEYMAP_MODAL_SKIP;
192 }
193 break;
194 }
195 case TFM_MODAL_INSERTOFS_TOGGLE_DIR: {
196 if (t->spacetype != SPACE_NODE) {
197- return false;
198+ return KEYMAP_MODAL_SKIP;
199 }
200 break;
201 }
202 case TFM_MODAL_AUTOIK_LEN_INC:
203 case TFM_MODAL_AUTOIK_LEN_DEC: {
204 if ((t->flag & T_AUTOIK) == 0) {
205- return false;
206+ return KEYMAP_MODAL_SKIP;
207 }
208 break;
209 }
210 }
211- return true;
212+ return KEYMAP_MODAL_RUN_AND_DRAW;
213 }
214
215 /* called in transform_ops.c, on each regeneration of keymaps */
216@@ -1044,13 +1050,30 @@ static void transform_event_xyz_constraint(TransInfo *t, short key_type, char cm
217 }
218 }
219
220-int transformEvent(TransInfo *t, const wmEvent *event)
221+int transformEvent(TransInfo *t, wmOperator *op, const wmEvent *event)
222 {
223 char cmode = constraintModeToChar(t);
224 bool handled = false;
225 const int modifiers_prev = t->modifiers;
226 const int mode_prev = t->mode;
227
228+ if (t->state == TRANS_WAITING) {
229+ if ((event->type == t->launch_event) && (event->val == KM_RELEASE)) {
230+ t->state = TRANS_CANCEL;
231+ return OPERATOR_CANCELLED | OPERATOR_PASS_THROUGH;
232+ }
233+ else if (ISTWEAK(event->type)) {
234+ bContext *C = t->context;
235+ initTransform(C, t, op, event, t->mode);
236+ t->context = C;
237+ G.moving = special_transform_moving(t);
238+ }
239+ else {
240+ t->state = TRANS_WAITING;
241+ return OPERATOR_PASS_THROUGH;
242+ }
243+ }
244+
245 t->redraw |= handleMouseInput(t, &t->mouse, event);
246
247 /* Handle modal numinput events first, if already activated. */
248@@ -1625,7 +1648,7 @@ int transformEvent(TransInfo *t, const wmEvent *event)
249 WM_window_status_area_tag_redraw(CTX_wm_window(t->context));
250 }
251
252- if (handled || t->redraw) {
253+ if (handled || t->redraw || ELEM(t->state, TRANS_CONFIRM, TRANS_CANCEL)) {
254 return 0;
255 }
256 else {
257@@ -2323,6 +2346,17 @@ bool initTransform(bContext *C, TransInfo *t, wmOperator *op, const wmEvent *eve
258
259 /* added initialize, for external calls to set stuff in TransInfo, like undo string */
260
261+ if (op && (t->state != TRANS_WAITING) &&
262+ ((prop = RNA_struct_find_property(op->ptr, "wait_for_tweak")) &&
263+ RNA_property_is_set(op->ptr, prop))) {
264+ if (RNA_property_boolean_get(op->ptr, prop)) {
265+ t->mode = mode;
266+ t->state = TRANS_WAITING;
267+ t->launch_event = event ? WM_userdef_event_type_from_keymap_type(event->type) : -1;
268+ return true;
269+ }
270+ }
271+
272 t->state = TRANS_STARTING;
273
274 if ((prop = RNA_struct_find_property(op->ptr, "cursor_transform")) &&
275@@ -2785,7 +2819,10 @@ int transformEnd(bContext *C, TransInfo *t)
276
277 t->context = C;
278
279- if (t->state != TRANS_STARTING && t->state != TRANS_RUNNING) {
280+ if (t->state == TRANS_WAITING) {
281+ exit_code = 0;
282+ }
283+ else if (t->state != TRANS_STARTING && t->state != TRANS_RUNNING) {
284 /* handle restoring objects */
285 if (t->state == TRANS_CANCEL) {
286 /* exception, edge slide transformed UVs too */
287@@ -2797,7 +2834,10 @@ int transformEnd(bContext *C, TransInfo *t)
288 }
289
290 exit_code = OPERATOR_CANCELLED;
291- restoreTransObjects(t); // calls recalcData()
292+ if (t->depsgraph) { /* Just some silly NULL-check to ensure this is only called after correct
293+ initialization */
294+ restoreTransObjects(t); // calls recalcData()
295+ }
296 }
297 else {
298 if (t->flag & T_CLNOR_REBUILD) {
299diff --git a/source/blender/editors/transform/transform.h b/source/blender/editors/transform/transform.h
300index ff2afbc0cd7..ba9172cb55f 100644
301--- a/source/blender/editors/transform/transform.h
302+++ b/source/blender/editors/transform/transform.h
303@@ -714,6 +714,7 @@ typedef struct TransInfo {
304
305 /* transinfo->state */
306 enum {
307+ TRANS_WAITING = -1,
308 TRANS_STARTING = 0,
309 TRANS_RUNNING = 1,
310 TRANS_CONFIRM = 2,
311@@ -879,7 +880,7 @@ bool initTransform(struct bContext *C,
312 const struct wmEvent *event,
313 int mode);
314 void saveTransform(struct bContext *C, struct TransInfo *t, struct wmOperator *op);
315-int transformEvent(TransInfo *t, const struct wmEvent *event);
316+int transformEvent(TransInfo *t, struct wmOperator *op, const struct wmEvent *event);
317 void transformApply(struct bContext *C, TransInfo *t);
318 int transformEnd(struct bContext *C, TransInfo *t);
319
320diff --git a/source/blender/editors/transform/transform_convert.c b/source/blender/editors/transform/transform_convert.c
321index 5862faaf667..2e3af5d9418 100644
322--- a/source/blender/editors/transform/transform_convert.c
323+++ b/source/blender/editors/transform/transform_convert.c
324@@ -2332,7 +2332,10 @@ void special_aftertrans_update(bContext *C, TransInfo *t)
325
326 int special_transform_moving(TransInfo *t)
327 {
328- if (t->spacetype == SPACE_SEQ) {
329+ if (t->state == TRANS_WAITING) {
330+ /* Pass. */
331+ }
332+ else if (t->spacetype == SPACE_SEQ) {
333 return G_TRANSFORM_SEQ;
334 }
335 else if (t->spacetype == SPACE_GRAPH) {
336diff --git a/source/blender/editors/transform/transform_ops.c b/source/blender/editors/transform/transform_ops.c
337index b2d8671fbce..e9b3c476f2a 100644
338--- a/source/blender/editors/transform/transform_ops.c
339+++ b/source/blender/editors/transform/transform_ops.c
340@@ -413,7 +413,8 @@ static int transform_modal(bContext *C, wmOperator *op, const wmEvent *event)
341
342 /* XXX insert keys are called here, and require context */
343 t->context = C;
344- exit_code = transformEvent(t, event);
345+ exit_code = transformEvent(t, op, event);
346+ BLI_assert((t->state != TRANS_WAITING) || (exit_code & OPERATOR_PASS_THROUGH));
347 t->context = NULL;
348
349 /* XXX, workaround: active needs to be calculated before transforming,
350@@ -430,9 +431,12 @@ static int transform_modal(bContext *C, wmOperator *op, const wmEvent *event)
351
352 exit_code |= transformEnd(C, t);
353
354- if ((exit_code & OPERATOR_RUNNING_MODAL) == 0) {
355+ if (t->state == TRANS_WAITING) {
356+ /* pass */
357+ }
358+ else if ((exit_code & OPERATOR_RUNNING_MODAL) == 0) {
359 transformops_exit(C, op);
360- exit_code &= ~OPERATOR_PASS_THROUGH; /* preventively remove passthrough */
361+ // exit_code &= ~OPERATOR_PASS_THROUGH; /* preventively remove passthrough */
362 }
363 else {
364 if (mode_prev != t->mode) {
365@@ -1159,6 +1163,16 @@ static void TRANSFORM_OT_seq_slide(struct wmOperatorType *ot)
366
367 WM_operatortype_props_advanced_begin(ot);
368
369+ /* Might want to support this in Transform_Properties(). */
370+ prop = RNA_def_boolean(
371+ ot->srna,
372+ "wait_for_tweak",
373+ 0,
374+ "Wait for Tweak Event",
375+ "Let events pass to other operators and only start applying transformations when a tweak "
376+ "event from the initial event source is recognized");
377+ RNA_def_property_flag(prop, PROP_HIDDEN);
378+
379 Transform_Properties(ot, P_SNAP);
380 }
381
382diff --git a/source/blender/makesdna/DNA_windowmanager_types.h b/source/blender/makesdna/DNA_windowmanager_types.h
383index 57c0a29382d..3a8807d92b7 100644
384--- a/source/blender/makesdna/DNA_windowmanager_types.h
385+++ b/source/blender/makesdna/DNA_windowmanager_types.h
386@@ -406,7 +406,7 @@ typedef struct wmKeyMap {
387 /* runtime */
388 /** Verify if enabled in the current context, use #WM_keymap_poll instead of direct calls. */
389 bool (*poll)(struct bContext *);
390- bool (*poll_modal_item)(const struct wmOperator *op, int value);
391+ int (*poll_modal_item)(const struct wmOperator *op, int value);
392
393 /** For modal, #EnumPropertyItem for now. */
394 const void *modal_items;
395@@ -424,6 +424,12 @@ enum {
396 KEYMAP_TOOL = (1 << 7), /* keymap for active tool system */
397 };
398
399+enum {
400+ KEYMAP_MODAL_SKIP = 0,
401+ KEYMAP_MODAL_RUN_AND_DRAW = 1,
402+ KEYMAP_MODAL_RUN_ONLY = 2,
403+};
404+
405 /**
406 * This is similar to addon-preferences,
407 * however unlike add-ons key-config's aren't saved to disk.
408diff --git a/source/blender/windowmanager/intern/wm_event_system.c b/source/blender/windowmanager/intern/wm_event_system.c
409index 6b4327d5f44..92aeb1ce985 100644
410--- a/source/blender/windowmanager/intern/wm_event_system.c
411+++ b/source/blender/windowmanager/intern/wm_event_system.c
412@@ -2065,7 +2065,8 @@ static wmKeyMapItem *wm_eventmatch_modal_keymap_items(const wmKeyMap *keymap,
413 {
414 for (wmKeyMapItem *kmi = keymap->items.first; kmi; kmi = kmi->next) {
415 if (wm_eventmatch(event, kmi)) {
416- if ((keymap->poll_modal_item == NULL) || (keymap->poll_modal_item(op, kmi->propvalue))) {
417+ if ((keymap->poll_modal_item == NULL) ||
418+ (keymap->poll_modal_item(op, kmi->propvalue) != KEYMAP_MODAL_SKIP)) {
419 return kmi;
420 }
421 }
422@@ -5276,6 +5277,7 @@ bool WM_window_modal_keymap_status_draw(bContext *UNUSED(C), wmWindow *win, uiLa
423 return false;
424 }
425 const EnumPropertyItem *items = keymap->modal_items;
426+ bool any_drawn = false;
427
428 uiLayout *row = uiLayoutRow(layout, true);
429 for (int i = 0; items[i].identifier; i++) {
430@@ -5283,9 +5285,10 @@ bool WM_window_modal_keymap_status_draw(bContext *UNUSED(C), wmWindow *win, uiLa
431 continue;
432 }
433 if ((keymap->poll_modal_item != NULL) &&
434- (keymap->poll_modal_item(op, items[i].value) == false)) {
435+ (keymap->poll_modal_item(op, items[i].value) != KEYMAP_MODAL_RUN_AND_DRAW)) {
436 continue;
437 }
438+ any_drawn = true;
439
440 bool show_text = true;
441
442@@ -5330,7 +5333,7 @@ bool WM_window_modal_keymap_status_draw(bContext *UNUSED(C), wmWindow *win, uiLa
443 }
444 }
445 }
446- return true;
447+ return any_drawn;
448 }
449
450 /** \} */

Well, the complaints about this approach from Bastien are still valid. The modal selection stuff makes selection even more complicated and contains hard coded event checks. I'm not sure if we can make this generic/reusable enough to work well. There's quite a difference between select operators.

There are no hardcoded event checks, it just checks the original event release + mouse move.

Here I did some refactoring to demonstrate what I mean.
rB3b23685c7dc1: Cleanup: simplify node modal select implementation

Note how node_select_exec, node_select_modal, node_select_invoke no longer contain any node specific logic, which is all left to node_mouse_select.

Note that if wait_to_deselect_others is made a hidden operator property, all that logic can be put in the exec function.

And then node definition looks something like this:

/* api callbacks */
ot->poll = ED_operator_node_active;
ot->exec = node_select_exec;
ot->invoke = WM_generic_select_invoke;
ot->modal = WM_generic_select_modal;

/* properties */
WM_generic_select_properties(ot);
RNA_def_boolean(ot->srna, "extend", false, "Extend", "");
...

Which is consistent with the way we do box select operators for example.