Hi Bogdan, Thanks for elaborating :)
> On 2 Jul 2026, at 14:35, Bogdan Vatra <[email protected]> wrote: > Hi, > > @Igor, @Volker: Thanks for your feedback! > > QPersistentModelIndex is not a proper solution here, for a practical reason: > are you going to use QPMI *everywhere*? We experience crashes not only in our > code but inside Qt's own code, QSortFilterProxyModel, QIdentityProxyModel, > etc. which rely on internalPointer for their own bookkeeping. No amount of > QPMI in application code fixes that. Using QPMI everywhere is also quite > hacky; it's like having a QPersistentString that I must use to check whether > my QString is still valid... If we have bugs in our proxy models, then let’s fix those bugs - we should, and hopefully can, do that for Qt 6. > On "QModelIndex should be thought of like an STL iterator": the analogy is > forced, because with an STL container *I* have full control. The iterator is > only invalidated when *my* code changes the container. With QAIM, framework > code invalidates indices behind my back: any Q*ModelView or delegate may call > QCoreApplication::processEvents(), and Qt's own signals carry QModelIndex by > value across queued connections, so an index can already be stale at the > moment it is delivered. A single posted dataChanged (or any structural signal) > is enough to make internalPointer point to something that no longer exists. If a call made in a loop iterating a std::vector results in a modification of that std::vector (because it emits a signal or processEvents()), then your iterators become invalid. Maybe it’s your own code modifying that std::vector, maybe it’s not; and you might not have the choice either (if whatever is connected to the signal calls std::vector::erase, what are you gonna do?). In the code snippet that’s the basis for your proposals, you hold on to a QModelIndex and then make calls that plausibly can result in a change of the state/structure/contents of the model. For that specific case, a short-lived QPMI is the right solution, just as `QPointer<Me> = this;` is sometimes the right solution for checking whether the following signal emission or event processing might have deleted `this`. Or you ask the model to recreate the QModelIndex whenever you made such outbound calls, if you know that you still want the same row/column (and don’t have to care about the parent if you only care about flat models). > This is the daily reality of highly dynamic models, the current contract is > not just hard to follow, Qt itself breaks it on my behalf. Indeed, that QModelIndexes are emitted as signal parameters, and that it’s then possible to connect to such signals using a queued connection, is definitely a foot-gun. I don’t know if it’s possible to prohibit that, at least at runtime... given that all calls that take a QModelIndex should be consider “GUI API calls”, a cross-thread connection like that should not be useful anyway. All interactions with QAIM should only happen in the GUI thread. But even if we could do that, it’s probably a breaking change with significant fall-out. Might be worth investigating. >> And the model knows when a model index might have been invalidated, and >> would need to somehow update the payload in constructed QModelIndex >> instances - what should QModelIndex::internalData() return if the index >> has been removed from the model? > > It returns the QVariant. :) That's the point: the payload is a *value*, not a > reference into model state, so there is nothing for the model to update. Let’s say that the model index holds some representation of “item 2” as a value; that item 2 is then removed from the model, and item 3 is now item 2. What does the representation of "item 2" stored in the model index mean? What should QModelIndex::data() return? This might not be UB, but it’s perhaps even worse if this results in incorrect writes or reads of data into the backing storage, isn’t it? The wrong database record might suddenly be updated. In a tree, the wrong parent might get the new child. And if we don’t maintain a parallel data structure that can inform us that “item 2 is no longer what you think it is”, then this will happen silently. >> a view or other generic client code cannot generally assume anything >> about what internalData() might return > > Of course, the problem is on the model side where we don't know if > internalPointer is still valid or not. E.g. QSortFilterProxyModel *is* a > model, and it is precisely where these crashes happen today. Qt's own proxies > would be the first beneficiaries ;-) Maybe it would help if you could show code that's correctly using QModelIndex (your earlier code snippet was IMHO not) where our models today are crashing. We can at least improve the documentation, and maybe some static analyser rules, to help people avoid such patterns. >> This can already be done by anyone right now, just adding a Qt::UserRole >> to the model to return a key-for-index, and implementing a persistent >> mapping from key-to-item/index in their model. > > We do exactly this today (an in-house bimap between ids and items), and yes, > it works, a lookup miss is how we detect a stale index. But look at what it > costs: a parallel data structure that must be maintained in lockstep with the > model, CRUD on the map for every item created or destroyed, and a lookup on > every single access, on the hottest path in model/view. And every author of a > dynamic model has to hand-roll this same boilerplate; it is reference > counting, reimplemented badly, once per model. A shared_ptr in the payload is > the same validity guarantee for an O(1) refcount and zero bookkeeping. And it > doesn't help Qt's own proxies either way: QSortFilterProxyModel cannot use my > application-level bimap, so its internalPointer crashes remain no matter how > good my map is. > > On cost: rather than debate this in the abstract, I went ahead and implemented > it for Qt 7: https://codereview.qt-project.org/c/qt/qtbase/+/749550. > QModelIndex does lose trivial copyability, but an empty payload costs next to > nothing at copy time, so only models that opt in actually pay. I'll attach > benchmarks for the index-heavy paths (views, QSFPM) to the review, so we can > settle the cost question on numbers rather than assumptions. If QModelIndex holds some value-representation of “item x”, then you still need to be able to map this “item x value” back to the backend storage. How would you do that, for the hyper-generic “hierarchy of tables” thing that QAbstractItemModel provides? Of course we can give that monkey to the model implementer, but then that’s not different from the status quo. For “tree of trees”: QList<int>{..., grandparent_row, parent_row, row, column} is a plausible representation, and I honestly would have loved to be able to store such a path-to-data in QModelIndex when implementing QRangeModel. But code that holds on to a QModelIndex holding such a path would still be getting wrong data if it accesses the model with such an index, after the respective item had been removed. We’d still have to invalidate that index somehow when the item gets removed, and return some error condition from data() or rowCount(). So we’d still need QPMI or some equivalent data structure for the model to verify the validity of an index. All that being said, increasing the size of QModelIndex to be able to hold more than a void * would perhaps not be a bad thing. Perhaps paired with a more comprehensive life-time control interface in QAIM so that the model can optionally implement copy/move/destroy semantics in addition to “create". Volker PS: Qt Contributors Summit is in October, and I have a hunch that after a few more emails in this thread, sitting down in person might move this thing more efficiently in the right direction. *Hint hint nudge nudge* ;) -- Development mailing list [email protected] https://lists.qt-project.org/listinfo/development
