Merge lp:~zsombi/ubuntu-ui-toolkit/layouts-fix1280359 into lp:ubuntu-ui-toolkit

[Zsombor Egri]

Item instances set as property values were excluded from layouting, causing segfaults when re-layouting was performed or when the component got deleted due to or during re-layouting.

Item instances set as property values were excluded from layouting, causing segfaults when re-layouting was performed or when the component got deleted due to or during re-layouting.

[Nekhelesh Ramananthan (nik90)]

I confirm that with this re-layouting works as expected without any crashes for the clock app.

review: Approve (testing)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:939
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/1734/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/3283
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/2950
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-amd64-ci/682
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-armhf-ci/682
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-armhf-ci/682/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/2889
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/3285
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/3285/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/2952
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/2952/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/5354
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4039

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-ui-toolkit-ci/1734/rebuild

review: Approve (continuous-integration)

[Florian Boucault (fboucault)]

looks great to me!

review: Approve

=== modified file 'modules/Ubuntu/Layouts/plugin/ullayouts.cpp'
--- modules/Ubuntu/Layouts/plugin/ullayouts.cpp	2013-06-26 11:06:39 +0000
+++ modules/Ubuntu/Layouts/plugin/ullayouts.cpp	2014-02-18 13:41:01 +0000
@@ -200,6 +200,11 @@
            .addChange(new PropertyChange(item, "enabled", activate));
 }
 
