Page MenuHome

VSE: add option to select handles with box selection
AcceptedPublic

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

Details

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

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?

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
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?

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.

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.