Modifying Tree Widget nodes before the Tree is Expanded/Drawn in macOS about toga HOT 11 CLOSED

Thanks for the report. As best as I can work out, this is a macOS specific issue; analogous problems don't exist with other platforms.

The core of the issue is that macOS Trees (and tables, for that matter) have a single render method, and the objects representing the actual tree nodes are created on first render. This is an optimisation; if you have a tree with a million rows, you don't need to create a million objects to display the first 20.

However, the "on demand" aspect is what causes issues here. If the widget has never been rendered, it doesn't have an implementation; so requesting the that the implementation be updated, or that the the implementation be used as a parent, causes a crash.

In many cases, the fix here will be to just ignore the problem.

In the case of an update to an existing node, if the node hasn't been displayed, there's nothing visible to update, so the lack of an implementation is no problem. The implementation will be created if/when the widget is displayed, and should assume the current value at that time.

In the case of deleting a node, if the node (or it's parent) hasn't been displayed, there's no visual representation to delete, so any calls to remove a native representation can be ignored.

The only mildly complicated case is adding a child. If the parent hasn't been displayed, then the child won't need to be displayed either; however, if the parent is later displayed, there might be a need to trigger the creation of child nodes. It's possible this might "just happen", but that definitely needs to be confirmed.

As a final aside - there's also something odd going on with the provided example code and the expand call - on both macOS and GTK, the expand call doesn't appear to be actually expanding the tree.

from toga.

I think tree.expand() isn't doing anything because at the stage that it is called no children have been added. If you add a second expand call after adding the child node it expands the tree.

from toga.

Interesting... I do wonder if that's a bug or a feature. A node with no children should still be expandable - there's just nothing there, and there's something to be said for showing there's nothing there. This sounds suspiciously like an if children: vs if children is None: when children = [] bug.

from toga.

Just talking macOS, but given the tree widget is based on NSOutlineView, then the docs suggest that this is expected behaviour.

If item is not expandable or is already expanded, does nothing.

Before you append/insert the child there is no disclosure triangle to expand.

from toga.

Ah - that's where I was going wrong. I mistakenly remembered that the initial state of the root node's children was an empty list, but it's not - it's None, indicating no children, and the child list is created when the first child is added. So - my "is None vs children = []" analysis was correct... but I was wrong about it being incorrect behaviour.

from toga.

As you suggested, a try:except block sorts out the append/insert issue. It also handles removing a child or updating a childs fields before expansion. The additions are in insert, remove, and change respectively. One small problem(?), if you append and then remove a child immediately, i.e.,

root_node.append(leaf_node)
root_node.remove(root_node[0])

The disclosure triange remains visible, even though there are no children.

While I was here I thought I would test the other functions which include _impl. If you try to expand a specific node (e.g., tree.expand(root_node)) you get the same AttributeError. The try:except block removes the error but the node is not expanded. My thinking is before expanding it needs something like:

        try:
            node_impl = node._impl
        except AttributeError:
            node_impl = TogaData.alloc().init()
            node_impl.attrs = {"node": node}
            node._impl = node_impl

But with the actual attrs for the node assigned. It isn't clear to me how to do this.

Collapse has the same problem but the try:except solution works.

Regarding adding tests for these cases should I add something to test_tree.py in widgets or do something else?

from toga.

As you suggested, a try:except block sorts out the append/insert issue. It also handles removing a child or updating a childs fields before expansion. The additions are in insert, remove, and change respectively. One small problem(?), if you append and then remove a child immediately, i.e.,

root_node.append(leaf_node)
root_node.remove(root_node[0])

The disclosure triange remains visible, even though there are no children.

That sounds entirely consistent to me. It's the other end of the "bug" that I thought I was seeing.

When a node is created, it sets children=None - this node cannot have children. That tells the Tree widget that no disclosure triangle is needed.

You then add a child, which sets children = [child1]. This tells the tree a disclosure triangle is required, and if expanded, that a child could be displayed.

You then delete the child, which removes the child from children: children = []. This tells the tree that the node could have children, but it doesn't - so there's a disclosure triangle, but pressing it does nothing.

It's a bit like the difference between an empty directory and a file with the same name in a file explorer. Adding a child to a node is a bit like converting the file into a directory and putting a child file in that directory; removing the child file doesn't change the directory back to a file.

While I was here I thought I would test the other functions which include _impl. If you try to expand a specific node (e.g., tree.expand(root_node)) you get the same AttributeError. The try:except block removes the error but the node is not expanded. My thinking is before expanding it needs something like:

        try:
            node_impl = node._impl
        except AttributeError:
            node_impl = TogaData.alloc().init()
            node_impl.attrs = {"node": node}
            node._impl = node_impl

But with the actual attrs for the node assigned. It isn't clear to me how to do this.

You don't need to. The attrs in this case are the attributes on the node - they only exist provide a path back to the data node that contains the properties of the item on the tree.

The code you've got here looks like all you need - except that it's duplicating the code around L32. I'd suggest that you could factor this out into an idempotent node_impl() method that can be used anywhere that the user requests node._impl, and ensures that the impl exists. That might even be a way around adding the try-except logic everywhere - if you can guarantee that the _impl exists whenever it's accessed, the need to ignore attribute errors goes away (or, rather, the existence of the AttributeError is used to immediately backfill the missing _impl).

Collapse has the same problem but the try:except solution works.

Regarding adding tests for these cases should I add something to test_tree.py in widgets or do something else?

To be clear - there's two tests named test_tree.py - one in core, and one in testbed. In this case, it's the testbed version that needs to be updated, because we need to confirm specific behavior of the Cocoa implementation (and confirm that the same problem doesn't exist for other platforms).

But yes - all we need is a modification to the existing expand/insert/etc tests in the testbed test_tree.py (or new tests if it isn't easy to shoehorn the specific test case into the existing test data).

from toga.

Thanks - that all makes sense. Although I think you made up the word idempotent! 8-)

from toga.

Thanks - that all makes sense. Although I think you made up the word idempotent! 8-)

Unsure if that was intended as a joke, but just in case: Idempotence is an important concept in computer science - in practice, it means calling a method N times has no more effect on a system than calling it once. If you know a method is idempotent, it radically changes the way you can use that method in a system.

For example, one of the big differences between HTTP verbs like GET and POST is that GET is definitionally idempotent, but POST isn't. Multiple calls to GET the same resource have the same effect on the system - which means that GET requests can be cached. POST requests aren't idempotent, so they can't be cached. (And before anyone nitpicks: yes, there's more in the HTTP spec that impacts on this, including but not limited to the definition of "safe" methods... but that's a digression for our purposes here)

In Toga's case, "Get the impl, and if it doesn't exist, create it" would be idempotent because the impl is created the first time, but the existing instance is returned on subsequent calls (or if it existed to start with). A version that returned a new impl every time would not be idempotent, because the number of calls would alter the numbers of instances that were created, and the instance that is currently in use.

from toga.

Sorry poor attempt at humour, although I did have to look it up. To be fair when I did Computer Science we were using a Vax 11.

So the mods are working on my tests and pass the existing testbed tests. When I look at test_tree.py, as you would imagine it is already testing expand/collapse/append/insert/remove. But it is always expanding the tree first. I'm thinking all I need to do is to comment out those intitial tree expansions. Is that sufficient?

from toga.

Sorry poor attempt at humour, although I did have to look it up. To be fair when I did Computer Science we were using a Vax 11.

Ah, the VAX... such memories... not good ones, mind... but memories :-)

So the mods are working on my tests and pass the existing testbed tests. When I look at test_tree.py, as you would imagine it is already testing expand/collapse/append/insert/remove. But it is always expanding the tree first. I'm thinking all I need to do is to comment out those intitial tree expansions. Is that sufficient?

We need to retain the existing set of tests, because they're testing the behavior when the tree is expanded - but "the same set of tests but not expanded" could quite possibly be sufficient. However, I suspect you'd also want to check "what if I add when non-expanded then expand", and a few other cases like that, so a literal duplicate probably isn't the best match - but the general pattern of the test should be at least useful as a template.

from toga.