elvisangelaccio requested changes to this revision. elvisangelaccio added inline comments. This revision now requires changes to proceed.
INLINE COMMENTS > ktooltippositiontest.cpp:39-52 > +void KTooltipPositionTest::init() > +{ > + m_container = new QWidget(); > + m_target = new QLabel(QStringLiteral("dummy file")); > + QHBoxLayout* layout = new QHBoxLayout(m_container); > + layout->addWidget(m_target); > + m_margin = > m_container->style()->pixelMetric(QStyle::PM_ToolTipLabelFrameWidth); Imho these variables could be local to `shouldNotObscureTarget()`, so that we don't need to define `init()` and `cleanup()` and we don't need to make them class members. > ktooltippositiontest.cpp:65 > + positions.insert(QStringLiteral("centered") > + , QPoint(m_screenGeometry.width() / 2, m_screenGeometry.height() / > 2)); > + Weird comma position :p > ktooltippositiontest.cpp:71 > + //FIXME: Compose names w/o compiler warning -Wformat-security > + > QTest::addRow(QStringLiteral("small/%1").arg(i.key()).toLatin1().constData()) > + << i.value() Weird, I usually use `qPrintable()` for this, but QTest::addRow(qPrintable(QStringLiteral("small/%1").arg(i.key()))) still gives me the compiler warning. Not sure what is going on here. > ktooltippositiontest.cpp:85 > + > + if (m_screenGeometry.width() >= 1600 && m_screenGeometry.width() >= > 900) { > + QTest::addRow(QStringLiteral("super > large/%1").arg(i.key()).toLatin1().constData()) Did you mean if (m_screenGeometry.width() >= 1600 && m_screenGeometry.height() >= 900) { ... } ? > ktooltippositiontest.cpp:111 > + : m_screenGeometry.right() + position.x() - m_container->width() > + , position.y() >= 0 > + ? position.y() Weird comma position. Maybe two local QPoint variables could also improve the readability here. > ktooltippositiontest.cpp:122 > + > + QLabel* contentWidget(new QLabel(content)); > + contentWidget->setMaximumSize(m_screenGeometry.width() - 20, > m_screenGeometry.height() - 20); Unusual style, we usually do `QLabel* contentWidget = new QLabel(content);` Also, where is this label deleted? > ktooltippositiontest.cpp:125 > + > + QWindow* transientParent(m_container->windowHandle()); > + KToolTipWidget tooltip; Same as above about the style. But are we sure we want to //create// a new QWindow? Why not just pass `m_container->windowHandle()` to `showBelow()` ? Also, what if the transient parent is null? We should probably add a `QVERIFY(m_container->windowHandle())` > ktooltippositiontest.cpp:131 > + > + //TODO: Remove before landing > + qDebug() << "target: " << targetRect; We can actually keep (and maybe improve) this debug output. qDebug() will only print something if the test fails. > ktooltippositiontest.cpp:136 > + QVERIFY2(!tooltipRect.intersects(targetRect), "Target obscured"); > + QVERIFY2(tooltipRect.intersected(m_screenGeometry) == tooltipRect, > "Displayed offscreen"); > + This should be a QCOMPARE > ktooltippositiontest.cpp:139 > + // Check margins > + if (tooltipRect.bottom() < targetRect.top() ) { > + QCOMPARE(m_margin, targetRect.top() - tooltipRect.bottom()); remove space before ) > ktooltippositiontest.cpp:148 > + } else { > + QFAIL("Test is wrong"); > + } What do you mean with "test is wrong"? Can this branch ever happen? > ktooltippositiontest.cpp:151 > + > + tooltip.hide(); > +} `QVERIFY(tooltip.isHidden());` ? > ktooltippositiontest.h:38-39 > + > + void shouldNotObscureTarget_data(); > + void shouldNotObscureTarget(); > + Is there a reason why we are creating a new test binary only for this test function? Why not use `ktooltipwidgettest.cpp`? > ktooltipwidget.cpp:122 > // - the content is not drawn inside rect > - > - const QSize size = content->sizeHint(); > - const int margin = > q->style()->pixelMetric(QStyle::PM_ToolTipLabelFrameWidth); > + q->adjustSize(); > + const QSize size = q->sizeHint(); `centerBelow()` is `const`, but this actually changes the tooltip, right? Maybe we can move this call to `addWidget()` ? > ktooltipwidget.cpp:142 > + // Disallow negative x. Happens when rect is wider than screen. > + x = qMax(screenGeometry.left(), screenGeometry.right() - > size.width() + 1); > } This looks unrelated to this patch. I don't see a testcase for it and if I revert this change, the new tests still pass. Ideally we need a failing test case that is fixed by this line of code. Btw, don't we have a similar "negative y" problem? REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D9973 To: michaelh, elvisangelaccio, #frameworks, #dolphin, ngraham Cc: cfeck, michaelh