VSE: add option to select handles with box selection
ClosedPublic
Actions

Authored by Alessio Monti di Sopra (a.monti) on Dec 6 2019, 12:47 PM.

Details

Reviewers

Richard Antalik (ISS)
Julian Eisel (Severin)
Campbell Barton (campbellbarton)

Maniphest Tasks

T70730: VSE: "Include Handles" option for Box Select

Commits

rB5314161491d4: VSE: Add option to select handles with box selection

Summary

The patch adds an "Handle" option to the SEQUENCER_OT_box_select operator, that allows to select the handles instead of whole strips.
I mapped this to Alt+B, so that I could also consistently use the Alt modifier for the mouse tweak shortcuts (I guess this would need to be better addressed when the active tools gets implemented in the vse, but for the moment this seemed a good solution to me).

A difference from the proposed design in T70730 is that covering the entire strip with the box actually selects both handles; testing this I found that it did make more sense to me to keep the two behaviours more distinct.

At the moment, when the box covers one handle, the behaviour is the following:

Select -> Strip and handle get selected
Deselect, strip selected with no handles selected -> The opposite handle get selected (left one in the example)
Deselect, both handles selected -> Handle get deselected
Deselect, strip selected, covered handle is the only one selected -> Both the strip and the handle get deselected

When the box covers the entire strip everything get (de)selected.
Regular box selection now also deselect the handles.

Diff Detail

Repository: rB Blender

Event Timeline

Alessio Monti di Sopra (a.monti) created this revision.Dec 6 2019, 12:47 PM

Alessio Monti di Sopra (a.monti) added a task: T70730: VSE: "Include Handles" option for Box Select.

Alessio Monti di Sopra (a.monti) edited the summary of this revision. (Show Details)Dec 6 2019, 12:55 PM

Works well.

Noticed that close to the handle, but not over the handle, will select it too, but if the drawing of handles will change at some point, it won't matter much?

Don't know if +Shift should be used for extending selection, to be in consistency, or extended selection should just be default behavior, as it is now?

In D6372#148880, @Peter Fog (tintwotin) wrote:

Works well.

Noticed that close to the handle, but not over the handle, will select it too, but if the drawing of handles will change at some point, it won't matter much?

Ooook, I didn't test this while zoomed in a lot. I know how to solve it, but will need to rearrange a couple of functions.

Don't know if +Shift should be used for extending selection, to be in consistency, or extended selection should just be default behavior, as it is now?

Not sure about what you mean here, there are actually two default behaviours that differ between keyboard and mouse events (this is true for box selections all over Blender):
if you called it from a keyboard shortcut you have extend selection by default and MMB to subtract, while when using a mouse click+drag event the default is "new selection" and you have +Shift to extend and +Ctrl to subtract.

use the correct handle size for selection

Overall this seems good to me, I would remove selecting "other side" when deselecting - feels odd to me, I would rather prefer no action.

remove "opposite handle" logic

Functionality seems OK to me.
I don't want to turn this patch to cleanup/refactor, but I left inlines that could be done here.

source/blender/editors/space_sequencer/sequencer_select.c
1047	This 0.75f factor could be defined by some name.
1050	`if (box_rectf.xmax > right_handle_xmin)` would be self-explanatory same for left

This revision is now accepted and ready to land.Dec 15 2019, 11:34 PM

Alessio Monti di Sopra (a.monti) added inline comments.Dec 18 2019, 12:50 PM

source/blender/editors/space_sequencer/sequencer_select.c
1047	This comes from here: https://developer.blender.org/diffusion/B/browse/master/source/blender/editors/space_sequencer/sequencer_draw.c$482 The original `handsize_clamped` value is used to define whether to draw text and its position, and for the actual handles drawing it gets cropped to a square and used to get the correct proportions for the small triangles: https://developer.blender.org/diffusion/B/browse/master/source/blender/editors/space_sequencer/sequencer_draw.c$965 I don't honestly know how to define that 0.75 factor here in a meaningful way...
1050	I've changed the selection box name to `box_rectf` and also the sequence one to `seq_rectf` (was called `rq`), but I'm not sure it's worth to define two floats that are going to be used only once. What if the comments above were something like this? /* Right Handle - Selection box extends past the right handle's xmin. / / Left Handle - Selection box starts before the left handle's xmax. */ Do you think it would be clear enough?

Richard Antalik (ISS) added inline comments.Dec 18 2019, 1:31 PM

source/blender/editors/space_sequencer/sequencer_select.c
1047	When I read that line, I had to look elswhere in the code to see if same factor is used just to see what it means and still it wasn't obvious. So if it is used as drawing aid it could be `#define SEQ_HANDLE_SIZE_DRAW_FACTOR 0.75f` with comment further explaining usage if not obvious.
1050	From performance perspective, it likely doesn't matter here. If code reads better with those 2 floats, than it's an improvement. My point was to make comments unnecessary if possible(reasonable) by making code more readable.

Alessio Monti di Sopra (a.monti) added inline comments.Dec 18 2019, 4:41 PM

source/blender/editors/space_sequencer/sequencer_select.c
1047	I think it could be better to just invert the logic in the drawing function, so that `handsize_clamped` is the correct draw size and you wouldn't need to divide it, and then you'd add a margin for the text drawing, like this: P1190 What do you think about it?
1050	Ok, I will add those then.