Qml Treeview: unexpected view output after specific sequence of deleting row then moving row

ashinjo

Hello, I'm working with QML Treeview, but getting unexpected view output after doing a specific sequence of deleting a row then moving a row in the TreeView. I'm still a bit new to C++ and Qt so I may be doing something wrong. I was wondering if anyone might know what's wrong. Also, I tested this on Qt 6.8.1 and Qt 6.8.2 on Linux.

To reproduce the error, I have a Treeview with the following rows:

Row A (Depth 0)
- Row B (Depth 1)
Row C (Depth 0)

Delete Row B. Now, Row A has no children.
Move Row C to be a child of Row A.
Row C should be indented underneath Row A, but it's now above row A visually.

To be specific, this issue appears when I have a parent row with exactly one child. I delete that child, resulting in the parent row having no children. Then, I try to move a row to become a new child.

My project's code is a bit messy so I modified the "tableofcontents" example code provided by Qt Creator to demonstrate the issue. I'll show screenshots below:

First, click delete on the "Block 1.1" that's nested under the "Block 1".

Second, click indent on the "Block 2".

Block 2 now appears in the wrong position. When I toggle Block 1's expander on and off, Block 2 appears twice.

This is the relevant C++ code.

bool TreeModel::moveRows(const QModelIndex &sourceParent, int sourceRow, int count, const QModelIndex &destinationParent, int destinationChild)
{
    TreeItem *oldParent;

    if(sourceParent == QModelIndex())
    {
        oldParent = rootItem.get();
    }
    else
    {
        oldParent = static_cast<TreeItem*>(sourceParent.internalPointer());
    }

    TreeItem *newParent = static_cast<TreeItem*>(destinationParent.internalPointer());

    beginMoveRows(sourceParent, sourceRow, sourceRow + count - 1, destinationParent, destinationChild);

    for(int i = 0; i < count; i++)
    {
        std::unique_ptr<TreeItem> itemToMove = oldParent->uniqueChild(sourceRow+i);
        oldParent->removeChild(sourceRow + i);
        itemToMove.get()->setParent(newParent);
        newParent->appendChild(std::move(itemToMove));
    }

    endMoveRows();
    return true;
}

bool TreeModel::removeRows(int row, int count, const QModelIndex &parent)
{
    beginRemoveRows(parent, row, row+count-1);
    TreeItem *parentItem = (parent == QModelIndex()) ? rootItem.get() : static_cast<TreeItem*>(parent.internalPointer());
    for(int i = 0; i < count; i++)
    {
        parentItem->removeChild(row);
    }
    endRemoveRows();

    return true;
}

void TreeModel::indent(const QModelIndex &sourceIndex)
{
    if(sourceIndex.row() == 0)
    {
        return;
    }

    QModelIndex parentIndex = parent(sourceIndex);
    QModelIndex previousSiblingIndex = index(sourceIndex.row() - 1, 0, parentIndex);
    TreeItem *previousSiblingItem = static_cast<TreeItem*>(previousSiblingIndex.internalPointer());
    moveRows(parentIndex, sourceIndex.row(), 1, previousSiblingIndex, previousSiblingItem->childCount());
}

void TreeModel::deleteRow(const QModelIndex &sourceIndex)
{
    QModelIndex parentIndex = parent(sourceIndex);
    removeRows(sourceIndex.row(), 1, parentIndex);
}

This is the relevant QML code:

contentItem: Row {
                x: viewDelegate._padding + (viewDelegate.depth + 1 * viewDelegate.indentation)
                width: parent.width - viewDelegate._padding - x

                Text
                {
                    text: "Depth: " + viewDelegate.depth + ", " + viewDelegate.model.display
                    elide: Text.ElideRight
                }

                Button
                {
                    text: "Indent"

                    onClicked:
                    {
                        const index = viewDelegate.treeView.index(viewDelegate.model.row, 0);
                        viewDelegate.treeView.model.indent(index);
                    }
                }

                Button
                {
                    text: "Delete"

                    onClicked:
                    {
                        const index = viewDelegate.treeView.index(viewDelegate.model.row, 0);
                        viewDelegate.treeView.model.deleteRow(index);
                    }
                }
            }

This is a link to the demo project that demonstrates the error:
https://drive.google.com/file/d/1zcQEb-cOmiw1wtLgqRWDJmg9npOZLnNx/view?usp=sharing

Bob64

