Compare float values in DecorationButton contains check
AbandonedPublic

Authored by romangg on Oct 2 2018, 8:06 PM.

Details

Reviewers
davidedmundson
zzag
Group Reviewers
KWin
Summary

In c9cfd840137b a position contains check was introduced in order to not count
a bottom or right edge position as being part of the DecorationButton like
QRectF::contains does.

This was done by converting the geometry to a QRect and using its contains
function, which does the comparision the desirable way for legacy reasons.

But this means we lose precision, since the decoration library in theory
supports floating values for positioning.

To not do this instead of using QRect's functionality and implicitly dropping
float precision therefore do the check for bottom and right edge explicitly.

Test Plan

Adapted autotest.

Diff Detail

Repository
R129 Window Decoration Library
Branch
decorationContainsFix
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8663
Build 8681: arc lint + arc unit
romangg created this revision.Oct 2 2018, 8:06 PM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 2 2018, 8:06 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
romangg requested review of this revision.Oct 2 2018, 8:06 PM
davidedmundson requested changes to this revision.Oct 2 2018, 9:46 PM
This comment was removed by davidedmundson.
This revision now requires changes to proceed.Oct 2 2018, 9:46 PM
zzag added a comment.EditedOct 2 2018, 10:31 PM

Also, fwiw, comparing floating point numbers with the plain < is not quite correct(e.g. 0.3 < 0.1 + 0.2 vs 0.1 + 0.2 < 0.3). Qt does the same [comparing with the plain <] so I guess that's fine.

src/decorationbutton.cpp
455

Can you please explain this part? Shouldn't it be d->geometry.right() < pos.x()?

romangg added inline comments.Oct 2 2018, 10:45 PM
src/decorationbutton.cpp
455

If rect.width() is smaller 0, then arbitrary QRect rect is to the left of its "anchor point" rect.x(). The right border is therefore defined by value rect.x().

Looking at the Qt source in this case rect.right() defines the left border of the rectangle. I assume we always want to exclude the border to the right. I also hope that no on in practice defines a decoration button with negative width. ;)

romangg requested review of this revision.Feb 19 2019, 1:40 PM
zzag added a comment.Feb 19 2019, 1:47 PM

Could you please rebase it on master?

zzag added a comment.Feb 19 2019, 1:48 PM

Sorry, I applied the change to the wrong repo

zzag added a comment.Feb 19 2019, 2:04 PM

Looks good to me.

I think the test is too much complicated. Would it be simpler to have something like

QTest::addColumn<QRectF>("geometry");
QTest::addColumn<QPointF>("pos");
QTest::addColumn<bool>("contains");

?

src/decorationbutton.cpp
455

No short names.

zzag requested changes to this revision.Feb 21 2019, 10:00 AM

See my comment above.

This revision now requires changes to proceed.Feb 21 2019, 10:00 AM
romangg updated this revision to Diff 52257.Feb 21 2019, 11:08 PM
romangg marked an inline comment as done.
  • Long variable names
romangg added a comment.EditedFeb 21 2019, 11:13 PM
In D15907#415347, @zzag wrote:

Looks good to me.

I think the test is too much complicated. Would it be simpler to have something like

QTest::addColumn<QRectF>("geometry");
QTest::addColumn<QPointF>("pos");
QTest::addColumn<bool>("contains");

?

Don't know. I've just checked again and the current autotest fails without the change on current master and works with the patch. That's sufficient for me. I don't want to invest too much time in optimizing an autotest. If you think it can be done better, feel free to rewrite it.

zzag added a comment.Feb 21 2019, 11:43 PM

Tests are not less important than the rest of the project. It's not good that we don't care about their readability.

I'm not convinced by "current autotest fails without the change on current master and works with the patch". Please make the test simpler. I don't think that it will take too much time to reorganize the test table.

In D15907#417151, @zzag wrote:

Tests are not less important than the rest of the project. It's not good that we don't care about their readability.

Oh I do care about readability. Did you notice the huge diagram so auto test readers have a picture of what the test is doing?

I'm not convinced by "current autotest fails without the change on current master and works with the patch". Please make the test simpler. I don't think that it will take too much time to reorganize the test table.

What's not to be convinced here? It's a fact, not an argument. No, I won't rewrite the test. If you want the autotest differently, do it yourself.

