Skip to content

Commit

Permalink
QFormLayout: Fix assert when showing with hidden rows
Browse files Browse the repository at this point in the history 
Amends a74cdf7, after which the
initialization of items in invisible rows is skipped. Since data members
in QFormLayoutItem were lazily initialized, this resulted in out-of-bounds
access of QList entries.

Use member initialization for all QFormLayoutItem fields, and check that
vLayoutIndex is valid before using it to access the list entry. Skip
labels and fields for which it is not initialized.

Add test case. As a drive-by, silence the test's provoked warning
messages via ignoreMessage.

Change-Id: I374b414a51df20b9af3087a2676061fc6b7f23e2
Reviewed-by: Fabian Kosmale <[email protected]>
Reviewed-by: Thiago Macieira <[email protected]>
  • Loading branch information
@vohi
vohi committed Mar 15, 2022
1 parent 7d00e72 commit 1822550
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 14 deletions.
28 changes: 14 additions & 14 deletions src/widgets/kernel/qformlayout.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ enum { ColumnCount = 2 };
// This owns the QLayoutItem
struct QFormLayoutItem
{
QFormLayoutItem(QLayoutItem* i) : item(i), fullRow(false), isHfw(false) { }
QFormLayoutItem(QLayoutItem* i) : item(i) { }
~QFormLayoutItem() { delete item; }

// Wrappers
Expand All @@ -133,27 +133,27 @@ struct QFormLayoutItem
// For use with FixedColumnMatrix
bool operator==(const QFormLayoutItem& other) { return item == other.item; }

QLayoutItem *item;
bool fullRow;
QLayoutItem *item = nullptr;
bool fullRow = false;
bool isVisible = true;

// set by updateSizes
bool isHfw;
bool isHfw = false;
QSize minSize;
QSize sizeHint;
QSize maxSize;

// also set by updateSizes
int sbsHSpace; // only used for side by side, for the field item only (not label)
int vSpace; // This is the spacing to the item in the row above
int sbsHSpace = -1; // only used for side by side, for the field item only (not label)
int vSpace = -1; // This is the spacing to the item in the row above

// set by setupVerticalLayoutData
bool sideBySide;
int vLayoutIndex;
bool sideBySide = false;
int vLayoutIndex = -1;

// set by setupHorizontalLayoutData
int layoutPos;
int layoutWidth;
int layoutPos = -1;
int layoutWidth = -1;
};

static void hideOrShowWidgetsInLayout(QLayout *layout, bool on)
Expand Down Expand Up @@ -540,7 +540,7 @@ void QFormLayoutPrivate::setupHfwLayoutData()
QFormLayoutItem *label = m_matrix(i, 0);
QFormLayoutItem *field = m_matrix(i, 1);

