Code review comment for lp:~dandrader/unity/phablet_edgeDragGesture

> 803 +#include <QQuickItem>
>
> 1025 +#include <QQmlExtensionPlugin>
>
> 1513 +#include <QtTest/QtTest>
> 1514 +#include <QObject>
> 1515 +#include <qpa/qwindowsysteminterface.h>
> 1516 +#include <QQuickView>
> 1517 +#include <QQmlEngine>
>
> Please use <QtQuick/QQuickItem> etc.

Done.

>
> =====
>
> Damper.h should #include <QtCore/QPointF>.

Done.

>
> =====
>
> 407 + void setMaxDelta(Type maxDelta) {
> 408 + m_maxDelta = maxDelta;
> 409 + }
>
> 418 + if (delta > 0 && delta > m_maxDelta) {
> 419 + m_value += delta - m_maxDelta;
> 420 + } else if (delta < 0 && delta < -m_maxDelta) {
> 421 + m_value += delta - m_maxDelta;
> 422 + }
>
> The > 0, < 0 seem unnecessary?

No, it's necessary. I do have to distinguish between a negative and a positive delta. Speaking of which, I just spotted a bug in diff line 421. It should be "m_value += delta + m_maxDelta;" instead. Fixed it.

> Although setMaxDelta() should make sure that
> it's > 0?

Yeah, added the check.

>
> =====
>
> 434 +/*
> 435 + A point that has its movement dampened.
> 436 + */
> 437 +class DampedPointF {
> 438 +public:
> 439 + void setMaxDelta(qreal maxDelta) {
> 440 + m_x.setMaxDelta(maxDelta);
> 441 + m_y.setMaxDelta(maxDelta);
> 442 + }
> 443 +
> 444 + qreal maxDelta() const { return m_x.maxDelta(); }
> 445 +
> 446 + void reset(const QPointF &point) {
> 447 + m_x.reset(point.x());
> 448 + m_y.reset(point.y());
> 449 + }
> 450 +
> 451 + void update(const QPointF &point) {
> 452 + m_x.update(point.x());
> 453 + m_y.update(point.y());
> 454 + }
> 455 +
> 456 + qreal x() const { return m_x.value(); }
> 457 + qreal y() const { return m_y.value(); }
> 458 +private:
> 459 + Damper<qreal> m_x;
> 460 + Damper<qreal> m_y;
> 461 +};
>
> I know it's probably minor (square vs. circular dampening area), but shouldn't
> we use QPointF::operator- and QPointF::manhattanLength here?

Using a circular area is computationally more expensive and we're just fine with this less accurate method.

Using a manhattan lenght would result in an area defined by a square rotated by 45 degrees instead of an axis-aligned square as it is now. So, in the end, these two approaches are equivalent (a square area).

>
> =====
>
> 561 + if (m_state == Rejected)
> 562 + return 0.0;
>
> Please wrap all blocks in braces.

Done.

>
> =====
>
> 633 + if (!pointInsideAllowedArea(touchPos)) {
> 634 + setState(Rejected);
> 635 + return;
> 636 + }
> 637 +
> 638 + m_previousDampedPos.setX(m_dampedPos.x());
> 639 + m_previousDampedPos.setY(m_dampedPos.y());
> 640 + m_dampedPos.update(touchPos);
> 641 +
> 642 + if (!movingInRightDirection()) {
> 643 + setState(Rejected);
> 644 + return;
> 645 + }
>
> 704 +bool DirectionalDragArea::movingInRightDirection() const
> 705 +{
> 706 + switch (m_direction) {
> 707 + case Upwards:
> 708 + return m_dampedPos.y() <= m_previousDampedPos.y();
> 709 + case Downwards:
> 710 + return m_dampedPos.y() >= m_previousDampedPos.y();
> 711 + case Leftwards:
> 712 + return m_dampedPos.x() <= m_previousDampedPos.x();
> 713 + default: // Rightwards:
> 714 + return m_dampedPos.x() >= m_previousDampedPos.x();
> 715 + }
> 716 +}
>
> I would think I should be able to move within maxDeviation from the start
> point in Recognizing state and that would not cause a rejection. But if I'm
> reading the above correctly, I'm only really allowed to move within
> movementDamperMaxDelta from the start position, no?

Right! Now that we have m_dampedPos we can simplify things quite a bit.
The dampening will already take care of what m_maxDeviation does. And while at it I noticed and fixed a bug in pointInsideAllowedArea(). Added more test data to cover them.

Fixed.

>
> =====
>
> 851 + Q_PROPERTY(qreal movementDamperMaxDelta
> 852 + READ movementDamperMaxDelta
> 853 + WRITE setMovementDamperMaxDelta
> 854 + NOTIFY movementDamperMaxDeltaChanged)
>
> I'd probably go for "damperMaxDelta", no need for the "movement".
>

That property was removed due to the modifications explained above.

> =====
>
> 824 + Q_PROPERTY(qreal dragValue READ dragValue NOTIFY dragValueChanged)
>
> Would "distance" be more self-explanatory?

Yeah! Its documentation is already explaining that it's a distance. :)
Done.

>
> =====
>
> I wonder if DirectionalDragArea::state (or status to avoid confusion) should
> be exposed to QML? Or maybe a "dragging" boolean?

Yeah, we could expose the state variable indeed.
Done.

>
> Shouldn't dragValue be reset to 0 when going back to WaitingForTouch? Or maybe
> that's a higher level's responsibility to make sure of that...

No, I think it should stay as it is. It's up to the application to decide what to do once the gesture ends.

> =====
>
> 1070 + LD_LIBRARY_PATH=${CMAKE_BINARY_DIR}/plugins/Ubuntu/Gestures
>
> This should be set as an ENVIRONMENT property on the test instead.

Why? So you want CMake to pass it as a DEFINE to tst_DirectionalDragArea.cpp during compilation?