Consider safety when implementing QAbstractItemModel

[LeonMatthesKDAB]

beginInsertRows and endInsertRows should probably be unsafe to call directly, as calling one without the other is undefined behavior, aka. unsafe.

However, it's currently safe to e.g. call vector_mut() and modify the vector on a QObject that inherits QAIM. But isn't that also undefined behavior? As we'd actually have to emit dataChanged. So is there any way we can assert that this is done?

[Be-ing]

beginInsertRows and endInsertRows should probably be unsafe to call directly, as calling one without the other is undefined behavior, aka. unsafe.

Is that really undefined behavior or is it a logic error?

[ahayzen-kdab]

Ultimately this part is up to the app developer as we don't provide bindings to QAbstractListModel etc. For the debate if it's a logic or undefined behaviour this is tricky as you can cause QML/C++ to try and call null pointers if you forget to call the right signals and don't have the right checks in your methods etc (we had a crash in the C++ model of demo threading once due to this :sweat_smile: ).

But the way i was thinking it would be done is via two ways

Manual implementation

The current derive manually from a QAbstractListModel and inherit the methods. In this route i would mark the begin/end as unsafe and probably leave the data changed as safe.

I would then create a helper guard thing that takes the pair eg PairGuard(|| unsafe { self.begin_insert_rows() }, || unsafe { self.end_insert_rows() }) this would then call the insert when it's created and then it's dropped call when it goes out of scope. Which means that it is always called.

Extended helper types

Something for later in like cxx-qt-extras or something, would be to have a VecModel or something like Slint. Then have this type implement a QAbstractListModel internally and provide only a safe API to the developer. This could then be used as a property in a Rust QObject to get it to QML/C++.

This extras library could then have various helpers for different types of models or structures that are composed of cxx-qt-lib types and aren't 1-to-1 bindings.

[LeonMatthesKDAB]

beginInsertRows and endInsertRows should probably be unsafe to call directly, as calling one without the other is undefined behavior, aka. unsafe.

Is that really undefined behavior or is it a logic error?

I'm not entirely sure, it might just be a logic error. It's undefined in the sense that Qt doesn't seem to define a certain behavior that views will have when this happens. However, it's probably not undefined in the Rust sense, as the behavior is likely not platform-dependent.

We should still figure out whether this is really true before making begin/end calls safe.

Same with modifying the underlying Rust vector. I can't think of a way right now to get Qt to SegFault with this, as the data implementation should use safe access to the underlying vector which ensures even invalidated indices are deterministic, even if that means a panic.

Also in previous discussions we defined any C++ code as basically a giant unsafe block. So if Qt code is segfaulting because of this, we could argue that's where the issue is, not in the "safe" Rust code.

[ahayzen-kdab]

Right, for the purposes of the example just mark them as unsafe then create a little helper like i showed which uses Drop to ensure the end call is always called. This is ultimately up to the developer of the app to decide if they want to mark them as safe / unsafe.

And as i noted i think later we should have a further crate that provides ways to go from a Vec -> ListModel with a safe API etc.