D9973: ktooltipwidget: Fix tooltip positioning

[Elvis Angelaccio]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20180304/8ec4479a/attachment.htm>