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

Proposed by Andrea Bernabei
Status: Merged
Approved by: Zsombor Egri
Approved revision: 1798
Merged at revision: 1800
Proposed branch: lp:~faenil/ubuntu-ui-toolkit/listitemlayout_fixes_and_docs
Merge into: lp:ubuntu-ui-toolkit/staging
Diff against target: 289 lines (+140/-14)
4 files modified
src/Ubuntu/Components/plugin/privates/threelabelsslot_p.cpp (+10/-7)
src/Ubuntu/Components/plugin/privates/threelabelsslot_p.h (+3/-1)
src/Ubuntu/Components/plugin/uclistitemlayout.cpp (+80/-4)
tests/unit_x11/tst_components/tst_slotslayout.qml (+47/-2)
To merge this branch: bzr merge lp:~faenil/ubuntu-ui-toolkit/listitemlayout_fixes_and_docs
Reviewer Review Type Date Requested Status
Zsombor Egri Approve
PS Jenkins bot continuous-integration Approve
Review via email: mp+281919@code.launchpad.net

Commit message

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

Visual changes reviewed by Jouni Helminen

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Zsombor Egri (zsombi) wrote :

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

review: Needs Information
Revision history for this message
Zsombor Egri (zsombi) wrote :

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
Revision history for this message
Andrea Bernabei (faenil) wrote :

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

