Merge lp:~zsombi/ubuntu-ui-toolkit/properTriggerCall into lp:ubuntu-ui-toolkit/staging

[Zsombor Egri]

Invoke the overridden trigger() function for Action and ActionItem derivates.

[lp:~zsombi/ubuntu-ui-toolkit/properTriggerCall]

1755. By Zsombor Egri on 2015-12-14

Fix ListItem's action triggering

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1755
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2619/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/1347
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/1350
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/1350/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/1346

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2619/rebuild

review: Approve (continuous-integration)

[Cris Dywan (kalikiana)]

For the record, two things at first I was skeptical about, but after all make sense to me. Having the trigger be a separate template rather than implemented in the subclass - this is indeed needed to support a trigger() method overridden in both C++ and QML. And separating the clicked invocation - it would seem to be a good place to implement the trigger, but it really is equivalent to calling the both of them accordingly.
So, looks very good!

review: Approve

=== modified file 'src/Ubuntu/Components/plugin/ucabstractbutton.cpp'
--- src/Ubuntu/Components/plugin/ucabstractbutton.cpp	2015-12-11 15:37:25 +0000
+++ src/Ubuntu/Components/plugin/ucabstractbutton.cpp	2015-12-14 06:15:34 +0000
@@ -17,6 +17,7 @@
 #include "ucabstractbutton.h"
 #include "ucabstractbutton_p.h"
 #include "uchaptics.h"
+#include "ucaction.h"
 #include <QtQuick/private/qquickitem_p.h>
 #include <QtQuick/private/qquickmousearea_p.h>
 #include <QtQml/private/qqmlglobal_p.h>
@@ -100,8 +101,6 @@
 {
     UCActionItemPrivate::completeComponentInitialization();
     Q_Q(UCAbstractButton);
-    // connect to the right slot, using macros so we get the proper slot
-    QObject::connect(mouseArea, SIGNAL(clicked(QQuickMouseEvent*)), q, SLOT(trigger()));
 
     // bind mouse area
     QObject::connect(mouseArea, &QQuickMouseArea::pressedChanged, q, &UCAbstractButton::pressedChanged);
@@ -139,6 +138,8 @@
     // play haptics
     HapticsProxy::instance().play(QVariant());
     Q_EMIT q->clicked();
+    // call the overridden QML trigger function
+    invokeTrigger<UCAbstractButton>(q, QVariant());
 }
 
 // handle pressAndHold
@@ -162,7 +163,10 @@
         case Qt::Key_Return:
         case Qt::Key_Space:
         {
-            trigger();
+            // trigger clicked signal first
+            Q_EMIT clicked();
+            // then invoke the overloaded trigger
+            invokeTrigger<UCAbstractButton>(this, QVariant());
             break;
         }
     }

=== modified file 'src/Ubuntu/Components/plugin/ucaction.cpp'
--- src/Ubuntu/Components/plugin/ucaction.cpp	2015-12-08 15:13:49 +0000
+++ src/Ubuntu/Components/plugin/ucaction.cpp	2015-12-14 06:15:34 +0000
@@ -316,7 +316,7 @@
     }
 
     // do not call trigger() directly but invoke, as it may get overridden in QML
-    metaObject()->invokeMethod(this, "trigger");
+    invokeTrigger<UCAction>(this, QVariant());
     return true;
 }
 

=== modified file 'src/Ubuntu/Components/plugin/ucaction.h'
--- src/Ubuntu/Components/plugin/ucaction.h	2015-12-11 09:54:17 +0000
+++ src/Ubuntu/Components/plugin/ucaction.h	2015-12-14 06:15:34 +0000
@@ -21,6 +21,28 @@
 #include <QtCore/QVariant>
 #include <QtCore/QUrl>
 
+// the function detects whether QML has an overridden trigger() slot available
+// and invokes the one with the appropriate signature
+template<class T>
+inline void invokeTrigger(T *object, const QVariant &value)
+{
+    bool invoked = false;
+    const QMetaObject *mo = object->metaObject();
+    int offset = mo->methodOffset();
+    int paramlessTriggerIndex = mo->indexOfSlot("trigger()") - offset;
+    int paramTriggerIndex = mo->indexOfSlot("trigger(QVariant)") - offset;
+
+    /* if we have the parametered version, call that even if the value given is invalid */
+    if (paramTriggerIndex >= 0) {
+        invoked = QMetaObject::invokeMethod(object, "trigger", Q_ARG(QVariant, value));
+    } else if (paramlessTriggerIndex >= 0) {
+        invoked = QMetaObject::invokeMethod(object, "trigger");
+    }
+    if (!invoked) {
+        object->trigger(value);
+    }
+}
+
 class QQmlComponent;
 class UCAction : public QObject
 {

=== modified file 'src/Ubuntu/Components/plugin/ucactionitem.cpp'
--- src/Ubuntu/Components/plugin/ucactionitem.cpp	2015-12-11 15:37:25 +0000
+++ src/Ubuntu/Components/plugin/ucactionitem.cpp	2015-12-14 06:15:34 +0000
@@ -72,17 +72,6 @@
     return QQmlPropertyPrivate::binding(property) != Q_NULLPTR;
 }
 
