Loading...

XML

Word

Printable

Details

Type: Bug
Resolution: Fixed
Priority: P3: Somewhat important
Fix Version/s: 6.2.11, 6.5.4, 6.6.1, 6.7.0 FF
Affects Version/s: 6.5.1
Component/s: Core: Item Models
Labels:
- regression

Platform/s:

All
Commits:
0416e080c (dev), 7da3dda66 (6.6), d528a88ec (tqtc/lts-6.5), ebaee2aed (tqtc/lts-6.2)

Description

Preface
Hello. My team is working on upgrading our application's Qt dependency from 5.15.2 to 6.5.1. In upgrading, we noticed a crash in our application due to behavioural differences between 5.15.2 and 6.5.1. Though I tested in 6.5.1, I took a look at the newest source in the qtbase dev branch, and there have been no changes that would have fixed this.

The crash was due to behavioural differences of QStandardItem - specifically, in the implementation of QStandardItem::takeChild.

Details
The following snippet of code behaves differently in Qt 5.15.2 and Qt 6.5.1

#include <iostream>
#include <memory>
#include <ranges>

#include <QStandardItem>
#include <QStandardItemModel>

namespace
{
    using std::views::iota;

    constexpr int arbitraryNumberOfChildren = 3;

    constexpr auto numberOfBytesInAPointer = sizeof(std::uintptr_t);
    constexpr auto numberOfNibblesInAByte = 2;
    constexpr auto numberOfCharactersProceedingAHexadecimalNumber = 2;
    constexpr auto numberOfFormatCharactersForPointer = numberOfBytesInAPointer * numberOfNibblesInAByte + numberOfCharactersProceedingAHexadecimalNumber;

    static void PrintChildren_(auto& item)
    {
        std::cout << std::format("  {}'s children", item->text().toStdString()) << std::endl;
        for (int i : iota(0, arbitraryNumberOfChildren))
        {
            std::cout << std::format("    {:#0{}x}", reinterpret_cast<std::uintptr_t>(item->child(i)), numberOfFormatCharactersForPointer) << std::endl;
        }
    };

    static void AddChildrenToItem_(auto& item)
    {
        for ([[maybe_unused]] int i : iota(0, arbitraryNumberOfChildren))
        {
            item->appendRow(new QStandardItem());
        }
    };
}

int main()
{
    auto firstParent = std::make_unique<QStandardItem>("firstParent");
    auto secondParent = std::make_unique<QStandardItem>("secondParent");

    AddChildrenToItem_(firstParent);
    PrintChildren_(firstParent);

    std::cout << "Swapping children" << std::endl;
    for([[maybe_unused]] int i : iota(0, arbitraryNumberOfChildren))
    {
        secondParent->setChild(i, firstParent->takeChild(i));
    }
    printChildren(secondParent);
}

Program Output - 5.15.2

firstParent's children
0x0000017dba593800
0x0000017dba5929f0
0x0000017dba593850
Swapping children
secondParent's children
0x0000017dba593800
0x0000017dba5929f0
0x0000017dba593850

Program Output - 6.5.1

firstParent's children
0x000001e54f056940
0x000001e54f0563f0
0x000001e54f056440
Swapping children
QStandardItem::setChild: Ignoring duplicate insertion of item 0x1e54f056940
QStandardItem::setChild: Ignoring duplicate insertion of item 0x1e54f0563f0
QStandardItem::setChild: Ignoring duplicate insertion of item 0x1e54f056440
secondParent's children
0x0000000000000000
0x0000000000000000
0x0000000000000000

