Qt-interest Archive, January 2007
Resetting a sub-tree of a QAbstractItemModel
Message 1 in thread
Hi folks
I'm facing a variant of a problem that appears to come up on this list,
and was hoping for a few ideas. I'm working on a QAbstractItemModel that
presents a directed cyclic graph as a tree to the user. To handle this
I'm demand-building an intermediate tree that keeps track of parentage
and node aliases. That all works well.
The problem arises when the user triggers a non-trivial modification to
the model tree, eg by changing a link from a node to another part of the
graph.
QAbstactItemModel doesn't seem to provide a clean way to handle this. It
permits one to reset the model completely, forcing all views to discard
their state and start again. It's also able to easily add or remove
ranges of rows. What it can't seem to easily do is reset a subtree of
the model completely, as reset() resets the whole model.
I've tried to achieve the same effect by removing all child rows from
the node being modified using beginRemoveRows(...) and endRemoveRows(),
then adding back the new set (the same way) of rows after the change is
made. The old node objects are deleted after the rows are removed and
they should no longer be accessible, and new ones are constructed when
the new rows are added.
I'd expect this to work, and in some situations it initially seems to.
However, sooner or later the model ends up being passed a QModelIndex
that points to deleted memory by the view, and subsequently crashing -
and _ONLY_ after an attempted sub-tree reset. This is despite the fact
that all rows that could've referred to those grandchild nodes were
removed from the view using beginRemoveRows(...) / endRemoveRows(). Now,
I don't deny the fairly strong possibility that I've simply made an
error in this fairly complex code, but I'm at a bit of a loss as to how
or where.
What I'm hoping is that:
- Perhaps Qt 4.3.x could contain a method
QAbstractItemModel::reset(const QModelIndex& index)
to reset the tree at and below `index'.
- Someone might have a few ideas about how to sanely
reset a sub-tree of a model in qt 4.2.x .
Any help or suggestions would be very much appreciated at this point.
--
[ signature omitted ]
Message 2 in thread
> - Perhaps Qt 4.3.x could contain a method
> QAbstractItemModel::reset(const QModelIndex& index)
> to reset the tree at and below `index'.
>
> - Someone might have a few ideas about how to sanely
> reset a sub-tree of a model in qt 4.2.x .
Did you try dataChanged()?
Martin
--
[ signature omitted ]
Message 3 in thread
Martin Gebert wrote:
>> - Perhaps Qt 4.3.x could contain a method
>> QAbstractItemModel::reset(const QModelIndex& index)
>> to reset the tree at and below `index'.
>>
>> - Someone might have a few ideas about how to sanely
>> reset a sub-tree of a model in qt 4.2.x .
>
>
> Did you try dataChanged()?
I certainly did. dataChanged() informs interested parties that that
particular row's contents have changed - but does not appear to affect
any stored information about tree rooted at that row.
I have no problems updating rows without children, nor with updates to
rows with children where no rows are added/removed to children,
grandchildren, etc. It's just when the entire subtree under a node
changes (potentially adding/removing rows and entire sub-trees) that I
run into issues.
I'd expect informing the model of the removal of all rows from the root
of the modified subtree to properly handle any grandchild nodes etc, but
at this point it seems not to given the fact that the model later passes
indexes with pointers to nodes that were in that removed subtree.
--
[ signature omitted ]
Message 4 in thread
Hi Craig!
No need to mail me your answer, I read this as a newsgroup...
> I'd expect informing the model of the removal of all rows from the root
> of the modified subtree to properly handle any grandchild nodes etc, but
> at this point it seems not to given the fact that the model later passes
> indexes with pointers to nodes that were in that removed subtree.
Suggested workaround (note: no final solution): Verify the validity of
the pointer before using...
I can faintly remember having changed the payload of one of my models
from pointers to IDs due to a similar reason... :-( And I found it handy
to have a private method in all of my models that translates indices to
entries, which requires your Entry classes sharing a common base class:
Entry& entry(const QModelIndex& index) const;
You may want to return Entry* == NULL here, or provide Entry::isValid()
for invalid indices.
Martin
--
[ signature omitted ]
Message 5 in thread
Martin Gebert schrieb:
>> I'd expect informing the model of the removal of all rows from the
>> root of the modified subtree to properly handle any grandchild nodes
>> etc, but at this point it seems not to given the fact that the model
>> later passes indexes with pointers to nodes that were in that removed
>> subtree.
>
>
> Suggested workaround (note: no final solution): Verify the validity of
> the pointer before using...
> I can faintly remember having changed the payload of one of my models
> from pointers to IDs due to a similar reason [...]
On further thought, this suggestions seem not to match your problem, so
forget it. If I finally understood you right, your problem is not with
invalid pointers but indices referring to now invalid entries in your
tree (perhaps carrying still perfectly valid pointers). So you need to
save and verify parent/row/column of any index against the current entry
position...
Martin
--
[ signature omitted ]
Message 6 in thread
Martin Gebert wrote:
> On further thought, this suggestions seem not to match your problem, so
> forget it. If I finally understood you right, your problem is not with
> invalid pointers but indices referring to now invalid entries in your
> tree (perhaps carrying still perfectly valid pointers).
That's right. As it happens they are usually not valid pointers, as
those nodes have been deleted after that part of the tree was replaced,
but it's not pointer validity that's the central issue.
As you said, the real problem is that my model is being passed
QModelIndex instances referring to nodes that're no longer part of the
tree because I removed all the rows of a node somewhere in the tree
above them. If for debuggin I choose to flag those nodes invalid and
leak them rather than deleting them, I end up aborting on access to a
flagged node.
> So you need to
> save and verify parent/row/column of any index against the current entry
> position...
The trouble is that the view is asking about parts of the tree that just
don't exist any more. There's no sensible thing to do except fail.
What's the parent of the node that _used_ to be the great-grandchild of
the invalidated subtree?
At this point I suspect that persistent indexes retained by the model
for some reason are part of the issue, but I'm not sure yet.
In any case, one possible workaround might be to depth-first walk the
subtree that's being invalidated, removing rows (leaf nodes) from each
twig node before moving on to its parent. In some ways I hope this
doesn't fix the issue, however, as it really should not be necessary.
--
[ signature omitted ]
Message 7 in thread
On 31.01.07 23:39:02, Craig Ringer wrote:
> As you said, the real problem is that my model is being passed QModelIndex
> instances referring to nodes that're no longer part of the tree because I
> removed all the rows of a node somewhere in the tree above them. If for
> debuggin I choose to flag those nodes invalid and leak them rather than
> deleting them, I end up aborting on access to a flagged node.
I suggest to send a mail to qt-bug trolltech com, as this is clearly a
bug in the views. A view shouldn't try to re-use model indexes that are
children of a deleted index. (Note view's do cache model indexes
internally).
Andreas
--
[ signature omitted ]
Message 8 in thread
Andreas Pakulat wrote:
> On 31.01.07 23:39:02, Craig Ringer wrote:
>> As you said, the real problem is that my model is being passed QModelIndex
>> instances referring to nodes that're no longer part of the tree because I
>> removed all the rows of a node somewhere in the tree above them. If for
>> debuggin I choose to flag those nodes invalid and leak them rather than
>> deleting them, I end up aborting on access to a flagged node.
>
> I suggest to send a mail to qt-bug trolltech com, as this is clearly a
> bug in the views. A view shouldn't try to re-use model indexes that are
> children of a deleted index. (Note view's do cache model indexes
> internally).
Indeed, and I think I've found out why it's happening, and it turns out
it's (surprise surprise) partly my fault. I do think the Qt behaviour is
unintuitive at best, and at minimum needs a runtime check in debug
builds to detect this otherwise very confusing failure condition.
I'll submit a detailed bug-report to qt-bug shortly, but for anyone who
runs into this and who's looking on this list, but in short, the
implementation of QAbstractItemModel's persistent index management was
relying on the parent index passed to beginRemoveRows(...) having a
column value of 0, and failing silently if it wasn't. The model thus
ended up retaining references to rows that no longer existed.
Detail:
When
void QAbstractItemModel::beginRemoveRows(const QModelIndex &parent,
int first, int last)
is called, it passes the `parent' argument to
void QAbstractItemModelPrivate::rowsAboutToBeRemoved(
const QModelIndex &parent, int first, int last)
which uses QModelIndex::operator== to compare `parent' to the parent of
each index in the persistent index store. It assumes that the passed
index refers to the first column, and if it does not it'll fail to match
indexes for rows that it should've found.
The documentation for QAbstractItemModel::parent(...) does recommend
that implementers return 0 for the column value of the index produced,
but this really isn't sufficient. The APIs presently seem to depend very
stongly on the column value being 0 in some contexts where a QModelIndex
is used to identify a row, but it doesn't make it clear where those
places are.
IMO debug builds need to Q_ASSERT( parent.col() == 0 ) where they rely
on that condition being true to work correctly. I'll put something to
that effect to qt-bugs once I have a patch ready.
--
[ signature omitted ]
Message 9 in thread
On 01.02.07 01:49:12, Craig Ringer wrote:
> Andreas Pakulat wrote:
> >On 31.01.07 23:39:02, Craig Ringer wrote:
> >>As you said, the real problem is that my model is being passed QModelIndex
> >>instances referring to nodes that're no longer part of the tree because I
> >>removed all the rows of a node somewhere in the tree above them. If for
> >>debuggin I choose to flag those nodes invalid and leak them rather than
> >>deleting them, I end up aborting on access to a flagged node.
> >I suggest to send a mail to qt-bug trolltech com, as this is clearly a
> >bug in the views. A view shouldn't try to re-use model indexes that are
> >children of a deleted index. (Note view's do cache model indexes
> >internally).
>
> Indeed, and I think I've found out why it's happening, and it turns out it's
> (surprise surprise) partly my fault. I do think the Qt behaviour is unintuitive
> at best, and at minimum needs a runtime check in debug builds to detect this
> otherwise very confusing failure condition.
I do agree that its at least underdocumented. However all documentation
show how to create parent/child relationships only on the first
column. So I guess one could conclude that column 0 indexes are the ones
that are used to keep track of the existing rows, although its in no way
obvious that you should conclude this way ;)
Andreas
--
[ signature omitted ]
