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.

=====

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

=====

 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? Although setMaxDelta() should make sure that it's > 0?

=====

 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?

=====

 561 + if (m_state == Rejected)
 562 + return 0.0;

Please wrap all blocks in braces.

=====

 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?

=====

 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".

=====

 824 + Q_PROPERTY(qreal dragValue READ dragValue NOTIFY dragValueChanged)

Would "distance" be more self-explanatory?

=====

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

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...

=====

Wow the touchscreen on the Nexus 10 is crappy...

=====

1070 + LD_LIBRARY_PATH=${CMAKE_BINARY_DIR}/plugins/Ubuntu/Gestures

This should be set as an ENVIRONMENT property on the test instead. Or! You could not use it as a shared library ;P