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

Changeset View

Standalone View

View Options

source/blender/editors/space_sequencer/sequencer_select.c

Context not available.
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;
Context not available.
switch (left_right) { switch (left_right) {
case SEQ_SELECT_LR_MOUSE: case SEQ_SELECT_LR_MOUSE:
/* Ctrl + left mouse will select left or right */
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…
/* Ctrl + left mouse close to playhead will select under playhead */
if ((x >= (CFRA - 5.0f)) && (x <= (CFRA + 5.0f))) {
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:
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);
} }
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…
Context not available.
{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 left"},
{SEQ_SELECT_LR_RIGHT, "RIGHT", 0, "Right", "Select right"}, {SEQ_SELECT_LR_RIGHT, "RIGHT", 0, "Right", "Select right"},
{SEQ_SELECT_LR_UNDER, "UNDER", 0, "Under", "Select under"},
{0, NULL, 0, NULL, NULL}, {0, NULL, 0, NULL, NULL},
}; };
PropertyRNA *prop; PropertyRNA *prop;
Context not available.