+// remove the deleted item from the excluded ones
+void ULLayoutsPrivate::_q_removeExcludedItem(QObject *excludedItem)
+{
+    excludedFromLayout.removeAll(static_cast<QQuickItem*>(excludedItem));
+}
 
 /*
  * Validates the declared conditional layouts by checking whether they have name
@@ -256,6 +261,12 @@
             // check if the item's  parent is included in the layout
             QQuickItem *pl = item->parentItem();
             marker = 0;
+            if (!pl && item->parent()) {
+                // this may be an item instance assigned to a property
+                // like "property var anItem: Item {}"
+                // in which case we must get the parent object of it, not the parent item
+                pl = qobject_cast<QQuickItem*>(item->parent());
+            }
             while (pl) {
                 marker = qobject_cast<ULLayoutsAttached*>(
                             qmlAttachedPropertiesObject<ULLayouts>(pl, false));
@@ -267,6 +278,9 @@
             if (!marker || (marker && marker->item().isEmpty())) {
                 // remember theese so we hide them once we switch away from default layout
                 excludedFromLayout << item;
+                // and make sure we remove the item from excluded ones in case the item is destroyed
+                QObject::connect(item, SIGNAL(destroyed(QObject*)),
+                                 q, SLOT(_q_removeExcludedItem(QObject*)));
             }
         }
     }
@@ -632,3 +646,5 @@
                                                  &ULLayoutsPrivate::at_layout,
                                                  &ULLayoutsPrivate::clear_layouts);
 }
+
+#include "moc_ullayouts.cpp"

=== modified file 'modules/Ubuntu/Layouts/plugin/ullayouts.h'
--- modules/Ubuntu/Layouts/plugin/ullayouts.h	2013-06-24 13:33:31 +0000
+++ modules/Ubuntu/Layouts/plugin/ullayouts.h	2014-02-18 13:41:01 +0000
@@ -77,6 +77,8 @@
     friend class ULConditionalLayout;
     Q_DECLARE_PRIVATE(ULLayouts)
     QScopedPointer<ULLayoutsPrivate> d_ptr;
+
+    Q_PRIVATE_SLOT(d_func(), void _q_removeExcludedItem(QObject*))
 };
 
 QML_DECLARE_TYPE(ULLayouts)

=== modified file 'modules/Ubuntu/Layouts/plugin/ullayouts_p.h'
--- modules/Ubuntu/Layouts/plugin/ullayouts_p.h	2013-06-25 13:42:22 +0000
+++ modules/Ubuntu/Layouts/plugin/ullayouts_p.h	2014-02-18 13:41:01 +0000
@@ -33,6 +33,7 @@
 
     ULLayoutsPrivate(ULLayouts *qq);
 
+    void _q_removeExcludedItem(QObject *excludedItem);
     void validateConditionalLayouts();
     void getLaidOutItems();
     void updateLayout();

=== added file 'tests/unit_x11/tst_layouts/DialerCrash.qml'
--- tests/unit_x11/tst_layouts/DialerCrash.qml	1970-01-01 00:00:00 +0000
+++ tests/unit_x11/tst_layouts/DialerCrash.qml	2014-02-18 13:41:01 +0000
@@ -0,0 +1,55 @@
+/*
+ * Copyright 2014 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.0
+import Ubuntu.Components 0.1
+import Ubuntu.Components.Pickers 0.1
+import Ubuntu.Layouts 0.1
+
+MainView {
+    width: units.gu(40)
+    height: units.gu(71)
+    backgroundColor: "#A55263"
+
+    property bool wide: true
+
+    Layouts {
+        objectName: "layoutManager"
+        layouts: [
+            ConditionalLayout {
+                name: "wideAspect"
+                when: wide
+
+                ItemLayout {
+                    item: "object1"
+                }
+            },
+
+            ConditionalLayout {
+                name: "regularAspect"
+                when: !wide
+
+                ItemLayout {
+                    item: "object2"
+                }
+            }
+        ]
+
+        Dialer {
+            Layouts.item: "object1"
+        }
+    }
+}

=== added file 'tests/unit_x11/tst_layouts/ExcludedItemDeleted.qml'
--- tests/unit_x11/tst_layouts/ExcludedItemDeleted.qml	1970-01-01 00:00:00 +0000
+++ tests/unit_x11/tst_layouts/ExcludedItemDeleted.qml	2014-02-18 13:41:01 +0000
@@ -0,0 +1,67 @@
+/*
+ * Copyright 2014 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.0
+import Ubuntu.Components 0.1
+import Ubuntu.Layouts 0.1
+
+Item {
+    property bool wide: true
+    width: units.gu(40)
+    height: units.gu(71)
+
+    function changeLayout() {
+        loader2.sourceComponent = undefined; wide = false
+    }
+
+    Layouts {
+        objectName: "layoutManager"
+        layouts: [
+            ConditionalLayout {
+                name: "wideAspect"
+                when: wide
+
+                ItemLayout {
+                    item: "object1"
+                }
+            },
+            ConditionalLayout {
+                name: "regularAspect"
+                when: !wide
+
+                ItemLayout {
+                    item: "object1"
+                    anchors.left: parent.left
+                }
+            }
+        ]
+
+        Column {
+            Loader {
+                Layouts.item: "object1"
+                id: loader
+                sourceComponent: ShaderEffectSource {
+                    sourceItem: Item {
+                    }
+                }
+            }
+            Loader {
+                id: loader2
+                sourceComponent: Item{}
+            }
+        }
+    }
+}

=== added file 'tests/unit_x11/tst_layouts/ItemInstanceAsProperty.qml'
--- tests/unit_x11/tst_layouts/ItemInstanceAsProperty.qml	1970-01-01 00:00:00 +0000
+++ tests/unit_x11/tst_layouts/ItemInstanceAsProperty.qml	2014-02-18 13:41:01 +0000
@@ -0,0 +1,63 @@
+/*
+ * Copyright 2014 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.0
+import Ubuntu.Components 0.1
+import Ubuntu.Layouts 0.1
+
+Item {
+    property bool wide: true
+    width: units.gu(40)
+    height: units.gu(71)
+
+    function changeLayout() {
+        loader.sourceComponent = undefined; wide = false
+    }
+
+    Layouts {
+        objectName: "layoutManager"
+        layouts: [
+            ConditionalLayout {
+                name: "wideAspect"
+                when: wide
+
+                ItemLayout {
+                    item: "object1"
+                }
+            },
+            ConditionalLayout {
+                name: "regularAspect"
+                when: !wide
+
+                ItemLayout {
+                    item: "object2"
+                    anchors.left: parent.left
+                }
+            }
+        ]
+
+        Item {
+            Layouts.item: "object1"
+            Loader {
+                id: loader
+                sourceComponent: ShaderEffectSource {
+                    sourceItem: Item {
+                    }
+                }
+            }
+        }
+    }
+}

=== modified file 'tests/unit_x11/tst_layouts/tst_layouts.cpp'
--- tests/unit_x11/tst_layouts/tst_layouts.cpp	2014-01-27 07:54:14 +0000
+++ tests/unit_x11/tst_layouts/tst_layouts.cpp	2014-02-18 13:41:01 +0000
@@ -897,6 +897,58 @@
         QCOMPARE(anchors->margins(), 20.0);
     }
 
+    // the following tests guard bug #1280359
+    void testCase_ItemInstanceAsProperty()
+    {
+        QScopedPointer<QQuickView> view(loadTest("ItemInstanceAsProperty.qml"));
+        QVERIFY(view);
+        QQuickItem *root = view->rootObject();
+        QVERIFY(root);
+
+        QQuickItem *layout = testItem(root, "layoutManager");
+        QVERIFY(layout);
+        QSignalSpy layoutChangeSpy(layout, SIGNAL(currentLayoutChanged()));
+
+        // invoke layout change
+        view->rootObject()->metaObject()->invokeMethod(view->rootObject(), "changeLayout");
+        layoutChangeSpy.wait();
+        QCOMPARE(layoutChangeSpy.count(), 1);
+    }
+
+    void testCase_DialerCrash()
+    {
+        QScopedPointer<QQuickView> view(loadTest("DialerCrash.qml"));
+        QVERIFY(view);
+        QQuickItem *root = view->rootObject();
+        QVERIFY(root);
+
+        QQuickItem *layout = testItem(root, "layoutManager");
+        QVERIFY(layout);
+        QSignalSpy layoutChangeSpy(layout, SIGNAL(currentLayoutChanged()));
+
+        // invoke layout change
+        view->rootObject()->setProperty("wide", false);
+        layoutChangeSpy.wait();
+        QCOMPARE(layoutChangeSpy.count(), 1);
+    }
+
+    void testCase_ExcludedItemDeleted()
+    {
+        QScopedPointer<QQuickView> view(loadTest("ExcludedItemDeleted.qml"));
+        QVERIFY(view);
+        QQuickItem *root = view->rootObject();
+        QVERIFY(root);
+
+        QQuickItem *layout = testItem(root, "layoutManager");
+        QVERIFY(layout);
+        QSignalSpy layoutChangeSpy(layout, SIGNAL(currentLayoutChanged()));
+
+        // invoke layout change
+        view->rootObject()->metaObject()->invokeMethod(view->rootObject(), "changeLayout");
+        layoutChangeSpy.wait();
+        QCOMPARE(layoutChangeSpy.count(), 1);
+    }
+
 };
 
 QTEST_MAIN(tst_Layouts)

=== modified file 'tests/unit_x11/tst_layouts/tst_layouts.pro'
--- tests/unit_x11/tst_layouts/tst_layouts.pro	2013-12-02 13:48:53 +0000
+++ tests/unit_x11/tst_layouts/tst_layouts.pro	2014-02-18 13:41:01 +0000
@@ -34,4 +34,7 @@
     AnchorRight.qml \
     AnchorTop.qml \
     AnchorBottom.qml \
-    AnchorAll.qml
+    AnchorAll.qml \
+    ItemInstanceAsProperty.qml \
+    DialerCrash.qml \
+    ExcludedItemDeleted.qml