Probably either the modifications aren't being done correctly in the underlying tree model, or the arguments to the beginMoveRows/beginRemoveRows are not correct. I haven't had enough coffee yet to be sure whether the latter are correct but they look as if they are at least along the right lines.

I would start off - if you haven't done so already - by checking that the state of the underlying tree is exactly as expected after each change.

ashinjo

@Bob64 Hi Bob. Thanks for the reply. I'm starting to wonder if this might be a bug?

Sorry for the long post. Most of it is screenshots. I just wanted to share what I found:

The underlying NodeModel seems to be correct. I added a print model function to show me the contents of the model at each step and it's always as expected.
So the error happens when a parent row deletes its only child. Then, I give the parent row a new child. However, if I unindent the child row then re-indent again, the issue disappears.
I implemented a "Move to previous parent sibling" function which moves a child row to it's parent's previous sibling in order to test if the move row is working as expected. It seems to move correctly until the child is added to a parent that deleted its own child row previously.

In summary, it seems like if i have a parent row that deletes its own child row, the parent row becomes "corrupted" and when I try to insert a new child to it, the view is messed up.

My issue + Underlying model

View:

Print model:

Action: Delete Block 1.1

View:

Print model:

Action: Indent Block 2

View:

Print model:

Unindent then reindent fixes the view

At this point, Block 2 is visually misplaced. If I unindent Block 2 then re-indent Block 2, it goes back to normal.

Action: Unindent Block 2

View:

Print model:

Action: Indent Block 2 again

View:

Print model:

Move Row works until child is added to parent row that deleted its own child.

View:

Print model:

Action: Delete Block 1.1

View:

Print model:

Action: Indent Block 4 to be child of Block 3

View:

Print Model:

Action: Move Block 4 to be child of Block 2

View:

Print Model:

Action: Move Block 4 to be child of Block 1. Wrong view.

View:

Print Model:

SGaist

Hi,

While the state might be correct, did you ensure that you are emitting the adequate signals from your model ? Especially when you start moving elements from one row of a branch to a different row of a different branch ?

ashinjo

@SGaist Hi SGaist, thank you for the help.

So the indent, unindent, and moveToPreviousSibling functions all call moveRows() function and the moveRows function calls beginMoveRows() and endMoveRows() inside of it. From what I understand (I could be wrong), I only need to call beginMoveRows and endMoveRows.

I attached a code snippet of moveRows at the bottom of this message.

bool TreeModel::moveRows(const QModelIndex &sourceParent, int sourceRow, int count, const QModelIndex &destinationParent, int destinationChild)
{
    TreeItem *oldParent;

    if(sourceParent == QModelIndex())
    {
        oldParent = rootItem.get();
    }
    else
    {
        oldParent = static_cast<TreeItem*>(sourceParent.internalPointer());
    }

    TreeItem *newParent =  destinationParent == QModelIndex() ? rootItem.get() : static_cast<TreeItem*>(destinationParent.internalPointer());

    beginMoveRows(sourceParent, sourceRow, sourceRow + count - 1, destinationParent, destinationChild);

    for(int i = 0; i < count; i++)
    {
        std::unique_ptr<TreeItem> itemToMove = oldParent->uniqueChild(sourceRow+i);
        oldParent->removeChild(sourceRow + i);
        itemToMove.get()->setParent(newParent);
        newParent->insertChild(std::move(itemToMove), destinationChild+i);
    }

    endMoveRows();
    return true;
}

Bob64

I have never used the move operations when working with tree models so don't take this as gospel, but from reading the documentation I believe you are correct that the begin and end calls are all that is needed here. That is consistent with other parts of the API that I am more familiar with.

I've stared at your code, and I can't see anything obviously wrong with your implementation. But at the same time it seems unlikely that this is a Qt bug. It is a mature Qt component that no doubt is widely used, so I would have thought any issues would have been shaken out by now.

I know it's not ideal, but if it were me doing this and I had run out of things to try, I would probably try an alternative implementation using remove and insert.

SGaist

I don't see anything that would be wrong.
Something you could do is compare your implementation with the one from the model used in QTreeWidget, it might give you some hints.

JonB

You should also test your model with QAbstractItemModelTester Class. Just in case that reveals anything basic, though the way you describe your move issue may not be tested to any depth by its tests, I don't know.

ashinjo

I decided to follow Bob's suggestion to replace moveRows with removeRows followed by insertRows and it seemed to work out for me. Thank you all for the suggestions.