Page MenuHome
Differential D7679 Diff 24958 source/blender/editors/space_sequencer/sequencer_select.c

Changeset View

Standalone View

View Options

source/blender/editors/space_sequencer/sequencer_select.c

Show First 20 Lines • Show All 406 Lines • ▼ Show 20 Lines if (marker) {
else { else {
/* XXX, in 2.4x, seq selection used to deselect all, need to re-thnik this for 2.5 */ /* XXX, in 2.4x, seq selection used to deselect all, need to re-thnik this for 2.5 */
/* deselect_markers(0, 0); */ /* deselect_markers(0, 0); */
marker->flag |= SELECT; marker->flag |= SELECT;
} }
ret_value = OPERATOR_FINISHED; ret_value = OPERATOR_FINISHED;
} }
/* Select left, right or under playhead. */
else if (left_right != SEQ_SELECT_LR_NONE) { else if (left_right != SEQ_SELECT_LR_NONE) {
/* Use different logic for this. */ /* Use different logic for this. */
float x; float x;
if (extend == false) { if (extend == false) {
ED_sequencer_deselect_all(scene); ED_sequencer_deselect_all(scene);
} }
switch (left_right) { switch (left_right) {
case SEQ_SELECT_LR_MOUSE: case SEQ_SELECT_LR_MOUSE:
/* 10px margin around playhead to select under playhead with mouse. */
float margin = BLI_rctf_size_x(&v2d->cur) / BLI_rcti_size_x(&v2d->mask) * 10;
x = UI_view2d_region_to_view_x(v2d, mval[0]); x = UI_view2d_region_to_view_x(v2d, mval[0]);
ISSUnsubmitted
Done
Inline Actions

What is this supposed to do? And why?

ISS: What is this supposed to do? And why?
tintwotinAuthorUnsubmitted
Done
Inline Actions

In the original function ctrl+left mouse to the left or right of playhead will select "Left" or "Right".

What is added here is ctrl+left mouse close to playhead will select "Center".

tintwotin: In the original function ctrl+left mouse to the left or right of playhead will select "Left" or…
ISSUnsubmitted
Done
Inline Actions

I guess that would be OK. It looks like it should do that, but if I just looked at code without any context, I would have no clue. So please add some comment that this is some kind of dead zone to allow 3 modes be done with mouse.

My main issue then is with implementation:

  • Use if () not if (())
  • CFRA +/- 5.0f will make dead zone 10 frames wide, which is dependant on zoom.

make variable float under_playhead_margin = UI_view2d_view_to_region_x(v2d, 5.0f) and use that instead of 5.0f value

ISS: I guess that would be OK. It looks like it should do that, but if I just looked at code without…
ISSUnsubmitted
Done
Inline Actions

Oops float under_playhead_margin = UI_view2d_view_to_region_x(v2d, 5.0f) was wrong, but I have fixed it.

ISS: Oops `float under_playhead_margin = UI_view2d_view_to_region_x(v2d, 5.0f)` was wrong, but I…
if (x >= CFRA - margin && x <= CFRA + margin) {
x = CFRA;
}
break; break;
case SEQ_SELECT_LR_LEFT: case SEQ_SELECT_LR_LEFT:
x = CFRA - 1.0f; x = CFRA - 1.0f;
break; break;
case SEQ_SELECT_LR_RIGHT: case SEQ_SELECT_LR_RIGHT:
x = CFRA + 1.0f;
break;
case SEQ_SELECT_LR_UNDER_PLAYHEAD:
default: default:
x = CFRA; x = CFRA;
break; break;
} }
SEQP_BEGIN (ed, seq) { SEQP_BEGIN (ed, seq) {
if (((x < CFRA) && (seq->enddisp <= CFRA)) || ((x >= CFRA) && (seq->startdisp >= CFRA))) { /* Select under playhead. */
if ((x == CFRA) && (seq->startdisp <= CFRA) && (seq->enddisp >= CFRA)) {
seq->flag = SELECT;
recurs_sel_seq(seq);
}
ISSUnsubmitted
Done
Inline Actions

I would rather have 3 functions here and comments what particular branch does and if it is linked to any option.

Separate function with descriptive names would be better perhaps but given this function is already undocumented and in quite bad shape, just comment is OK.

ISS: I would rather have 3 functions here and comments what particular branch does and if it is…
/* Select left or right. */
else if ((x < CFRA && seq->enddisp <= CFRA) || (x > CFRA && seq->startdisp >= CFRA)) {
seq->flag |= SELECT; seq->flag |= SELECT;
recurs_sel_seq(seq); recurs_sel_seq(seq);
} }
} }
SEQ_END; SEQ_END;
{ {
SpaceSeq *sseq = CTX_wm_space_seq(C); SpaceSeq *sseq = CTX_wm_space_seq(C);
▲ Show 20 Lines • Show All 174 Lines • ▼ Show 20 Linesstatic int sequencer_select_exec(bContext *C, wmOperator *op)
WM_event_add_notifier(C, NC_SCENE | ND_SEQUENCER | NA_SELECTED, scene); WM_event_add_notifier(C, NC_SCENE | ND_SEQUENCER | NA_SELECTED, scene);
return ret_value; return ret_value;
} }
void SEQUENCER_OT_select(wmOperatorType *ot) void SEQUENCER_OT_select(wmOperatorType *ot)
{ {
static const EnumPropertyItem sequencer_select_left_right_types[] = { static const EnumPropertyItem sequencer_select_left_right_types[] = {
ISSUnsubmitted
Done
Inline Actions

Option "center" doesn't really make sense. I guess something like "intersecting" or "under playhead" would be better.

ISS: Option "center" doesn't really make sense. I guess something like "intersecting" or "under…
tintwotinAuthorUnsubmitted
Done
Inline Actions

Since the menu is called Playhead > Left/Right, I guess "Under" would be enough?

tintwotin: Since the menu is called Playhead > Left/Right, I guess "Under" would be enough?
ISSUnsubmitted
Done
Inline Actions

I think it's enough for UI, though it would be better if it was called explicitly under playhead when used in code - it's more readable.
{SEQ_SELECT_LR_UNDER_PLAYHEAD, "UNDER_PLAYHEAD", 0, "Under", "Select under"},

Even description for those 3 options could be updated to Select left to playhead, Select right to playhead and Select under playhead

After all these operators could exist outside of menu providing context (custom addon) where it is nicer to hover on top of button to see what it actually does.

ISS: I think it's enough for UI, though it would be better if it was called explicitly under…
{SEQ_SELECT_LR_NONE, "NONE", 0, "None", "Don't do left-right selection"}, {SEQ_SELECT_LR_NONE, "NONE", 0, "None", "Don't do left-right selection"},
{SEQ_SELECT_LR_MOUSE, "MOUSE", 0, "Mouse", "Use mouse position for selection"}, {SEQ_SELECT_LR_MOUSE, "MOUSE", 0, "Mouse", "Use mouse position for selection"},
{SEQ_SELECT_LR_LEFT, "LEFT", 0, "Left", "Select left"}, {SEQ_SELECT_LR_LEFT, "LEFT", 0, "Left", "Select to the left of the playhead"},
{SEQ_SELECT_LR_RIGHT, "RIGHT", 0, "Right", "Select right"}, {SEQ_SELECT_LR_RIGHT, "RIGHT", 0, "Right", "Select to the right of the playhead"},
{SEQ_SELECT_LR_UNDER_PLAYHEAD, "UNDER", 0, "Under", "Select under the playhead"},
{0, NULL, 0, NULL, NULL}, {0, NULL, 0, NULL, NULL},
}; };
PropertyRNA *prop; PropertyRNA *prop;
/* Identifiers. */ /* Identifiers. */
ot->name = "Select"; ot->name = "Select";
ot->idname = "SEQUENCER_OT_select"; ot->idname = "SEQUENCER_OT_select";
ot->description = "Select a strip (last selected becomes the \"active strip\")"; ot->description = "Select a strip (last selected becomes the \"active strip\")";
▲ Show 20 Lines • Show All 847 Lines • Show Last 20 Lines