-void UCActionItemPrivate::completeComponentInitialization()
-{
-    UCStyledItemBasePrivate::completeComponentInitialization();
-    // make sure we connect to the right signals, so we detach and re-attach actions
-    // to make sure the SLOT macro picks up the custom trigger() slot
-    if (action) {
-        attachAction(false);
-        attachAction(true);
-    }
-}
-
 // update visible property
 void UCActionItemPrivate::_q_visibleBinding()
 {
@@ -111,6 +100,12 @@
     q_func()->setEnabled(enabled);
 }
 
+// invoke actions' overridden triger() function
+void UCActionItemPrivate::_q_invokeActionTrigger(const QVariant &value)
+{
+    invokeTrigger<UCAction>(action, value);
+}
+
 // setter called when bindings from QML set the value. Internal functions will
 // all use the setVisible setter, so initialization and (re)parenting related
 // visible alteration won't set the custom flag
@@ -145,7 +140,7 @@
     Q_Q(UCActionItem);
     if (attach) {
         QObject::connect(q, SIGNAL(triggered(QVariant)),
-                action, SLOT(trigger(QVariant)), Qt::DirectConnection);
+                q, SLOT(_q_invokeActionTrigger(QVariant)), Qt::DirectConnection);
         if (!(flags & CustomVisible)) {
             QObject::connect(action, SIGNAL(visibleChanged()),
                     q, SLOT(_q_visibleBinding()), Qt::DirectConnection);
@@ -168,7 +163,7 @@
         }
     } else {
         QObject::disconnect(q, SIGNAL(triggered(QVariant)),
-                   action, SLOT(trigger(QVariant)));
+                   q, SLOT(_q_invokeActionTrigger(QVariant)));
         if (!(flags & CustomVisible)) {
             QObject::disconnect(action, SIGNAL(visibleChanged()),
                        q, SLOT(_q_visibleBinding()));
@@ -371,7 +366,6 @@
 void UCActionItem::trigger(const QVariant &value)
 {
     if (isEnabled()) {
-        // FIXME: bug #1524234: invoke function from QMetaObject (zsombi)
         Q_EMIT triggered(value);
     }
 }

=== modified file 'src/Ubuntu/Components/plugin/ucactionitem.h'
--- src/Ubuntu/Components/plugin/ucactionitem.h	2015-12-11 10:28:46 +0000
+++ src/Ubuntu/Components/plugin/ucactionitem.h	2015-12-14 06:15:34 +0000
@@ -68,6 +68,7 @@
     Q_DECLARE_PRIVATE(UCActionItem)
     Q_PRIVATE_SLOT(d_func(), void _q_visibleBinding())
     Q_PRIVATE_SLOT(d_func(), void _q_enabledBinding())
+    Q_PRIVATE_SLOT(d_func(), void _q_invokeActionTrigger(const QVariant &value))
 };
 
 #endif // UCACTIONITEM_H

=== modified file 'src/Ubuntu/Components/plugin/ucactionitem_p.h'
--- src/Ubuntu/Components/plugin/ucactionitem_p.h	2015-12-11 10:28:46 +0000
+++ src/Ubuntu/Components/plugin/ucactionitem_p.h	2015-12-14 06:15:34 +0000
@@ -38,12 +38,10 @@
     void updateProperties();
     void attachAction(bool attach);
 
-    // overrides
-    void completeComponentInitialization() override;
-
     // private slots
     void _q_visibleBinding();
     void _q_enabledBinding();