if (label) {
if (label && label->vLayoutIndex > -1) {
if (label->isHfw) {
// We don't check sideBySide here, since a label is only
// ever side by side with its field
Expand All @@ -555,7 +555,7 @@ void QFormLayoutPrivate::setupHfwLayoutData()
}
}

if (field) {
if (field && field->vLayoutIndex > -1) {
int hfw = field->isHfw ? field->heightForWidth(field->layoutWidth) : 0;
int h = field->isHfw ? hfw : field->sizeHint.height();
int mh = field->isHfw ? hfw : field->minSize.height();
Expand Down Expand Up @@ -2226,7 +2226,7 @@ void QFormLayoutPrivate::arrangeWidgets(const QList<QLayoutStruct> &layouts, QRe
QFormLayoutItem *label = m_matrix(i, 0);
QFormLayoutItem *field = m_matrix(i, 1);

if (label) {
if (label && label->vLayoutIndex > -1) {
int height = layouts.at(label->vLayoutIndex).size;
if ((label->expandingDirections() & Qt::Vertical) == 0) {
/*
Expand All @@ -2253,7 +2253,7 @@ void QFormLayoutPrivate::arrangeWidgets(const QList<QLayoutStruct> &layouts, QRe
label->setGeometry(QStyle::visualRect(layoutDirection, rect, QRect(p, sz)));
}

if (field) {
if (field && field->vLayoutIndex > -1) {
QSize sz(field->layoutWidth, layouts.at(field->vLayoutIndex).size);
QPoint p(field->layoutPos + leftOffset + rect.x(), layouts.at(field->vLayoutIndex).pos);
/*
Expand Down
23 changes: 23 additions & 0 deletions tests/auto/widgets/kernel/qformlayout/tst_qformlayout.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ private slots:
void setWidget();
void setLayout();
void hideShowRow();
void showWithHiddenRow();

/*
QLayoutItem *itemAt(int row, ItemRole role) const;
Expand Down Expand Up @@ -817,6 +818,7 @@ void tst_QFormLayout::removeRow_QWidget()
QCOMPARE(layout->rowCount(), 0);

QWidget *w3 = new QWidget;
QTest::ignoreMessage(QtWarningMsg, "QFormLayout::takeRow: Invalid widget");
layout->removeRow(w3);
delete w3;
}
Expand Down Expand Up @@ -857,6 +859,7 @@ void tst_QFormLayout::removeRow_QLayout()
QCOMPARE(layout->rowCount(), 0);

QHBoxLayout *l3 = new QHBoxLayout;
QTest::ignoreMessage(QtWarningMsg, "QFormLayout::takeRow: Invalid layout");
layout->removeRow(l3);
delete l3;
}
Expand Down Expand Up @@ -896,6 +899,7 @@ void tst_QFormLayout::takeRow()
QCOMPARE(layout->rowCount(), 0);
QCOMPARE(result.fieldItem->widget(), w1.data());

QTest::ignoreMessage(QtWarningMsg, "QFormLayout::takeRow: Invalid row 0");
result = layout->takeRow(0);

QVERIFY(!result.fieldItem);
Expand Down Expand Up @@ -936,6 +940,7 @@ void tst_QFormLayout::takeRow_QWidget()
QCOMPARE(layout->rowCount(), 0);

QWidget *w3 = new QWidget;
QTest::ignoreMessage(QtWarningMsg, "QFormLayout::takeRow: Invalid widget");
result = layout->takeRow(w3);
delete w3;

Expand Down Expand Up @@ -983,6 +988,7 @@ void tst_QFormLayout::takeRow_QLayout()
QCOMPARE(layout->rowCount(), 0);

QHBoxLayout *l3 = new QHBoxLayout;
QTest::ignoreMessage(QtWarningMsg, "QFormLayout::takeRow: Invalid layout");
result = layout->takeRow(l3);
delete l3;

Expand Down Expand Up @@ -1012,7 +1018,9 @@ void tst_QFormLayout::setWidget()
QCOMPARE(layout.rowCount(), 6);

// should be ignored and generate warnings
QTest::ignoreMessage(QtWarningMsg, "QFormLayoutPrivate::setItem: Cell (3, 1) already occupied");
layout.setWidget(3, QFormLayout::FieldRole, &w4);
QTest::ignoreMessage(QtWarningMsg, "QFormLayoutPrivate::setItem: Invalid cell (-1, 1)");
layout.setWidget(-1, QFormLayout::FieldRole, &w4);

{
Expand Down Expand Up @@ -1080,7 +1088,9 @@ void tst_QFormLayout::setLayout()
QCOMPARE(layout.rowCount(), 6);

// should be ignored and generate warnings
QTest::ignoreMessage(QtWarningMsg, "QFormLayoutPrivate::setItem: Cell (3, 1) already occupied");
layout.setLayout(3, QFormLayout::FieldRole, &l4);
QTest::ignoreMessage(QtWarningMsg, "QLayout::addChildLayout: layout \"\" already has a parent");
layout.setLayout(-1, QFormLayout::FieldRole, &l4);
QCOMPARE(layout.count(), 3);
QCOMPARE(layout.rowCount(), 6);
Expand Down Expand Up @@ -1255,6 +1265,19 @@ void tst_QFormLayout::hideShowRow()
QVERIFY(rowInputWidget(row)->isHidden());
}

void tst_QFormLayout::showWithHiddenRow()
{
QWidget topLevel;
QFormLayout layout;

for (int row = 0; row < 3; ++row)
layout.addRow(QString("Label %1").arg(row), new QLineEdit);
layout.setRowVisible(1, false);

topLevel.setLayout(&layout);
topLevel.show();
}

void tst_QFormLayout::itemAt()
{
QWidget topLevel;
Expand Down

0 comments on commit 1822550

Please sign in to comment.