Merge lp:~faenil/ubuntu-ui-toolkit/listitemlayout_fixes_and_docs into lp:ubuntu-ui-toolkit/staging

[Andrea Bernabei]

Fix ListItemLayout multiline labels positioning and add docs about aliasing labels properties

Visual changes reviewed by Jouni Helminen

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1798
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2703/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/1426
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/1429
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/1429/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/1425

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

review: Approve (continuous-integration)

[Zsombor Egri (zsombi)]

Why the default top margin for the title was changed? Some design requirement?

review: Needs Information

[Zsombor Egri (zsombi)]

Ok, code looks nice, docs as well, design change should have been written in the description so reviewer knows what additional change is added beside the fix. Next time please do a separate MR for that. It doesn't belong to this fix logically.

review: Approve

[Andrea Bernabei (faenil)]

The change in title margin is a consequence of changing the labels anchors.

The change in title margin was suggested and reviewed by Jouni Helminen (Visual Designer) on 8th Jan

[Andrea Bernabei (faenil)]

> Why the default top margin for the title was changed? Some design requirement?

it is a consequence of the anchor changes. Without that, the visual would look different. So we're actually trying to keep the same visual as before here, not changing it ;)

=== modified file 'src/Ubuntu/Components/plugin/privates/threelabelsslot_p.cpp'
--- src/Ubuntu/Components/plugin/privates/threelabelsslot_p.cpp	2015-11-08 21:39:34 +0000
+++ src/Ubuntu/Components/plugin/privates/threelabelsslot_p.cpp	2016-01-07 19:16:43 +0000
@@ -38,7 +38,10 @@
 void UCThreeLabelsSlotPrivate::setTitleProperties()
 {
     if (m_title != Q_NULLPTR) {
-        m_title->setWrapMode(UCLabel::WordWrap);
+        //Using WrapAnywhere because ElideRight elides too early when used
+        //together with WrapWord, and that produces an unexpected result.
+        //This will cover most of the usecases.
+        m_title->setWrapMode(UCLabel::WrapAnywhere);
         m_title->setElideMode(UCLabel::ElideRight);
         m_title->setMaximumLineCount(1);
         m_title->setTextSize(UCLabel::Medium);
@@ -48,7 +51,7 @@
 void UCThreeLabelsSlotPrivate::setSubtitleProperties()
 {
     if (m_subtitle != Q_NULLPTR) {
-        m_subtitle->setWrapMode(UCLabel::WordWrap);
+        m_subtitle->setWrapMode(UCLabel::WrapAnywhere);
         m_subtitle->setElideMode(UCLabel::ElideRight);
         m_subtitle->setMaximumLineCount(1);
         m_subtitle->setTextSize(UCLabel::Small);
@@ -58,7 +61,7 @@
 void UCThreeLabelsSlotPrivate::setSummaryProperties()
 {
     if (m_summary != Q_NULLPTR) {
-        m_summary->setWrapMode(UCLabel::WordWrap);
+        m_summary->setWrapMode(UCLabel::WrapAnywhere);
         m_summary->setElideMode(UCLabel::ElideRight);
         m_summary->setMaximumLineCount(2);
         m_summary->setTextSize(UCLabel::Small);
@@ -99,19 +102,19 @@
         QQuickAnchors *subtitleAnchors = QQuickItemPrivate::get(m_subtitle)->anchors();
         subtitleAnchors->setTop(skipTitle
                                 ? top()
-                                : QQuickItemPrivate::get(m_title)->baseline());
+                                : QQuickItemPrivate::get(m_title)->bottom());
         subtitleAnchors->setTopMargin(skipTitle
                                       ? 0
-                                      : UCUnits::instance().dp(LABELSBLOCK_SPACING_DP));
+                                      : UCUnits::instance().dp(TITLE_SPACING_DP));
     }
 
     if (!skipSummary) {
         QQuickAnchors *summaryAnchors = QQuickItemPrivate::get(m_summary)->anchors();
         summaryAnchors->setTop(skipSubtitle
-                               ? (skipTitle ? top() : QQuickItemPrivate::get(m_title)->baseline())
+                               ? (skipTitle ? top() : QQuickItemPrivate::get(m_title)->bottom())
                                : QQuickItemPrivate::get(m_subtitle)->bottom());
         summaryAnchors->setTopMargin(skipSubtitle
-                                     ? (skipTitle ? 0 : UCUnits::instance().dp(LABELSBLOCK_SPACING_DP))
+                                     ? (skipTitle ? 0 : UCUnits::instance().dp(TITLE_SPACING_DP))
                                      : 0);
     }
     //Update height of the labels box

=== modified file 'src/Ubuntu/Components/plugin/privates/threelabelsslot_p.h'
--- src/Ubuntu/Components/plugin/privates/threelabelsslot_p.h	2015-10-01 08:47:31 +0000
+++ src/Ubuntu/Components/plugin/privates/threelabelsslot_p.h	2016-01-07 19:16:43 +0000
@@ -20,7 +20,9 @@
 #include <QQuickItem>
 #include <private/qquickitem_p.h>
 
-#define LABELSBLOCK_SPACING_DP                       4
+//The padding between title and the string below it (i.e. subtitle, or
+//summary, when subtitle is empty)
+#define TITLE_SPACING_DP                       2
 
 class UCLabel;
 class UCThreeLabelsSlotPrivate;

=== modified file 'src/Ubuntu/Components/plugin/uclistitemlayout.cpp'
--- src/Ubuntu/Components/plugin/uclistitemlayout.cpp	2015-11-10 11:39:51 +0000
+++ src/Ubuntu/Components/plugin/uclistitemlayout.cpp	2016-01-07 19:16:43 +0000
@@ -27,6 +27,7 @@
     \qmltype ListItemLayout
     \instantiates UCListItemLayout
     \inqmlmodule Ubuntu.Components 1.3
+    \since Ubuntu.Components 1.3
     \inherits SlotsLayout
     \ingroup ubuntu
 
@@ -74,7 +75,7 @@
     \l ListItem and ListItemLayout:
     \qml
     ListItem {
-        height: layout.height + divider.height
+        height: layout.height + (divider.visible ? divider.height : 0)
         ListItemLayout {
             id: layout
             title.text: "Hello developers!"
@@ -103,7 +104,7 @@
     \qml
     ListItem {
         id: listItem
-        height: layout.height + divider.height
+        height: layout.height + (divider.visible ? divider.height : 0)
 
         ListItemLayout {
             id: layout
@@ -155,8 +156,8 @@
     The \l title is positioned at the top, followed by \l subtitle and \l summary,
     respectively.
 
-    The \l subtitle has its top anchored to \l {title}'s baseline, with a margin of
-    4 DPs.
+    The \l subtitle has its top anchored to \l {title}'s bottom, with a margin of
+    2 DPs.
 
     The \l summary has its top tightly anchored to \l {subtitle}'s bottom.
 
@@ -208,6 +209,81 @@
     }
     \endqml
 
+    \section1 About aliasing labels properties
+    Due to the way ListItemsLayout's labels are created (see
+    \l {Optimizing memory consumption}) it is not possible to
+    directly alias their properties. It is still possible, however,
+    to expose an API that gives indirect read-write access to those
+    properties.
+    The following code:
+
+    \qml
+    ListItem {
+        property alias titleText: layout.title.text
+        ListItemLayout {
+            id: layout
+        }
+    }
+    \endqml
+
+    will return the error "Invalid alias location", because
+    the title object does not yet exist at time when the alias
+    is created.
+
+    If you need to expose an API for your component that allows
+    changing the properties of the labels, we recommend aliasing
+    the labels themselves. Let's assume you want to create a
+    QML component to use as a delegate of many
+    list views inside your application: you will probably have a qml
+    file holding the definition of the that delegate, and the content
+    of that file will be similar to:
+    \qml
+    //Content of CustomListItem.qml
+    import QtQuick 2.4
+    import Ubuntu.Components 1.3
+
+    ListItem {
+        id: listitem
+        property alias title: layout.title
+        property alias iconName: icon.name
+
+        height: layout.height + (divider.visible ? divider.height : 0)
+
+        ListItemLayout {
+            id: layout
+            Icon {
+                id: icon
+                width: units.gu(2)
+            }
+        }
+    }
+    \endqml
+
+    As you can see, we alias the label item itself instead of its
+    properties. This also has the advantage of only exposing one alias
+    instead of one for each property, thus making your QML app a bit more performant.
+    Once your delegate is defined, you can use it in your ListViews like usual.
+    \qml
+    //other UI code...
+
+    ListView {
+        anchors.fill: parent
+        model: ListModel {
+            id: listViewModel
+            ListElement { titleText: "Hello1"; icon: "message" }
+            ListElement { titleText: "Hello2"; icon: "email" }
+            ListElement { titleText: "Hello3"; icon: "email" }
+            ListElement { titleText: "Hello4"; icon: "message" }
+        }
+        delegate: CustomListItem {
+            title.text: model.titleText
+            iconName: model.icon
+        }
+    }
+    \endqml
+
+    Note how title's properties are all accessible via the "title" identifier.
+
 */
 UCListItemLayout::UCListItemLayout(QQuickItem *parent)
     : UCSlotsLayout(parent)

=== modified file 'tests/unit_x11/tst_components/tst_slotslayout.qml'
--- tests/unit_x11/tst_components/tst_slotslayout.qml	2015-11-09 16:20:34 +0000
+++ tests/unit_x11/tst_components/tst_slotslayout.qml	2016-01-07 19:16:43 +0000
@@ -66,6 +66,17 @@
             readonly property var trailingSlots: []
         }
         ListItemLayout {
+            id: layoutMultilineLabels
+            title.text: "test \n test"
+            title.maximumLineCount: 2
+            subtitle.text: "test2 \n test2 \n test2"
+            subtitle.maximumLineCount: 3
+            summary.text: "test3 \n test3"
+            summary.maximumLineCount: 2
+            readonly property var leadingSlots: []
+            readonly property var trailingSlots: []
+        }
+        ListItemLayout {
             id: layoutOneLeading
             readonly property var leadingSlots: [layoutOneLeading_leading1]
             readonly property var trailingSlots: []
@@ -375,6 +386,17 @@
             compare(item.padding.trailing, units.gu(1), "Default trailing padding")
         }
 
+        function checkDefaultWrapping(item) {
+            //we have to use WrapAnywhere because otherwise it will have unexpected behaviour
+            //when used together with ElideRight when the string wraps (it will elide too early).
+            compare(item.title.wrapMode, Label.WrapAnywhere, "Wrong default title wrapMode")
+            compare(item.subtitle.wrapMode, Label.WrapAnywhere, "Wrong default subtitle wrapMode")
+            compare(item.summary.wrapMode, Label.WrapAnywhere, "Wrong default summary wrapMode")
+            compare(item.title.elide, Label.ElideRight, "Wrong default title elide")
+            compare(item.subtitle.elide, Label.ElideRight, "Wrong default subtitle elide")
+            compare(item.summary.elide, Label.ElideRight, "Wrong default summary elide")
+        }
+
         function checkImplicitSize(item) {
             compare(item.implicitHeight, expectedImplicitHeight(item), "Implicit height check")
             compare(item.implicitWidth, column.width, "Fill parent's width")
@@ -718,7 +740,7 @@
                             "Default labels positioning, subtitle's y")
                 } else {
                     compare(listitemlayout.subtitle.y,
-                            listitemlayout.title.baselineOffset + units.dp(4),
+                            listitemlayout.title.y + listitemlayout.title.height + units.dp(2),
                             "Default labels positioning, subtitle's y")
                 }
             }
@@ -734,7 +756,7 @@
                         compare(listitemlayout.summary.y, 0,
                                 "Default labels positioning, summary's y")
                     } else {
-                        compare(listitemlayout.summary.y, listitemlayout.title.baselineOffset + units.dp(4),
+                        compare(listitemlayout.summary.y, listitemlayout.title.y + listitemlayout.title.height + units.dp(2),
                                 "Default labels positioning, summary's y")
                     }
                 } else  {
@@ -744,6 +766,10 @@
             }
         }
 
