This operator moves the view to show the selected visible strips.
Details
Diff Detail
- Repository
- rB Blender
Event Timeline
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.
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 | |
| source/blender/editors/space_sequencer/sequencer_view.c | ||
|---|---|---|
| 348–350 | Yep. That was by mistake. | |