(As you'll note, qt provides some helpful output for debugging.)

In 5.15.2, the children are successfully reparented. In 6.5.1, the attempt at reparenting the children is denied due to "duplicate insertion". In our program, we later try to use these children, but after upgrading to 6.5.1, we crash, because the children are null.

Taking a dive into the implementation of QStandardItem::setChild, we can find the following bit of code.

if (item) {
    if (item->d_func()->parent == nullptr) {
        item->d_func()->setParentAndModel(q, model);
    } else {
        qWarning("QStandardItemPrivate::setChild: Ignoring duplicate insertion of item %p",
                 item);
        return;
    }
}

This code indicates the behavioural difference between Qt 5.15.2 and Qt 6.5.1. In 5.15.2, QStandardItemPrivate::takeChild sets the parent of the child to nullptr before returning it. In 6.5.1, the parent of the returned child is not set to nullptr. This is verified by the following snippets of code from Qt 5.15.2 and Qt 6.5.1, respectively.

// 5.15.2
QStandardItem *QStandardItem::takeChild(int row, int column){
    Q_D(QStandardItem);
    QStandardItem *item = nullptr;
    int index = d->childIndex(row, column);
    if (index != -1) {
        item = d->children.at(index);
        if (item)
            item->d_func()->setParentAndModel(nullptr, nullptr);
        d->children.replace(index, 0);
    }
    return item;
}

// 6.5.1
QStandardItem *QStandardItem::takeChild(int row, int column){
    Q_D(QStandardItem);
    QStandardItem *item = nullptr;
    int index = d->childIndex(row, column);
    if (index != -1) {
        QModelIndex changedIdx;
        item = d->children.at(index);
        if (item && d->model) {
            QStandardItemPrivate *const item_d = item->d_func();
            const int savedRows = item_d->rows;
            const int savedCols = item_d->columns;
            const QVector<QStandardItem*> savedChildren = item_d->children;
            if (savedRows > 0) {
                d->model->d_func()->rowsAboutToBeRemoved(item, 0, savedRows - 1);
                item_d->rows = 0;
                item_d->children = QVector<QStandardItem*>(); //slightly faster than clear
                d->model->d_func()->rowsRemoved(item, 0, savedRows);
            }
            if (savedCols > 0) {
                d->model->d_func()->columnsAboutToBeRemoved(item, 0, savedCols - 1);
                item_d->columns = 0;
                if (!item_d->children.isEmpty())
                    item_d->children = QVector<QStandardItem*>(); //slightly faster than clear
                d->model->d_func()->columnsRemoved(item, 0, savedCols);
            }
            item_d->rows = savedRows;
            item_d->columns = savedCols;
            item_d->children = savedChildren;
            changedIdx = d->model->indexFromItem(item);
            item_d->setParentAndModel(nullptr, nullptr);
        }
        d->children.replace(index, nullptr);
        if (changedIdx.isValid())
            d->model->dataChanged(changedIdx, changedIdx);
    }
    return item;
}

The difference is that, in 6.5.1, we don't nullify the child's parent unless `this` has a model.

Inferences
Now, perhaps this behavioural difference is intentional. By making a small modification to the program, we can coerce QStandardItem::takeChild to behave the way we want.

auto model = std::make_unique<QStandardItemModel>();
auto firstParent = model->invisibleRootItem();
firstParent->setText("firstParent");
auto secondParent = std::make_unique<QStandardItem>("secondParent");

AddChildrenToItem_(firstParent);
PrintChildren_(firstParent);

std::cout << "Swapping children" << std::endl;
for (int i : iota(0, arbitraryNumberOfChildren))
{
    secondParent->setChild(i, firstParent->takeChild(i));
}
PrintChildren_(secondParent);

Because firstParent has a model, the call to firstParent->takeChild nullifies the parent of children before returning them. This allows the call to secondParent->setChild to properly accept the children.

The workaround here is pretty simple: simple enough that I considered not creating this bug. I could certainly see an argument that this is acceptable behaviour and consuming code just needs to adjust accordingly. However, I experimented a little, and modified the original example program as follows.

auto firstParent = std::make_unique<QStandardItem>("firstParent");
auto secondParent = std::make_unique<QStandardItem>("secondParent");

AddChildrenToItem_(firstParent);
PrintChildren_(firstParent);

std::cout << "Swapping children" << std::endl;
secondParent->insertColumn(0, firstParent->takeColumn(0));
PrintChildren_(secondParent);

Program Output - 6.5.1 modified to use takeColumn

firstParent's children
0x000001e54f055ea0
0x000001e54f055f40
0x000001e54f0570c0
Swapping children
secondParent's children
0x000001e54f055ea0
0x000001e54f055f40
0x000001e54f0570c0

This code behaves as I would expect: the children are reparented from firstParent to secondParent. Taking a look at the implementation of QStandardItem::takeColumn, we can see that the QStandardItemPrivate::takeChild logic of "don't nullify parent unless this->model is set" doesn't apply.

QList<QStandardItem*> QStandardItem::takeColumn(int column){
    Q_D(QStandardItem);
    QList<QStandardItem*> items;
    if ((column < 0) || (column >= columnCount()))
        return items;
    if (d->model)
        d->model->d_func()->columnsAboutToBeRemoved(this, column, column);

    const int rowCount = d->rowCount();
    items.reserve(rowCount);
    for (int row = rowCount - 1; row >= 0; --row) {
        int index = d->childIndex(row, column);
        QStandardItem *ch = d->children.at(index);
        if (ch)
            ch->d_func()->setParentAndModel(nullptr, nullptr);
        d->children.remove(index);
        items.prepend(ch);
    }
    d->columns--;
    if (d->model)
        d->model->d_func()->columnsRemoved(this, column, 1);
    return items;
}

Due to the fact that there's a mismatch in the logical paradigm of QStandardItemPrivate::takeChild and QStandardItem::takeColumn, I felt like I should create this report.

Conclusion
Because there are workarounds, this is not a high priority issue. The "bug" here feels to me that there are logical inconsistencies between QStandardItem's function implementations. That is - having the model set in an item may or may not produce a behavioural difference when attempting to take its children depending on what function you use to do so.

Hopefully this all makes sense. For convenience, I attached the sample code consolidated to a single main.cpp file. Please let me know if I can help to clarify anything.

Thanks!

Attachments

- Sort By Name
- Sort By Date
- Ascending
- Descending
- Thumbnails
- List

main.cpp
3 kB
06 Oct '23 16:23

Issue Links

resulted from

QTBUG-89145 QStandardItemModel takeItem / takeChild does not emit the right signals

Closed

Gerrit Reviews

- Issue Only
- Show All Reviews
- Show Open Reviews
- Show All Issues
- Show Open Issues

For Gerrit Dashboard: QTBUG-117900
#	Subject	Branch	Project	Status	CR	V
517603,6	QStandardItem: Fix reset parent in takeItem()	dev	qt/qtbase	Status: MERGED	+2	0
517680,2	QStandardItem: Fix reset parent in takeItem()	6.6	qt/qtbase	Status: MERGED	+2	0
517759,2	QStandardItem: Fix reset parent in takeItem()	tqtc/lts-6.5	qt/tqtc-qtbase	Status: MERGED	+2	0
517819,2	QStandardItem: Fix reset parent in takeItem()	tqtc/lts-6.2	qt/tqtc-qtbase	Status: MERGED	+2	0