john/McRogueFace
1
0
Fork 0
Code Issues 43 Pull requests Projects Releases Wiki Activity

fix(ui): maintain the parent link in every collection mutator

Browse source 
The parent link (#122/#183) is meant to be the inverse of collection
membership, but only append() maintained it.

insert(), extend() and setitem() called setParent(owner.lock())
unconditionally. A scene collection has no owner drawable, so that
resolved to null -- and setParent(nullptr) also clears parent_scene. The
drawable landed in the scene's UI vector, rendered fine, and reported a
.parent of None.

The slice arms never touched the link at all. A slice-delete left the
removed child pointing at a parent that no longer contained it; a
slice-assign never parented the incoming items, nor unparented the ones
they displaced. A stale parent is a live weak_ptr to a frame the child
has left, and it is what get_global_position() walks.

Route every mutator through link_child()/unlink_child(), which are now
the single place that knows a scene parents via setParentScene() and a
drawable via setParent().

Three latent traps in the same functions, found while fixing this:

  * collection[-1] on an EMPTY collection hung the engine forever:
    `while (index < 0) index += size()` adds zero on every pass.
  * getitem's `index > size() - 1` check underflowed to SIZE_MAX when
    size() was 0, so the bounds check passed and it indexed an empty
    vector.
  * insert() and setitem() resolved the index before detaching the
    incoming drawable from its old parent. If it was already a member of
    the same collection, detaching erased it and shifted everything
    after -- so an index validated against the old size could write past
    the end. Slice-assign now builds its result as an independent vector
    for the same reason.

closes #377

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
John McCardle 2026-07-14 07:53:21 -04:00
commit 53234288ad
3 changed files with 370 additions and 56 deletions
Download patch file Download diff file Expand all files Collapse all files

2
api/manifest.json
View file

File diff suppressed because one or more lines are too long

181
src/UICollection.cpp
View file

@ -23,6 +23,31 @@ static PyObject* convertDrawableToPython(std::shared_ptr<UIDrawable> drawable) {
return UIDrawable::pyobject_for(drawable);
}
// #377: the parent link is the inverse of collection membership, so EVERY mutator
// has to maintain it -- not just append(), which was the only one that did. These
// two helpers are the single place that knows how: a scene collection parents via
// setParentScene() (it has no owner drawable, so owner.lock() is null and
// setParent() would silently unparent the child), a drawable-owned one via
// setParent().
//
// #373: they are also the pin points. setParent()/setParentScene() re-evaluate the
// strong ref that keeps a Python subclass wrapper alive while its C++ object is in
// a collection, so linking and unlinking here is what makes the pin correct.
static void link_child(PyUICollectionObject* self, std::shared_ptr<UIDrawable> drawable) {
if (!self->scene_name.empty()) {
drawable->setParentScene(self->scene_name);
} else {
drawable->setParent(self->owner.lock());
}
}
// Call BEFORE erasing from the vector: setParent(nullptr) may drop the last strong
// ref to the wrapper, whose dealloc releases its shared_ptr to the drawable. While
// the vector still holds the element, `drawable` cannot be freed under us.
static void unlink_child(std::shared_ptr<UIDrawable> drawable) {
drawable->setParent(nullptr);
}
// Helper to extract shared_ptr<UIDrawable> from any UIDrawable Python subclass.
// Returns nullptr without setting an error if the type doesn't match.
static std::shared_ptr<UIDrawable> extractDrawable(PyObject* o) {
@ -109,8 +134,12 @@ PyObject* UICollection::getitem(PyUICollectionObject* self, Py_ssize_t index) {
PyErr_SetString(PyExc_RuntimeError, "the collection store returned a null pointer");
return NULL;
}
while (index < 0) index += self->data->size();
if (index > self->data->size() - 1)
// Same two traps as setitem(): the old `while (index < 0) index += size()` spun
// forever on an empty collection, and `index > size() - 1` underflowed to SIZE_MAX
// when size() was 0 -- so the bounds check passed and we indexed an empty vector.
Py_ssize_t size = static_cast<Py_ssize_t>(vec->size());
if (index < 0) index += size;
if (index < 0 || index >= size)
{
PyErr_SetString(PyExc_IndexError, "UICollection index out of range");
return NULL;
@ -128,11 +157,13 @@ int UICollection::setitem(PyUICollectionObject* self, Py_ssize_t index, PyObject
return -1;
}
// Handle negative indexing
while (index < 0) index += self->data->size();
// Handle negative indexing. NOT a `while (index < 0) index += size()` loop: on an
// empty collection that adds zero forever, and `children[-1] = x` hung the engine.
Py_ssize_t size = static_cast<Py_ssize_t>(self->data->size());
if (index < 0) index += size;
// Bounds check
if (index >= self->data->size()) {
if (index < 0 || index >= size) {
PyErr_SetString(PyExc_IndexError, "UICollection assignment index out of range");
return -1;
}
@ -140,7 +171,7 @@ int UICollection::setitem(PyUICollectionObject* self, Py_ssize_t index, PyObject
// Handle deletion
if (value == NULL) {
// #122: Clear the parent before removing
(*self->data)[index]->setParent(nullptr);
unlink_child((*self->data)[index]);
self->data->erase(self->data->begin() + index);
// #288: Invalidate parent Frame's render cache
auto owner_ptr = self->owner.lock();
@ -156,21 +187,30 @@ int UICollection::setitem(PyUICollectionObject* self, Py_ssize_t index, PyObject
PyErr_SetString(PyExc_TypeError, "UICollection can only contain Drawable objects");
return -1;
}
int old_z_index = (*vec)[index]->z_index; // Preserve the z_index
// #183: Remove from old parent, which may be a scene rather than a drawable.
// Do this BEFORE indexing: if the incoming drawable is already a member of THIS
// collection, detaching it erases it from `vec` and shifts every later element,
// so an index validated against the old size could write past the end.
new_drawable->removeFromParent();
if (index >= static_cast<Py_ssize_t>(vec->size())) {
PyErr_SetString(PyExc_IndexError, "UICollection assignment index out of range");
return -1;
}
// Hold a strong ref: unlinking may release the last ref to the old element's
// Python wrapper, whose dealloc drops the wrapper's shared_ptr to it.
std::shared_ptr<UIDrawable> old_drawable = (*vec)[index];
int old_z_index = old_drawable->z_index; // Preserve the z_index
// #122: Clear parent of old element
(*vec)[index]->setParent(nullptr);
// #122: Remove new drawable from its old parent if it has one
if (auto old_parent = new_drawable->getParent()) {
new_drawable->removeFromParent();
}
unlink_child(old_drawable);
// Preserve the z_index of the replaced element
new_drawable->z_index = old_z_index;
// #122: Set new parent
new_drawable->setParent(self->owner.lock());
// #377: parent to the scene, not to a null owner, when this is a scene collection
link_child(self, new_drawable);
// Replace the element
(*vec)[index] = new_drawable;
@ -367,10 +407,16 @@ int UICollection::ass_subscript(PyUICollectionObject* self, PyObject* key, PyObj
// Sort in descending order and delete
std::sort(indices.begin(), indices.end(), std::greater<Py_ssize_t>());
for (Py_ssize_t idx : indices) {
// #377: unlink before erasing -- a slice delete used to leave the
// removed child pointing at a parent that no longer contains it
unlink_child((*self->data)[idx]);
self->data->erase(self->data->begin() + idx);
}
} else {
// Contiguous slice - can delete in one go
for (Py_ssize_t i = start; i < stop; i++) {
unlink_child((*self->data)[i]);
}
self->data->erase(self->data->begin() + start, self->data->begin() + stop);
}
@ -419,38 +465,67 @@ int UICollection::ass_subscript(PyUICollectionObject* self, PyObject* key, PyObj
new_items.push_back(drawable);
}
// Now perform the assignment
if (step == 1) {
// Contiguous slice
if (slicelength != value_len) {
// Need to resize
auto it_start = self->data->begin() + start;
auto it_stop = self->data->begin() + stop;
self->data->erase(it_start, it_stop);
self->data->insert(self->data->begin() + start, new_items.begin(), new_items.end());
} else {
// Same size, just replace
for (Py_ssize_t i = 0; i < slicelength; i++) {
// Preserve z_index
new_items[i]->z_index = (*self->data)[start + i]->z_index;
(*self->data)[start + i] = new_items[i];
}
}
} else {
// Extended slice
if (slicelength != value_len) {
if (step != 1 && slicelength != value_len) {
PyErr_Format(PyExc_ValueError,
"attempt to assign sequence of size %zd to extended slice of size %zd",
value_len, slicelength);
return -1;
}
// #377: build the post-assignment contents as an independent vector before
// touching anything. An incoming item may already be a member of THIS
// collection, and detaching it from its old parent mutates the very vector
// we would otherwise be indexing into -- shifting `start`/`stop` out from
// under the erase. Assembling a copy first makes aliasing a non-issue.
//
// `previous` also holds a strong ref to every displaced element, so
// unlinking one (which may release the last ref to its Python wrapper,
// whose dealloc drops the wrapper's shared_ptr) cannot free it under us.
std::vector<std::shared_ptr<UIDrawable>> previous = *self->data;
std::vector<std::shared_ptr<UIDrawable>> result = previous;
if (step == 1) {
if (slicelength != value_len) {
result.erase(result.begin() + start, result.begin() + stop);
result.insert(result.begin() + start, new_items.begin(), new_items.end());
} else {
for (Py_ssize_t i = 0; i < slicelength; i++) {
new_items[i]->z_index = previous[start + i]->z_index; // Preserve z_index
result[start + i] = new_items[i];
}
}
} else {
for (Py_ssize_t i = 0, cur = start; i < slicelength; i++, cur += step) {
// Preserve z_index
new_items[i]->z_index = (*self->data)[cur]->z_index;
(*self->data)[cur] = new_items[i];
new_items[i]->z_index = previous[cur]->z_index; // Preserve z_index
result[cur] = new_items[i];
}
}
auto contains_ptr = [](const std::vector<std::shared_ptr<UIDrawable>>& v,
const UIDrawable* p) {
return std::any_of(v.begin(), v.end(),
[p](const std::shared_ptr<UIDrawable>& e) { return e.get() == p; });
};
// Detach incoming items from whatever parent they had. Safe to do now:
// this can only mutate `*self->data`, which we are about to overwrite.
for (auto& item : new_items) {
item->removeFromParent();
}
*self->data = result;
// Anything the slice displaced is no longer a member -- drop its parent link.
for (auto& old_item : previous) {
if (!contains_ptr(result, old_item.get())) {
unlink_child(old_item);
}
}
for (auto& item : result) {
link_child(self, item);
}
// Mark scene as needing resort after slice assignment
McRFPy_API::markSceneNeedsSort();
// #288: Invalidate parent Frame's render cache
@ -519,12 +594,7 @@ PyObject* UICollection::append(PyUICollectionObject* self, PyObject* o)
drawable->z_index = new_z_index;
// #183: Set new parent - either scene or drawable
if (!self->scene_name.empty()) {
drawable->setParentScene(self->scene_name);
} else {
std::shared_ptr<UIDrawable> owner_ptr = self->owner.lock();
drawable->setParent(owner_ptr);
}
link_child(self, drawable);
self->data->push_back(drawable);
@ -576,7 +646,9 @@ PyObject* UICollection::extend(PyUICollectionObject* self, PyObject* iterable)
drawable->removeFromParent();
drawable->z_index = current_z_index;
drawable->setParent(owner_ptr);
// #377: was setParent(owner_ptr), which unparented the drawable outright on a
// scene collection (no owner drawable => owner.lock() is null)
link_child(self, drawable);
self->data->push_back(drawable);
Py_DECREF(item);
@ -621,7 +693,7 @@ PyObject* UICollection::remove(PyUICollectionObject* self, PyObject* o)
for (auto it = vec->begin(); it != vec->end(); ++it) {
if (it->get() == search_drawable.get()) {
// #122: Clear the parent before removing
(*it)->setParent(nullptr);
unlink_child(*it);
vec->erase(it);
McRFPy_API::markSceneNeedsSort();
// #288: Invalidate parent Frame's render cache
@ -671,7 +743,7 @@ PyObject* UICollection::pop(PyUICollectionObject* self, PyObject* args)
std::shared_ptr<UIDrawable> drawable = (*vec)[index];
// #122: Clear the parent before removing
drawable->setParent(nullptr);
unlink_child(drawable);
// Remove from vector
vec->erase(vec->begin() + index);
@ -711,6 +783,12 @@ PyObject* UICollection::insert(PyUICollectionObject* self, PyObject* args)
return NULL;
}
// #183: Remove from old parent, which may be a scene rather than a drawable.
// Do this BEFORE resolving the index: if the drawable is already a member of
// THIS collection, detaching it erases it from `vec`, and an index clamped
// against the old size could then land one past the end.
drawable->removeFromParent();
// Handle negative indexing and clamping (Python list.insert behavior)
Py_ssize_t size = static_cast<Py_ssize_t>(vec->size());
if (index < 0) {
@ -722,13 +800,8 @@ PyObject* UICollection::insert(PyUICollectionObject* self, PyObject* args)
index = size;
}
// #122: Remove from old parent if it has one
if (auto old_parent = drawable->getParent()) {
drawable->removeFromParent();
}
// #122: Set new parent
drawable->setParent(self->owner.lock());
// #377: parent to the scene, not to a null owner, when this is a scene collection
link_child(self, drawable);
// Insert at position
vec->insert(vec->begin() + index, drawable);

241
tests/regression/issue_377_collection_parent_link_test.py Normal file
View file

@ -0,0 +1,241 @@
#!/usr/bin/env python3
"""
Issue #377: every UICollection mutator must maintain the parent link.
`x in collection` and `x.parent is <collection owner>` are supposed to be two views of
the same fact (#122/#183). Only append() maintained it. The rest got it wrong two ways:
* insert/extend/setitem called setParent(owner.lock()) unconditionally. A SCENE
collection has no owner drawable, so that resolved to null -- and setParent(nullptr)
also clears parent_scene. The drawable landed in the scene's UI vector, rendered
fine, and reported .parent of None.
* The slice arms never touched the link at all: a slice-delete left the removed child
pointing at a parent that no longer contained it, and a slice-assign never parented
the incoming items nor unparented the ones they displaced.
Two adjacent traps in the same functions are also covered here: `collection[-1]` on an
EMPTY collection hung the engine forever (`while (index < 0) index += size()` adds zero
each pass), and getitem's `index > size() - 1` bounds check underflowed to SIZE_MAX when
size() was 0, so it went on to index an empty vector.
This invariant is what #373's identity pin hangs off, so it has to hold for every path.
"""
import sys
import mcrfpy
failures = []
def check(cond, msg):
if not cond:
failures.append(msg)
print(f"FAIL: {msg}")
else:
print(f" ok: {msg}")
def frame(n=10):
return mcrfpy.Frame(pos=(0, 0), size=(n, n))
def check_linked(child, owner, collection, label):
"""The two views of membership must agree."""
check(child in collection, f"{label}: child is in the collection")
check(child.parent is owner, f"{label}: child.parent is the owner (got {child.parent!r})")
def check_unlinked(child, collection, label):
check(child not in collection, f"{label}: child is NOT in the collection")
check(child.parent is None, f"{label}: child.parent is None (got {child.parent!r})")
def test_scene_collection_mutators():
"""A scene collection parents via parent_scene; only append used to do it."""
scene = mcrfpy.Scene("link_scene")
a, b, c, d = frame(), frame(), frame(), frame()
scene.children.append(a)
check_linked(a, scene, scene.children, "scene append")
scene.children.insert(0, b)
check_linked(b, scene, scene.children, "scene insert")
scene.children.extend([c])
check_linked(c, scene, scene.children, "scene extend")
scene.children[0] = d
check_linked(d, scene, scene.children, "scene setitem")
def test_frame_collection_mutators():
scene = mcrfpy.Scene("link_frame")
owner = mcrfpy.Frame(pos=(0, 0), size=(200, 200))
scene.children.append(owner)
a, b, c, d = frame(), frame(), frame(), frame()
owner.children.append(a)
check_linked(a, owner, owner.children, "frame append")
owner.children.insert(0, b)
check_linked(b, owner, owner.children, "frame insert")
owner.children.extend([c])
check_linked(c, owner, owner.children, "frame extend")
replaced = owner.children[0]
owner.children[0] = d
check_linked(d, owner, owner.children, "frame setitem")
check_unlinked(replaced, owner.children, "frame setitem (displaced element)")
def test_removal_mutators_unlink():
scene = mcrfpy.Scene("link_remove")
owner = mcrfpy.Frame(pos=(0, 0), size=(200, 200))
scene.children.append(owner)
a, b, c = frame(), frame(), frame()
owner.children.extend([a, b, c])
owner.children.remove(a)
check_unlinked(a, owner.children, "frame remove()")
popped = owner.children.pop(0)
check_unlinked(popped, owner.children, "frame pop()")
del owner.children[0]
check_unlinked(c, owner.children, "frame __delitem__")
check(len(owner.children) == 0, "the collection is empty after removing everything")
def test_slice_delete_unlinks():
scene = mcrfpy.Scene("link_slicedel")
owner = mcrfpy.Frame(pos=(0, 0), size=(200, 200))
scene.children.append(owner)
a, b, c, d = frame(), frame(), frame(), frame()
owner.children.extend([a, b, c, d])
del owner.children[0:2] # contiguous
check(len(owner.children) == 2, "contiguous slice-delete removed two elements")
check_unlinked(a, owner.children, "contiguous slice-delete (a)")
check_unlinked(b, owner.children, "contiguous slice-delete (b)")
check_linked(c, owner, owner.children, "contiguous slice-delete (survivor c)")
e, f = frame(), frame()
owner.children.extend([e, f]) # now: c, d, e, f
del owner.children[0::2] # extended slice: removes c and e
check_unlinked(c, owner.children, "extended slice-delete (c)")
check_unlinked(e, owner.children, "extended slice-delete (e)")
check_linked(d, owner, owner.children, "extended slice-delete (survivor d)")
check_linked(f, owner, owner.children, "extended slice-delete (survivor f)")
def test_slice_assign_links():
scene = mcrfpy.Scene("link_sliceassign")
owner = mcrfpy.Frame(pos=(0, 0), size=(200, 200))
scene.children.append(owner)
a, b = frame(), frame()
owner.children.extend([a, b])
c, d = frame(), frame()
owner.children[0:1] = [c, d] # resizing contiguous assign
check_linked(c, owner, owner.children, "slice-assign (new element c)")
check_linked(d, owner, owner.children, "slice-assign (new element d)")
check_unlinked(a, owner.children, "slice-assign (displaced element a)")
check_linked(b, owner, owner.children, "slice-assign (untouched element b)")
e = frame()
owner.children[0:1] = [e] # same-size contiguous assign
check_linked(e, owner, owner.children, "same-size slice-assign (new element)")
check_unlinked(c, owner.children, "same-size slice-assign (displaced element)")
# Extended slice assign
g, h = frame(), frame()
owner.children[0::2] = [g, h]
check_linked(g, owner, owner.children, "extended slice-assign (g)")
check_linked(h, owner, owner.children, "extended slice-assign (h)")
def test_slice_assign_on_scene():
"""Slice-assign into a scene collection must parent to the SCENE."""
scene = mcrfpy.Scene("link_slicescene")
a, b = frame(), frame()
scene.children.extend([a, b])
c = frame()
scene.children[0:1] = [c]
check_linked(c, scene, scene.children, "scene slice-assign")
check_unlinked(a, scene.children, "scene slice-assign (displaced)")
def test_reparent_between_owners():
"""Appending to a new owner must remove from the old one, not leave it in both."""
scene = mcrfpy.Scene("link_reparent")
one = mcrfpy.Frame(pos=(0, 0), size=(100, 100))
two = mcrfpy.Frame(pos=(0, 0), size=(100, 100))
scene.children.extend([one, two])
x = frame()
one.children.append(x)
check_linked(x, one, one.children, "child starts in owner one")
two.children.append(x)
check(x not in one.children, "reparenting removed the child from its old owner")
check_linked(x, two, two.children, "reparenting linked the child to its new owner")
def test_empty_collection_negative_index():
"""`collection[-1]` on an empty collection used to hang the engine forever."""
scene = mcrfpy.Scene("link_empty")
try:
scene.children[-1]
check(False, "getitem[-1] on an empty collection raises IndexError")
except IndexError:
check(True, "getitem[-1] on an empty collection raises IndexError")
try:
scene.children[-1] = frame()
check(False, "setitem[-1] on an empty collection raises IndexError")
except IndexError:
check(True, "setitem[-1] on an empty collection raises IndexError")
# Negative indexing still works when there IS something to index.
a, b = frame(), frame()
scene.children.extend([a, b])
check(scene.children[-1] is b, "negative indexing resolves from the end")
check(scene.children[-2] is a, "negative indexing reaches the first element")
try:
scene.children[-3]
check(False, "an out-of-range negative index raises IndexError")
except IndexError:
check(True, "an out-of-range negative index raises IndexError")
def main():
test_scene_collection_mutators()
test_frame_collection_mutators()
test_removal_mutators_unlink()
test_slice_delete_unlinks()
test_slice_assign_links()
test_slice_assign_on_scene()
test_reparent_between_owners()
test_empty_collection_negative_index()
if failures:
print(f"\nFAILED ({len(failures)} checks)")
for f in failures:
print(f" - {f}")
sys.exit(1)
print("\nPASS")
sys.exit(0)
if __name__ == "__main__":
main()