From 2ec56d158dc140f68efb45e2e0613f0e4026ddf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan-Arve=20S=C3=A6ther?= Date: Tue, 23 Jun 2009 11:02:11 +0200 Subject: Fixed count(), itemAt() and removeAt() in QGraphicsLinearLayout. Due to this, the behaviour of count(), itemAt() and removeAt() was different between QGraphicsLinearLayout and QGraphicsGridLayout. QGraphicsGridLayout does it right. This adds some behaviour changes: 1. QGraphicsLinearLayout::count() is no longer affected by inserted stretches (through insertStretch) This means that code like this will break: QGraphicsLinearLayout *linearLayout = new QGraphicsLinearLayout; linearLayout->addItem(new QGraphicsWidget); linearLayout->addStretch(); int count = linearLayout->count(); linearLayout->removeAt(count - 1); // before this patch it would do nothing (and it wouldn't // actually remove the stretch), with the new patch it would // remove the QGraphicsWidget. 2. count(), itemAt() and removeAt() now prints a warning for an invalid index. The documentation actually says that "The reimplementation can assume that index is valid (i.e., it respects the value of count()", but I decided that its too risky to not assume that it is valid, since it would break the following common pattern: while(layout->itemAt(0)) { layout->removeAt(0); } Cleaned up autotests (and a small codeblock) that assumed itemAt/removeAt with an invalid index to work, since we should try to follow our own advice. :-) This change is also an alignment with what QGraphicsGridLayout does (it checks the index argument and prints a warning too.) --- src/gui/graphicsview/qgraphicslayout_p.cpp | 4 +- src/gui/graphicsview/qgraphicslinearlayout.cpp | 14 ++++-- .../tst_qgraphicsgridlayout.cpp | 3 ++ .../tst_qgraphicslinearlayout.cpp | 53 ++++++++++++---------- 4 files changed, 45 insertions(+), 29 deletions(-) diff --git a/src/gui/graphicsview/qgraphicslayout_p.cpp b/src/gui/graphicsview/qgraphicslayout_p.cpp index 6296207..83bf14b 100644 --- a/src/gui/graphicsview/qgraphicslayout_p.cpp +++ b/src/gui/graphicsview/qgraphicslayout_p.cpp @@ -120,8 +120,8 @@ static bool removeLayoutItemFromLayout(QGraphicsLayout *lay, QGraphicsLayoutItem if (!lay) return false; - QGraphicsLayoutItem *child; - for (int i = 0; (child = lay->itemAt(i)); ++i) { + for (int i = lay->count() - 1; i >= 0; --i) { + QGraphicsLayoutItem *child = lay->itemAt(i); if (child && child->isLayout()) { if (removeLayoutItemFromLayout(static_cast(child), layoutItem)) return true; diff --git a/src/gui/graphicsview/qgraphicslinearlayout.cpp b/src/gui/graphicsview/qgraphicslinearlayout.cpp index 2e78fc0..3b037cf 100644 --- a/src/gui/graphicsview/qgraphicslinearlayout.cpp +++ b/src/gui/graphicsview/qgraphicslinearlayout.cpp @@ -323,7 +323,11 @@ void QGraphicsLinearLayout::removeItem(QGraphicsLayoutItem *item) void QGraphicsLinearLayout::removeAt(int index) { Q_D(QGraphicsLinearLayout); - if (QGridLayoutItem *gridItem = d->engine.itemAt(d->gridRow(index), d->gridColumn(index))) { + if (index < 0 || index >= d->engine.itemCount()) { + qWarning("QGraphicsLinearLayout::removeAt: invalid index %d", index); + return; + } + if (QGridLayoutItem *gridItem = d->engine.itemAt(index)) { if (QGraphicsLayoutItem *layoutItem = gridItem->layoutItem()) layoutItem->setParentLayoutItem(0); d->removeGridItem(gridItem); @@ -463,7 +467,7 @@ QSizePolicy::ControlTypes QGraphicsLinearLayout::controlTypes(LayoutSide side) c int QGraphicsLinearLayout::count() const { Q_D(const QGraphicsLinearLayout); - return d->engine.rowCount(d->orientation); + return d->engine.itemCount(); } /*! @@ -472,8 +476,12 @@ int QGraphicsLinearLayout::count() const QGraphicsLayoutItem *QGraphicsLinearLayout::itemAt(int index) const { Q_D(const QGraphicsLinearLayout); + if (index < 0 || index >= d->engine.itemCount()) { + qWarning("QGraphicsLinearLayout::itemAt: invalid index %d", index); + return 0; + } QGraphicsLayoutItem *item = 0; - if (QGridLayoutItem *gridItem = d->engine.itemAt(d->gridRow(index), d->gridColumn(index))) + if (QGridLayoutItem *gridItem = d->engine.itemAt(index)) item = gridItem->layoutItem(); return item; } diff --git a/tests/auto/qgraphicsgridlayout/tst_qgraphicsgridlayout.cpp b/tests/auto/qgraphicsgridlayout/tst_qgraphicsgridlayout.cpp index ce4828a..2f70b62 100644 --- a/tests/auto/qgraphicsgridlayout/tst_qgraphicsgridlayout.cpp +++ b/tests/auto/qgraphicsgridlayout/tst_qgraphicsgridlayout.cpp @@ -153,8 +153,11 @@ void tst_QGraphicsGridLayout::qgraphicsgridlayout() layout.columnStretchFactor(0); layout.count(); layout.horizontalSpacing(); + QTest::ignoreMessage(QtWarningMsg, "QGraphicsGridLayout::itemAt: invalid row, column 0, 0"); layout.itemAt(0, 0); + QTest::ignoreMessage(QtWarningMsg, "QGraphicsGridLayout::itemAt: invalid index 0"); layout.itemAt(0); + QTest::ignoreMessage(QtWarningMsg, "QGraphicsGridLayout::removeAt: invalid index 0"); layout.removeAt(0); layout.rowAlignment(0); layout.rowCount(); diff --git a/tests/auto/qgraphicslinearlayout/tst_qgraphicslinearlayout.cpp b/tests/auto/qgraphicslinearlayout/tst_qgraphicslinearlayout.cpp index b61b2bb..c5ce1b9 100644 --- a/tests/auto/qgraphicslinearlayout/tst_qgraphicslinearlayout.cpp +++ b/tests/auto/qgraphicslinearlayout/tst_qgraphicslinearlayout.cpp @@ -210,17 +210,16 @@ void tst_QGraphicsLinearLayout::qgraphicslinearlayout() layout.setOrientation(Qt::Vertical); layout.orientation(); QTest::ignoreMessage(QtWarningMsg, "QGraphicsLinearLayout::insertItem: cannot insert null item"); - //QCOMPARE(layout.count(), 0); + QCOMPARE(layout.count(), 0); layout.addItem(0); - //QCOMPARE(layout.count(), 0); + QCOMPARE(layout.count(), 0); layout.addStretch(0); - //QCOMPARE(layout.count(), 1); + QCOMPARE(layout.count(), 0); QTest::ignoreMessage(QtWarningMsg, "QGraphicsLinearLayout::insertItem: cannot insert null item"); layout.insertItem(0, 0); layout.insertStretch(0, 0); layout.removeItem(0); - layout.removeAt(0); - //QCOMPARE(layout.count(), 1); + QCOMPARE(layout.count(), 0); layout.setSpacing(0); layout.spacing(); QTest::ignoreMessage(QtWarningMsg, "QGraphicsLinearLayout::setStretchFactor: cannot assign a stretch factor to a null item"); @@ -231,8 +230,7 @@ void tst_QGraphicsLinearLayout::qgraphicslinearlayout() QCOMPARE(layout.alignment(0), 0); layout.setGeometry(QRectF()); layout.geometry(); - //QCOMPARE(layout.count(), 1); - layout.itemAt(0); + QCOMPARE(layout.count(), 0); layout.invalidate(); layout.sizeHint(Qt::MinimumSize, QSizeF()); } @@ -537,7 +535,10 @@ void tst_QGraphicsLinearLayout::insertItem() QSizeF oldSizeHint = layout.sizeHint(Qt::PreferredSize, QSizeF()); layout.insertItem(insertItemAt, item); QCOMPARE(layout.count(), itemCount + layoutCount + 1); - QCOMPARE(layout.itemAt(insertItemAt), insertItemAt == -1 ? (QGraphicsLayoutItem*)0 : item); + + if (insertItemAt >= 0 && (itemCount + layoutCount >= 0)) { + QCOMPARE(layout.itemAt(itemCount + layoutCount), item); + } layout.activate(); QSizeF newSizeHint = layout.sizeHint(Qt::PreferredSize, QSizeF()); @@ -599,8 +600,7 @@ void tst_QGraphicsLinearLayout::insertStretch() } widget->setLayout(layout); layout->insertStretch(insertItemAt, stretch); - QCOMPARE(layout->itemAt(insertItemAt), (QGraphicsLayoutItem*)0); - QCOMPARE(layout->count(), itemCount + layoutCount + 1); + QCOMPARE(layout->count(), itemCount + layoutCount); layout->activate(); view.show(); @@ -669,7 +669,6 @@ void tst_QGraphicsLinearLayout::invalidate() void tst_QGraphicsLinearLayout::itemAt_data() { QTest::addColumn("index"); - QTest::newRow("-1") << -1; QTest::newRow("0") << 0; QTest::newRow("1") << 1; QTest::newRow("2") << 2; @@ -681,7 +680,10 @@ void tst_QGraphicsLinearLayout::itemAt() // see also the insertItem() etc tests QFETCH(int, index); SubQGraphicsLinearLayout layout; - QCOMPARE(layout.itemAt(index), (QGraphicsLayoutItem*)0); + for (int i = 0; i < 3; ++i) + layout.addItem(new QGraphicsWidget); + + QVERIFY(layout.itemAt(index) != 0); } void tst_QGraphicsLinearLayout::orientation_data() @@ -779,19 +781,18 @@ void tst_QGraphicsLinearLayout::removeAt() layout.addItem(new SubQGraphicsLinearLayout); QSizeF oldSizeHint = layout.sizeHint(Qt::PreferredSize, QSizeF()); - QGraphicsLayoutItem *w = layout.itemAt(removeItemAt); - QGraphicsLayoutItem *wParent = 0; + QGraphicsLayoutItem *w = 0; + if (removeItemAt >= 0 && removeItemAt < layout.count()) + w = layout.itemAt(removeItemAt); if (w) { - wParent = w->parentLayoutItem(); + QGraphicsLayoutItem *wParent = w->parentLayoutItem(); QCOMPARE(wParent, static_cast(&layout)); - } - layout.removeAt(removeItemAt); - if (w) { + layout.removeAt(removeItemAt); wParent = w->parentLayoutItem(); QCOMPARE(wParent, static_cast(0)); + delete w; } - delete w; - QCOMPARE(layout.count(), itemCount + layoutCount - ((removeItemAt == -1) ? 0 : 1)); + QCOMPARE(layout.count(), itemCount + layoutCount - (w ? 1 : 0)); layout.activate(); QSizeF newSizeHint = layout.sizeHint(Qt::PreferredSize, QSizeF()); @@ -829,11 +830,15 @@ void tst_QGraphicsLinearLayout::removeItem() for (int i = 0; i < layoutCount; ++i) layout.addItem(new SubQGraphicsLinearLayout); - QGraphicsLayoutItem *w = layout.itemAt(removeItemAt); + QGraphicsLayoutItem *w = 0; + if (removeItemAt >= 0 && removeItemAt < layout.count()) + w = layout.itemAt(removeItemAt); QSizeF oldSizeHint = layout.sizeHint(Qt::PreferredSize, QSizeF()); - layout.removeItem(w); - delete w; - QCOMPARE(layout.count(), itemCount + layoutCount - ((removeItemAt == -1) ? 0 : 1)); + if (w) { + layout.removeItem(w); + delete w; + } + QCOMPARE(layout.count(), itemCount + layoutCount - (w ? 1 : 0)); layout.activate(); QSizeF newSizeHint = layout.sizeHint(Qt::PreferredSize, QSizeF()); -- cgit v0.12