From 95717ea834a0d9514513809a1b1b179916e6cb35 Mon Sep 17 00:00:00 2001 From: Christian Ehrlicher Date: Sun, 8 Sep 2019 20:19:30 +0200 Subject: tst_QAbstractItemView: cleanup Cleanup QAbstractItemView autotest: - use range-based for loops - use nullptr - use member initialization - use new signal/slot syntax - use static invocations - use override - replaced QCoreApplication::processEvents with QTRY_VERIFY/QTRY_COMPARE Change-Id: Iba91811db6fb925364fc88ec36357e758b937329 Reviewed-by: Friedemann Kleint --- .../qabstractitemview/tst_qabstractitemview.cpp | 664 +++++++++------------ 1 file changed, 280 insertions(+), 384 deletions(-) diff --git a/tests/auto/widgets/itemviews/qabstractitemview/tst_qabstractitemview.cpp b/tests/auto/widgets/itemviews/qabstractitemview/tst_qabstractitemview.cpp index bcfc477733..3316c8ab93 100644 --- a/tests/auto/widgets/itemviews/qabstractitemview/tst_qabstractitemview.cpp +++ b/tests/auto/widgets/itemviews/qabstractitemview/tst_qabstractitemview.cpp @@ -26,39 +26,32 @@ ** ****************************************************************************/ - -#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include #include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - Q_DECLARE_METATYPE(Qt::ItemFlags); using namespace QTestPrivate; +using IntList = QVector; // Move cursor out of widget area to avoid undesired interaction on Mac. static inline void moveCursorAway(const QWidget *topLevel) @@ -74,8 +67,9 @@ class GeometriesTestView : public QTableView { Q_OBJECT public: - GeometriesTestView() : QTableView(), updateGeometriesCalled(false) {} - bool updateGeometriesCalled; + using QTableView::QTableView; + using QTableView::selectedIndexes; + bool updateGeometriesCalled = false; protected slots: void updateGeometries() override { updateGeometriesCalled = true; QTableView::updateGeometries(); } }; @@ -151,34 +145,48 @@ private slots: void currentFollowsIndexWidget(); void checkFocusAfterActivationChanges_data(); void checkFocusAfterActivationChanges(); +private: + static QAbstractItemView *viewFromString(const QByteArray &viewType, QWidget *parent = nullptr) + { + if (viewType == "QListView") + return new QListView(parent); + if (viewType == "QTableView") + return new QTableView(parent); + if (viewType == "QTreeView") + return new QTreeView(parent); + if (viewType == "QHeaderView") + return new QHeaderView(Qt::Vertical, parent); + Q_ASSERT(false); + return nullptr; + } }; class MyAbstractItemDelegate : public QAbstractItemDelegate { public: - MyAbstractItemDelegate() : QAbstractItemDelegate() { calledVirtualDtor = false; } - void paint(QPainter *, const QStyleOptionViewItem &, const QModelIndex &) const {} - QSize sizeHint(const QStyleOptionViewItem &, const QModelIndex &) const { return size; } - QWidget *createEditor(QWidget *parent, const QStyleOptionViewItem &, const QModelIndex &) const + using QAbstractItemDelegate::QAbstractItemDelegate; + void paint(QPainter *, const QStyleOptionViewItem &, const QModelIndex &) const override {} + QSize sizeHint(const QStyleOptionViewItem &, const QModelIndex &) const override { return size; } + QWidget *createEditor(QWidget *parent, const QStyleOptionViewItem &, const QModelIndex &) const override { openedEditor = new QWidget(parent); return openedEditor; } - void destroyEditor(QWidget *editor, const QModelIndex &) const + void destroyEditor(QWidget *editor, const QModelIndex &) const override { calledVirtualDtor = true; editor->deleteLater(); } void changeSize() { size = QSize(50, 50); emit sizeHintChanged(QModelIndex()); } - mutable bool calledVirtualDtor; - mutable QWidget *openedEditor; + mutable QWidget *openedEditor = nullptr; QSize size; + mutable bool calledVirtualDtor = false; }; class DialogItemDelegate : public QStyledItemDelegate { public: - DialogItemDelegate() : QStyledItemDelegate() { } + using QStyledItemDelegate::QStyledItemDelegate; QWidget *createEditor(QWidget *parent, const QStyleOptionViewItem &, const QModelIndex &) const { openedEditor = new QDialog(parent); @@ -194,8 +202,8 @@ public: result = static_cast(dialog->result()); } - mutable QDialog::DialogCode result; - mutable QDialog *openedEditor; + mutable QDialog *openedEditor = nullptr; + mutable QDialog::DialogCode result = QDialog::Rejected; }; // Testing get/set functions @@ -207,27 +215,27 @@ void tst_QAbstractItemView::getSetCheck() // void QAbstractItemView::setItemDelegate(QAbstractItemDelegate *) MyAbstractItemDelegate *var1 = new MyAbstractItemDelegate; obj1->setItemDelegate(var1); - QCOMPARE((QAbstractItemDelegate*)var1, obj1->itemDelegate()); - obj1->setItemDelegate((QAbstractItemDelegate *)0); - QCOMPARE((QAbstractItemDelegate *)0, obj1->itemDelegate()); + QCOMPARE(var1, obj1->itemDelegate()); + obj1->setItemDelegate(nullptr); + QCOMPARE(obj1->itemDelegate(), nullptr); delete var1; - // EditTriggers QAbstractItemView::editTriggers() + // EditTriggers ) // void QAbstractItemView::setEditTriggers(EditTriggers) - obj1->setEditTriggers(QAbstractItemView::EditTriggers(QAbstractItemView::NoEditTriggers)); - QCOMPARE(QAbstractItemView::EditTriggers(QAbstractItemView::NoEditTriggers), obj1->editTriggers()); - obj1->setEditTriggers(QAbstractItemView::EditTriggers(QAbstractItemView::CurrentChanged)); - QCOMPARE(QAbstractItemView::EditTriggers(QAbstractItemView::CurrentChanged), obj1->editTriggers()); - obj1->setEditTriggers(QAbstractItemView::EditTriggers(QAbstractItemView::DoubleClicked)); - QCOMPARE(QAbstractItemView::EditTriggers(QAbstractItemView::DoubleClicked), obj1->editTriggers()); - obj1->setEditTriggers(QAbstractItemView::EditTriggers(QAbstractItemView::SelectedClicked)); - QCOMPARE(QAbstractItemView::EditTriggers(QAbstractItemView::SelectedClicked), obj1->editTriggers()); - obj1->setEditTriggers(QAbstractItemView::EditTriggers(QAbstractItemView::EditKeyPressed)); - QCOMPARE(QAbstractItemView::EditTriggers(QAbstractItemView::EditKeyPressed), obj1->editTriggers()); - obj1->setEditTriggers(QAbstractItemView::EditTriggers(QAbstractItemView::AnyKeyPressed)); - QCOMPARE(QAbstractItemView::EditTriggers(QAbstractItemView::AnyKeyPressed), obj1->editTriggers()); - obj1->setEditTriggers(QAbstractItemView::EditTriggers(QAbstractItemView::AllEditTriggers)); - QCOMPARE(QAbstractItemView::EditTriggers(QAbstractItemView::AllEditTriggers), obj1->editTriggers()); + obj1->setEditTriggers(QAbstractItemView::NoEditTriggers); + QCOMPARE(obj1->editTriggers(), QAbstractItemView::NoEditTriggers); + obj1->setEditTriggers(QAbstractItemView::CurrentChanged); + QCOMPARE(obj1->editTriggers(), QAbstractItemView::CurrentChanged); + obj1->setEditTriggers(QAbstractItemView::DoubleClicked); + QCOMPARE(obj1->editTriggers(), QAbstractItemView::DoubleClicked); + obj1->setEditTriggers(QAbstractItemView::SelectedClicked); + QCOMPARE(obj1->editTriggers(), QAbstractItemView::SelectedClicked); + obj1->setEditTriggers(QAbstractItemView::EditKeyPressed); + QCOMPARE(obj1->editTriggers(), QAbstractItemView::EditKeyPressed); + obj1->setEditTriggers(QAbstractItemView::AnyKeyPressed); + QCOMPARE(obj1->editTriggers(), QAbstractItemView::AnyKeyPressed); + obj1->setEditTriggers(QAbstractItemView::AllEditTriggers); + QCOMPARE(obj1->editTriggers(), QAbstractItemView::AllEditTriggers); // bool QAbstractItemView::tabKeyNavigation() // void QAbstractItemView::setTabKeyNavigation(bool) @@ -286,29 +294,18 @@ void tst_QAbstractItemView::cleanup() void tst_QAbstractItemView::emptyModels_data() { - QTest::addColumn("viewType"); + QTest::addColumn("viewType"); - QTest::newRow("QListView") << "QListView"; - QTest::newRow("QTableView") << "QTableView"; - QTest::newRow("QTreeView") << "QTreeView"; - QTest::newRow("QHeaderView") << "QHeaderView"; + const QVector widgets{ "QListView", "QTreeView", "QTableView", "QHeaderView" }; + for (const QByteArray &widget : widgets) + QTest::newRow(widget) << widget; } void tst_QAbstractItemView::emptyModels() { - QFETCH(QString, viewType); - - QScopedPointer view; - if (viewType == "QListView") - view.reset(new QListView()); - else if (viewType == "QTableView") - view.reset(new QTableView()); - else if (viewType == "QTreeView") - view.reset(new QTreeView()); - else if (viewType == "QHeaderView") - view.reset(new QHeaderView(Qt::Vertical)); - else - QVERIFY(0); + QFETCH(QByteArray, viewType); + + QScopedPointer view(viewFromString(viewType)); centerOnScreen(view.data()); moveCursorAway(view.data()); view->show(); @@ -323,37 +320,21 @@ void tst_QAbstractItemView::emptyModels() void tst_QAbstractItemView::setModel_data() { - QTest::addColumn("viewType"); - - QTest::newRow("QListView") << "QListView"; - QTest::newRow("QTableView") << "QTableView"; - QTest::newRow("QTreeView") << "QTreeView"; - QTest::newRow("QHeaderView") << "QHeaderView"; + emptyModels_data(); } void tst_QAbstractItemView::setModel() { - QFETCH(QString, viewType); - - QScopedPointer view; + QFETCH(QByteArray, viewType); - if (viewType == "QListView") - view.reset(new QListView()); - else if (viewType == "QTableView") - view.reset(new QTableView()); - else if (viewType == "QTreeView") - view.reset(new QTreeView()); - else if (viewType == "QHeaderView") - view.reset(new QHeaderView(Qt::Vertical)); - else - QVERIFY(0); + QScopedPointer view(viewFromString(viewType)); centerOnScreen(view.data()); moveCursorAway(view.data()); view->show(); QVERIFY(QTest::qWaitForWindowExposed(view.data())); QStandardItemModel model(20,20); - view->setModel(0); + view->setModel(nullptr); view->setModel(&model); basic_tests(view.data()); } @@ -463,11 +444,11 @@ void tst_QAbstractItemView::basic_tests(QAbstractItemView *view) QCOMPARE(view->sizeHintForIndex(QModelIndex()), QSize()); QCOMPARE(view->indexAt(QPoint(-1, -1)), QModelIndex()); - if (!view->model()){ + if (!view->model()) { QCOMPARE(view->indexAt(QPoint(10, 10)), QModelIndex()); QCOMPARE(view->sizeHintForRow(0), -1); QCOMPARE(view->sizeHintForColumn(0), -1); - }else if (view->itemDelegate()){ + } else if (view->itemDelegate()) { view->sizeHintForRow(0); view->sizeHintForColumn(0); } @@ -487,7 +468,7 @@ void tst_QAbstractItemView::basic_tests(QAbstractItemView *view) view->rowsInserted(QModelIndex(), -1, -1); view->rowsAboutToBeRemoved(QModelIndex(), -1, -1); view->selectionChanged(QItemSelection(), QItemSelection()); - if (view->model()){ + if (view->model()) { view->currentChanged(QModelIndex(), QModelIndex()); view->currentChanged(QModelIndex(), view->model()->index(0,0)); } @@ -498,9 +479,9 @@ void tst_QAbstractItemView::basic_tests(QAbstractItemView *view) view->horizontalScrollbarAction(QAbstractSlider::SliderSingleStepAdd); view->verticalScrollbarValueChanged(10); view->horizontalScrollbarValueChanged(10); - view->closeEditor(0, QAbstractItemDelegate::NoHint); - view->commitData(0); - view->editorDestroyed(0); + view->closeEditor(nullptr, QAbstractItemDelegate::NoHint); + view->commitData(nullptr); + view->editorDestroyed(nullptr); // Will assert as it should // view->setIndexWidget(QModelIndex(), 0); @@ -510,7 +491,7 @@ void tst_QAbstractItemView::basic_tests(QAbstractItemView *view) view->verticalOffset(); // view->isIndexHidden(QModelIndex()); // will (correctly) assert - if(view->model()) + if (view->model()) view->isIndexHidden(view->model()->index(0,0)); view->setSelection(QRect(0, 0, 10, 10), QItemSelectionModel::ClearAndSelect); @@ -518,9 +499,9 @@ void tst_QAbstractItemView::basic_tests(QAbstractItemView *view) view->visualRegionForSelection(QItemSelection()); view->selectedIndexes(); - view->edit(QModelIndex(), QAbstractItemView::NoEditTriggers, 0); + view->edit(QModelIndex(), QAbstractItemView::NoEditTriggers, nullptr); - view->selectionCommand(QModelIndex(), 0); + view->selectionCommand(QModelIndex(), nullptr); #if QT_CONFIG(draganddrop) if (!view->model()) @@ -529,17 +510,17 @@ void tst_QAbstractItemView::basic_tests(QAbstractItemView *view) view->viewOptions(); view->setState(QAbstractItemView::NoState); - QVERIFY(view->state()==QAbstractItemView::NoState); + QCOMPARE(view->state(), QAbstractItemView::NoState); view->setState(QAbstractItemView::DraggingState); - QVERIFY(view->state()==QAbstractItemView::DraggingState); + QCOMPARE(view->state(), QAbstractItemView::DraggingState); view->setState(QAbstractItemView::DragSelectingState); - QVERIFY(view->state()==QAbstractItemView::DragSelectingState); + QCOMPARE(view->state(), QAbstractItemView::DragSelectingState); view->setState(QAbstractItemView::EditingState); - QVERIFY(view->state()==QAbstractItemView::EditingState); + QCOMPARE(view->state(), QAbstractItemView::EditingState); view->setState(QAbstractItemView::ExpandingState); - QVERIFY(view->state()==QAbstractItemView::ExpandingState); + QCOMPARE(view->state(), QAbstractItemView::ExpandingState); view->setState(QAbstractItemView::CollapsingState); - QVERIFY(view->state()==QAbstractItemView::CollapsingState); + QCOMPARE(view->state(), QAbstractItemView::CollapsingState); #endif view->startAutoScroll(); @@ -575,7 +556,7 @@ void tst_QAbstractItemView::noModel() view.scrollTo(view.model()->index(10,10)); QApplication::processEvents(); - view.setModel(0); + view.setModel(nullptr); // Due to the model is removed, this will generate a valueChanged signal on both scrollbars. (value to 0) QApplication::processEvents(); QCOMPARE(view.model(), nullptr); @@ -585,7 +566,7 @@ void tst_QAbstractItemView::dragSelect() { // From task #86108 - QStandardItemModel model(64,64); + QStandardItemModel model(64, 64); QTableView view; view.setModel(&model); @@ -607,7 +588,7 @@ void tst_QAbstractItemView::dragSelect() void tst_QAbstractItemView::rowDelegate() { - QStandardItemModel model(4,4); + QStandardItemModel model(4, 4); MyAbstractItemDelegate delegate; QTableView view; @@ -629,7 +610,7 @@ void tst_QAbstractItemView::rowDelegate() void tst_QAbstractItemView::columnDelegate() { - QStandardItemModel model(4,4); + QStandardItemModel model(4, 4); MyAbstractItemDelegate delegate; QTableView view; @@ -665,42 +646,35 @@ void tst_QAbstractItemView::sizeHintChangeTriggersLayout() QVERIFY(QTest::qWaitForWindowExposed(&view)); view.updateGeometriesCalled = false; delegate.changeSize(); - QCoreApplication::sendPostedEvents(); - QVERIFY(view.updateGeometriesCalled); + QTRY_VERIFY(view.updateGeometriesCalled); view.updateGeometriesCalled = false; rowDelegate.changeSize(); - QCoreApplication::sendPostedEvents(); - QVERIFY(view.updateGeometriesCalled); + QTRY_VERIFY(view.updateGeometriesCalled); view.updateGeometriesCalled = false; columnDelegate.changeSize(); - QCoreApplication::sendPostedEvents(); - QVERIFY(view.updateGeometriesCalled); + QTRY_VERIFY(view.updateGeometriesCalled); } void tst_QAbstractItemView::selectAll() { - QStandardItemModel model(4,4); - QTableView view; + QStandardItemModel model(4, 4); + GeometriesTestView view; view.setModel(&model); - QAbstractItemView *tst_view = &view; - - QCOMPARE(tst_view->selectedIndexes().count(), 0); + QCOMPARE(view.selectedIndexes().count(), 0); view.selectAll(); - QCOMPARE(tst_view->selectedIndexes().count(), 4*4); + QCOMPARE(view.selectedIndexes().count(), 4 * 4); } void tst_QAbstractItemView::ctrlA() { - QStandardItemModel model(4,4); - QTableView view; + QStandardItemModel model(4, 4); + GeometriesTestView view; view.setModel(&model); - QAbstractItemView *tst_view = &view; - - QCOMPARE(tst_view->selectedIndexes().count(), 0); + QCOMPARE(view.selectedIndexes().count(), 0); QTest::keyClick(&view, Qt::Key_A, Qt::ControlModifier); - QCOMPARE(tst_view->selectedIndexes().count(), 4*4); + QCOMPARE(view.selectedIndexes().count(), 4 * 4); } void tst_QAbstractItemView::persistentEditorFocus() @@ -716,7 +690,7 @@ void tst_QAbstractItemView::persistentEditorFocus() view.openPersistentEditor(model.index(0, 2)); //these are spinboxes because we put numbers inside - QList list = view.viewport()->findChildren(); + const QList list = view.viewport()->findChildren(); QCOMPARE(list.count(), 2); //these should be the 2 editors view.setCurrentIndex(model.index(0, 0)); @@ -726,15 +700,15 @@ void tst_QAbstractItemView::persistentEditorFocus() view.show(); QVERIFY(QTest::qWaitForWindowExposed(&view)); - for (int i = 0; i < list.count(); ++i) { - QTRY_VERIFY(list.at(i)->isVisible()); - QPoint p = QPoint(5, 5); + const QPoint p(5, 5); + for (QSpinBox *sb : list) { + QTRY_VERIFY(sb->isVisible()); QMouseEvent mouseEvent(QEvent::MouseButtonPress, p, Qt::LeftButton, Qt::LeftButton, Qt::NoModifier); - qApp->sendEvent(list.at(i), &mouseEvent); - if (!qApp->focusWidget()) + QCoreApplication::sendEvent(sb, &mouseEvent); + if (!QApplication::focusWidget()) QSKIP("Some window managers don't handle focus that well"); - QTRY_COMPARE(qApp->focusWidget(), static_cast(list.at(i))); + QTRY_COMPARE(QApplication::focusWidget(), sb); } } @@ -960,65 +934,63 @@ void tst_QAbstractItemView::dragAndDropOnChild() class TestModel : public QStandardItemModel { + Q_OBJECT public: - TestModel(int rows, int columns) : QStandardItemModel(rows, columns) - { - setData_count = 0; - } - - virtual bool setData(const QModelIndex &/*index*/, const QVariant &/*value*/, int /*role = Qt::EditRole*/) + using QStandardItemModel::QStandardItemModel; + bool setData(const QModelIndex &, const QVariant &, int) override { ++setData_count; return true; } + void emitDataChanged() + { + emit dataChanged(index(0, 0), index(0, 1)); + } - int setData_count; + int setData_count = 0; }; -typedef QList IntList; - void tst_QAbstractItemView::setItemDelegate_data() { // default is rows, a -1 will switch to columns QTest::addColumn("rowsOrColumnsWithDelegate"); QTest::addColumn("cellToEdit"); QTest::newRow("4 columndelegates") - << (IntList() << -1 << 0 << 1 << 2 << 3) + << (IntList{ -1, 0, 1, 2, 3 }) << QPoint(0, 0); QTest::newRow("2 identical rowdelegates on the same row") - << (IntList() << 0 << 0) + << (IntList{ 0, 0 }) << QPoint(0, 0); QTest::newRow("2 identical columndelegates on the same column") - << (IntList() << -1 << 2 << 2) + << (IntList{ -1, 2, 2 }) << QPoint(2, 0); QTest::newRow("2 duplicate delegates, 1 row and 1 column") - << (IntList() << 0 << -1 << 2) + << (IntList{ 0, -1, 2 }) << QPoint(2, 0); QTest::newRow("4 duplicate delegates, 2 row and 2 column") - << (IntList() << 0 << 0 << -1 << 2 << 2) + << (IntList{ 0, 0, -1, 2, 2 }) << QPoint(2, 0); } void tst_QAbstractItemView::setItemDelegate() { - QFETCH(IntList, rowsOrColumnsWithDelegate); + QFETCH(const IntList, rowsOrColumnsWithDelegate); QFETCH(QPoint, cellToEdit); QTableView v; - QItemDelegate *delegate = new QItemDelegate(&v); + QStyledItemDelegate *delegate = new QStyledItemDelegate(&v); TestModel model(5, 5); v.setModel(&model); bool row = true; - foreach (int rc, rowsOrColumnsWithDelegate) { + for (int rc : rowsOrColumnsWithDelegate) { if (rc == -1) { row = !row; } else { - if (row) { + if (row) v.setItemDelegateForRow(rc, delegate); - } else { + else v.setItemDelegateForColumn(rc, delegate); - } } } centerOnScreen(&v); @@ -1070,45 +1042,32 @@ void tst_QAbstractItemView::noFallbackToRoot() void tst_QAbstractItemView::setCurrentIndex_data() { - QTest::addColumn("viewType"); - QTest::addColumn("itemFlags"); + QTest::addColumn("viewType"); + QTest::addColumn("itemFlags"); QTest::addColumn("result"); - QStringList widgets; - widgets << "QListView" << "QTreeView" << "QHeaderView" << "QTableView"; - - foreach(QString widget, widgets) { - QTest::newRow((widget+QLatin1String(": no flags")).toLocal8Bit().constData()) - << widget << (int)0 << false; - QTest::newRow((widget+QLatin1String(": checkable")).toLocal8Bit().constData()) - << widget << (int)Qt::ItemIsUserCheckable << false; - QTest::newRow((widget+QLatin1String(": selectable")).toLocal8Bit().constData()) - << widget << (int)Qt::ItemIsSelectable << false; - QTest::newRow((widget+QLatin1String(": enabled")).toLocal8Bit().constData()) - << widget << (int)Qt::ItemIsEnabled << true; - QTest::newRow((widget+QLatin1String(": enabled|selectable")).toLocal8Bit().constData()) - << widget << (int)(Qt::ItemIsSelectable|Qt::ItemIsEnabled) << true; + const QVector widgets{ "QListView", "QTreeView", "QTableView", "QHeaderView" }; + for (const QByteArray &widget : widgets) { + QTest::newRow(widget + ": no flags") + << widget << Qt::ItemFlags(Qt::NoItemFlags) << false; + QTest::newRow(widget + ": checkable") + << widget << Qt::ItemFlags(Qt::ItemIsUserCheckable) << false; + QTest::newRow(widget + ": selectable") + << widget << Qt::ItemFlags(Qt::ItemIsSelectable) << false; + QTest::newRow(widget + ": enabled") + << widget << Qt::ItemFlags(Qt::ItemIsEnabled) << true; + QTest::newRow(widget + ": enabled|selectable") + << widget << (Qt::ItemIsSelectable|Qt::ItemIsEnabled) << true; } } void tst_QAbstractItemView::setCurrentIndex() { - QFETCH(QString, viewType); - QFETCH(int, itemFlags); + QFETCH(QByteArray, viewType); + QFETCH(Qt::ItemFlags, itemFlags); QFETCH(bool, result); - QScopedPointer view; - - if (viewType == "QListView") - view.reset(new QListView()); - else if (viewType == "QTableView") - view.reset(new QTableView()); - else if (viewType == "QTreeView") - view.reset(new QTreeView()); - else if (viewType == "QHeaderView") - view.reset(new QHeaderView(Qt::Vertical)); - else - QVERIFY(0); + QScopedPointer view(viewFromString(viewType)); centerOnScreen(view.data()); moveCursorAway(view.data()); @@ -1121,31 +1080,30 @@ void tst_QAbstractItemView::setCurrentIndex() model->appendRow(item); item = new QStandardItem("test item"); - item->setFlags(Qt::ItemFlags(itemFlags)); + item->setFlags(itemFlags); model->appendRow(item); view->setModel(model); - view->setCurrentIndex(model->index(0,0)); - QCOMPARE(view->currentIndex(), model->index(0,0)); - view->setCurrentIndex(model->index(1,0)); - QVERIFY(view->currentIndex() == model->index(result ? 1 : 0,0)); + view->setCurrentIndex(model->index(0, 0)); + QCOMPARE(view->currentIndex(), model->index(0, 0)); + view->setCurrentIndex(model->index(1, 0)); + QVERIFY(view->currentIndex() == model->index(result ? 1 : 0, 0)); } void tst_QAbstractItemView::task221955_selectedEditor() { - QPushButton *button; - QTreeWidget tree; tree.setColumnCount(2); - tree.addTopLevelItem(new QTreeWidgetItem(QStringList() << "Foo" <<"1")); - tree.addTopLevelItem(new QTreeWidgetItem(QStringList() << "Bar" <<"2")); - tree.addTopLevelItem(new QTreeWidgetItem(QStringList() << "Baz" <<"3")); + tree.addTopLevelItem(new QTreeWidgetItem({"Foo", "1"})); + tree.addTopLevelItem(new QTreeWidgetItem({"Bar", "2"})); + tree.addTopLevelItem(new QTreeWidgetItem({"Baz", "3"})); QTreeWidgetItem *dummy = new QTreeWidgetItem(); tree.addTopLevelItem(dummy); - tree.setItemWidget(dummy, 0, button = new QPushButton("More...")); + QPushButton *button = new QPushButton("More..."); + tree.setItemWidget(dummy, 0, button); button->setAutoFillBackground(true); // as recommended in doc centerOnScreen(&tree); @@ -1185,10 +1143,10 @@ void tst_QAbstractItemView::task250754_fontChange() QVBoxLayout *vLayout = new QVBoxLayout(&w); vLayout->addWidget(&tree); - QStandardItemModel *m = new QStandardItemModel(this); - for (int i=0; i<20; ++i) { + QStandardItemModel *m = new QStandardItemModel(&w); + for (int i = 0; i < 20; ++i) { QStandardItem *item = new QStandardItem(QStringLiteral("Item number ") + QString::number(i)); - for (int j=0; j<5; ++j) { + for (int j = 0; j < 5; ++j) { QStandardItem *child = new QStandardItem(QStringLiteral("Child Item number ") + QString::number(j)); item->setChild(j, 0, child); } @@ -1222,7 +1180,7 @@ void tst_QAbstractItemView::task200665_itemEntered() { //we test that view will emit entered //when the scrollbar move but not the mouse itself - QStandardItemModel model(1000,1); + QStandardItemModel model(1000, 1); QListView view; view.setModel(&model); centerOnScreen(&view); @@ -1231,7 +1189,7 @@ void tst_QAbstractItemView::task200665_itemEntered() QVERIFY(QTest::qWaitForWindowExposed(&view)); QCursor::setPos(view.geometry().center()); QTRY_COMPARE(QCursor::pos(), view.geometry().center()); - QSignalSpy spy(&view, SIGNAL(entered(QModelIndex))); + QSignalSpy spy(&view, &QAbstractItemView::entered); view.verticalScrollBar()->setValue(view.verticalScrollBar()->maximum()); QTRY_COMPARE(spy.count(), 1); @@ -1239,13 +1197,13 @@ void tst_QAbstractItemView::task200665_itemEntered() void tst_QAbstractItemView::task257481_emptyEditor() { - QIcon icon = qApp->style()->standardIcon(QStyle::SP_ComputerIcon); + QIcon icon = QApplication::style()->standardIcon(QStyle::SP_ComputerIcon); QStandardItemModel model; - model.appendRow( new QStandardItem(icon, QString()) ); - model.appendRow( new QStandardItem(icon, "Editor works") ); - model.appendRow( new QStandardItem( QString() ) ); + model.appendRow(new QStandardItem(icon, QString())); + model.appendRow(new QStandardItem(icon, "Editor works")); + model.appendRow(new QStandardItem(QString())); QTreeView treeView; treeView.setRootIsDecorated(false); @@ -1255,29 +1213,27 @@ void tst_QAbstractItemView::task257481_emptyEditor() treeView.show(); QVERIFY(QTest::qWaitForWindowExposed(&treeView)); - treeView.edit(model.index(0,0)); + treeView.edit(model.index(0, 0)); QList lineEditors = treeView.viewport()->findChildren(); QCOMPARE(lineEditors.count(), 1); - QVERIFY(!lineEditors.first()->size().isEmpty()); + QVERIFY(!lineEditors.constFirst()->size().isEmpty()); - treeView.edit(model.index(1,0)); + treeView.edit(model.index(1, 0)); lineEditors = treeView.viewport()->findChildren(); QCOMPARE(lineEditors.count(), 1); - QVERIFY(!lineEditors.first()->size().isEmpty()); + QVERIFY(!lineEditors.constFirst()->size().isEmpty()); - treeView.edit(model.index(2,0)); + treeView.edit(model.index(2, 0)); lineEditors = treeView.viewport()->findChildren(); QCOMPARE(lineEditors.count(), 1); - QVERIFY(!lineEditors.first()->size().isEmpty()); + QVERIFY(!lineEditors.constFirst()->size().isEmpty()); } void tst_QAbstractItemView::shiftArrowSelectionAfterScrolling() { QStandardItemModel model; - for (int i=0; i<10; ++i) { - QStandardItem *item = new QStandardItem(QString::number(i)); - model.setItem(i, 0, item); - } + for (int i = 0; i < 10; ++i) + model.setItem(i, 0, new QStandardItem(QString::number(i))); QListView view; view.setFixedSize(160, 250); // Minimum width for windows with frame on Windows 8 @@ -1311,10 +1267,8 @@ void tst_QAbstractItemView::shiftArrowSelectionAfterScrolling() void tst_QAbstractItemView::shiftSelectionAfterRubberbandSelection() { QStandardItemModel model; - for (int i=0; i<3; ++i) { - QStandardItem *item = new QStandardItem(QString::number(i)); - model.setItem(i, 0, item); - } + for (int i = 0; i < 3; ++i) + model.setItem(i, 0, new QStandardItem(QString::number(i))); QListView view; view.setFixedSize(160, 450); // Minimum width for windows with frame on Windows 8 @@ -1388,10 +1342,8 @@ void tst_QAbstractItemView::shiftSelectionAfterRubberbandSelection() void tst_QAbstractItemView::ctrlRubberbandSelection() { QStandardItemModel model; - for (int i=0; i<3; ++i) { - QStandardItem *item = new QStandardItem(QString::number(i)); - model.setItem(i, 0, item); - } + for (int i = 0; i < 3; ++i) + model.setItem(i, 0, new QStandardItem(QString::number(i))); QListView view; view.setFixedSize(160, 450); // Minimum width for windows with frame on Windows 8 @@ -1435,7 +1387,7 @@ void tst_QAbstractItemView::QTBUG6407_extendedSelection() { QListWidget view; view.setSelectionMode(QAbstractItemView::ExtendedSelection); - for(int i = 0; i < 50; ++i) + for (int i = 0; i < 50; ++i) view.addItem(QString::number(i)); QFont font = view.font(); @@ -1448,14 +1400,14 @@ void tst_QAbstractItemView::QTBUG6407_extendedSelection() view.show(); QApplication::setActiveWindow(&view); QVERIFY(QTest::qWaitForWindowActive(&view)); - QCOMPARE(static_cast(&view), QApplication::activeWindow()); + QCOMPARE(&view, QApplication::activeWindow()); view.verticalScrollBar()->setValue(view.verticalScrollBar()->maximum()); QModelIndex index49 = view.model()->index(49,0); QPoint p = view.visualRect(index49).center(); QVERIFY(view.viewport()->rect().contains(p)); - QTest::mouseClick(view.viewport(), Qt::LeftButton, 0, p); + QTest::mouseClick(view.viewport(), Qt::LeftButton, {}, p); QCOMPARE(view.currentIndex(), index49); QCOMPARE(view.selectedItems().count(), 1); @@ -1478,9 +1430,10 @@ void tst_QAbstractItemView::QTBUG6407_extendedSelection() void tst_QAbstractItemView::QTBUG6753_selectOnSelection() { QTableWidget table(5, 5); - for (int i = 0; i < table.rowCount(); ++i) + for (int i = 0; i < table.rowCount(); ++i) { for (int j = 0; j < table.columnCount(); ++j) table.setItem(i, j, new QTableWidgetItem("choo-be-doo-wah")); + } centerOnScreen(&table); moveCursorAway(&table); @@ -1520,69 +1473,47 @@ void tst_QAbstractItemView::testClickedSignal() view.showNormal(); QApplication::setActiveWindow(&view); QVERIFY(QTest::qWaitForWindowActive(&view)); - QCOMPARE(static_cast(&view), QApplication::activeWindow()); + QCOMPARE(&view, QApplication::activeWindow()); QModelIndex index49 = view.model()->index(49,0); QPoint p = view.visualRect(index49).center(); QVERIFY(view.viewport()->rect().contains(p)); - QSignalSpy clickedSpy(&view, SIGNAL(clicked(QModelIndex))); + QSignalSpy clickedSpy(&view, &QTableWidget::clicked); - QTest::mouseClick(view.viewport(), Qt::LeftButton, 0, p); + QTest::mouseClick(view.viewport(), Qt::LeftButton, {}, p); #ifdef Q_OS_WINRT QEXPECT_FAIL("", "Fails on WinRT - QTBUG-68297", Abort); #endif QCOMPARE(clickedSpy.count(), 1); - QTest::mouseClick(view.viewport(), Qt::RightButton, 0, p); + QTest::mouseClick(view.viewport(), Qt::RightButton, {}, p); // We expect that right-clicks do not cause the clicked() signal to // be emitted. QCOMPARE(clickedSpy.count(), 1); } -class StateChangeDelegate : public QItemDelegate { - Q_OBJECT - -public: - explicit StateChangeDelegate(QObject* parent = 0) : - QItemDelegate(parent) - {} - - void setEditorData(QWidget *editor, const QModelIndex &index) const override { - Q_UNUSED(index); - static bool w = true; - editor->setEnabled(w); - w = !w; - } -}; - -class StateChangeModel : public QStandardItemModel { - Q_OBJECT - +class StateChangeDelegate : public QStyledItemDelegate +{ + Q_OBJECT public: - explicit StateChangeModel(QObject *parent = 0) : - QStandardItemModel(parent) - {} - - void emitDataChanged() { - emit dataChanged(index(0, 0), index(0, 1)); - } + using QStyledItemDelegate::QStyledItemDelegate; + void setEditorData(QWidget *editor, const QModelIndex &index) const override + { + Q_UNUSED(index); + static bool w = true; + editor->setEnabled(w); + w = !w; + } }; - void tst_QAbstractItemView::testChangeEditorState() { // Test for QTBUG-25370 - StateChangeModel model; - { - QStandardItem* item = new QStandardItem("a"); - model.setItem(0, 0, item); - } - { - QStandardItem* item = new QStandardItem("b"); - model.setItem(0, 1, item); - } + TestModel model; + model.setItem(0, 0, new QStandardItem("a")); + model.setItem(0, 1, new QStandardItem("b")); QTableView view; view.setEditTriggers(QAbstractItemView::CurrentChanged); @@ -1593,7 +1524,7 @@ void tst_QAbstractItemView::testChangeEditorState() view.show(); QApplication::setActiveWindow(&view); QVERIFY(QTest::qWaitForWindowActive(&view)); - QCOMPARE(static_cast(&view), QApplication::activeWindow()); + QCOMPARE(&view, QApplication::activeWindow()); model.emitDataChanged(); // No segfault - the test passes. @@ -1659,12 +1590,12 @@ void tst_QAbstractItemView::testNoActivateOnDisabledItem() QApplication::setActiveWindow(&treeView); QVERIFY(QTest::qWaitForWindowActive(&treeView)); - QSignalSpy activatedSpy(&treeView, SIGNAL(activated(QModelIndex))); + QSignalSpy activatedSpy(&treeView, &QAbstractItemView::activated); // Ensure clicking on a disabled item doesn't emit itemActivated. QModelIndex itemIndex = treeView.model()->index(0, 0); QPoint clickPos = treeView.visualRect(itemIndex).center(); - QTest::mouseClick(treeView.viewport(), Qt::LeftButton, 0, clickPos); + QTest::mouseClick(treeView.viewport(), Qt::LeftButton, {}, clickPos); QCOMPARE(activatedSpy.count(), 0); } @@ -1706,18 +1637,18 @@ void tst_QAbstractItemView::testFocusPolicy() // itemview accepts focus => editor is closed => return focus to the itemview QPoint clickpos = table->visualRect(model.index(1, 1)).center(); QTest::mouseDClick(table->viewport(), Qt::LeftButton, Qt::NoModifier, clickpos); - QWidget *editor = qApp->focusWidget(); + QWidget *editor = QApplication::focusWidget(); QVERIFY(editor); QTest::keyClick(editor, Qt::Key_Escape, Qt::NoModifier); - QCOMPARE(qApp->focusWidget(), table); + QCOMPARE(QApplication::focusWidget(), table); // itemview doesn't accept focus => editor is closed => clear the focus table->setFocusPolicy(Qt::NoFocus); QTest::mouseDClick(table->viewport(), Qt::LeftButton, Qt::NoModifier, clickpos); - editor = qApp->focusWidget(); + editor = QApplication::focusWidget(); QVERIFY(editor); QTest::keyClick(editor, Qt::Key_Escape, Qt::NoModifier); - QVERIFY(!qApp->focusWidget()); + QVERIFY(!QApplication::focusWidget()); } void tst_QAbstractItemView::QTBUG31411_noSelection() @@ -1742,18 +1673,18 @@ void tst_QAbstractItemView::QTBUG31411_noSelection() QVERIFY(QTest::qWaitForWindowActive(&window)); qRegisterMetaType(); - QSignalSpy selectionChangeSpy(table->selectionModel(), SIGNAL(selectionChanged(QItemSelection,QItemSelection))); + QSignalSpy selectionChangeSpy(table->selectionModel(), &QItemSelectionModel::selectionChanged); QVERIFY(selectionChangeSpy.isValid()); QPoint clickpos = table->visualRect(model.index(1, 1)).center(); QTest::mouseClick(table->viewport(), Qt::LeftButton, Qt::NoModifier, clickpos); QTest::mouseDClick(table->viewport(), Qt::LeftButton, Qt::NoModifier, clickpos); - QPointer editor1 = qApp->focusWidget(); + QPointer editor1 = QApplication::focusWidget(); QVERIFY(editor1); QTest::keyClick(editor1, Qt::Key_Tab, Qt::NoModifier); - QPointer editor2 = qApp->focusWidget(); + QPointer editor2 = QApplication::focusWidget(); QVERIFY(editor2); QTest::keyClick(editor2, Qt::Key_Escape, Qt::NoModifier); @@ -1762,26 +1693,22 @@ void tst_QAbstractItemView::QTBUG31411_noSelection() void tst_QAbstractItemView::QTBUG39324_settingSameInstanceOfIndexWidget() { - QStringList list; - list << "FOO" << "bar"; - QScopedPointer model(new QStringListModel(list)); + QStringListModel model({ "FOO", "bar" }); QScopedPointer table(new QTableView()); - table->setModel(model.data()); + table->setModel(&model); - QModelIndex index = model->index(0,0); + QModelIndex index = model.index(0,0); QLineEdit *lineEdit = new QLineEdit(); table->setIndexWidget(index, lineEdit); table->setIndexWidget(index, lineEdit); - QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete); + QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete); table->show(); } void tst_QAbstractItemView::shiftSelectionAfterChangingModelContents() { - QStringList list; - list << "A" << "B" << "C" << "D" << "E" << "F"; - QStringListModel model(list); + QStringListModel model({ "A", "B", "C", "D", "E", "F" }); QSortFilterProxyModel proxyModel; proxyModel.setSourceModel(&model); proxyModel.sort(0, Qt::AscendingOrder); @@ -1838,9 +1765,8 @@ void tst_QAbstractItemView::shiftSelectionAfterChangingModelContents() QVERIFY(selected.contains(indexF)); // Move to "A" by pressing "Up" repeatedly - while (view.currentIndex() != indexA) { + while (view.currentIndex() != indexA) QTest::keyClick(&view, Qt::Key_Up); - } selected = view.selectionModel()->selectedIndexes(); QCOMPARE(selected.count(), 1); QVERIFY(selected.contains(indexA)); @@ -1895,22 +1821,22 @@ void tst_QAbstractItemView::shiftSelectionAfterChangingModelContents() void tst_QAbstractItemView::QTBUG48968_reentrant_updateEditorGeometries() { - - QStandardItemModel *m = new QStandardItemModel(this); - for (int i=0; i<10; ++i) { + QTreeView tree; + QStandardItemModel *m = new QStandardItemModel(&tree); + for (int i = 0; i < 10; ++i) { QStandardItem *item = new QStandardItem(QString("Item number %1").arg(i)); item->setEditable(true); - for (int j=0; j<5; ++j) { + for (int j = 0; j < 5; ++j) { QStandardItem *child = new QStandardItem(QString("Child Item number %1").arg(j)); item->setChild(j, 0, child); } m->setItem(i, 0, item); } - QTreeView tree; tree.setModel(m); tree.setRootIsDecorated(false); - QObject::connect(&tree, SIGNAL(doubleClicked(QModelIndex)), &tree, SLOT(setRootIndex(QModelIndex))); + connect(&tree, &QTreeView::doubleClicked, + &tree, &QTreeView::setRootIndex); tree.show(); QVERIFY(QTest::qWaitForWindowActive(&tree)); @@ -1922,7 +1848,7 @@ void tst_QAbstractItemView::QTBUG48968_reentrant_updateEditorGeometries() // Add more children to idx QStandardItem *item = m->itemFromIndex(idx); - for (int j=5; j<10; ++j) { + for (int j = 5; j < 10; ++j) { QStandardItem *child = new QStandardItem(QString("Child Item number %1").arg(j)); item->setChild(j, 0, child); } @@ -1932,14 +1858,16 @@ void tst_QAbstractItemView::QTBUG48968_reentrant_updateEditorGeometries() class ScrollModeProxyStyle: public QProxyStyle { + Q_OBJECT public: - ScrollModeProxyStyle(QAbstractItemView::ScrollMode sm, QStyle *style = 0) + ScrollModeProxyStyle(QAbstractItemView::ScrollMode sm, QStyle *style = nullptr) : QProxyStyle(style) , scrollMode(sm == QAbstractItemView::ScrollPerItem ? QAbstractItemView::ScrollPerPixel : QAbstractItemView::ScrollPerItem) { } - int styleHint(QStyle::StyleHint hint, const QStyleOption *opt, const QWidget *w, QStyleHintReturn *returnData) const override + int styleHint(QStyle::StyleHint hint, const QStyleOption *opt, + const QWidget *w, QStyleHintReturn *returnData) const override { if (hint == SH_ItemView_ScrollMode) return scrollMode; @@ -1955,14 +1883,16 @@ void tst_QAbstractItemView::QTBUG50102_SH_ItemView_ScrollMode() QListView view; // Default comes from the style - auto styleScrollMode = static_cast(view.style()->styleHint(QStyle::SH_ItemView_ScrollMode, 0, &view, 0)); + auto styleScrollMode = static_cast( + view.style()->styleHint(QStyle::SH_ItemView_ScrollMode, nullptr, &view, nullptr)); QCOMPARE(view.verticalScrollMode(), styleScrollMode); QCOMPARE(view.horizontalScrollMode(), styleScrollMode); // Change style, get new value ScrollModeProxyStyle proxyStyle1(styleScrollMode); view.setStyle(&proxyStyle1); - auto proxyScrollMode = static_cast(view.style()->styleHint(QStyle::SH_ItemView_ScrollMode, 0, &view, 0)); + auto proxyScrollMode = static_cast( + view.style()->styleHint(QStyle::SH_ItemView_ScrollMode, nullptr, &view, nullptr)); QVERIFY(styleScrollMode != proxyScrollMode); QCOMPARE(view.verticalScrollMode(), proxyScrollMode); QCOMPARE(view.horizontalScrollMode(), proxyScrollMode); @@ -1988,10 +1918,7 @@ void tst_QAbstractItemView::QTBUG50535_update_on_new_selection_model() class ListView : public QListView { public: - ListView() - : m_paintEventsCount(0), m_deselectedMustBeEmpty(false), m_selectionChangedOk(true) - { - } + using QListView::QListView; void setSelectionModel(QItemSelectionModel *model) override { @@ -1999,7 +1926,7 @@ void tst_QAbstractItemView::QTBUG50535_update_on_new_selection_model() QListView::setSelectionModel(model); m_deselectedMustBeEmpty = false; } - int m_paintEventsCount; + int m_paintEventsCount = 0; bool selectionChangedOk() const { return m_selectionChangedOk; } protected: @@ -2017,16 +1944,17 @@ void tst_QAbstractItemView::QTBUG50535_update_on_new_selection_model() m_selectionChangedOk = false; // Make sure both selections belong to the same model - foreach (const QModelIndex &nmi, selected.indexes()) { - foreach (const QModelIndex &omi, deselected.indexes()) { + const auto selIndexes = selected.indexes(); + const auto deselIndexes = deselected.indexes(); + for (const QModelIndex &nmi : selIndexes) { + for (const QModelIndex &omi : deselIndexes) m_selectionChangedOk = m_selectionChangedOk && (nmi.model() == omi.model()); - } } QListView::selectionChanged(selected, deselected); } private: - bool m_deselectedMustBeEmpty; - bool m_selectionChangedOk; + bool m_deselectedMustBeEmpty = false; + bool m_selectionChangedOk = true; }; // keep the current/selected row in the "low range", i.e. be sure it's visible, otherwise we @@ -2074,7 +2002,7 @@ void tst_QAbstractItemView::testSelectionModelInSyncWithView() { QStandardItemModel model; for (int i = 0; i < 10; ++i) - model.appendRow(new QStandardItem(QStringLiteral("%1").arg(i))); + model.appendRow(new QStandardItem(QString::number(i))); class ListView : public QListView { @@ -2128,8 +2056,7 @@ class SetSelectionTestView : public QListView { Q_OBJECT public: - SetSelectionTestView() : QListView() {} - + using QListView::QListView; signals: void setSelectionCalled(const QRect &rect); @@ -2147,9 +2074,7 @@ void tst_QAbstractItemView::testClickToSelect() // to the virtual method QAbstractItemView::setSelection(const QRect &, SelectionFlags) // is the 1x1 rect which conains exactly the clicked pixel if no modifiers are pressed. - QStringList list; - list << "A" << "B" << "C"; - QStringListModel model(list); + QStringListModel model({ "A", "B", "C" }); SetSelectionTestView view; view.setModel(&model); @@ -2207,28 +2132,24 @@ void tst_QAbstractItemView::testDialogAsEditor() QCOMPARE(delegate.result, QDialog::Accepted); } -class HoverItemDelegate : public QItemDelegate +class HoverItemDelegate : public QStyledItemDelegate { public: - HoverItemDelegate() - : QItemDelegate() - , m_paintedWithoutHover(false) - { } - - void paint(QPainter *painter, const QStyleOptionViewItem &opt, const QModelIndex &index) const override + using QStyledItemDelegate::QStyledItemDelegate; + void paint(QPainter *painter, const QStyleOptionViewItem &opt, + const QModelIndex &index) const override { Q_UNUSED(painter); if (!(opt.state & QStyle::State_MouseOver)) { // We don't want to set m_paintedWithoutHover for any item so check for the item at 0,0 - if (index.row() == 0 && index.column() == 0) { + if (index.row() == 0 && index.column() == 0) m_paintedWithoutHover = true; - } } } - mutable bool m_paintedWithoutHover; + mutable bool m_paintedWithoutHover = false; }; void tst_QAbstractItemView::QTBUG46785_mouseout_hover_state() @@ -2249,7 +2170,7 @@ void tst_QAbstractItemView::QTBUG46785_mouseout_hover_state() // Move the mouse into the center of the item at 0,0 to cause a paint event to occur QTest::mouseMove(table.viewport(), itemRect.center()); - QTest::mouseClick(table.viewport(), Qt::LeftButton, 0, itemRect.center()); + QTest::mouseClick(table.viewport(), Qt::LeftButton, {}, itemRect.center()); delegate.m_paintedWithoutHover = false; @@ -2263,14 +2184,13 @@ void tst_QAbstractItemView::QTBUG46785_mouseout_hover_state() void tst_QAbstractItemView::testClearModelInClickedSignal() { - QStringList list{"A", "B"}; - QStringListModel model(list); + QStringListModel model({"A", "B"}); QListView view; view.setModel(&model); view.show(); - QWidget::connect(&view, &QListView::clicked, [&view](const QModelIndex &index) + connect(&view, &QListView::clicked, [&view](const QModelIndex &index) { view.setModel(nullptr); QCOMPARE(index.data().toString(), QStringLiteral("B")); @@ -2291,14 +2211,16 @@ void tst_QAbstractItemView::inputMethodEnabled_data() QTest::addColumn("itemFlags"); QTest::addColumn("result"); - QList widgets; - widgets << "QListView" << "QTreeView" << "QTableView"; - - for (const QByteArray &widget : qAsConst(widgets)) { - QTest::newRow(widget + ": no flags") << widget << Qt::ItemFlags(Qt::NoItemFlags) << false; - QTest::newRow(widget + ": checkable") << widget << Qt::ItemFlags(Qt::ItemIsUserCheckable) << false; - QTest::newRow(widget + ": selectable") << widget << Qt::ItemFlags(Qt::ItemIsSelectable) << false; - QTest::newRow(widget + ": enabled") << widget << Qt::ItemFlags(Qt::ItemIsEnabled) << false; + const QVector widgets{ "QListView", "QTreeView", "QTableView" }; + for (const QByteArray &widget : widgets) { + QTest::newRow(widget + ": no flags") + << widget << Qt::ItemFlags(Qt::NoItemFlags) << false; + QTest::newRow(widget + ": checkable") + << widget << Qt::ItemFlags(Qt::ItemIsUserCheckable) << false; + QTest::newRow(widget + ": selectable") + << widget << Qt::ItemFlags(Qt::ItemIsSelectable) << false; + QTest::newRow(widget + ": enabled") + << widget << Qt::ItemFlags(Qt::ItemIsEnabled) << false; QTest::newRow(widget + ": selectable|enabled") << widget << Qt::ItemFlags(Qt::ItemIsSelectable | Qt::ItemIsEnabled) << false; QTest::newRow(widget + ": editable|enabled") @@ -2314,15 +2236,7 @@ void tst_QAbstractItemView::inputMethodEnabled() QFETCH(Qt::ItemFlags, itemFlags); QFETCH(bool, result); - QScopedPointer view; - if (viewType == "QListView") - view.reset(new QListView()); - else if (viewType == "QTableView") - view.reset(new QTableView()); - else if (viewType == "QTreeView") - view.reset(new QTreeView()); - else - QVERIFY(0); + QScopedPointer view(viewFromString(viewType)); centerOnScreen(view.data()); view->show(); @@ -2349,14 +2263,14 @@ void tst_QAbstractItemView::inputMethodEnabled() // Check focus by switching the activation of the window to force a focus in view->setCurrentIndex(model->index(1, 0)); - QApplication::setActiveWindow(0); + QApplication::setActiveWindow(nullptr); QApplication::setActiveWindow(view.data()); QVERIFY(QTest::qWaitForWindowActive(view.data())); QCOMPARE(view->testAttribute(Qt::WA_InputMethodEnabled), result); view->setCurrentIndex(QModelIndex()); QVERIFY(!view->testAttribute(Qt::WA_InputMethodEnabled)); - QApplication::setActiveWindow(0); + QApplication::setActiveWindow(nullptr); QApplication::setActiveWindow(view.data()); QVERIFY(QTest::qWaitForWindowActive(view.data())); QModelIndex index = model->index(1, 0); @@ -2367,7 +2281,7 @@ void tst_QAbstractItemView::inputMethodEnabled() QCOMPARE(view->testAttribute(Qt::WA_InputMethodEnabled), result); index = model->index(0, 0); - QApplication::setActiveWindow(0); + QApplication::setActiveWindow(nullptr); QApplication::setActiveWindow(view.data()); QVERIFY(QTest::qWaitForWindowActive(view.data())); p = view->visualRect(index).center(); @@ -2378,7 +2292,7 @@ void tst_QAbstractItemView::inputMethodEnabled() // There is a case when it goes to the first visible item so we // make the flags of the first item match the ones we are testing // to check the attribute correctly - QApplication::setActiveWindow(0); + QApplication::setActiveWindow(nullptr); view->setCurrentIndex(QModelIndex()); view->reset(); item->setFlags(Qt::ItemFlags(itemFlags)); @@ -2391,9 +2305,8 @@ void tst_QAbstractItemView::currentFollowsIndexWidget_data() { QTest::addColumn("viewType"); - QList widgets; - widgets << "QListView" << "QTreeView" << "QTableView"; - for (const QByteArray &widget : qAsConst(widgets)) + const QVector widgets{ "QListView", "QTreeView", "QTableView" }; + for (const QByteArray &widget : widgets) QTest::newRow(widget) << widget; } @@ -2401,15 +2314,7 @@ void tst_QAbstractItemView::currentFollowsIndexWidget() { QFETCH(QByteArray, viewType); - QScopedPointer view; - if (viewType == "QListView") - view.reset(new QListView()); - else if (viewType == "QTableView") - view.reset(new QTableView()); - else if (viewType == "QTreeView") - view.reset(new QTreeView()); - else - QVERIFY(0); + QScopedPointer view(viewFromString(viewType)); centerOnScreen(view.data()); view->show(); @@ -2438,17 +2343,18 @@ void tst_QAbstractItemView::currentFollowsIndexWidget() QCOMPARE(view->currentIndex(), item1->index()); } -class EditorItemDelegate : public QItemDelegate +class EditorItemDelegate : public QStyledItemDelegate { + Q_OBJECT public: - EditorItemDelegate() : QItemDelegate(), openedEditor(nullptr) { } + using QStyledItemDelegate::QStyledItemDelegate; QWidget *createEditor(QWidget *parent, const QStyleOptionViewItem &, const QModelIndex &) const override { openedEditor = new QLineEdit(parent); return openedEditor; } - mutable QPointer openedEditor; + mutable QPointer openedEditor = nullptr; }; // Testing the case reported in QTBUG-62253. @@ -2458,18 +2364,14 @@ public: // on the original window. void tst_QAbstractItemView::checkFocusAfterActivationChanges_data() { - QTest::addColumn("viewType"); - - QTest::newRow("QListView") << "QListView"; - QTest::newRow("QTableView") << "QTableView"; - QTest::newRow("QTreeView") << "QTreeView"; + currentFollowsIndexWidget_data(); } void tst_QAbstractItemView::checkFocusAfterActivationChanges() { - QFETCH(QString, viewType); + QFETCH(QByteArray, viewType); - const QRect availableGeo = qApp->primaryScreen()->availableGeometry(); + const QRect availableGeo = QGuiApplication::primaryScreen()->availableGeometry(); const int halfWidth = availableGeo.width() / 2; QWidget otherTopLevel; otherTopLevel.setGeometry(availableGeo.x(), availableGeo.y(), @@ -2480,13 +2382,7 @@ void tst_QAbstractItemView::checkFocusAfterActivationChanges() w.setGeometry(availableGeo.x() + halfWidth, availableGeo.y(), halfWidth, availableGeo.height()); QLineEdit *le = new QLineEdit(&w); - QAbstractItemView *view = 0; - if (viewType == "QListView") - view = new QListView(&w); - else if (viewType == "QTableView") - view = new QTableView(&w); - else if (viewType == "QTreeView") - view = new QTreeView(&w); + QAbstractItemView *view = viewFromString(viewType, &w); QStandardItemModel model(5, 5); view->setModel(&model); -- cgit v1.2.3 From 746ab5f16d5c297567341797869b124868a926fe Mon Sep 17 00:00:00 2001 From: Lars Knoll Date: Wed, 2 Oct 2019 15:12:17 +0200 Subject: Remove potential out of bounds accesses to QList Change-Id: I13431e45ef329921a8846c38047f704a299a1a94 Reviewed-by: Marc Mutz Reviewed-by: Frederik Gladhorn --- src/corelib/animation/qanimationgroup.cpp | 5 ++++- src/corelib/kernel/qobject.cpp | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/corelib/animation/qanimationgroup.cpp b/src/corelib/animation/qanimationgroup.cpp index ed40817222..69e2cfc9bc 100644 --- a/src/corelib/animation/qanimationgroup.cpp +++ b/src/corelib/animation/qanimationgroup.cpp @@ -195,8 +195,11 @@ void QAnimationGroup::insertAnimation(int index, QAbstractAnimation *animation) return; } - if (QAnimationGroup *oldGroup = animation->group()) + if (QAnimationGroup *oldGroup = animation->group()) { oldGroup->removeAnimation(animation); + // ensure we don't insert out of bounds if oldGroup == this + index = qMin(index, d->animations.size()); + } d->animations.insert(index, animation); QAbstractAnimationPrivate::get(animation)->group = this; diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index cf107498dd..fb0d54c801 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -2157,7 +2157,9 @@ void QObjectPrivate::setParent_helper(QObject *o) // cleared our entry in parentD->children. } else { const int index = parentD->children.indexOf(q); - if (parentD->isDeletingChildren) { + if (index < 0) { + // we're probably recursing into setParent() from a ChildRemoved event, don't do anything + } else if (parentD->isDeletingChildren) { parentD->children[index] = 0; } else { parentD->children.removeAt(index); -- cgit v1.2.3 From ebf695bc779a63a5730df05ab246305c0ab342e4 Mon Sep 17 00:00:00 2001 From: Lars Knoll Date: Fri, 4 Oct 2019 16:53:56 +0200 Subject: Deprecate calling QList::insert/removeAt with out of bounds index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Users should not call any QList API with indices that are out of bounds. Deprecate this behavior and make sure users get warnings in debug mode and assertions if they disable deprecated functionality. [ChangeLog][Important Behavior Changes] Calling QList::insert() or removeAt() with an out of bounds index is deprecated and will not be supported in Qt 6 anymore. Change-Id: I97adecc2e2aabd36ea2cc69e0895d625f78b32a0 Reviewed-by: Mårten Nordheim Reviewed-by: Frederik Gladhorn --- src/corelib/tools/qlist.cpp | 7 ++++--- src/corelib/tools/qlist.h | 18 ++++++++++++++++-- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/corelib/tools/qlist.cpp b/src/corelib/tools/qlist.cpp index dfebd57e34..3dc962236c 100644 --- a/src/corelib/tools/qlist.cpp +++ b/src/corelib/tools/qlist.cpp @@ -847,9 +847,10 @@ void **QListData::erase(void **xi) /*! \fn template void QList::insert(int i, const T &value) - Inserts \a value at index position \a i in the list. If \a i <= 0, - the value is prepended to the list. If \a i >= size(), the - value is appended to the list. + Inserts \a value at index position \a i in the list. + + If \a i == 0, the value is prepended to the list. If \a i == size(), + the value is appended to the list. Example: \snippet code/src_corelib_tools_qlistdata.cpp 8 diff --git a/src/corelib/tools/qlist.h b/src/corelib/tools/qlist.h index 425ffa42a5..ffd470efcd 100644 --- a/src/corelib/tools/qlist.h +++ b/src/corelib/tools/qlist.h @@ -580,8 +580,16 @@ inline T &QList::operator[](int i) detach(); return reinterpret_cast(p.at(i))->t(); } template inline void QList::removeAt(int i) -{ if(i >= 0 && i < p.size()) { detach(); - node_destruct(reinterpret_cast(p.at(i))); p.remove(i); } } +{ +#if !QT_DEPRECATED_SINCE(5, 15) + Q_ASSERT_X(i >= 0 && i < p.size(), "QList::removeAt", "index out of range"); +#elif !defined(QT_NO_DEBUG) + if (i < 0 || i >= p.size()) + qWarning("QList::removeAt(): Index out of range."); +#endif + detach(); + node_destruct(reinterpret_cast(p.at(i))); p.remove(i); +} template inline T QList::takeAt(int i) { Q_ASSERT_X(i >= 0 && i < p.size(), "QList::take", "index out of range"); @@ -676,6 +684,12 @@ inline void QList::prepend(const T &t) template inline void QList::insert(int i, const T &t) { +#if !QT_DEPRECATED_SINCE(5, 15) + Q_ASSERT_X(i >= 0 && i <= p.size(), "QList::insert", "index out of range"); +#elif !defined(QT_NO_DEBUG) + if (i < 0 || i > p.size()) + qWarning("QList::insert(): Index out of range."); +#endif if (d->ref.isShared()) { Node *n = detach_helper_grow(i, 1); QT_TRY { -- cgit v1.2.3 From f60b9df73d53774f0e83215f41b9cf91cbe09197 Mon Sep 17 00:00:00 2001 From: Lars Knoll Date: Wed, 21 Nov 2018 13:44:01 +0100 Subject: Deprecate reverse iteration on QSet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit std::unordered_set only supports forward iteration for good reasons. Align our API with this by deprecating reverse iteration and the operator+/-() for iterators. [ChangeLog][QtCore][QSet] Reverse iteration over QSet is now deprecated. Change-Id: Ia6e3346a6474c454c63010d855850ae4ff12e1a4 Reviewed-by: Mårten Nordheim Reviewed-by: Frederik Gladhorn --- src/corelib/tools/qset.h | 70 ++++++++++++++++++++++++++++----------------- src/corelib/tools/qset.qdoc | 8 ++++-- 2 files changed, 49 insertions(+), 29 deletions(-) diff --git a/src/corelib/tools/qset.h b/src/corelib/tools/qset.h index 2e72832185..879e2176f2 100644 --- a/src/corelib/tools/qset.h +++ b/src/corelib/tools/qset.h @@ -108,7 +108,11 @@ public: friend class QSet; public: +#if QT_DEPRECATED_SINCE(5, 15) typedef std::bidirectional_iterator_tag iterator_category; +#else + typedef std::forward_iterator_tag iterator_category; +#endif typedef qptrdiff difference_type; typedef T value_type; typedef const T *pointer; @@ -128,13 +132,15 @@ public: { return i != o.i; } inline iterator &operator++() { ++i; return *this; } inline iterator operator++(int) { iterator r = *this; ++i; return r; } - inline iterator &operator--() { --i; return *this; } - inline iterator operator--(int) { iterator r = *this; --i; return r; } - inline iterator operator+(int j) const { return i + j; } - inline iterator operator-(int j) const { return i - j; } - friend inline iterator operator+(int j, iterator k) { return k + j; } - inline iterator &operator+=(int j) { i += j; return *this; } - inline iterator &operator-=(int j) { i -= j; return *this; } +#if QT_DEPRECATED_SINCE(5, 15) + inline QT_DEPRECATED iterator &operator--() { --i; return *this; } + inline QT_DEPRECATED iterator operator--(int) { iterator r = *this; --i; return r; } + inline QT_DEPRECATED iterator operator+(int j) const { return i + j; } + inline QT_DEPRECATED iterator operator-(int j) const { return i - j; } + friend inline QT_DEPRECATED iterator operator+(int j, iterator k) { return k + j; } + inline QT_DEPRECATED iterator &operator+=(int j) { i += j; return *this; } + inline QT_DEPRECATED iterator &operator-=(int j) { i -= j; return *this; } +#endif }; class const_iterator @@ -145,7 +151,11 @@ public: friend class QSet; public: +#if QT_DEPRECATED_SINCE(5, 15) typedef std::bidirectional_iterator_tag iterator_category; +#else + typedef std::forward_iterator_tag iterator_category; +#endif typedef qptrdiff difference_type; typedef T value_type; typedef const T *pointer; @@ -163,19 +173,18 @@ public: inline bool operator!=(const const_iterator &o) const { return i != o.i; } inline const_iterator &operator++() { ++i; return *this; } inline const_iterator operator++(int) { const_iterator r = *this; ++i; return r; } - inline const_iterator &operator--() { --i; return *this; } - inline const_iterator operator--(int) { const_iterator r = *this; --i; return r; } - inline const_iterator operator+(int j) const { return i + j; } - inline const_iterator operator-(int j) const { return i - j; } - friend inline const_iterator operator+(int j, const_iterator k) { return k + j; } - inline const_iterator &operator+=(int j) { i += j; return *this; } - inline const_iterator &operator-=(int j) { i -= j; return *this; } +#if QT_DEPRECATED_SINCE(5, 15) + inline QT_DEPRECATED const_iterator &operator--() { --i; return *this; } + inline QT_DEPRECATED const_iterator operator--(int) { const_iterator r = *this; --i; return r; } + inline QT_DEPRECATED const_iterator operator+(int j) const { return i + j; } + inline QT_DEPRECATED const_iterator operator-(int j) const { return i - j; } + friend inline QT_DEPRECATED const_iterator operator+(int j, const_iterator k) { return k + j; } + inline QT_DEPRECATED const_iterator &operator+=(int j) { i += j; return *this; } + inline QT_DEPRECATED const_iterator &operator-=(int j) { i -= j; return *this; } +#endif }; // STL style - typedef std::reverse_iterator reverse_iterator; - typedef std::reverse_iterator const_reverse_iterator; - inline iterator begin() { return q_hash.begin(); } inline const_iterator begin() const noexcept { return q_hash.begin(); } inline const_iterator cbegin() const noexcept { return q_hash.begin(); } @@ -185,12 +194,17 @@ public: inline const_iterator cend() const noexcept { return q_hash.end(); } inline const_iterator constEnd() const noexcept { return q_hash.constEnd(); } - reverse_iterator rbegin() { return reverse_iterator(end()); } - reverse_iterator rend() { return reverse_iterator(begin()); } - const_reverse_iterator rbegin() const noexcept { return const_reverse_iterator(end()); } - const_reverse_iterator rend() const noexcept { return const_reverse_iterator(begin()); } - const_reverse_iterator crbegin() const noexcept { return const_reverse_iterator(end()); } - const_reverse_iterator crend() const noexcept { return const_reverse_iterator(begin()); } +#if QT_DEPRECATED_SINCE(5, 15) + typedef std::reverse_iterator reverse_iterator; + typedef std::reverse_iterator const_reverse_iterator; + + reverse_iterator QT_DEPRECATED rbegin() { return reverse_iterator(end()); } + reverse_iterator QT_DEPRECATED rend() { return reverse_iterator(begin()); } + const_reverse_iterator QT_DEPRECATED rbegin() const noexcept { return const_reverse_iterator(end()); } + const_reverse_iterator QT_DEPRECATED rend() const noexcept { return const_reverse_iterator(begin()); } + const_reverse_iterator QT_DEPRECATED crbegin() const noexcept { return const_reverse_iterator(end()); } + const_reverse_iterator QT_DEPRECATED crend() const noexcept { return const_reverse_iterator(begin()); } +#endif iterator erase(iterator i) { return erase(m2c(i)); } @@ -429,17 +443,19 @@ public: inline bool hasNext() const { return c->constEnd() != i; } inline const T &next() { n = i++; return *n; } inline const T &peekNext() const { return *i; } - inline bool hasPrevious() const { return c->constBegin() != i; } - inline const T &previous() { n = --i; return *n; } - inline const T &peekPrevious() const { iterator p = i; return *--p; } inline void remove() { if (c->constEnd() != n) { i = c->erase(n); n = c->end(); } } inline const T &value() const { Q_ASSERT(item_exists()); return *n; } inline bool findNext(const T &t) { while (c->constEnd() != (n = i)) if (*i++ == t) return true; return false; } - inline bool findPrevious(const T &t) +#if QT_DEPRECATED_SINCE(5, 15) + inline QT_DEPRECATED bool hasPrevious() const { return c->constBegin() != i; } + inline QT_DEPRECATED const T &previous() { n = --i; return *n; } + inline QT_DEPRECATED const T &peekPrevious() const { iterator p = i; return *--p; } + inline QT_DEPRECATED bool findPrevious(const T &t) { while (c->constBegin() != i) if (*(n = --i) == t) return true; n = c->end(); return false; } +#endif }; #endif // QT_NO_JAVA_STYLE_ITERATORS diff --git a/src/corelib/tools/qset.qdoc b/src/corelib/tools/qset.qdoc index 084523ed4b..7c752d0318 100644 --- a/src/corelib/tools/qset.qdoc +++ b/src/corelib/tools/qset.qdoc @@ -906,8 +906,6 @@ Calling this function on QSet::constEnd() leads to undefined results. - - \sa operator--() */ /*! @@ -924,6 +922,7 @@ /*! \fn template QSet::iterator &QSet::iterator::operator--() \fn template QSet::const_iterator &QSet::const_iterator::operator--() + \obsolete The prefix -- operator (\c{--it}) makes the preceding item current and returns an iterator to the new current item. @@ -937,6 +936,7 @@ /*! \fn template QSet::iterator QSet::iterator::operator--(int) \fn template QSet::const_iterator QSet::const_iterator::operator--(int) + \obsolete \overload @@ -947,6 +947,7 @@ /*! \fn template QSet::iterator QSet::iterator::operator+(int j) const \fn template QSet::const_iterator QSet::const_iterator::operator+(int j) const + \obsolete Returns an iterator to the item at \a j positions forward from this iterator. (If \a j is negative, the iterator goes backward.) @@ -959,6 +960,7 @@ /*! \fn template QSet::iterator QSet::iterator::operator-(int j) const \fn template QSet::const_iterator QSet::const_iterator::operator-(int j) const + \obsolete Returns an iterator to the item at \a j positions backward from this iterator. (If \a j is negative, the iterator goes forward.) @@ -971,6 +973,7 @@ /*! \fn template QSet::iterator &QSet::iterator::operator+=(int j) \fn template QSet::const_iterator &QSet::const_iterator::operator+=(int j) + \obsolete Advances the iterator by \a j items. (If \a j is negative, the iterator goes backward.) @@ -983,6 +986,7 @@ /*! \fn template QSet::iterator &QSet::iterator::operator-=(int j) \fn template QSet::const_iterator &QSet::const_iterator::operator-=(int j) + \obsolete Makes the iterator go back by \a j items. (If \a j is negative, the iterator goes forward.) -- cgit v1.2.3 From f67481094fa7f16dec281f9a96dd0de3dc0c453f Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Wed, 9 Oct 2019 23:35:06 +0200 Subject: QObject: use delegate constructors Avoid the massive code duplication in the two QObject constructors. The only slight difference is the code path checking for isWidget; I'd say that paying for that one is worth the price of de-duplicating. Change-Id: I3af749738fe7d6b7adf287009d1815396a2f1407 Reviewed-by: Lars Knoll --- src/corelib/kernel/qobject.cpp | 24 +----------------------- 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index fb0d54c801..bb1b48b0a6 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -906,30 +906,8 @@ static bool check_parent_thread(QObject *parent, */ QObject::QObject(QObject *parent) - : d_ptr(new QObjectPrivate) + : QObject(*new QObjectPrivate, parent) { - Q_ASSERT_X(this != parent, Q_FUNC_INFO, "Cannot parent a QObject to itself"); - - Q_D(QObject); - d_ptr->q_ptr = this; - d->threadData = (parent && !parent->thread()) ? parent->d_func()->threadData : QThreadData::current(); - d->threadData->ref(); - if (parent) { - QT_TRY { - if (!check_parent_thread(parent, parent ? parent->d_func()->threadData : 0, d->threadData)) - parent = 0; - setParent(parent); - } QT_CATCH(...) { - d->threadData->deref(); - QT_RETHROW; - } - } -#if QT_VERSION < 0x60000 - qt_addObject(this); -#endif - if (Q_UNLIKELY(qtHookData[QHooks::AddQObject])) - reinterpret_cast(qtHookData[QHooks::AddQObject])(this); - Q_TRACE(QObject_ctor, this); } /*! -- cgit v1.2.3 From d79726e9ebf65d1722f73d56cbc2ac68c7489a45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Mon, 7 Oct 2019 15:48:30 +0200 Subject: Win: QUdpSocket: Use QVarLengthArray for retrieving size Increased size of the peek buffer to 2048 from 1500 and now uses QVarLengthArray with space for 10 stack-allocated WSABUF instances, but still growing at the pace of 5. In benchmarking (created for and included in this patch) this shows better performance when retrieving the datagram size for larger datagrams, and the same performance as before for smaller datagrams (at the cost of 2048 - 1500 + 16 * 10 = 708 bytes extra stack space). Benchmarks: With changes: ********* Start testing of tst_QUdpSocket ********* Config: Using QtTest library 5.13.1, Qt 5.13.1 (x86_64-little_endian-llp64 shared (dynamic) release build; by MSVC 2019) PASS : tst_QUdpSocket::initTestCase() PASS : tst_QUdpSocket::pendingDatagramSize(52) RESULT : tst_QUdpSocket::pendingDatagramSize():"52": 0.0038 msecs per iteration (total: 63, iterations: 16384) PASS : tst_QUdpSocket::pendingDatagramSize(1024) RESULT : tst_QUdpSocket::pendingDatagramSize():"1024": 0.0039 msecs per iteration (total: 64, iterations: 16384) PASS : tst_QUdpSocket::pendingDatagramSize(2049) RESULT : tst_QUdpSocket::pendingDatagramSize():"2049": 0.0038 msecs per iteration (total: 63, iterations: 16384) PASS : tst_QUdpSocket::pendingDatagramSize(4500) RESULT : tst_QUdpSocket::pendingDatagramSize():"4500": 0.0039 msecs per iteration (total: 64, iterations: 16384) PASS : tst_QUdpSocket::pendingDatagramSize(4098) RESULT : tst_QUdpSocket::pendingDatagramSize():"4098": 0.0040 msecs per iteration (total: 66, iterations: 16384) PASS : tst_QUdpSocket::pendingDatagramSize(8192) RESULT : tst_QUdpSocket::pendingDatagramSize():"8192": 0.0040 msecs per iteration (total: 67, iterations: 16384) PASS : tst_QUdpSocket::pendingDatagramSize(12000) RESULT : tst_QUdpSocket::pendingDatagramSize():"12000": 0.010 msecs per iteration (total: 90, iterations: 8192) PASS : tst_QUdpSocket::pendingDatagramSize(25000) RESULT : tst_QUdpSocket::pendingDatagramSize():"25000": 0.021 msecs per iteration (total: 88, iterations: 4096) PASS : tst_QUdpSocket::pendingDatagramSize(32768) RESULT : tst_QUdpSocket::pendingDatagramSize():"32768": 0.033 msecs per iteration (total: 69, iterations: 2048) PASS : tst_QUdpSocket::pendingDatagramSize(64512) RESULT : tst_QUdpSocket::pendingDatagramSize():"64512": 0.088 msecs per iteration (total: 91, iterations: 1024) PASS : tst_QUdpSocket::cleanupTestCase() Totals: 12 passed, 0 failed, 0 skipped, 0 blacklisted, 3090ms ********* Finished testing of tst_QUdpSocket ********* Without changes: ********* Start testing of tst_QUdpSocket ********* Config: Using QtTest library 5.13.1, Qt 5.13.1 (x86_64-little_endian-llp64 shared (dynamic) release build; by MSVC 2019) PASS : tst_QUdpSocket::initTestCase() PASS : tst_QUdpSocket::pendingDatagramSize(52) RESULT : tst_QUdpSocket::pendingDatagramSize():"52": 0.0039 msecs per iteration (total: 65, iterations: 16384) PASS : tst_QUdpSocket::pendingDatagramSize(1024) RESULT : tst_QUdpSocket::pendingDatagramSize():"1024": 0.0039 msecs per iteration (total: 65, iterations: 16384) PASS : tst_QUdpSocket::pendingDatagramSize(2049) RESULT : tst_QUdpSocket::pendingDatagramSize():"2049": 0.0040 msecs per iteration (total: 66, iterations: 16384) PASS : tst_QUdpSocket::pendingDatagramSize(4500) RESULT : tst_QUdpSocket::pendingDatagramSize():"4500": 0.0040 msecs per iteration (total: 67, iterations: 16384) PASS : tst_QUdpSocket::pendingDatagramSize(4098) RESULT : tst_QUdpSocket::pendingDatagramSize():"4098": 0.0040 msecs per iteration (total: 67, iterations: 16384) PASS : tst_QUdpSocket::pendingDatagramSize(8192) RESULT : tst_QUdpSocket::pendingDatagramSize():"8192": 0.010 msecs per iteration (total: 90, iterations: 8192) PASS : tst_QUdpSocket::pendingDatagramSize(12000) RESULT : tst_QUdpSocket::pendingDatagramSize():"12000": 0.010 msecs per iteration (total: 90, iterations: 8192) PASS : tst_QUdpSocket::pendingDatagramSize(25000) RESULT : tst_QUdpSocket::pendingDatagramSize():"25000": 0.033 msecs per iteration (total: 69, iterations: 2048) PASS : tst_QUdpSocket::pendingDatagramSize(32768) RESULT : tst_QUdpSocket::pendingDatagramSize():"32768": 0.0502 msecs per iteration (total: 103, iterations: 2048) PASS : tst_QUdpSocket::pendingDatagramSize(64512) RESULT : tst_QUdpSocket::pendingDatagramSize():"64512": 0.13 msecs per iteration (total: 70, iterations: 512) PASS : tst_QUdpSocket::cleanupTestCase() Totals: 12 passed, 0 failed, 0 skipped, 0 blacklisted, 3192ms ********* Finished testing of tst_QUdpSocket ********* Fixes: QTBUG-78275 Change-Id: If86a226620244aa4e470600c6c1db4a7863b5617 Reviewed-by: Timur Pocheptsov Reviewed-by: Thiago Macieira --- src/network/socket/qnativesocketengine_win.cpp | 15 ++-- .../network/socket/qudpsocket/qudpsocket.pro | 8 +++ .../network/socket/qudpsocket/tst_qudpsocket.cpp | 80 ++++++++++++++++++++++ tests/benchmarks/network/socket/socket.pro | 3 +- 4 files changed, 99 insertions(+), 7 deletions(-) create mode 100644 tests/benchmarks/network/socket/qudpsocket/qudpsocket.pro create mode 100644 tests/benchmarks/network/socket/qudpsocket/tst_qudpsocket.cpp diff --git a/src/network/socket/qnativesocketengine_win.cpp b/src/network/socket/qnativesocketengine_win.cpp index 9edabd7822..dd115c33dc 100644 --- a/src/network/socket/qnativesocketengine_win.cpp +++ b/src/network/socket/qnativesocketengine_win.cpp @@ -53,6 +53,8 @@ #include #include +#include + //#define QNATIVESOCKETENGINE_DEBUG #if defined(QNATIVESOCKETENGINE_DEBUG) # include @@ -1141,13 +1143,14 @@ qint64 QNativeSocketEnginePrivate::nativePendingDatagramSize() const qint64 ret = -1; int recvResult = 0; DWORD flags; - // We start at 1500 bytes (the MTU for Ethernet V2), which should catch - // almost all uses (effective MTU for UDP under IPv4 is 1468), except - // for localhost datagrams and those reassembled by the IP layer. - char udpMessagePeekBuffer[1500]; - std::vector buf; + // We increase the amount we peek by 2048 * 5 on each iteration + // Grabs most cases fast and early. + char udpMessagePeekBuffer[2048]; + const int increments = 5; + QVarLengthArray buf; for (;;) { - buf.resize(buf.size() + 5, {sizeof(udpMessagePeekBuffer), udpMessagePeekBuffer}); + buf.reserve(buf.size() + increments); + std::fill_n(std::back_inserter(buf), increments, WSABUF{sizeof(udpMessagePeekBuffer), udpMessagePeekBuffer}); flags = MSG_PEEK; DWORD bytesRead = 0; diff --git a/tests/benchmarks/network/socket/qudpsocket/qudpsocket.pro b/tests/benchmarks/network/socket/qudpsocket/qudpsocket.pro new file mode 100644 index 0000000000..8df5340e2e --- /dev/null +++ b/tests/benchmarks/network/socket/qudpsocket/qudpsocket.pro @@ -0,0 +1,8 @@ +TEMPLATE = app +TARGET = tst_bench_qudpsocket + +QT = network testlib + +CONFIG += release + +SOURCES += tst_qudpsocket.cpp diff --git a/tests/benchmarks/network/socket/qudpsocket/tst_qudpsocket.cpp b/tests/benchmarks/network/socket/qudpsocket/tst_qudpsocket.cpp new file mode 100644 index 0000000000..e6dbbf9dfa --- /dev/null +++ b/tests/benchmarks/network/socket/qudpsocket/tst_qudpsocket.cpp @@ -0,0 +1,80 @@ +/**************************************************************************** +** +** Copyright (C) 2019 The Qt Company Ltd. +** Contact: https://www.qt.io/licensing/ +** +** This file is part of the test suite of the Qt Toolkit. +** +** $QT_BEGIN_LICENSE:GPL-EXCEPT$ +** Commercial License Usage +** Licensees holding valid commercial Qt licenses may use this file in +** accordance with the commercial license agreement provided with the +** Software or, alternatively, in accordance with the terms contained in +** a written agreement between you and The Qt Company. For licensing terms +** and conditions see https://www.qt.io/terms-conditions. For further +** information use the contact form at https://www.qt.io/contact-us. +** +** GNU General Public License Usage +** Alternatively, this file may be used under the terms of the GNU +** General Public License version 3 as published by the Free Software +** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT +** included in the packaging of this file. Please review the following +** information to ensure the GNU General Public License requirements will +** be met: https://www.gnu.org/licenses/gpl-3.0.html. +** +** $QT_END_LICENSE$ +** +****************************************************************************/ + +#include +#include +#include +#include +#include + +class tst_QUdpSocket : public QObject +{ + Q_OBJECT +public: + tst_QUdpSocket(); + +private slots: + void pendingDatagramSize_data(); + void pendingDatagramSize(); +}; + +tst_QUdpSocket::tst_QUdpSocket() +{ +} + +void tst_QUdpSocket::pendingDatagramSize_data() +{ + QTest::addColumn("size"); + for (int value : {52, 1024, 2049, 4500, 4098, 8192, 12000, 25000, 32 * 1024, 63 * 1024}) + QTest::addRow("%d", value) << value; +} + +void tst_QUdpSocket::pendingDatagramSize() +{ + QFETCH(int, size); + QUdpSocket socket; + socket.bind(); + + QNetworkDatagram datagram; + datagram.setData(QByteArray(size, 'a')); + datagram.setDestination(QHostAddress::SpecialAddress::LocalHost, socket.localPort()); + + auto sent = socket.writeDatagram(datagram); + QCOMPARE(sent, size); + + auto res = QTest::qWaitFor([&socket]() { return socket.hasPendingDatagrams(); }, 5000); + QVERIFY(res); + + QBENCHMARK { + auto pendingSize = socket.pendingDatagramSize(); + Q_UNUSED(pendingSize); + } +} + +QTEST_MAIN(tst_QUdpSocket) +#include "tst_qudpsocket.moc" diff --git a/tests/benchmarks/network/socket/socket.pro b/tests/benchmarks/network/socket/socket.pro index 2d676a2c6e..d428a4d973 100644 --- a/tests/benchmarks/network/socket/socket.pro +++ b/tests/benchmarks/network/socket/socket.pro @@ -1,3 +1,4 @@ TEMPLATE = subdirs SUBDIRS = \ - qtcpserver + qtcpserver \ + qudpsocket -- cgit v1.2.3 From 7fbadeddc14304d595aeaaaa6d398dcb21d86404 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Thu, 10 Oct 2019 10:09:38 -0700 Subject: Fix infinite recursion caused by the RDSEED feature We made qRandomCpu() call qCpuHasFeature(), which needs to test that RDRND works and does so by calling qRandomCpu(). So disentangle that by making sure we don't recurse. And by making sure that we fill the buffer with RDRND data, not with RDSEED. Fixes: QTBUG-79162 Change-Id: Ib5d667bf77a740c28d2efffd15cc583a9afcb97a Reviewed-by: Thiago Macieira --- src/corelib/tools/qsimd.cpp | 103 +++++++++++++++++++++++++++----------------- 1 file changed, 63 insertions(+), 40 deletions(-) diff --git a/src/corelib/tools/qsimd.cpp b/src/corelib/tools/qsimd.cpp index 6e3b0f9faf..d7c1d8c4a9 100644 --- a/src/corelib/tools/qsimd.cpp +++ b/src/corelib/tools/qsimd.cpp @@ -1,7 +1,7 @@ /**************************************************************************** ** ** Copyright (C) 2016 The Qt Company Ltd. -** Copyright (C) 2018 Intel Corporation. +** Copyright (C) 2019 Intel Corporation. ** Contact: https://www.qt.io/licensing/ ** ** This file is part of the QtCore module of the Qt Toolkit. @@ -188,6 +188,8 @@ static inline quint64 detectProcessorFeatures() # define PICreg "%%rbx" #endif +static bool checkRdrndWorks() noexcept; + static int maxBasicCpuidSupported() { #if defined(Q_CC_EMSCRIPTEN) @@ -376,37 +378,8 @@ static quint64 detectProcessorFeatures() features &= ~AllAVX512; } -#if defined(Q_PROCESSOR_X86) && QT_COMPILER_SUPPORTS_HERE(RDRND) - /** - * Some AMD CPUs (e.g. AMD A4-6250J and AMD Ryzen 3000-series) have a - * failing random generation instruction, which always returns - * 0xffffffff, even when generation was "successful". - * - * This code checks if hardware random generator generates four consecutive - * equal numbers. If it does, then we probably have a failing one and - * should disable it completely. - * - * https://bugreports.qt.io/browse/QTBUG-69423 - */ - if (features & CpuFeatureRDRND) { - const qsizetype testBufferSize = 4; - unsigned testBuffer[4] = {}; - - const qsizetype generated = qRandomCpu(testBuffer, testBufferSize); - - if (Q_UNLIKELY(generated == testBufferSize && - testBuffer[0] == testBuffer[1] && - testBuffer[1] == testBuffer[2] && - testBuffer[2] == testBuffer[3])) { - - fprintf(stderr, "WARNING: CPU random generator seem to be failing, disable hardware random number generation\n"); - fprintf(stderr, "WARNING: RDRND generated: 0x%x 0x%x 0x%x 0x%x\n", - testBuffer[0], testBuffer[1], testBuffer[2], testBuffer[3]); - - features &= ~CpuFeatureRDRND; - } - } -#endif + if (features & CpuFeatureRDRND && !checkRdrndWorks()) + features &= ~(CpuFeatureRDRND | CpuFeatureRDSEED); return features; } @@ -662,15 +635,9 @@ static unsigned *qt_random_rdseed(unsigned *ptr, unsigned *) } # endif -QT_FUNCTION_TARGET(RDRND) qsizetype qRandomCpu(void *buffer, qsizetype count) noexcept +static QT_FUNCTION_TARGET(RDRND) unsigned *qt_random_rdrnd(unsigned *ptr, unsigned *end) noexcept { - unsigned *ptr = reinterpret_cast(buffer); - unsigned *end = ptr + count; int retries = 10; - - if (qCpuHasFeature(RDSEED)) - ptr = qt_random_rdseed(ptr, end); - while (ptr + sizeof(qregisteruint)/sizeof(*ptr) <= end) { if (_rdrandXX_step(reinterpret_cast(ptr))) ptr += sizeof(qregisteruint)/sizeof(*ptr); @@ -688,8 +655,64 @@ QT_FUNCTION_TARGET(RDRND) qsizetype qRandomCpu(void *buffer, qsizetype count) no } out: + return ptr; +} + +static QT_FUNCTION_TARGET(RDRND) Q_DECL_COLD_FUNCTION bool checkRdrndWorks() noexcept +{ + /* + * Some AMD CPUs (e.g. AMD A4-6250J and AMD Ryzen 3000-series) have a + * failing random generation instruction, which always returns + * 0xffffffff, even when generation was "successful". + * + * This code checks if hardware random generator generates four consecutive + * equal numbers. If it does, then we probably have a failing one and + * should disable it completely. + * + * https://bugreports.qt.io/browse/QTBUG-69423 + */ + constexpr qsizetype TestBufferSize = 4; + unsigned testBuffer[TestBufferSize] = {}; + + unsigned *end = qt_random_rdrnd(testBuffer, testBuffer + TestBufferSize); + if (end < testBuffer + 3) { + // Random generation didn't produce enough data for us to make a + // determination whether it's working or not. Assume it isn't, but + // don't print a warning. + return false; + } + + // Check the results for equality + if (testBuffer[0] == testBuffer[1] + && testBuffer[0] == testBuffer[2] + && end == testBuffer + TestBufferSize && testBuffer[0] == testBuffer[3]) { + fprintf(stderr, "WARNING: CPU random generator seem to be failing, " + "disabling hardware random number generation\n" + "WARNING: RDRND generated:"); + for (unsigned *ptr = testBuffer; ptr < end; ++ptr) + fprintf(stderr, " 0x%x", *ptr); + fprintf(stderr, "\n"); + return false; + } + + // We're good + return true; +} + +QT_FUNCTION_TARGET(RDRND) qsizetype qRandomCpu(void *buffer, qsizetype count) noexcept +{ + unsigned *ptr = reinterpret_cast(buffer); + unsigned *end = ptr + count; + + if (qCpuHasFeature(RDSEED)) + ptr = qt_random_rdseed(ptr, end); + + // fill the buffer with RDRND if RDSEED didn't + ptr = qt_random_rdrnd(ptr, end); return ptr - reinterpret_cast(buffer); } -#endif +#elif defined(Q_PROCESSOR_X86) && !defined(Q_OS_NACL) && !defined(Q_PROCESSOR_ARM) +static bool checkRdrndWorks() noexcept { return false; } +#endif // Q_PROCESSOR_X86 && RDRND QT_END_NAMESPACE -- cgit v1.2.3 From 58c4fa1061d07d35805bbb5cc8af7b36129d4509 Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Thu, 10 Oct 2019 16:07:55 +0200 Subject: JPEG image handler: drop a use of "volatile" The reading code protects a local variable with volatile. In this case the only possible reason to apply volatile seems to be protecting the variable across the subsequent setjmp/longjmp. However, the variable is never accessed after the longjmp (the function returns). So, drop the volatile. Change-Id: Ibecb11a9edcc6027b2fd52b555287ad53375a5d0 Reviewed-by: Allan Sandfeld Jensen Reviewed-by: Edward Welbourne --- src/plugins/imageformats/jpeg/qjpeghandler.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/plugins/imageformats/jpeg/qjpeghandler.cpp b/src/plugins/imageformats/jpeg/qjpeghandler.cpp index 1f1675e490..a09e6dfd5a 100644 --- a/src/plugins/imageformats/jpeg/qjpeghandler.cpp +++ b/src/plugins/imageformats/jpeg/qjpeghandler.cpp @@ -248,13 +248,12 @@ static bool ensureValidImage(QImage *dest, struct jpeg_decompress_struct *info, static bool read_jpeg_image(QImage *outImage, QSize scaledSize, QRect scaledClipRect, - QRect clipRect, volatile int inQuality, + QRect clipRect, int quality, Rgb888ToRgb32Converter converter, j_decompress_ptr info, struct my_error_mgr* err ) { if (!setjmp(err->setjmp_buffer)) { // -1 means default quality. - int quality = inQuality; if (quality < 0) quality = 75; -- cgit v1.2.3 From 8aa3329a7186b087163f68ef880c8ffdd47d01bf Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Thu, 10 Oct 2019 12:20:05 +0200 Subject: JPEG image handler: remove undefined behavior from setjmp/longjmp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The JPEG writing code features a setjmp/longjmp pair to deal with error handling. In doing so, it creates UB by touching local objects after the setjmp and then after the corresponding longjmp. The rules on what we can do are quite strict: objects that are 1) local to the function calling setjmp; 2) not qualified with volatile; 3) written into after the setjmp; have indeterminate state after the corresponding longjmp call (man 3 longjmp, C 2x draft N2346 §7.13.2.1.2). Not making any assumptions on any compiler used: let's just say that using them in any way is UB. Luckily, no compiler exploits this (yet), and the code works just fine. But we know the drill -- never play this game against compilers, because you will lose. So: we have a couple of those objects around in the writing routine (cinfo, row_pointer), that violate the rules above. Unfortunately we can't simply mark them as volatile: libjpeg's API expects them not to be volatile. Casting volatileness away and then touching an object in any way is undefined behavior out of the bat (C 2x draft N2346 §6.7.3.7, C++ [dcl.type.cv]). Given the code needs to do 3), and we can't work around 2), then work around 1): define them to be non-local to the function doing the setjmp. Introduce a small helper that declares such objects and then calls the function doing the actual work, with the setjmp/longjmp. An overall alternative would be of course stop using setjmp/longjmp, but libjpeg's API doesn't really seem to allow this -- when the library calls user's error handler, that error handler is expected not to return to the library (so a longjmp or an exit/abort are mandatory). Side note: all the code using libjpeg I've researched to debug this has this very same strange issue: * GDK-pixbuf's [1] * ImageMagick's [2] * and even libjpeg's [3] and libjpeg-turbo's [4] own examples. [1] https://github.com/GNOME/gdk-pixbuf/blob/master/gdk-pixbuf/io-jpeg.c#L581 [2] https://github.com/ImageMagick/ImageMagick/blob/master/coders/jpeg.c#L2338 [3] https://www.ijg.org/ [4] https://github.com/libjpeg-turbo/libjpeg-turbo/blob/master/example.txt#L331 Change-Id: I34a810db468f73423478cd3ac71b888f4b11cb28 Reviewed-by: Allan Sandfeld Jensen Reviewed-by: Edward Welbourne --- src/plugins/imageformats/jpeg/qjpeghandler.cpp | 34 ++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/src/plugins/imageformats/jpeg/qjpeghandler.cpp b/src/plugins/imageformats/jpeg/qjpeghandler.cpp index a09e6dfd5a..dd01138722 100644 --- a/src/plugins/imageformats/jpeg/qjpeghandler.cpp +++ b/src/plugins/imageformats/jpeg/qjpeghandler.cpp @@ -528,7 +528,14 @@ static inline void write_icc_profile(const QImage &image, j_compress_ptr cinfo) } } -static bool write_jpeg_image(const QImage &image, QIODevice *device, volatile int sourceQuality, const QString &description, bool optimize, bool progressive) +static bool do_write_jpeg_image(struct jpeg_compress_struct &cinfo, + JSAMPROW *row_pointer, + const QImage &image, + QIODevice *device, + int sourceQuality, + const QString &description, + bool optimize, + bool progressive) { bool success = false; const QVector cmap = image.colorTable(); @@ -536,10 +543,6 @@ static bool write_jpeg_image(const QImage &image, QIODevice *device, volatile in if (image.format() == QImage::Format_Invalid || image.format() == QImage::Format_Alpha8) return false; - struct jpeg_compress_struct cinfo; - JSAMPROW row_pointer[1]; - row_pointer[0] = 0; - struct my_jpeg_destination_mgr *iod_dest = new my_jpeg_destination_mgr(device); struct my_error_mgr jerr; @@ -712,6 +715,27 @@ static bool write_jpeg_image(const QImage &image, QIODevice *device, volatile in } delete iod_dest; + return success; +} + +static bool write_jpeg_image(const QImage &image, + QIODevice *device, + int sourceQuality, + const QString &description, + bool optimize, + bool progressive) +{ + // protect these objects from the setjmp/longjmp pair inside + // do_write_jpeg_image (by making them non-local). + struct jpeg_compress_struct cinfo; + JSAMPROW row_pointer[1]; + row_pointer[0] = 0; + + const bool success = do_write_jpeg_image(cinfo, row_pointer, + image, device, + sourceQuality, description, + optimize, progressive); + delete [] row_pointer[0]; return success; } -- cgit v1.2.3