Uploaded image for project: 'Qt'
  1. Qt
  2. QTBUG-117900

[REG 5.15.2->6.5.1] Behavioural inconsistency when reparenting QStandardItems

    XMLWordPrintable

Details

    • All
    • 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

        1. main.cpp
          3 kB
          Evan Hampton

        Issue Links

          No reviews matched the request. Check your Options in the drop-down menu of this sections header.

          Activity

            People

              qt.team.quick.subscriptions Qt Quick and Widgets Team
              evan.hampton Evan Hampton
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Gerrit Reviews

                  There are no open Gerrit changes