Revision history for this message
Andrea Bernabei (faenil) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/Ubuntu/Components/plugin/privates/threelabelsslot_p.cpp'
2--- src/Ubuntu/Components/plugin/privates/threelabelsslot_p.cpp 2015-11-08 21:39:34 +0000
3+++ src/Ubuntu/Components/plugin/privates/threelabelsslot_p.cpp 2016-01-07 19:16:43 +0000
4@@ -38,7 +38,10 @@
5 void UCThreeLabelsSlotPrivate::setTitleProperties()
6 {
7 if (m_title != Q_NULLPTR) {
8- m_title->setWrapMode(UCLabel::WordWrap);
9+ //Using WrapAnywhere because ElideRight elides too early when used
10+ //together with WrapWord, and that produces an unexpected result.
11+ //This will cover most of the usecases.
12+ m_title->setWrapMode(UCLabel::WrapAnywhere);
13 m_title->setElideMode(UCLabel::ElideRight);
14 m_title->setMaximumLineCount(1);
15 m_title->setTextSize(UCLabel::Medium);
16@@ -48,7 +51,7 @@
17 void UCThreeLabelsSlotPrivate::setSubtitleProperties()
18 {
19 if (m_subtitle != Q_NULLPTR) {
20- m_subtitle->setWrapMode(UCLabel::WordWrap);
21+ m_subtitle->setWrapMode(UCLabel::WrapAnywhere);
22 m_subtitle->setElideMode(UCLabel::ElideRight);
23 m_subtitle->setMaximumLineCount(1);
24 m_subtitle->setTextSize(UCLabel::Small);
25@@ -58,7 +61,7 @@
26 void UCThreeLabelsSlotPrivate::setSummaryProperties()
27 {
28 if (m_summary != Q_NULLPTR) {
29- m_summary->setWrapMode(UCLabel::WordWrap);
30+ m_summary->setWrapMode(UCLabel::WrapAnywhere);
31 m_summary->setElideMode(UCLabel::ElideRight);
32 m_summary->setMaximumLineCount(2);
33 m_summary->setTextSize(UCLabel::Small);
34@@ -99,19 +102,19 @@
35 QQuickAnchors *subtitleAnchors = QQuickItemPrivate::get(m_subtitle)->anchors();
36 subtitleAnchors->setTop(skipTitle
37 ? top()
38- : QQuickItemPrivate::get(m_title)->baseline());
39+ : QQuickItemPrivate::get(m_title)->bottom());
40 subtitleAnchors->setTopMargin(skipTitle
41 ? 0
42- : UCUnits::instance().dp(LABELSBLOCK_SPACING_DP));
43+ : UCUnits::instance().dp(TITLE_SPACING_DP));
44 }
45
46 if (!skipSummary) {
47 QQuickAnchors *summaryAnchors = QQuickItemPrivate::get(m_summary)->anchors();
48 summaryAnchors->setTop(skipSubtitle
49- ? (skipTitle ? top() : QQuickItemPrivate::get(m_title)->baseline())
50+ ? (skipTitle ? top() : QQuickItemPrivate::get(m_title)->bottom())
51 : QQuickItemPrivate::get(m_subtitle)->bottom());
52 summaryAnchors->setTopMargin(skipSubtitle
53- ? (skipTitle ? 0 : UCUnits::instance().dp(LABELSBLOCK_SPACING_DP))
54+ ? (skipTitle ? 0 : UCUnits::instance().dp(TITLE_SPACING_DP))
55 : 0);
56 }
57 //Update height of the labels box
58
59=== modified file 'src/Ubuntu/Components/plugin/privates/threelabelsslot_p.h'
60--- src/Ubuntu/Components/plugin/privates/threelabelsslot_p.h 2015-10-01 08:47:31 +0000
61+++ src/Ubuntu/Components/plugin/privates/threelabelsslot_p.h 2016-01-07 19:16:43 +0000
62@@ -20,7 +20,9 @@
63 #include <QQuickItem>
64 #include <private/qquickitem_p.h>
65
66-#define LABELSBLOCK_SPACING_DP 4
67+//The padding between title and the string below it (i.e. subtitle, or
68+//summary, when subtitle is empty)
69+#define TITLE_SPACING_DP 2
70
71 class UCLabel;
72 class UCThreeLabelsSlotPrivate;
73
74=== modified file 'src/Ubuntu/Components/plugin/uclistitemlayout.cpp'
75--- src/Ubuntu/Components/plugin/uclistitemlayout.cpp 2015-11-10 11:39:51 +0000
76+++ src/Ubuntu/Components/plugin/uclistitemlayout.cpp 2016-01-07 19:16:43 +0000
77@@ -27,6 +27,7 @@
78 \qmltype ListItemLayout
79 \instantiates UCListItemLayout
80 \inqmlmodule Ubuntu.Components 1.3
81+ \since Ubuntu.Components 1.3
82 \inherits SlotsLayout
83 \ingroup ubuntu
84
85@@ -74,7 +75,7 @@
86 \l ListItem and ListItemLayout:
87 \qml
88 ListItem {
89- height: layout.height + divider.height
90+ height: layout.height + (divider.visible ? divider.height : 0)
91 ListItemLayout {
92 id: layout
93 title.text: "Hello developers!"
94@@ -103,7 +104,7 @@
95 \qml
96 ListItem {
97 id: listItem
98- height: layout.height + divider.height
99+ height: layout.height + (divider.visible ? divider.height : 0)
100
101 ListItemLayout {
102 id: layout
103@@ -155,8 +156,8 @@
104 The \l title is positioned at the top, followed by \l subtitle and \l summary,
105 respectively.
106
107- The \l subtitle has its top anchored to \l {title}'s baseline, with a margin of
108- 4 DPs.
109+ The \l subtitle has its top anchored to \l {title}'s bottom, with a margin of
110+ 2 DPs.
111
112 The \l summary has its top tightly anchored to \l {subtitle}'s bottom.
113
114@@ -208,6 +209,81 @@
115 }
116 \endqml
117
118+ \section1 About aliasing labels properties
119+ Due to the way ListItemsLayout's labels are created (see
120+ \l {Optimizing memory consumption}) it is not possible to
121+ directly alias their properties. It is still possible, however,
122+ to expose an API that gives indirect read-write access to those
123+ properties.
124+ The following code:
125+
126+ \qml
127+ ListItem {
128+ property alias titleText: layout.title.text
129+ ListItemLayout {
130+ id: layout
131+ }
132+ }
133+ \endqml
134+
135+ will return the error "Invalid alias location", because
136+ the title object does not yet exist at time when the alias
137+ is created.
138+
139+ If you need to expose an API for your component that allows
140+ changing the properties of the labels, we recommend aliasing
141+ the labels themselves. Let's assume you want to create a
142+ QML component to use as a delegate of many
143+ list views inside your application: you will probably have a qml
144+ file holding the definition of the that delegate, and the content
145+ of that file will be similar to:
146+ \qml
147+ //Content of CustomListItem.qml
148+ import QtQuick 2.4
149+ import Ubuntu.Components 1.3
150+
151+ ListItem {
152+ id: listitem
153+ property alias title: layout.title
154+ property alias iconName: icon.name
155+
156+ height: layout.height + (divider.visible ? divider.height : 0)
157+
158+ ListItemLayout {
159+ id: layout
160+ Icon {
161+ id: icon
162+ width: units.gu(2)
163+ }
164+ }
165+ }
166+ \endqml
167+
168+ As you can see, we alias the label item itself instead of its
169+ properties. This also has the advantage of only exposing one alias
170+ instead of one for each property, thus making your QML app a bit more performant.
171+ Once your delegate is defined, you can use it in your ListViews like usual.
172+ \qml
173+ //other UI code...
174+
175+ ListView {
176+ anchors.fill: parent
177+ model: ListModel {
178+ id: listViewModel
179+ ListElement { titleText: "Hello1"; icon: "message" }
180+ ListElement { titleText: "Hello2"; icon: "email" }
181+ ListElement { titleText: "Hello3"; icon: "email" }
182+ ListElement { titleText: "Hello4"; icon: "message" }
183+ }
184+ delegate: CustomListItem {
185+ title.text: model.titleText
186+ iconName: model.icon
187+ }
188+ }
189+ \endqml
190+
191+ Note how title's properties are all accessible via the "title" identifier.
192+
193 */
194 UCListItemLayout::UCListItemLayout(QQuickItem *parent)
195 : UCSlotsLayout(parent)
196
197=== modified file 'tests/unit_x11/tst_components/tst_slotslayout.qml'
198--- tests/unit_x11/tst_components/tst_slotslayout.qml 2015-11-09 16:20:34 +0000
199+++ tests/unit_x11/tst_components/tst_slotslayout.qml 2016-01-07 19:16:43 +0000
200@@ -66,6 +66,17 @@
201 readonly property var trailingSlots: []
202 }
203 ListItemLayout {
204+ id: layoutMultilineLabels
205+ title.text: "test \n test"
206+ title.maximumLineCount: 2
207+ subtitle.text: "test2 \n test2 \n test2"
208+ subtitle.maximumLineCount: 3
209+ summary.text: "test3 \n test3"
210+ summary.maximumLineCount: 2
211+ readonly property var leadingSlots: []
212+ readonly property var trailingSlots: []
213+ }
214+ ListItemLayout {
215 id: layoutOneLeading
216 readonly property var leadingSlots: [layoutOneLeading_leading1]
217 readonly property var trailingSlots: []
218@@ -375,6 +386,17 @@
219 compare(item.padding.trailing, units.gu(1), "Default trailing padding")
220 }
221
222+ function checkDefaultWrapping(item) {
223+ //we have to use WrapAnywhere because otherwise it will have unexpected behaviour
224+ //when used together with ElideRight when the string wraps (it will elide too early).
225+ compare(item.title.wrapMode, Label.WrapAnywhere, "Wrong default title wrapMode")
226+ compare(item.subtitle.wrapMode, Label.WrapAnywhere, "Wrong default subtitle wrapMode")
227+ compare(item.summary.wrapMode, Label.WrapAnywhere, "Wrong default summary wrapMode")
228+ compare(item.title.elide, Label.ElideRight, "Wrong default title elide")
229+ compare(item.subtitle.elide, Label.ElideRight, "Wrong default subtitle elide")
230+ compare(item.summary.elide, Label.ElideRight, "Wrong default summary elide")
231+ }
232+
233 function checkImplicitSize(item) {
234 compare(item.implicitHeight, expectedImplicitHeight(item), "Implicit height check")
235 compare(item.implicitWidth, column.width, "Fill parent's width")
236@@ -718,7 +740,7 @@
237 "Default labels positioning, subtitle's y")
238 } else {
239 compare(listitemlayout.subtitle.y,
240- listitemlayout.title.baselineOffset + units.dp(4),
241+ listitemlayout.title.y + listitemlayout.title.height + units.dp(2),
242 "Default labels positioning, subtitle's y")
243 }
244 }
245@@ -734,7 +756,7 @@
246 compare(listitemlayout.summary.y, 0,
247 "Default labels positioning, summary's y")
248 } else {
249- compare(listitemlayout.summary.y, listitemlayout.title.baselineOffset + units.dp(4),
250+ compare(listitemlayout.summary.y, listitemlayout.title.y + listitemlayout.title.height + units.dp(2),
251 "Default labels positioning, summary's y")
252 }
253 } else {
254@@ -744,6 +766,10 @@
255 }
256 }
257
258+ function test_defaultLabelsWrappingAndElide() {
259+ checkDefaultWrapping(layoutLabels)
260+ }
261+
262 function test_defaultMainSlotHeight() {
263 var titleText = layoutLabels.title.text
264 var subtitleText = layoutLabels.subtitle.text
265@@ -873,5 +899,24 @@
266 compare(obj !== null, true, "QML ListItemLayout: testing labels' QML context.")
267 obj.destroy()
268 }
269+
270+ //first version of ListItemLayout anchored subtitle to title's baseline, but that breaks
271+ //when title is multiline
272+ function test_multilineLabelsPositioning() {
273+ compare(layoutMultilineLabels.title.maximumLineCount, 2,
274+ "Multiline labels positioning: wrong title maximumLineCount")
275+ compare(layoutMultilineLabels.subtitle.maximumLineCount, 3,
276+ "Multiline labels positioning: wrong subtitle maximumLineCount")
277+ compare(layoutMultilineLabels.summary.maximumLineCount, 2,
278+ "Multiline labels positioning: wrong summary maximumLineCount")
279+
280+ compare(layoutMultilineLabels.title.lineCount, 2,
281+ "Multiline labels positioning: wrong title lineCount")
282+ compare(layoutMultilineLabels.subtitle.lineCount, 3,
283+ "Multiline labels positioning: wrong subtitle lineCount")
284+ compare(layoutMultilineLabels.summary.lineCount, 2,
285+ "Multiline labels positioning: wrong summary lineCount")
286+ checkLabelsY(layoutMultilineLabels)
287+ }
288 }
289 }

Subscribers

People subscribed via source and target branches