D15907: Compare float values in DecorationButton contains check

romangg abandoned this revision. romangg added a comment. 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! REPOSITORY R129 Window Decoration Library REVISION DETAIL https://phabricat

D15907: Compare float values in DecorationButton contains check

davidedmundson requested changes to this revision. davidedmundson added a comment. This revision now requires changes to proceed. > 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.

D15907: Compare float values in DecorationButton contains check

romangg added a subscriber: zzag. romangg added a comment. 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 th

D15907: Compare float values in DecorationButton contains check

zzag resigned from this revision. zzag added a comment. 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

D15907: Compare float values in DecorationButton contains check

romangg added a comment. QRectF with negative size is untested. REPOSITORY R129 Window Decoration Library REVISION DETAIL https://phabricator.kde.org/D15907 To: romangg, #kwin, zzag, davidedmundson Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensr

D15907: Compare float values in DecorationButton contains check

zzag added a comment. void DecorationButtonTest::testContains_data() { QTest::addColumn("geometry"); QTest::addColumn("pos"); QTest::addColumn("contains"); QTest::newRow("left edge (integer)") << QRectF(0, 0, 10, 10) << QPointF(0, 5) << true;

D15907: Compare float values in DecorationButton contains check

romangg added a comment. 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 diagra

D15907: Compare float values in DecorationButton contains check

zzag added a comment. 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 th

D15907: Compare float values in DecorationButton contains check

romangg added a comment. 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("geometry"); > QTest::addColumn("pos"); >

D15907: Compare float values in DecorationButton contains check

romangg updated this revision to Diff 52257. romangg marked an inline comment as done. romangg added a comment. - Long variable names REPOSITORY R129 Window Decoration Library CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15907?vs=42753&id=52257 BRANCH decorationContainsFix R

D15907: Compare float values in DecorationButton contains check

zzag requested changes to this revision. zzag added a comment. This revision now requires changes to proceed. See my comment above. REPOSITORY R129 Window Decoration Library REVISION DETAIL https://phabricator.kde.org/D15907 To: romangg, #kwin, zzag, davidedmundson Cc: plasma-devel, jral

D15907: Compare float values in DecorationButton contains check

zzag added a comment. Looks good to me. I think the test is too much complicated. Would it be simpler to have something like QTest::addColumn("geometry"); QTest::addColumn("pos"); QTest::addColumn("contains"); ? INLINE COMMENTS > decorationbutton.cpp:455 > +// ad

D15907: Compare float values in DecorationButton contains check

zzag added a comment. Sorry, I applied the change to the wrong repo REPOSITORY R129 Window Decoration Library REVISION DETAIL https://phabricator.kde.org/D15907 To: romangg, #kwin, zzag, davidedmundson Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, j

D15907: Compare float values in DecorationButton contains check

zzag added a comment. Could you please rebase it on master? REPOSITORY R129 Window Decoration Library REVISION DETAIL https://phabricator.kde.org/D15907 To: romangg, #kwin, zzag, davidedmundson Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreute

D15907: Compare float values in DecorationButton contains check

romangg added a comment. ping REPOSITORY R129 Window Decoration Library REVISION DETAIL https://phabricator.kde.org/D15907 To: romangg, #kwin, zzag, davidedmundson Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ma

D15907: Compare float values in DecorationButton contains check

romangg added inline comments. INLINE COMMENTS > zzag wrote in decorationbutton.cpp:455 > Can you please explain this part? Shouldn't it be `d->geometry.right() < > pos.x()`? If `rect.width()` is smaller 0, then arbitrary QRect `rect` is to the left of its "anchor point" `rect.x()`. The right

D15907: Compare float values in DecorationButton contains check

zzag added a comment. 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 so I guess that's fine. INLINE COMMENTS > decorationbutton.cpp:455 > +// additional make sure pos is not on the right or

D15907: Compare float values in DecorationButton contains check

davidedmundson requested changes to this revision. davidedmundson added a comment. This revision now requires changes to proceed. This restores the bug the last patch fixed. REPOSITORY R129 Window Decoration Library REVISION DETAIL https://phabricator.kde.org/D15907 To: romangg, #kwin, z

D15907: Compare float values in DecorationButton contains check

romangg created this revision. romangg added reviewers: KWin, zzag, davidedmundson. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. romangg requested review of this revision. REVISION SUMMARY In c9cfd840137b