fix(snap): add includePlayhead param to collectTargets, match upstream (#86)#138
Open
H-Chris233 wants to merge 2 commits into
Open
fix(snap): add includePlayhead param to collectTargets, match upstream (#86)#138H-Chris233 wants to merge 2 commits into
H-Chris233 wants to merge 2 commits into
Conversation
appergb#86) - collectTargets 新增 includePlayhead=false 参数(上游默认行为) - 剃刀切(razor) 显式传 true -> playhead 仍可吸附 - 移动/修剪 用默认 false -> 不吸附 playhead(匹配上游) Issue: appergb#86
appergb
requested changes
Jun 24, 2026
appergb
left a comment
Owner
There was a problem hiding this comment.
@H-Chris233 感谢 PR。新增 includePlayhead 形参的方向是对的,但 move/trim 的取值与上游相反,这版实际偏离了上游而不是对齐。
我逐行核对了上游 Timeline/TimelineInputController.swift 的全部 SnapEngine.collectTargets(...) 调用方:
| 上游操作 | includePlayhead |
位置 |
|---|---|---|
| timelineRange 框选 | true | TimelineInputController.swift:216 |
| moveClip 移动 | true | :241 |
| trimLeft 修剪 | true | :287 |
| trimRight 修剪 | true | :316 |
| razor 切割 | true | :513(以及 TimelineView.swift razor 预览) |
| 外部拖入新素材 | false(默认) | TimelineView.swift:948 applyExternalSnap |
也就是说:上游里 move/trim 拖拽是吸附 playhead 的(true),唯一用默认 false 的是"从媒体面板把新素材拖进时间线"那条路径(applyExternalSnap)。
本 PR 的处理:
- razor split(
TimelineContainer.tsx:346)→ 显式true✅ 正确,与上游一致。 - move(
TimelineContainer.tsx:435)、trim(:464)→ 保持 3 参调用,落到新默认false❌ —— 这会移除上游本来就有的"拖拽吸附播放头"行为。改动前 OpenTake 这两处已经和上游一致(吸附 playhead),这版把它改坏了。
PR 描述里"Move drag → false(修正旧 bug)""Trim drag → false"是对上游的误读:上游 includePlayhead: Bool = false 只是函数签名默认值,而所有交互式拖拽调用方都显式传了 true。
需要的修改
TimelineContainer.tsx:435(move)与:464(trim)显式传includePlayhead: true,对齐TimelineInputController.swift:241/287/316。includePlayhead=false仅应留给"外部素材拖入"路径(对齐TimelineView.swift:948)。OpenTake 当前的 3 个collectTargets调用点都不是该路径,所以三处都应吸附 playhead。- 若你观察到 move/trim 有真实的吸附 bug,根因更可能在阈值/sticky(
SNAP.stickyMultiplier、threshold)而非includePlayhead——可另开 issue,不要用关闭 playhead 吸附来掩盖。
形参本身保留没问题,改对取值即可。CI 已双绿,改完我再复审。
This was referenced Jun 24, 2026
Upstream TimelineInputController.swift shows ALL interactive drag operations (razor, move, trimLeft, trimRight, timelineRange) pass includePlayhead=true. The only false case is external-drag-in (TimelineView.swift:948 applyExternalSnap), which OpenTake does not yet have.
Collaborator
Author
|
已按上游 TimelineInputController.swift 全部调用方修正:
三处交互拖拽全对齐上游 (:216/:241/:287/:316/:513)。 CI 双绿,请复审。 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
1:1 port of upstream SnapEngine.swift includePlayhead behavior.
Change: collectTargets 新增 includePlayhead = false 参数,匹配上游默认不吸附 playhead。
Callers:
Aligns with: Rust SnapEngine::collect_targets(include_playhead: bool), Swift SnapEngine.swift:36-48
Closes #86