So with no extend/ripple(+alt), the strip end will overwrite or else ripple? Then the snap point will be quite important, to avoid overwriting when users just want to extend to next strip.

A thought on all of the snapping options, I hope you'll make a snap-all default, so when a casual user clicks the magnet, it'll snap at all points.

In D11646#299435, @Peter Fog (tintwotin) wrote:

So with no extend/ripple(+alt), the strip end will overwrite or else ripple? Then the snap point will be quite important, to avoid overwriting when users just want to extend to next strip.

Idea is to make process of actual overwriting of footage simpler, so you don't have to adjust 2 strips but only one. I haven't tried that in practice how that works though. Probably won't work great for me, but I don't think it would break my current workflow. It shouldn't break current workflow at all really. Once I make patch it can be tested.

A thought on all of the snapping options, I hope you'll make a snap-all default, so when a casual user clicks the magnet, it'll snap at all points.

Sure, there should be sensible defaults.

Typically in NLEs, there would be button to toggle between insert(ripple) and overwrite for trimming, adding material and drag and drop.

There seems to be a small inaccuracy on snapping on hold:

Also normal splits seem to snap to an added distance of plus one frame, making the strip outline red.

Btw. the snapping function isn't written out in a word in the UI of the other editors.
Layout:

In D11646#299521, @Peter Fog (tintwotin) wrote:

There seems to be a small inaccuracy on snapping on hold:

Can you provide your file? I can't reproduce this issue

Could it be because the snap_threshold was set very high?

In D11646#299594, @Peter Fog (tintwotin) wrote:

Could it be because the snap_threshold was set very high?

Actually I think I can reproduce this with 2 color strips, but not video strips, will check where the problem is

@Germano Cavalcante (mano-wii) I want to implement simple visual indication of where snapping ocurs, would you recommend to add that here? otherwise this should be ready to review.

Is is working really well. Thank you for this major improvement of the UX.

One minor thing I noticed is when having a very long timeline range ex. 100000 frames and you zoom out to this range, the 15px doesn't seem to take the zoom level into account. I don't know if it should, but moving anything at this zoom level will snap everything when you enter transform. Maybe this is related to this: T85868

On the UI, this is "before":

If you want to simplify the UI like this:

(I removed the "Snapping" next to the icon, since in ex. 3D View there is no word to the snapping drop down in the header. And tried to remove the redundant texts.)

Looking at it again, maybe Distance can be placed on top, so there is no misunderstanding of the "Distance" somehow should be related to "Ignore"?

Here's a bit of code:

class SEQUENCER_PT_snapping(Panel):
    bl_space_type = 'SEQUENCE_EDITOR'
    bl_region_type = 'HEADER'
    bl_label = ""

    def draw(self, context):
        tool_settings = context.tool_settings.sequencer_tool_settings

        layout = self.layout
        layout.use_property_split = True
        layout.use_property_decorate = False

        col = layout.column(heading="Snap to", align=True)
        col.prop(tool_settings, "snap_to_strip_hold_offset", text="Hold Offset")
        col.prop(tool_settings, "snap_to_playhead", text="Playhead")

        col = layout.column(heading="Ignore", align=True)
        col.prop(tool_settings, "snap_ignore_muted", text="Muted Strips")
        col.prop(tool_settings, "snap_ignore_sound",text="Sound Strips")

        col = layout.column()
        col.prop(tool_settings, "snap_distance", slider=True, text="Distance")

In D11646#300491, @Richard Antalik (ISS) wrote:

@Germano Cavalcante (mano-wii) I want to implement simple visual indication of where snapping ocurs, would you recommend to add that here? otherwise this should be ready to review.

If not essential, it's better in a separate patch. (This even helps when organizing commits).

Looking at the patch again, I noticed things that were outside convention.
If transform_snap_sequencer_apply is only used for transform_mode_edge_seq_slide mode, I see no reason to spread the definitions across the transform code. Everything should be defined inside transform_mode_edge_seq_slide.c.`
Custom data used only by the mode are stored in t->custom.mode.data

If this type of snap is intended to be shared in different modes (like in transform_mode_translate.c), it should set the callbacks t->tsnap.calcSnap and t->tsnap.targetSnap.
The existing custom sequencer snap code itself looks like it should also just be in the mode it's used for. (That was a slip at the time of splitting all the transform code into multiple files).
(I should have noticed this before)

In D11646#300500, @Peter Fog (tintwotin) wrote:

Is is working really well. Thank you for this major improvement of the UX.

One minor thing I noticed is when having a very long timeline range ex. 100000 frames and you zoom out to this range, the 15px doesn't seem to take the zoom level into account.

I can't reproduce this issue even on minimum possible zoom level

If you want to simplify the UI like this:

(I removed the "Snapping" next to the icon, since in ex. 3D View there is no word to the snapping drop down in the header. And tried to remove the redundant texts.)

I think this looks better, so I will use this layout. Thanks for suggestion.

In D11646#300558, @Germano Cavalcante (mano-wii) wrote:

In D11646#300491, @Richard Antalik (ISS) wrote:

@Germano Cavalcante (mano-wii) I want to implement simple visual indication of where snapping ocurs, would you recommend to add that here? otherwise this should be ready to review.

Looking at the patch again, I noticed things that were outside convention.
If transform_snap_sequencer_apply is only used for transform_mode_edge_seq_slide mode, I see no reason to spread the definitions across the transform code. Everything should be defined inside transform_mode_edge_seq_slide.c.`
Custom data used only by the mode are stored in t->custom.mode.data

If this type of snap is intended to be shared in different modes (like in transform_mode_translate.c), it should set the callbacks t->tsnap.calcSnap and t->tsnap.targetSnap.
The existing custom sequencer snap code itself looks like it should also just be in the mode it's used for. (That was a slip at the time of splitting all the transform code into multiple files).
(I should have noticed this before)

This snapping probably can be shared with extend feature. What is convention then - to have separate file for snapping functions? Or reuse snapping from different mode, but snapping functions should be defined in file for one of transform modes?
I will check how t->tsnap.calcSnap and t->tsnap.targetSnap works too.

Here are the changes I made in the patch to make the transform code more integrated:

With this changes the sequencer snap works in other modes.
(There were other things I saw that could change, but it's not important).

This is one way if you want to make the sequencer snap system more generic in the transform code.

Note: I am seeing memory leaks with the original patch:

Error: Not freed memory blocks: 5, total unfreed memory 0.001137 MB
SeqCollection len: 24 00000177F4DF9438
SeqCollection GSet len: 64 00000177F5C6C978
ghash_buckets_resize len: 40 00000177F478E1F8
memory pool len: 48 00000177F4790038
BLI_Mempool Chunk len: 1016 00000177DF6ADF58

I think your approach should work for this purpose.

I was confused by API though - why would you use targetSnap callback if you set it always to CalcSnapGeometry which that dispatches actions based on t->spacetype? I assumed it was meant to be assigned directly to snapping function for particular mode.

In any case I will look at this mem leaks.

Some problems I only noticed after seeing a second time.

In D11646#302108, @Germano Cavalcante (mano-wii) wrote:

Some problems I only noticed after seeing a second time.

I haven't gone over all changes yet too, so if I found some issue I will fix.

Thanks for review!