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

* instead of the pair direction, positiveDirection, I'd go for a single enum with 4 values, what do you think?
* we talked yesterday about the possibility to add a time threshold as well (within which the touch must cross the distance threshold), do you think you could add it here or in a later change?
* there seems to be a bunch of things common with the QtWidgets QGestureRecognizer (state names, for example) [1] - do you think it would make sense to converge on the same terminology?
* could you add some newlines to tests/plugins/UbuntuGestures/CMakeLists.txt for readability?

=====

 360 +add_definitions(-DUBUNTUGESTURES_LIBRARY)
 361 +
 362 +add_library(UbuntuGestureQml MODULE ${UbuntuGestureQml_SOURCES})

 749 +class UBUNTUGESTURES_EXPORT DirectionalDragArea : public QQuickItem {

 856 +#if defined(UBUNTUGESTURES_LIBRARY)
 857 +# define UBUNTUGESTURES_EXPORT Q_DECL_EXPORT
 858 +#else
 859 +# define UBUNTUGESTURES_EXPORT Q_DECL_IMPORT
 860 +#endif

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

 982 +target_link_libraries(DirectionalDragAreaTestExec UbuntuGestureQml)

maybe directly use the .cpp in test sources instead? We don't really want it to be a library, do we.

=====

 538 + if (touchPoint->state() == Qt::TouchPointReleased) {

is it not possible here for touchPoint to be invalid? why wouldn't QTouchEvent::touchPoints() be enough here?

=====

I'm wondering if the "pointInside..." calculations couldn't be simplified by using a qreal factor instead of the angle - you'd just need to make sure that:

-d <= Δy <= Δx * f + d

where y is perpendicular to the gesture direction (horizontal positive / left-to-right in that case), d is max deviation, f is the factor; with f = 1 that would mean 45° widening angle; what do you think?

=====

 666 +bool DirectionalDragArea::movingInRightDirection(const QPointF &point)
 667 +{
 668 + if (m_orientation == Horizontal) {
 669 + if (m_positiveDirection) {
 670 + return point.x() >= m_previousPos.x();

shouldn't this take some deviation into account? i.e. if your finger goes slightly back in Recognizing state, won't that mean rejecting the gesture altogether?

=====

 744 + recognition will fail. It will also fail it the drag or flick is too short. E.g. a

→ "...fail if the drag..."

=====

 740 +/*
 741 + An area that detects axis-aligned single-finger drag gestures

could we ask for a little more documentation on the properties?

=====

 755 + // stuff that will be set in stone at some point

as discussed, I do agree we need strong defaults here, but allowing advanced use where those would be modifiable could be possible as well

=====

 768 + bool positiveDirection() const { return m_positiveDirection; }

 773 + qreal maxDeviation() const {return m_maxDeviation;}

 776 + qreal wideningAngle() const {return m_wideningAngle;}

 779 + qreal distanceThreshold() const {return m_distanceThreshold;}

please use spaces after/before curly braces consistently

=====

 764 + enum Orientation { Horizontal = Qt::Horizontal, Vertical = Qt::Vertical };

 812 + enum {

please consider using strongly-typed enums (although I don't think we've checked how QML currently deals with passing them to/from QML - so we might need to wait there)

=====

 939 +# QML tests that do not require graphical capabitlies.
 940 +add_custom_target(qmlunittests)
 941 +
 942 +# QML tests that require graphical capabitlies.
 943 +add_custom_target(qmluitests)
 944 +
 945 +add_custom_target(qmltests)
 946 +add_dependencies(qmltests qmlunittests qmluitests)
 947 +
 948 add_custom_target(alltests)
 949 -add_dependencies(alltests check)
 950 +add_dependencies(alltests check qmltests)

1611 -add_custom_target(qmltests)
1612 -add_custom_target(qmlunittests)
1613 -add_custom_target(qmluitests)
1614 -
1615 -add_dependencies(alltests qmltests)
1616 -

1624 -set(qmltest_DEFAULT_TARGETS qmlunittests qmltests)
1625 +set(qmltest_DEFAULT_TARGETS qmlunittests)

1633 -set(qmltest_DEFAULT_TARGETS qmluitests qmltests)
1634 +set(qmltest_DEFAULT_TARGETS qmluitests)

please extract those changes to a separate MP

=====

 968 + ${CMAKE_CURRENT_SOURCE_DIR}/../../../plugins/Ubuntu/Gestures

${CMAKE_SOURCE_DIR}/plugins/Ubuntu/Gestures looks cleaner

=====

 986 +add_custom_target(UbutunGesturesTestQmlFiles ALL

→ "UbuntuGestures..."

=====

ninja: error: '../tests/plugins/UbuntuGestures/*.qml', needed by 'tests/plugins/UbuntuGestures/CMakeFiles/UbutunGesturesTestQmlFiles', missing and no known rule to make it

ninja seems to have issues with that rule, it might be unable to expand "*.qml" internally - probably best to use CMake's GLOB.

=====

1372 + //width: units.gu(40)
1373 + //height: units.gu(71)

hmm... any way to make the example work with a pointer? can we enable pointer composition?

do you have a Nexus 10 to try it out? I'm getting quite some false negatives with the examples

also, tapping in the edge area results in the red rectangle to stay on screen

=====

Awesome work, BTW!

[1] http://qt-project.org/doc/qt-5.0/qtwidgets/qgesturerecognizer.html