[Qt] Tree Widget

[corranwebster]

Implement the Qt Tree widget.

Fairly straightforward other than the complexities of the QAbstractItemModel for a tree. We store references to node objects on the QModelIndex objects to avoid having to walk up and down the tree constantly.

For the widget warning check, Qt doesn't know whether a cell contains a widget or not until it is actually visible, so the warning gets issued on expanding the rows, not on display of the table and we needed to adjust the tests slightly to accomodate this.

Like the Table, we have with evenly-spaced non-changable column widths for now (see #4109).

Includes a fix of #4111 since I needed that for manual testing. It also includes a fix to a couple of fixtures which should have been async.

Ref #3914.
Fixes #4111.

To Do:

• screenshot
• issues with tests which reparent widget.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[freakboy3742]

The code looks good; however, in manual testing of the tree example (in a VM running Ubuntuu 24.0.3), I'm getting a segfault when I try to remove a row. Not sure why that isn't manifesting during the testbed, though.

[corranwebster]

The code looks good; however, in manual testing of the tree example (in a VM running Ubuntuu 24.0.3), I'm getting a segfault when I try to remove a row. Not sure why that isn't manifesting during the testbed, though.

I can replicate on KDE (it doesn't happen when running Qt on macOS for whatever reason). Also not sure why it's not happening in the tests, but it may be because Qt is trying to re-draw or size the widget during the remove operation.

I have a reasonable idea of what's going on:

• for efficiency, on every QModelIndex we create, we cache a reference to the node it corresponds to.
  • if we don't do this, every time we want to get the node that corresponds to an index we need to walk up the tree to the root on the Qt side, and then down the tree on the Python side
• however PySide is not doing refcounting on that reference
• this is supposed to be fine, because Qt expects you to call beginRemoveRows() before deleting anything which (amongst other things) invalidates old QModelIndex objects and prevents any redraws or similar which might access the indices.
• but we are deleting the Row on the Python side before we are calling beginRemoveRows()
• for some reason Qt is looking for the parent of the deleted row during the time between the deletion of the object and the beginRemoveRows() call. As best I can tell this isn't anything we're triggering directly: it's happening while remove_row is running, but before any interaction with any Qt objects.
• at this point, the reference on the QModelIndex() is pointed to a garbage collected Python object, so we get the segfault

So the solutions I can see right now are:

1. take the performance hit of not caching the nodes
2. have a "pre-remove" event as was being discussed in Add Pre-Change Notifications to Sources #4028 so we can call beginRemoveRows() earlier
3. use weakref objects for the caching of the nodes and make sure the weakref objects stay alive
4. some other Python-side lookup/caching strategy (eg. keeping a Python-side mirror of Qt's indexes)

Of these, 1 is simple; 2 is "right"; 3 is performant; and 4 is generalised.

One note on #4028 is that I was working under the assumption that Qt never did anything while we are running pure Python code; that assumption appears to be false.

[corranwebster]

The simple fix doesn't work.

The attempted fix also doesn't work the problem, but it does delay the segfault until after the remove_items is done - something is holding on to the QModelIndex() referencing the deleted row after the remove_items has returned.

[freakboy3742]

• for some reason Qt is looking for the parent of the deleted row during the time between the deletion of the object and the beginRemoveRows() call. As best I can tell this isn't anything we're triggering directly: it's happening while remove_row is running, but before any interaction with any Qt objects.

If there's any sort of independent/threaded redraw mechanism, this seems entirely plausible - the precise timing of a redraw being triggered won't always coincide with the Toga-side row removal. This also potentially explains why it doesn't trigger in CI, because CI has very little "breathing room" for background cycles, and the high-churn environment means garbage collection isn't really representative of "real world" usage.

So the solutions I can see right now are:

1. take the performance hit of not caching the nodes
2. have a "pre-remove" event as was being discussed in Add Pre-Change Notifications to Sources #4028 so we can call beginRemoveRows() earlier
3. use weakref objects for the caching of the nodes and make sure the weakref objects stay alive
4. some other Python-side lookup/caching strategy (eg. keeping a Python-side mirror of Qt's indexes)

My initial impression is that 3 would be an acceptable fix here. This is a good example where we have a native representation referencing a canonical object, where the lifecycles of the two won't necessarily align, and deletion of the canonical object shouldn't be blocked on a reference from the native layer.

[corranwebster]

If there's any sort of independent/threaded redraw mechanism, this seems entirely plausible - the precise timing of a redraw being triggered won't always coincide with the Toga-side row removal. This also potentially explains why it doesn't trigger in CI, because CI has very little "breathing room" for background cycles, and the high-churn environment means garbage collection isn't really representative of "real world" usage.

There's something more going wrong here: I put in some non-weak caching and some instrumentation and I can see that the selection is holding stale QModelItem references. But even clearing the selection before removing the row causes segfaults later, so I think there are more things holding the stale references. It may be that we just need to do a complete reset of the model on changes, rather than trying to preserve state across smaller changes.

In any case, I've added an extra sub-test to the row-change sub-test that removes the currently selected row and which replicates the segfault (and on macOS Qt as well, where it wasn't happening).

There is a secondary issue about what the selection should look like after a selected row is deleted: should the same row be selected but with the new contents (GTK3, Cocoa), should the selection just drop that selected row, or should the selection be completely cleared (GTK4 does one of the latter two)? I think that's outside the scope of this PR, though - not segfaulting is enough. It looks like the selection should be cleared on all backends.

[corranwebster]

@freakboy3742 This is ready for a re-review, but may need a re-design if behaviour is not acceptable.

Compared to the last review, this iteration now completely resets the model on inserts and deletes, which guarantees that all QItemModel objects are invalidated so we are guaranteed no dead references. However this has side-effects that may be undesirable because resets collapse all nodes and reset the selection to an empty state. This may not be an acceptable UX.

There is a core problem that when we get insert or remove notifications, the changes to the underlying data have already happened and so the data and the Qt Tree structure are out of sync which leads to complex, fragile code to handle this situation; or tracking the nodes more directly (which leads to assumptions which may not be true for non-TreeSource data sources, such as nodes being hashable or uniquely identified by id). So I ended up abandoning the attempt as perfect being the enemy of good enough.

Fundamentally, there are two approaches to solving this:

• have pre-insert and pre-remove notifications, so we can call the beginInsertRow/endInsertRow and beginRemoveRow/endRemoveRow pairs while the Tree and Qt model are in a consistent state
• giving up on a custom model and use a QTreeWidget with its standard model (which means non-lazy construction–all text and icon data needs to be populated up-front–which has performance implications beyond ~1000-ish items).

[freakboy3742]

Confirming that this resolves the crash issue; but my inclination is that the "reset" behavior on any insertion or deletion is going to be surprising.

The performance implications of using the built-in model seems like a regression when we know there's a better option available; so moving to pre-insert/remove notifications seems like the way forward to me.

That said - this works - I'm happy to land this as-is and treat adding pre notifications as a follow up; or if you think we can get pre-notifications in place in this PR without too much effort, I'm happy to do that as well. Let me know which approach you'd prefer.