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

[Zsombor Egri]

ActionItem visible and enabled internal bindings not broken when the property values are having new bindings or value assignments.

ActionItem visible and enabled internal bindings not broken when the property values are having new bindings or value assignments.

[Zsombor Egri (zsombi)]

tst_actionitem.qml test had to be moved under X11 as visible property is not set on the dynamically created item.

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

1635. By Zsombor Egri on 2015-09-15

fix

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1634
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2223/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/951
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/953
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/953/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/950

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1635
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2224/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/952
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/954
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/954/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/951

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

review: Needs Fixing (continuous-integration)

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

1636. By Zsombor Egri on 2015-09-16

override visible and enabled properties with custom setters to capture QML binding caused updates

1637. By Zsombor Egri on 2015-09-16

remove text property override in TextInputPopovers as those already have text property

1638. By Zsombor Egri on 2015-09-16

staging sync

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1638
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2230/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/958
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/960
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/960/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/957

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

review: Approve (continuous-integration)

[Zoltan Balogh (bzoltan)]

Does the job

review: Approve

=== modified file 'src/Ubuntu/Components/1.2/TextInputPopover.qml'
--- src/Ubuntu/Components/1.2/TextInputPopover.qml	2015-08-19 06:55:11 +0000
+++ src/Ubuntu/Components/1.2/TextInputPopover.qml	2015-09-16 05:29:55 +0000
@@ -83,13 +83,6 @@
             model: actions.length
             AbstractButton {
                 id: button
-                /*
-                  Workaround for autopilot used in the text input's context menu to access
-                  action.text so we can get the proper button by text, action being not
-                  accessible. https://bugs.launchpad.net/autopilot/+bug/1334599
-                  */
-                // FIXME: AbstractButton has text property, which is getting the action.text, so no need to override!
-                property string text: action.text
                 width: Math.max(units.gu(5), implicitWidth) + units.gu(2)
                 height: units.gu(6)
                 action: actions[modelData]

=== modified file 'src/Ubuntu/Components/1.3/TextInputPopover.qml'
--- src/Ubuntu/Components/1.3/TextInputPopover.qml	2015-07-02 23:33:22 +0000
+++ src/Ubuntu/Components/1.3/TextInputPopover.qml	2015-09-16 05:29:55 +0000
@@ -83,12 +83,6 @@
             model: actions.length
             AbstractButton {
                 id: button
-                /*
-                  Workaround for autopilot used in the text input's context menu to access
-                  action.text so we can get the proper button by text, action being not
-                  accessible. https://bugs.launchpad.net/autopilot/+bug/1334599
-                  */
-                property string text: action.text
                 width: Math.max(units.gu(5), implicitWidth) + units.gu(2)
                 height: units.gu(6)
                 action: actions[modelData]

=== modified file 'src/Ubuntu/Components/plugin/ucactionitem.cpp'
--- src/Ubuntu/Components/plugin/ucactionitem.cpp	2015-08-25 11:31:29 +0000
+++ src/Ubuntu/Components/plugin/ucactionitem.cpp	2015-09-16 05:29:55 +0000
@@ -16,6 +16,10 @@
 
 #include "ucactionitem.h"
 #include "ucaction.h"
+#include "ucstyleditembase_p.h"
+#define foreach Q_FOREACH
+#include <QtQml/private/qqmlbinding_p.h>
+#undef foreach
 
 /*!
  * \qmltype ActionItem
@@ -40,8 +44,14 @@
     , m_action(Q_NULLPTR)
     , m_flags(0)
 {
-    connect(this, &UCActionItem::visibleChanged, this, &UCActionItem::_q_visibleChanged);
-    connect(this, &UCActionItem::enabledChanged, this, &UCActionItem::_q_enabledChanged);
+    connect(this, &UCActionItem::enabledChanged, this, &UCActionItem::enabledChanged2);
+    connect(this, &UCActionItem::visibleChanged, this, &UCActionItem::visibleChanged2);
+}
+
+bool UCActionItem::hasBindingOnProperty(const QString &name)
+{
+    QQmlProperty property(this, name, qmlContext(this));
+    return QQmlPropertyPrivate::binding(property) != Q_NULLPTR;
 }
 
 void UCActionItem::componentComplete()
@@ -55,40 +65,47 @@
     }
 }
 
-void UCActionItem::_q_visibleChanged()
-{
-    m_flags |= CustomVisible;
-    disconnect(this, &UCActionItem::visibleChanged, this, &UCActionItem::_q_visibleChanged);
-}
-
-void UCActionItem::_q_enabledChanged()
-{
-    m_flags |= CustomEnabled;
-    disconnect(this, &UCActionItem::enabledChanged, this, &UCActionItem::_q_enabledChanged);
-}
-
 // update visible property
-void UCActionItem::_q_updateVisible()
+void UCActionItem::_q_visibleBinding()
 {
+    if (m_flags & CustomVisible) {
+        return;
+    }
+    if (hasBindingOnProperty(QStringLiteral("visible"))) {
+        m_flags |= CustomEnabled;
+        return;
+    }
     bool visible = m_action ? m_action->m_visible : true;
     setVisible(visible);
-    // reset flag and reconnect signal handler disconnected by the
-    m_flags &= ~CustomVisible;
-    if (m_action) {
-        connect(this, &UCActionItem::visibleChanged, this, &UCActionItem::_q_visibleChanged);
-    }
 }
 
 // update enabled property
-void UCActionItem::_q_updateEnabled()
+void UCActionItem::_q_enabledBinding()
 {
+    if (m_flags & CustomEnabled) {
+        return;
+    }
+    if (hasBindingOnProperty(QStringLiteral("enabled"))) {
+        m_flags |= CustomEnabled;
+        return;
+    }
     bool enabled = m_action ? m_action->m_enabled : true;
     setEnabled(enabled);
-    // reset flag and reconnect signal handler disconnected by the
-    m_flags &= ~CustomEnabled;
-    if (m_action) {
-        connect(this, &UCActionItem::enabledChanged, this, &UCActionItem::_q_enabledChanged);
-    }
+}
+
+// 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
+void UCActionItem::setVisible2(bool visible)
+{
+    // set the custom flag and forward the value to the original proepry setter
+    m_flags |= CustomVisible;
+    setVisible(visible);
+}
+void UCActionItem::setEnabled2(bool enabled)
+{
+    m_flags |= CustomEnabled;
+    setEnabled(enabled);
 }
 
 void UCActionItem::updateProperties()
@@ -109,10 +126,14 @@
     if (attach) {
         connect(this, SIGNAL(triggered(QVariant)),
                 m_action, SLOT(trigger(QVariant)), Qt::DirectConnection);
-        connect(m_action, &UCAction::visibleChanged,
-                this, &UCActionItem::_q_updateVisible, Qt::DirectConnection);
-        connect(m_action, &UCAction::enabledChanged,
-                this, &UCActionItem::_q_updateEnabled, Qt::DirectConnection);
+        if (!(m_flags & CustomVisible)) {
+            connect(m_action, &UCAction::visibleChanged,
+                    this, &UCActionItem::_q_visibleBinding, Qt::DirectConnection);
+        }
+        if (!(m_flags & CustomEnabled)) {
+            connect(m_action, &UCAction::enabledChanged,
+                    this, &UCActionItem::_q_enabledBinding, Qt::DirectConnection);
+        }
         if (!(m_flags & CustomText)) {
             connect(m_action, &UCAction::textChanged,
                     this, &UCActionItem::textChanged, Qt::DirectConnection);
@@ -128,10 +149,14 @@
     } else {
         disconnect(this, SIGNAL(triggered(QVariant)),
                    m_action, SLOT(trigger(QVariant)));
-        disconnect(m_action, &UCAction::visibleChanged,
-                   this, &UCActionItem::_q_updateVisible);
-        disconnect(m_action, &UCAction::enabledChanged,
-                   this, &UCActionItem::_q_updateEnabled);
+        if (!(m_flags & CustomVisible)) {
+            disconnect(m_action, &UCAction::visibleChanged,
+                       this, &UCActionItem::_q_visibleBinding);
+        }
+        if (!(m_flags & CustomEnabled)) {
+            disconnect(m_action, &UCAction::enabledChanged,
+                       this, &UCActionItem::_q_enabledBinding);
+        }
         if (!(m_flags & CustomText)) {
             disconnect(m_action, &UCAction::textChanged,
                        this, &UCActionItem::textChanged);
@@ -167,8 +192,8 @@
     if (m_action) {
         attachAction(true);
     }
-    _q_updateVisible();
-    _q_updateEnabled();
+    _q_visibleBinding();
+    _q_enabledBinding();
     updateProperties();
 }
 

=== modified file 'src/Ubuntu/Components/plugin/ucactionitem.h'
--- src/Ubuntu/Components/plugin/ucactionitem.h	2015-08-25 11:31:29 +0000
+++ src/Ubuntu/Components/plugin/ucactionitem.h	2015-09-16 05:29:55 +0000
@@ -26,6 +26,10 @@
     Q_PROPERTY(QString text READ text WRITE setText RESET resetText NOTIFY textChanged)
     Q_PROPERTY(QUrl iconSource READ iconSource WRITE setIconSource RESET resetIconSource NOTIFY iconSourceChanged)
     Q_PROPERTY(QString iconName READ iconName WRITE setIconName RESET resetIconName NOTIFY iconNameChanged)
+
+    // overrides
+    Q_PROPERTY(bool enabled READ isEnabled WRITE setEnabled2 NOTIFY enabledChanged2)
+    Q_PROPERTY(bool visible READ isVisible WRITE setVisible2 NOTIFY visibleChanged2 FINAL)
 public:
     explicit UCActionItem(QQuickItem *parent = 0);
 
@@ -40,6 +44,9 @@
     void setIconName(const QString &iconName);
     void resetIconName();
 
+    void setVisible2(bool visible);
+    void setEnabled2(bool enabled);
+
 Q_SIGNALS:
     void actionChanged();
     void textChanged();
@@ -47,14 +54,15 @@
     void iconNameChanged();
     void triggered(const QVariant &value);
 
+    void enabledChanged2();
+    void visibleChanged2();
+
 public Q_SLOTS:
     void trigger(const QVariant &value = QVariant());
 
 protected Q_SLOTS:
-    void _q_visibleChanged();
-    void _q_enabledChanged();
-    void _q_updateVisible();
-    void _q_updateEnabled();
+    void _q_visibleBinding();
+    void _q_enabledBinding();
 
 protected:
     enum {
@@ -72,6 +80,7 @@
 
     void componentComplete();
 
+    bool hasBindingOnProperty(const QString &name);
     void updateProperties();
     void attachAction(bool attach);
 };

=== added file 'tests/resources/navigation/Blackbox.qml'
--- tests/resources/navigation/Blackbox.qml	1970-01-01 00:00:00 +0000
+++ tests/resources/navigation/Blackbox.qml	2015-09-16 05:29:55 +0000
@@ -0,0 +1,48 @@
+/*
+ * 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.2
+
+MainView {
+    width: units.gu(40)
+    height: units.gu(60)
+    Component.onCompleted: pageStack.push(page1)
+
+    PageStack {
+        id: pageStack
+    }
+
+    Page {
+        id: page1
+        title: "one"
+        Button {
+            anchors.centerIn: parent
+            text: "next"
+            onClicked: pageStack.push(page2)
+        }
+    }
+    Page {
+        id: page2
+        title: "two"
+        visible: false
+
+        head.backAction: Action {
+            iconName: "back"
+            onTriggered: pageStack.pop()
+        }
+    }
+}

=== renamed file 'tests/unit/tst_components/tst_actionitem.qml' => 'tests/unit_x11/tst_components/tst_actionitem.qml'
--- tests/unit/tst_components/tst_actionitem.qml	2015-08-18 16:58:41 +0000
+++ tests/unit_x11/tst_components/tst_actionitem.qml	2015-09-16 05:29:55 +0000
@@ -18,86 +18,182 @@
 import QtTest 1.0
 import Ubuntu.Components 1.1
 
-TestCase {
-     name: "ActionItemAPI"
-
-     SignalSpy {
-         id: triggerSpy
-         target: action1
-         signalName: "triggered"
-     }
-
-     function initTestCase() {
-         compare(item1.action, null, "action is null by default")
-         compare(item1.text, "", "text is empty string set by default")
-         compare(item1.iconSource, "", "iconSource is empty string by default")
-         compare(item1.iconName, "", "iconSource is empty string by default")
-     }
-
-     function cleanup() {
-         item1.action = null;
-         triggerSpy.clear();
-     }
-
-     function test_action() {
-         compare(item1.action, null,"Action is null by default")
-         item1.action = action1
-         compare(item1.action, action1, "Action can be set")
-         compare(item1.text, action1.text, "text is automatically set to action text")
-         compare(item1.iconSource, action1.iconSource, "iconSource is automatically set to action iconSource")
-         item1.triggered(null)
-         triggerSpy.wait(400);
-     }
-
-     // NOTE: This test must be run AFTER test_action(), otherwise setting the action will
-     // not update the text
-     function test_text() {
-         compare(item1.text, "", "text is empty string by default")
-         var newText = "new text"
-         item1.text = newText
-         compare(item1.text, newText, "text can be set")
-         item1.text = ""
-         compare(item1.text, "", "text can be unset")
-     }
-
-     // NOTE: This test must be run AFTER test_action(), otherwise setting the action will
-     // will not update the iconSource
-     function test_iconSource() {
-         compare(item1.iconSource, "", "iconSource is empty string by default")
-         var newIconSource = Qt.resolvedUrl("../../../examples/ubuntu-ui-toolkit-gallery/small_avatar.png")
-         item1.iconSource = newIconSource
-         compare(item1.iconSource, newIconSource, "iconSource can be set")
-         item1.iconSource = ""
-         compare(item1.iconSource, "", "iconSource can be unset")
-     }
-
-     // NOTE: This test must be run AFTER test_action(), otherwise setting the action will
-     // will not update the iconName
-     function test_iconName() {
-         compare(item1.iconName, "", "iconName is empty string by default")
-         var newIconName = "compose"
-         item1.iconName = newIconName
-         compare(item1.iconName, newIconName, "iconName can be set")
-         item1.iconName = ""
-         compare(item1.iconName, "", "iconName can be unset")
-     }
-
-     function test_signal_triggered() {
-         signalSpy.signalName = "triggered";
-         compare(signalSpy.valid,true,"triggered signal exists")
-     }
-
-     ActionItem {
-         id: item1
-         SignalSpy {
-             id: signalSpy
-             target: parent
-         }
-     }
-
-     Action {
-         id: action1
-         text: "actionText"
-         iconSource: "imageURL"
-     }
+Item {
+    id: main
+    width: units.gu(20)
+    height: units.gu(20)
+
+    property bool customVisible: true
+    property bool customEnabled: true
+
+    ActionItem {
+        id: item1
+        SignalSpy {
+            id: signalSpy
+            target: parent
+        }
+    }
+
+    Component {
+        id: dynamicItem
+        ActionItem {
+            action: action1
+        }
+    }
+    Component {
+        id: dynamicItem2
+        ActionItem {
+            action: action1
+            visible: customVisible
+            enabled: customEnabled
+        }
+    }
+
+    Action {
+        id: action1
+        objectName: "action1"
+        text: "actionText"
+        iconSource: "imageURL"
+    }
+    Action {
+        id: action2
+        objectName: "action2"
+    }
+
+    Loader {
+        id: loader
+        asynchronous: false
+    }
+
+    TestCase {
+        id: testCase
+        when: windowShown
+        name: "ActionItemAPI"
+
+        SignalSpy {
+            id: triggerSpy
+            target: action1
+            signalName: "triggered"
+        }
+
+        function initTestCase() {
+            compare(item1.action, null, "action is null by default")
+            compare(item1.text, "", "text is empty string set by default")
+            compare(item1.iconSource, "", "iconSource is empty string by default")
+            compare(item1.iconName, "", "iconSource is empty string by default")
+        }
+
+        function cleanup() {
+            loader.sourceComponent = null;
+            item1.action = null;
+            action1.visible = true;
+            action1.enabled = true;
+            action2.visible = true;
+            action2.enabled = true;
+            main.customEnabled = true;
+            main.customVisible = true;
+            triggerSpy.clear();
+        }
+
+        function test_action() {
+            compare(item1.action, null,"Action is null by default")
+            item1.action = action1
+            compare(item1.action, action1, "Action can be set")
+            compare(item1.text, action1.text, "text is automatically set to action text")
+            compare(item1.iconSource, action1.iconSource, "iconSource is automatically set to action iconSource")
+            item1.triggered(null)
+            triggerSpy.wait(400);
+        }
+
+        // NOTE: This test must be run AFTER test_action(), otherwise setting the action will
+        // not update the text
+        function test_text() {
+            compare(item1.text, "", "text is empty string by default")
+            var newText = "new text"
+            item1.text = newText
+            compare(item1.text, newText, "text can be set")
+            item1.text = ""
+            compare(item1.text, "", "text can be unset")
+        }
+
+        // NOTE: This test must be run AFTER test_action(), otherwise setting the action will
+        // will not update the iconSource
+        function test_iconSource() {
+            compare(item1.iconSource, "", "iconSource is empty string by default")
+            var newIconSource = Qt.resolvedUrl("../../../examples/ubuntu-ui-toolkit-gallery/small_avatar.png")
+            item1.iconSource = newIconSource
+            compare(item1.iconSource, newIconSource, "iconSource can be set")
+            item1.iconSource = ""
+            compare(item1.iconSource, "", "iconSource can be unset")
+        }
+
+        // NOTE: This test must be run AFTER test_action(), otherwise setting the action will
+        // will not update the iconName
+        function test_iconName() {
+            compare(item1.iconName, "", "iconName is empty string by default")
+            var newIconName = "compose"
+            item1.iconName = newIconName
+            compare(item1.iconName, newIconName, "iconName can be set")
+            item1.iconName = ""
+            compare(item1.iconName, "", "iconName can be unset")
+        }
+
+        function test_signal_triggered() {
+            signalSpy.signalName = "triggered";
+            compare(signalSpy.valid,true,"triggered signal exists")
+        }
+
+        function test_default_bindings_visible_enabled_data() {
+            return [
+                        {tag: "visible", property: "visible"},
+                        {tag: "enabled", property: "enabled"},
+                    ];
+        }
+        function test_default_bindings_visible_enabled(data) {
+            item1.action = action1;
+            action1[data.property] = false;
+            compare(item1[data.property], action1[data.property], "The item1 and action1 '" + data.property + "' value differs");
+        }
+
+        function test_custom_bindings_visible_enabled_bug1495408_data() {
+            return [
+                {tag: "visible", component: dynamicItem, property: "visible"},
+                {tag: "enabled", component: dynamicItem, property: "enabled"},
+                {tag: "visible binding", component: dynamicItem2, property: "visible", customProperty: "customVisible"},
+                {tag: "enabled binding", component: dynamicItem2, property: "enabled", customProperty: "customEnabled"},
+            ];
+        }
+        function test_custom_bindings_visible_enabled_bug1495408(data) {
+            loader.sourceComponent = data.component;
+            var item = loader.item;
+            compare(item[data.property], action1[data.property], "The item and action1 '" + data.property + "' value differs");
+            if (data.customProperty) {
+                main[data.customProperty] = false;
+            } else {
+                item[data.property] = false;
+            }
+            // change the action so the internal bindings are updated
+            item.action = action2;
+            expectFail(data.tag, "default binding must be broken");
+            compare(item[data.property], item.action[data.property], "The item's and action's '" + data.property + "' value is the same");
+        }
+        function test_custom_bindings_visible_enabled_reparenting_bug1495408_data() {
+            return [
+                {tag: "visible binding", component: dynamicItem2, property: "visible"},
+                {tag: "enabled binding", component: dynamicItem2, property: "enabled"},
+            ];
+        }
+        function test_custom_bindings_visible_enabled_reparenting_bug1495408(data) {
+            loader.sourceComponent = dynamicItem2;
+            var item = loader.item;
+            compare(item[data.property], item.action[data.property], "The item and item.action '" + data.property + "' value differs");
+            // then reparent
+            item.parent = item1;
+            // change the action property
+
+            item.action[data.property] = false;
+            expectFail(data.tag, "default binding must be broken");
+            compare(item[data.property], item.action[data.property], "The item and action2 '" + data.property + "' value is the same");
+        }
+    }
 }