Skip to content

fix(snap): add includePlayhead param to collectTargets, match upstream (#86)#138

Open
H-Chris233 wants to merge 2 commits into
appergb:mainfrom
H-Chris233:fix/snap-include-playhead-86
Open

fix(snap): add includePlayhead param to collectTargets, match upstream (#86)#138
H-Chris233 wants to merge 2 commits into
appergb:mainfrom
H-Chris233:fix/snap-include-playhead-86

Conversation

@H-Chris233

Copy link
Copy Markdown
Collaborator

1:1 port of upstream SnapEngine.swift includePlayhead behavior.

Change: collectTargets 新增 includePlayhead = false 参数,匹配上游默认不吸附 playhead。

Callers:

  • Razor split → true (可吸附 playhead + clip 边)
  • Move drag → false (仅吸附 clip 边,修正旧 bug)
  • Trim drag → false (同上)

Aligns with: Rust SnapEngine::collect_targets(include_playhead: bool), Swift SnapEngine.swift:36-48

Closes #86

appergb#86)

- collectTargets 新增 includePlayhead=false 参数(上游默认行为)
- 剃刀切(razor) 显式传 true -> playhead 仍可吸附
- 移动/修剪 用默认 false -> 不吸附 playhead(匹配上游)

Issue: appergb#86
@H-Chris233 H-Chris233 requested a review from appergb as a code owner June 24, 2026 04:48
@H-Chris233 H-Chris233 assigned H-Chris233 and unassigned H-Chris233 Jun 24, 2026

@appergb appergb left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

需要的修改

  1. TimelineContainer.tsx:435(move)与 :464(trim)显式传 includePlayhead: true,对齐 TimelineInputController.swift:241/287/316
  2. includePlayhead=false 仅应留给"外部素材拖入"路径(对齐 TimelineView.swift:948)。OpenTake 当前的 3 个 collectTargets 调用点都不是该路径,所以三处都应吸附 playhead。
  3. 若你观察到 move/trim 有真实的吸附 bug,根因更可能在阈值/sticky(SNAP.stickyMultiplier、threshold)而非 includePlayhead——可另开 issue,不要用关闭 playhead 吸附来掩盖。

形参本身保留没问题,改对取值即可。CI 已双绿,改完我再复审。

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.
@H-Chris233

Copy link
Copy Markdown
Collaborator Author

已按上游 TimelineInputController.swift 全部调用方修正:

  • razor (:346): true ✅ 不变
  • move (:435): true ✅ 已修 (原误落 default false)
  • trim (:464): true ✅ 已修 (同上)

三处交互拖拽全对齐上游 (:216/:241/:287/:316/:513)。includePlayhead=false 仅留给"外部素材拖入"路径 (TimelineView.swift:948),当前无该路径故无调用方用默认值。

CI 双绿,请复审。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[P2] Snap 逻辑中 includePlayhead 行为与上游有差异

2 participants