+    void _q_invokeActionTrigger(const QVariant &value);
 
     enum {
         CustomText = 0x01,

=== modified file 'src/Ubuntu/Components/plugin/ucbottomedgehint.cpp'
--- src/Ubuntu/Components/plugin/ucbottomedgehint.cpp	2015-12-11 12:19:28 +0000
+++ src/Ubuntu/Components/plugin/ucbottomedgehint.cpp	2015-12-14 06:15:34 +0000
@@ -21,6 +21,7 @@
 #include "ucstyleditembase_p.h"
 #include "quickutils.h"
 #include "ucunits.h"
+#include "ucaction.h"
 #include "gestures/ucswipearea.h"
 #include "propertychange_p.h"
 #include <QtQml/private/qqmlproperty_p.h>
@@ -46,7 +47,7 @@
     Q_Q(UCBottomEdgeHint);
     QObject::connect(q, &UCBottomEdgeHint::clicked, [=]() {
         // make sure the overloaded trigger is called!
-        q->metaObject()->invokeMethod(q, "trigger", Q_ARG(QVariant, QVariant()));
+        invokeTrigger<UCBottomEdgeHint>(q, QVariant());
     });
     /*
      * we cannot use setStyleName as that will trigger style loading

=== modified file 'src/Ubuntu/Components/plugin/uclistitem.cpp'
--- src/Ubuntu/Components/plugin/uclistitem.cpp	2015-11-16 16:24:42 +0000
+++ src/Ubuntu/Components/plugin/uclistitem.cpp	2015-12-14 06:15:34 +0000
@@ -1223,7 +1223,7 @@
             if (!swiped) {
                 Q_EMIT q->clicked();
                 if (mainAction) {
-                    Q_EMIT mainAction->trigger(index());
+                    invokeTrigger<UCAction>(mainAction, index());
                 }
             }
             snapOut();

=== modified file 'tests/unit/tst_components/tst_action.qml'
--- tests/unit/tst_components/tst_action.qml	2015-08-25 11:31:29 +0000
+++ tests/unit/tst_components/tst_action.qml	2015-12-14 06:15:34 +0000
@@ -137,10 +137,25 @@
          verify(!data.inactive.active, "Context deactivation error");
      }
 
-     function test_overloaded_action_trigger() {
-         triggeredSignalSpy.target = suppressTrigger;
-         suppressTrigger.trigger();
-         compare(triggeredSignalSpy.count, 0, "Overloaded trigger should not trigger action");
+     function test_overloaded_action_trigger_data() {
+         return [
+             {tag: "parametered override without parameter", action: suppressTrigger, invoked: true},
+             {tag: "parametered override with parameter", action: suppressTrigger, value: 1, type: Action.Integer, invoked: true},
+             {tag: "paremeterless override without parameter", action: override, invoked: true},
+             {tag: "paremeterless override with parameter", action: override, value: 1, type: Action.Integer, invoked: true},
+         ];
+     }
+     function test_overloaded_action_trigger(data) {
+         data.action.invoked = false;
+         data.action.parameterType = Action.None;
+         testItem.action = data.action;
+         if (data.value) {
+             data.action.parameterType = data.type;
+             testItem.trigger(data.value);
+         } else {
+             testItem.trigger(data.value);
+         }
+         compare(data.action.invoked, data.invoked);
      }
 
      Action {
@@ -198,7 +213,19 @@
 
      Action {
          id: suppressTrigger
-         function trigger() {}
+         property bool invoked: false
+         // we must override the parametered version as Button connects to the parametered version
+         function trigger(v) { invoked = true }
+     }
+
+     Action {
+         id: override
+         property bool invoked: false
+         function trigger() { invoked = true }
+     }
+
+     ActionItem {
+         id: testItem
      }
 
 }

=== added file 'tests/unit_x11/tst_bottomedge/OverriddenHintTrigger.qml'
--- tests/unit_x11/tst_bottomedge/OverriddenHintTrigger.qml	1970-01-01 00:00:00 +0000
+++ tests/unit_x11/tst_bottomedge/OverriddenHintTrigger.qml	2015-12-14 06:15:34 +0000
@@ -0,0 +1,45 @@
+/*
+ * Copyright 2015 Canonical Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+import QtQuick 2.4
+import Ubuntu.Components 1.3
+
+Item {
+    id: holder
+    width: units.gu(40)
+    height: units.gu(71)
+
+    Action {
+        id: testAction
+        objectName: "testAction"
+        text: "Test"
+        function trigger() {}
+    }
+
+    BottomEdge {
+        hint {
+            action: testAction
+        }
+        objectName: "testItem"
+        contentComponent: Rectangle {
+            width: holder.width
+            height: holder.height
+            color: UbuntuColors.green
+        }
+    }
+}
+

=== modified file 'tests/unit_x11/tst_bottomedge/tst_bottomedge.cpp'
--- tests/unit_x11/tst_bottomedge/tst_bottomedge.cpp	2015-12-02 15:31:53 +0000
+++ tests/unit_x11/tst_bottomedge/tst_bottomedge.cpp	2015-12-14 06:15:34 +0000
@@ -185,6 +185,22 @@
         QTRY_COMPARE_WITH_TIMEOUT(test->testItem()->status(), UCBottomEdge::Committed, 1000);
     }
 
+    void test_overridden_triggers_bug1524234()
+    {
+        QScopedPointer<BottomEdgeTestCase> test(new BottomEdgeTestCase("OverriddenHintTrigger.qml"));
+        test->testItem()->hint()->setStatus(UCBottomEdgeHint::Locked);
+        UCBottomEdgeHint *hint = test->testItem()->hint();
+        UCAction *action = hint->action();
+        QSignalSpy actionSpy(action, SIGNAL(triggered(QVariant)));
+        QSignalSpy hintSpy(hint, SIGNAL(triggered(QVariant)));
+
+        QTest::mouseClick(test->testItem()->hint()->window(), Qt::LeftButton, 0, UbuntuTestCase::centerOf(hint, true).toPoint());
+        QTRY_COMPARE_WITH_TIMEOUT(test->testItem()->status(), UCBottomEdge::Committed, 1000);
+
+        QCOMPARE(actionSpy.count(), 0);
+        QCOMPARE(hintSpy.count(), 1);
+    }
+
     void test_revealed_when_hint_threshold_passed_data()
     {
         QTest::addColumn<bool>("withMouse");

=== modified file 'tests/unit_x11/tst_bottomedge/tst_bottomedge.pro'
--- tests/unit_x11/tst_bottomedge/tst_bottomedge.pro	2015-12-02 15:31:53 +0000
+++ tests/unit_x11/tst_bottomedge/tst_bottomedge.pro	2015-12-14 06:15:34 +0000
@@ -21,4 +21,5 @@
     AutoCollapseInPageHeader.qml \
     AutoCollapseInPageWithPageHeader.qml \
     LeanActiveRegionChange.qml \
-    UncoveredByRegion.qml
+    UncoveredByRegion.qml \
+    OverriddenHintTrigger.qml

=== modified file 'tests/unit_x11/tst_components/tst_abstractbutton13.qml'
--- tests/unit_x11/tst_components/tst_abstractbutton13.qml	2015-12-05 05:59:07 +0000
+++ tests/unit_x11/tst_components/tst_abstractbutton13.qml	2015-12-14 06:15:34 +0000
@@ -41,6 +41,18 @@
             height: width
             function trigger() {}
         }
+        AbstractButton {
+            id: suppressTrigger2
+            width: units.gu(10)
+            height: width
+            function trigger(v) {}
+        }
+        AbstractButton {
+            id: suppressTrigger3
+            width: units.gu(10)
+            height: width
+            function trigger(v) { triggered(v) }
+        }
         Loader {
             id: loader
             width: units.gu(10)
@@ -64,6 +76,11 @@
         onTriggered: triggerCount++
     }
 
+    Action {
+        id: override
+        function trigger() {}
+    }
+
     SignalSpy {
         id: signalSpy
         target: absButton
@@ -87,6 +104,7 @@
 
         function cleanup() {
             signalSpy.clear();
+            signalSpy.target = absButton;
             triggeredSpy.clear();
             loader.click = false;
             loader.longPress = false;
@@ -104,13 +122,38 @@
             absButton.action = null
         }
 
-        function test_custom_trigger_function() {
-            suppressTrigger.action = action1;
+        function test_custom_trigger_function_data() {
+            return [
+                {tag: "parameterless trigger", testItem: suppressTrigger},
+                {tag: "parameted trigger", testItem: suppressTrigger2},
+            ];
+        }
+        function test_custom_trigger_function(data) {
+            data.testItem.action = action1;
             triggeredSpy.target = action1;
-            mouseClick(suppressTrigger, centerOf(suppressTrigger).x, centerOf(suppressTrigger).y);
+            mouseClick(data.testItem, centerOf(data.testItem).x, centerOf(data.testItem).y);
             compare(triggeredSpy.count, 0, "Trigger should be overridden");
         }
 
+        function test_custom_trigger_overrides_triggered_bug1524234_data() {
+            return [
+                {tag: "parameterless trigger", testItem: suppressTrigger, action: action1, triggeredWatch: action1, triggeredCount: 0 },
+                {tag: "parameted trigger", testItem: suppressTrigger2, action: action1, triggeredWatch: action1, triggeredCount: 0 },
+                {tag: "button trigger fired", testItem: suppressTrigger3, action: action1, triggeredWatch: suppressTrigger3, triggeredCount: 1 },
+                {tag: "action trigger fired", testItem: suppressTrigger3, action: action1, triggeredWatch: action1, triggeredCount: 1 },
+                {tag: "action trigger overridden", testItem: suppressTrigger3, action: override, triggeredWatch: override, triggeredCount: 0 },
+            ];
+        }
+        function test_custom_trigger_overrides_triggered_bug1524234(data) {
+            data.testItem.action = data.action;
+            triggeredSpy.target = data.triggeredWatch;
+            signalSpy.target = data.testItem;
+            data.testItem.forceActiveFocus();
+            keyClick(Qt.Key_Space);
+            signalSpy.wait(200);
+            compare(triggeredSpy.count, data.triggeredCount);
+        }
+
         // fixing bugs 1365471 and 1458028
         function test_no_pressAndHold_connected_clicks_bug1365471_bug1458028() {
             signalSpy.target = absButton;