zzag added a comment.Feb 22 2019, 10:07 AM
void DecorationButtonTest::testContains_data()
{
    QTest::addColumn<QRectF>("geometry");
    QTest::addColumn<QPointF>("pos");
    QTest::addColumn<bool>("contains");

    QTest::newRow("left edge (integer)")   << QRectF(0, 0, 10, 10) << QPointF(0, 5) << true;
    QTest::newRow("top edge (integer)")    << QRectF(0, 0, 10, 10) << QPointF(5, 0) << true;
    QTest::newRow("right edge (integer)")  << QRectF(0, 0, 10, 10) << QPointF(9, 5) << true;
    QTest::newRow("bottom edge (integer)") << QRectF(0, 0, 10, 10) << QPointF(5, 9) << true;
    QTest::newRow("inside (integer)")      << QRectF(0, 0, 10, 10) << QPointF(5, 5) << true;

    QTest::newRow("outside 1 (integer)") << QRectF(0, 0, 10, 10) << QPointF(-1, 5) << false;
    QTest::newRow("outside 2 (integer)") << QRectF(0, 0, 10, 10) << QPointF(5, -1) << false;
    QTest::newRow("outside 3 (integer)") << QRectF(0, 0, 10, 10) << QPointF(10, 5) << false;
    QTest::newRow("outside 4 (integer)") << QRectF(0, 0, 10, 10) << QPointF(5, 10) << false;

    const qreal eps = 1e-3;
    QTest::newRow("left edge (float)")   << QRectF(0.1, 0.1, 10.0, 10.0) << QPointF(0.1, 5.1)        << true;
    QTest::newRow("top edge (float)")    << QRectF(0.1, 0.1, 10.0, 10.0) << QPointF(5.1, 0.1)        << true;
    QTest::newRow("right edge (float)")  << QRectF(0.1, 0.1, 10.0, 10.0) << QPointF(10.1 - eps, 5.1) << true;
    QTest::newRow("bottom edge (float)") << QRectF(0.1, 0.1, 10.0, 10.0) << QPointF(5.1, 10.1 - eps) << true;
    QTest::newRow("inside (float)")      << QRectF(0.1, 0.1, 10.0, 10.0) << QPointF(5.0, 5.0)        << true;

    QTest::newRow("outside 1 (float)") << QRectF(0.1, 0.1, 10.0, 10.0) << QPointF(0.1 - eps, 5.1)  << false;
    QTest::newRow("outside 2 (float)") << QRectF(0.1, 0.1, 10.0, 10.0) << QPointF(5.1, 0.1 - eps)  << false;
    QTest::newRow("outside 3 (float)") << QRectF(0.1, 0.1, 10.0, 10.0) << QPointF(10.1, 5.1)       << false;
    QTest::newRow("outside 4 (float)") << QRectF(0.1, 0.1, 10.0, 10.0) << QPointF(5.1, 10.1)       << false;
    QTest::newRow("outside 5 (float)") << QRectF(0.1, 0.1, 10.0, 10.0) << QPointF(10.1 + eps, 5.1) << false;
    QTest::newRow("outside 6 (float)") << QRectF(0.1, 0.1, 10.0, 10.0) << QPointF(5.1, 10.1 + eps) << false;
}

void DecorationButtonTest::testContains()
{
    MockBridge bridge;
    MockDecoration mockDecoration(&bridge);

    QFETCH(QRectF, geometry);
    MockButton button(KDecoration2::DecorationButtonType::Custom, &mockDecoration);
    button.setGeometry(geometry);
    button.setEnabled(true);
    button.setVisible(true);

    QFETCH(QPointF, pos);
    QTEST(button.contains(pos), "contains");
}

QRectF with negative size is untested.

zzag resigned from this revision.Feb 22 2019, 10:52 AM

Alright, I'm done.

I don't understand what's the point of review where comments are ignored. The fact that reviewer has to address review comments strikes me odd the most, like it's his/her/their responsibility. It's really sad that we don't care about simplicity of code.

romangg added a subscriber: zzag.Feb 22 2019, 11:48 AM
In D15907#417286, @zzag wrote:

Alright, I'm done.

Maybe you misunderstood the last comment. Your autotest does not test the case if the size of the button is negative. Since the new runtime code has different code paths for d->geometry.width() < 0 and d->geometry.height() < 0 this case should be tested. The current autotest does it. You could have added some more rows for that to your autotest proposal to do this as well.

I don't understand what's the point of review where comments are ignored. The fact that reviewer has to address review comments strikes me odd the most, like it's his/her/their responsibility. It's really sad that we don't care about simplicity of code.

I won't discuss reviews in general with you right now. Take a look at my code contributions in the last few months that you commented on. How many of your comments did I react to and how many not? And if you don't get why I ignored some of your comments think again in the broad scope of the project and in particular in the context of the current discussion about this single autotest. Also stop using untrue passive-aggressive statements like "It's really sad that we don't care about simplicity of code." if you don't want to generate animosity.

davidedmundson requested changes to this revision.Feb 25 2019, 2:37 PM

You could have added some more rows for that to your autotest proposal to do this as well.

As can you. It's your patch. Zzag already did 90% of the cleanup.
Would probably have been quicker than writing the reply.

This revision now requires changes to proceed.Feb 25 2019, 2:37 PM
romangg abandoned this revision.Feb 25 2019, 2:52 PM

No, it wouldn't. But you know what, fix your faulty patches yourselves. I have enough time wasted on this super-small issue. Get your priorities straight!