Fix edit algorithms deleting the wrong child via remove_child(<pointer>)#2025
Conversation
Composition only declares `remove_child(int index, ErrorStatus*)`, but
`overwrite()` and `insert()` call it with a `Composable*` / `Retainer`:
composition->remove_child(transition); // overwrite() and insert()
composition->remove_child(items.back()); // overwrite()
The pointer argument decays to `bool` (always true) -> `int(1)`, so these calls
remove the child at index 1 (or, via adjusted_vector_index, the last child when
size == 1), NOT the item that was passed. As a result:
- overwrite() over a multi-item span deletes the wrong clips: the
partially-overwritten clip is removed while a fully-overwritten clip survives.
- transition removal in overwrite()/insert() removes index 1 instead of the
transition.
Pass the already-computed (and bounds-checked) `index` for transitions, and the
index of `items.back()` in the removal loop.
Repro (overwrite A|M|B with X over a span that ends at the track end):
before: A[0+5] B[20+5] X[2+10] (M removed, B kept -- wrong)
after: A[0+5] M[10+3] X[2+10] (M trimmed, B removed -- correct)
Adds a regression test (fails before, passes after); the existing
test_editAlgorithm suite continues to pass.
Signed-off-by: Troy Hernandez <troy.hernandez@pm.me>
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2025 +/- ##
==========================================
- Coverage 84.33% 84.33% -0.01%
==========================================
Files 181 181
Lines 13267 13268 +1
Branches 1214 1214
==========================================
Hits 11189 11189
- Misses 1895 1896 +1
Partials 183 183
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
Oh, good catch! Would you mind adding (cc @meshula to check this) |
…child-wrong-index Signed-off-by: Troy Hernandez <troy.hernandez@pm.me>
Requested in PR review: the implicit Retainer -> bool -> int conversion chain is what allowed remove_child(<Retainer>) to compile and silently remove the child at index 1. Marking the conversion explicit turns that mistake into a compile error, while contextual conversion keeps if (retainer), &&, and friends working unchanged. The library and all C++ tests build cleanly with no call-site changes needed, and the full test suite passes. Signed-off-by: Troy Hernandez <troy.hernandez@pm.me>
|
Done in 6670f49 (also merged main to pick up #2020, which touches the same functions).
|
Composition only declares
remove_child(int index, ErrorStatus*), butoverwrite()andinsert()ineditAlgorithm.cppcall it with aComposable*/Retainer:The pointer argument decays to
bool(alwaystrue) →int(1), so these calls remove the child at index 1 (or, viaadjusted_vector_index, the last child whensize() == 1), not the item that was passed.Impact
overwrite()over a multi-item span deletes the wrong clips: the partially-overwritten clip is removed while a fully-overwritten clip survives in place.overwrite()/insert()removes index 1 instead of the intended transition.Repro
Track
A|M|B(each 5/8/5 frames),overwrite(X, track, [8, 18))— a span that partially coversMand fully coversBto the track end:Fix
Pass the already-computed (and bounds-checked)
indexfor transitions, andindex_of_child(items.back())in the removal loop.Tests
test_edit_overwrite_partial_first_full_last, which fails before this change and passes after.test_editAlgorithmsuite continues to pass.I verified the fix and the regression test by building the library locally (the test fails on the unfixed tree, passes on the fixed tree).