Box children are not property removed, when iteration over _children

[matz2532]

Describe the bug

When iterating over _children from a toga.Box to remove all children, one child is still left.

for child in self.testBox._children:
  self.testBox.remove(child)

Steps to reproduce

1. Creating a new briefcase
2. and starting the app with briefcase dev using the following code in the app.py
3. You would expect all children of the self.testBox to be removed.
4. However, the mid toga.Label is not iterated over. (Actually, doesn't matter wheter it's a Label or a nested Box.)
5. Trying to remove the last child by iterating a second time over _children does the job (set removeTwice = True)

"""
_children are not properly iterated over, when being removed
"""
import toga
from toga.style import Pack
from toga.style.pack import COLUMN, ROW


class ChildRemovalBug(toga.App):
    def startup(self):
        main_box = toga.Box(style=Pack(direction=COLUMN))
        self.testBox = toga.Box(style=Pack(direction=COLUMN, padding=15))

        topBox = toga.Label(id="top", text="top", style=Pack(padding=5))
        midBox = toga.Label(id="mid", text="mid", style=Pack(padding=5))
        bottomBox = toga.Label(id="bottom", text="bottom", style=Pack(padding=5))

        self.testBox.add(topBox)
        self.testBox.add(midBox)
        self.testBox.add(bottomBox)

        button = toga.Button(text="Remove children", on_press=self.removeChildren, style=Pack(padding=5))

        main_box.add(self.testBox)
        main_box.add(button)

        self.main_window = toga.MainWindow(title=self.formal_name)
        self.main_window.content = main_box
        self.main_window.show()

    def removeChildren(self, widget, removeTwice: bool = False):
        for child in self.testBox._children:
            print("starting child-id", child.id)

        for child in self.testBox._children:
            self.testBox.remove(child)
            print("removing child-id", child.id)

        for child in self.testBox._children:
            print("still present child-id", child.id)

        if removeTwice:
            for child in self.testBox._children:
                self.testBox.remove(child)
                print("removed last child-id", child.id)

            for child in self.testBox._children:
                print("still present in second round child-id", child.id)


def main():
    return ChildRemovalBug()

Expected behavior

I would have expected to be able to remove all children objects at the same time.
Maybe there is another way should handle to remove children objects, I'm not aware.

Screenshots

Before Clicking 'Remove children':

After click:

Environment

• Operating System: Windows
• Python version: 3.10.4
• Software versions:
  • Briefcase: 0.3.15
  • Toga: 0.3.1

Logs

Additional context

No response

[freakboy3742]

Thanks for the report. In this case, I don't think this is a bug, although it's definitely a quirk of Python that won't necessarily be obvious.

Firstly - as a quick aside, your code is referencing _children. That's not a good idea; the underscore indicates that the property or method is a private API. Unless you really know what you're doing, you shouldn't rely on private methods or properties in your code. In this case, children exists, and is the public API you should be using. As it happens, the functionality (and bug/quirk) is the same, so I'll use that name in my explanations that follow.

The issues you're having is that you're iterating over a list while deleting objects from the same list.

Consider the first deletion pass in your example:

        for child in self.testBox.children:
            self.testBox.remove(child)
            print("removing child-id", child.id)

You go into this block of code with 3 children. You delete item 0. The list now has 2 elements. You delete item 1. Which item is being referenced? The original item 1 (child id mid)? Or the current item 1 (child id bottom)? While you might expect the answer to be the former, Python's list implementation does the latter. At that point, you're at the end of the list, so Python stops - with the mid element still present.

There's a couple of ways around this problem.

If the specific use case is "remove all children", Toga has a clear() API that will remove all children from a box. That removes the need to iterate at all.

The second approach is to use the same loop, but iterate in reverse:

        for child in self.testBox.children[::-1]:
            self.testBox.remove(child)
            print("removing child-id", child.id)

If you remove from the end of the list, the item order isn't ambiguous, because removing the last element of the list doesn't change the position of elements that come before it.

Lastly, you can make the list you're iterating over unambiguous, and iterate over a copy of the list of children:

        for child in self.testBox.children[:]:
            self.testBox.remove(child)
            print("removing child-id", child.id)

By taking a copy, you get a list of fixed length; when you remove children, the "live" list of children shrinks, but the copy doesn't, so the index references remain valid during iteration. By taking a copy, you're essentially making a distinction between "This is the list of elements I want to delete", and "this is the list of elements that are currently children of my box". The former is a static list; the latter changes as you delete elements.

I hope that makes sense; unfortunately, there's not much we can do to protect against this from an API perspective because the conflict is occurring at the raw Python level.

[matz2532]

Thanks for the comment. I was aware about this quirk of python, but didn't connect it with this case.
I previously just checked the code for the Box class and didn't particularly check the inhereted class Widget and Node.

However something to correct for anybody reading this post in the future. The function clear does not remove but rather disconnects all children from the parent, which can lead to an error in case you want to reinstantiate the same widget (when you are setting it's id). It's probably better to not remove and recreate any widget, but for now building the functionality is more important for me and .children[:] did the job.

[freakboy3742]

However something to correct for anybody reading this post in the future. The function clear does not remove but rather disconnects all children from the parent, which can lead to an error in case you want to reinstantiate the same widget (when you are setting it's id).

I'm not sure what you mean by this - box.clear() is literally a wrapper around box.remove(*box.children), and there shouldn't be any issue with re-adding a widget to a layout if it has been previously added and removed.

If you've got a case where this isn't working, that sounds like a bug. Can you give an example of a sequence of actions that is problematic?

[matz2532]

I'm using PyCharm and with Ctrl+Left Mouse clicking on .clear() following the toga.Box.clear(), I arrived at the node.py class rather than the toga.widgets.base class you are linking. My widgets base class does not contain the function clear.
And the clear function of the node looks like this:

    def clear(self):
        """Clear all children from this node.

        Raises:
            ValueError: If this node is a leaf, and cannot have children.
        """
        if self._children is None:
            # This is a leaf, so do nothing.
            return

        for child in self._children:
            child._parent = None
            self._set_root(child, None)
        self._children = []

Could this be due to me getting an older version of toga through pip rather than using the repository itself? I have the toga version 0.3.1

[freakboy3742]

Ah - that would do it. We've made a lot of improvements since the release of 0.3.1, as part of a comprehensive audit of functionality (which includes building a 100% coverage test suite). I forgot that the Toga-level "clear" implementation wasn't added until #1893.

[matz2532]

Is there a way to update using pip or do I need to clone the git repository and add it to my PATH?

[freakboy3742]

Any git repo can be added as a requirement; in this case, you need to install 2 packages:

pip install git+https://github.com/beeware/toga.git#subdirectory=core
pip install git+https://github.com/beeware/toga.git#subdirectory=winforms

I'm assuming, based on the screenshot in the original report, that you want the winforms backend; adjust the second requirement as appropriate if you want a different backend.