+        function test_defaultLabelsWrappingAndElide() {
+            checkDefaultWrapping(layoutLabels)
+        }
+
         function test_defaultMainSlotHeight() {
             var titleText = layoutLabels.title.text
             var subtitleText = layoutLabels.subtitle.text
@@ -873,5 +899,24 @@
             compare(obj !== null, true, "QML ListItemLayout: testing labels' QML context.")
             obj.destroy()
         }
+
+        //first version of ListItemLayout anchored subtitle to title's baseline, but that breaks
+        //when title is multiline
+        function test_multilineLabelsPositioning() {
+            compare(layoutMultilineLabels.title.maximumLineCount, 2,
+                    "Multiline labels positioning: wrong title maximumLineCount")
+            compare(layoutMultilineLabels.subtitle.maximumLineCount, 3,
+                    "Multiline labels positioning: wrong subtitle maximumLineCount")
+            compare(layoutMultilineLabels.summary.maximumLineCount, 2,
+                    "Multiline labels positioning: wrong summary maximumLineCount")
+
+            compare(layoutMultilineLabels.title.lineCount, 2,
+                    "Multiline labels positioning: wrong title lineCount")
+            compare(layoutMultilineLabels.subtitle.lineCount, 3,
+                    "Multiline labels positioning: wrong subtitle lineCount")
+            compare(layoutMultilineLabels.summary.lineCount, 2,
+                    "Multiline labels positioning: wrong summary lineCount")
+            checkLabelsY(layoutMultilineLabels)
+        }
     }
 }