VSE: handles fixes and cleanup
Needs ReviewPublic
Actions

Authored by Alessio Monti di Sopra (a.monti) on Apr 11 2020, 9:22 PM.

Details

Reviewers

Richard Antalik (ISS)
Campbell Barton (campbellbarton)

Summary

A couple of fixes regarding handles drawing and selection due to changes in my previous patches:

Now text alignment is correct for effect strips that have inputs (and so no handles drawn)
select_box(handles) now skips those strips, and also locked ones
select() now avoid selecting locked strips' handles

Small clean-up in sequencer_draw.c
- sequence_handle_size_get_clamped() --> sequence_handle_pixelsize_get()
- handsize_clamped --> handsize
- Pass handsize as an argument of calculate_seq_text_offsets() instead of calculating it again

In future please don't mix code cleanup with functional changes, mixing in these changes slows down code review.

Actual changes here look OK, however code could be more straightforward.

source/blender/editors/space_sequencer/sequencer_draw.c
1079	Is there any reason for this to be set non-zero when `SEQ_LOCK` is enabled?
source/blender/editors/space_sequencer/sequencer_edit.c
418	Why is this removed?
source/blender/editors/space_sequencer/sequencer_select.c
1032	If `BKE_sequence_tx_test(seq) && !(seq->flag & SEQ_LOCK)` is synonymous with "show handles" we could have a function `BKE_sequence_has_handles(seq)` instead, for increased readability.

This revision now requires changes to proceed.Apr 16 2020, 12:45 PM

In future please don't mix code cleanup with functional changes, mixing in these changes slows down code review.

Sorry about that, I hoped it was clear enough from the patch description.

source/blender/editors/space_sequencer/sequencer_draw.c
1079	Not really actually, it was just because handles code were skipped completely for locked strips. But it may be better to do the same as for effects, don't draw the handles but still draw the start/end frame labels when other selected strips are being moved.
source/blender/editors/space_sequencer/sequencer_edit.c
418	Before the other patch it was doing the same thing as `sequence_handle_size_get_clamped()`(handle min/max draw sizes were defined in `sequencer_draw.c` as 3/4 of `7pixelx` and `40pixel` respectively). Now it basically does nothing, since the max for `handsize` here is `2 * 8 * pixelx`, while on the min side the only difference should be 1px at one zoom level just before the other condition becomes true. If you prefer we could keep it as a clamp_min but I don't thing it would make much difference in practice.
source/blender/editors/space_sequencer/sequencer_select.c
1032	Ok, I will add it. Is it ok to call `BKE_sequence_tx_test(seq)` there or would it be better to make the condition checks more explicit, copying them over?

source/blender/editors/space_sequencer/sequencer_select.c
1032	The issue with checking `SEQ_LOCK` within `BKE_sequence_tx_test`, is it's possible it may cause other problems. The strips can be transformed (indirectly, within a meta-strip for e.g), they just happen to be locked, making the transform operator skip them. So I think it would make the code more readable to have a `BKE_sequence_has_handles` function, even if internally it happens to use `BKE_sequence_tx_test`.

Thanks @Campbell Barton (campbellbarton) for review, I have only 1 suggestion and 1 question now.

source/blender/editors/space_sequencer/sequencer_draw.c
532	I guess this is only visual change? Not sure about reason behind this change
1079–1080	Wouldn't it be better if `sequence_handle_pixelsize_get()` would return 0 if `BKE_sequence_has_handles()` is false?

source/blender/editors/space_sequencer/sequencer_draw.c
532	Not really, it's necessary since now `handsize` can be zero.
1079–1080	If so though, to avoid double checks, in the selection functions the condition would need to be `if (handsize > 0.0f) {select}`, and it seemed less clear to me.

Richard Antalik (ISS) added inline comments.

source/blender/editors/space_sequencer/sequencer_draw.c
532	Ok I see purpose now - this will provide 2px offset from handle or strip start/end
1079–1080	In this case I am more concerned about readability than double checks. So I wouldn't want you to change select functions. I have 2 reasons: I would expect `sequence_handle_pixelsize_get()` to not require additional checks before inspecting it in detail I don't like ternary operators Not requesting changes here really, just suggesting my preference.