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?

Done.

> * 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?

I can add it 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?

You mean those signals?

    void dragStarted();
    void directionalDragRecognized();
    void dragRejected();
    void dragEnded();

Compared to this enum?

    enum ResultFlag { Ignore, MayBeGesture, TriggerGesture, FinishGesture, CancelGesture, ConsumeEventHint }

Well, they are intented for different audiences (the former for application code and the latter for the gesture fw engine) in a bit different contexts. I preffer the signal naming the way they are, as they look very clear in meaning IMHO. One thing I see could change though is s/dragRejected/dragCancelled

> * could you add some newlines to tests/plugins/UbuntuGestures/CMakeLists.txt
> for readability?

Done.

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

Why not? Makes testing simpler. Recompiling the whole plugin in the test directory seems wasteful.

>
> =====
>
> 538 + if (touchPoint->state() == Qt::TouchPointReleased) {
>
> is it not possible here for touchPoint to be invalid?

No. A point has to be released before it disappears from the list of active points.

> why wouldn't QTouchEvent::touchPoints() be enough here?

We are tracking a specific touch point. Further touch points might come in after the gesture is recgonized but we just don't care.

>
> =====
>
> 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?

I think you mean:
Δx >= -d && abs(Δy) <= (Δx * f) + d

Yes, we can skip use of trigonometric functions by using a widening factor parameter instead of a widening angle.
Done.

>
> =====
>
> 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?

Yes, it depends on how filtered touch movement is (to reduce noise).
Works fine on the Galaxy Nexus (my dev device) as its touchscreen input seem to be highly filtered/dampened. :)
Done.

>
> =====
>
> 744 + recognition will fail. It will also fail it the drag or flick is too
> short. E.g. a
>
> → "...fail if the drag..."
>

Fixed.

> =====
>
> 740 +/*
> 741 + An area that detects axis-aligned single-finger drag gestures
>
> could we ask for a little more documentation on the properties?

The added svg is the best thing to look for. But I've added more textual documentation to them as well.

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

Ok.

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

Fixed.

>
> =====
>
> 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)

Yeah, I think we might need to wait indeed.

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

Done.

>
> =====
>
> 968 + ${CMAKE_CURRENT_SOURCE_DIR}/../../../plugins/Ubuntu/Gestures
>
> ${CMAKE_SOURCE_DIR}/plugins/Ubuntu/Gestures looks cleaner

Definitely. Done.

>
> =====
>
> 986 +add_custom_target(UbutunGesturesTestQmlFiles ALL
>
> → "UbuntuGestures..."

Fixed.

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

Done.

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

No, I don't. Do you have a Galaxy Nexus to try it out and see if results differ? That's the device I have.

It seems that drags coming from the top, left and right borders are being filtered out by mir as those borders are reserved for the shell and qmlscene is not the shell? Because no input events come at all in those situations. If you start dragging already from withing the window it works.
Also from the bottom border it always works, I guess it's because the bottom edge is for application's tool bars.

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

Details, details... But I couldn't reproduce it myself