Page MenuHome

Sequencer: Preview frame selected operator
ClosedPublic

Authored by ok what (ok_what) on Mar 2 2022, 12:04 PM.

Details

Summary

This operator moves the view to show the selected visible strips.

Diff Detail

Repository
rB Blender

Event Timeline

ok what (ok_what) requested review of this revision.Mar 2 2022, 12:04 PM
ok what (ok_what) created this revision.

Overall patch looks good, but we should not introduce duplicate operators for same functionality within one area. Ideally in sequencer_view_selected_exec() you should check on which area you operate if needed. Best is if code can be mostly identical for both areas. if it can't you can at least dispatch to region specific functions there, see sequencer_select_exec for example, even though it's not cleanest code:

if (region->regiontype == RGN_TYPE_PREVIEW) {
  seq = seq_select_seq_from_preview(C, mval, toggle, extend, center);
}
else {
  seq = find_nearest_seq(scene, v2d, &handle_clicked, mval);
}

For selection I would use selected_strips_from_context() - all (new at least) operators should use this function to keep behavior consistent. If all strips are required, than all_strips_from_context() should be used.

Lastly operators working in preview have code

if (sequencer_view_has_preview_poll(C) && !sequencer_view_preview_only_poll(C)) {
  return OPERATOR_CANCELLED;
}

which prevents them from working in combined view, because there it may not be clear from context what operator should do. See T92584.

I used SEQUENCER_OT_view_all and SEQUENCER_OT_view_all_preview as reference. I assume they are separate to have separate keymaps for each area, but I can see that being unnecessary, so I can change that.

In addition to those, it probably makes sense to add a padding to the area that's zoomed to, since most editors have that.

I've now updated the patch to merge them into one operator.

I also added a 10% padding like in the node editor.

I realized that a second operator isn't necessary for separate keybindings, so the keybindings are still kept separate.

Richard Antalik (ISS) requested changes to this revision.Apr 5 2022, 3:57 PM

Sorry for delay, Patch seems to work well, got some mainly cosmetic issues though.

source/blender/editors/space_sequencer/sequencer_view.c
265

I don't think you have to declare argument types with struct. These are defined in various headers. Since this is not done normally within codebase, then you shouldn't do that

275

Comments should start with capital letter and end with fullstop. See https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments

279

I would clarify both function names so seq_view_collection_rect_timeline

300–302

See below for clarification - as this patch stands, this is not needed and function can have void return type

348–350

Since you check if (SEQ_collection_len(strips) == 0) above, this will never happen if I am not mistaken, so checking status can be removed.

source/blender/sequencer/intern/strip_transform.c
513

This should have SEQ_image_transform prefix, otherwise I wouldn't be able to tell from name if this is boundbox in timeline or what exactly

This revision now requires changes to proceed.Apr 5 2022, 3:57 PM
ok what (ok_what) updated this revision to Diff 50413.EditedApr 13 2022, 8:32 PM
  • Address inlines.
  • Fix bug leading to division by zero.
  • Clang format
ok what (ok_what) marked 6 inline comments as done.Apr 13 2022, 8:42 PM
ok what (ok_what) added inline comments.
source/blender/editors/space_sequencer/sequencer_view.c
348–350

Yep. That was by mistake.
A similar check was missing from the preview one leading to a division by zero if framing a strip with the scale of 0. That should now be fixed. This bug also seems to exist in the UV editor.

ok what (ok_what) marked an inline comment as done.Apr 13 2022, 8:43 PM
This revision is now accepted and ready to land.Apr 19 2022, 11:35 PM
This revision was automatically updated to reflect the committed changes.