Ubuntu Music App

Merge lp:~ahayzen/music-app/fix-1526274-use-layouts into lp:music-app

  1. fix-1526274-use-layouts
  2. Merge into trunk
Proposed by Andrew Hayzen on 2016-01-06
Status: Merged
Approved by: Nicholas Skaggs on 2016-01-11
Approved revision: 965
Merged at revision: 961
Proposed branch: lp:~ahayzen/music-app/fix-1526274-use-layouts
Merge into: lp:music-app
Diff against target: 344 lines (+79/-157)
6 files modified
app/components/Delegates/MusicListItem.qml (+52/-24)
app/components/MusicRow.qml (+0/-79)
app/components/Queue.qml (+10/-18)
app/ui/Songs.qml (+8/-18)
app/ui/SongsView.qml (+8/-18)
debian/changelog (+1/-0)
To merge this branch: bzr merge lp:~ahayzen/music-app/fix-1526274-use-layouts
Reviewer Review Type Date Requested Status
Jenkins Bot continuous-integration Approve on 2016-01-11
Victor Thompson 2016-01-06 Approve on 2016-01-09
Review via email: mp+281757@code.launchpad.net

Commit Message

* Use ListItemLayout for listitems to improve performance and match design guidelines

Description of the Change

* Use ListItemLayout for listitems to improve performance and match design guidelines

Running the analyse tool on this, this effectively removes what was done in the async loader before, it's hard to tell the exact gain. But it is around 5ms per delegate, which reduces the time by half from ~10ms to ~5ms.

You don't notice much of a gain on the mako as it was *just* hitting 60fps (needs to be less than 16ms), but on lower powered devices this will help.

To post a comment you must log in.
Andrew Hayzen (ahayzen) wrote :

adt-run [15:08:43]: test autopilot: - - - - - - - - - - results - - - - - - - - - -
autopilot PASS

:-)

Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
Victor Thompson (vthompson) wrote :

Please see inline comments.

review: Needs Fixing
Andrew Hayzen (ahayzen) wrote :

Fixed comments, please re-review :-)

1) Please fix the spelling of "noticeable"
That code was actually being removed?

2) Order alphabetically?
Obviously my coding OCD levels were weak :-)

Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Victor Thompson (vthompson) wrote :

lgtm! :)

review: Approve
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-autolanding/1/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-land-mp/1401/console

review: Needs Fixing (continuous-integration)
Victor Thompson (vthompson) wrote :

Re-approving to determine if AP failure is repeatable.

Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-autolanding/2/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-land-mp/1402/console

review: Needs Fixing (continuous-integration)
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-autolanding/13/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-land-mp/1413/console

review: Needs Fixing (continuous-integration)
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-autolanding/14/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-land-mp/1414/console

review: Needs Fixing (continuous-integration)
Nicholas Skaggs (nskaggs) wrote :

Turns out the human is at fault here. The autolanding job(s) were a bit misconfigured on a couple params.

Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-autolanding/15/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-land-mp/1415/console

review: Needs Fixing (continuous-integration)
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-autolanding/16/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-land-mp/1416/console

review: Needs Fixing (continuous-integration)
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'app/components/Delegates/MusicListItem.qml'
2--- app/components/Delegates/MusicListItem.qml 2015-10-29 23:02:17 +0000
3+++ app/components/Delegates/MusicListItem.qml 2016-01-09 12:50:05 +0000
4@@ -1,5 +1,5 @@
5 /*
6- * Copyright (C) 2013, 2014, 2015
7+ * Copyright (C) 2013, 2014, 2015, 2016
8 * Andrew Hayzen <ahayzen@gmail.com>
9 * Nekhelesh Ramananthan <krnekhelesh@gmail.com>
10 * Victor Thompson <victor.thompson@gmail.com>
11@@ -23,17 +23,20 @@
12
13 ListItem {
14 color: styleMusic.mainView.backgroundColor
15+ height: listItemLayout.height
16 highlightColor: Qt.lighter(color, 1.2)
17
18 // Store the currentColor so that actions can bind to it
19 property var currentColor: highlighted ? highlightColor : color
20
21- property alias column: musicRow.column
22- property alias imageSource: musicRow.imageSource
23+ property alias imageSource: image.imageSource
24
25 property bool multiselectable: false
26 property bool reorderable: false
27
28+ property alias subtitle: listItemLayout.subtitle
29+ property alias title: listItemLayout.title
30+
31 signal itemClicked()
32
33 onClicked: {
34@@ -58,27 +61,52 @@
35 visible: false
36 }
37
38- MusicRow {
39- id: musicRow
40- anchors {
41- fill: parent
42- // When not in selectMode we want a margin between the Image and the left edge
43- // when in selectMode the checkbox has its own margin so we don't want a double margin
44- leftMargin: selectMode ? 0 : units.gu(2)
45- rightMargin: selectMode ? 0 : units.gu(2)
46- }
47-
48- // Animate margin changes so it isn't noticible
49- Behavior on anchors.leftMargin {
50- NumberAnimation {
51-
52- }
53- }
54-
55- Behavior on anchors.rightMargin {
56- NumberAnimation {
57-
58- }
59+ ListItemLayout {
60+ id: listItemLayout
61+
62+ padding.bottom: image.visible ? units.gu(.5) : units.gu(1.5)
63+ padding.top: image.visible ? units.gu(.5) : units.gu(1.5)
64+
65+ subtitle.color: styleMusic.common.subtitle
66+ subtitle.fontSize: "x-small"
67+ subtitle.wrapMode: Text.WrapAnywhere
68+
69+ title.color: styleMusic.common.music
70+ title.fontSize: "small"
71+ title.wrapMode: Text.WrapAnywhere
72+
73+ Image {
74+ id: image
75+ anchors {
76+ verticalCenter: parent.verticalCenter
77+ }
78+ asynchronous: true
79+ fillMode: Image.PreserveAspectCrop
80+ height: width
81+ SlotsLayout.position: SlotsLayout.Leading
82+ source: {
83+ if (imageSource !== undefined && imageSource !== "") {
84+ if (imageSource.art !== undefined) {
85+ imageSource.art
86+ } else {
87+ "image://albumart/artist=" + imageSource.author + "&album=" + imageSource.album
88+ }
89+ } else {
90+ ""
91+ }
92+ }
93+ sourceSize.height: height
94+ sourceSize.width: width
95+ width: units.gu(6)
96+ visible: imageSource !== undefined
97+
98+ onStatusChanged: {
99+ if (status === Image.Error) {
100+ source = Qt.resolvedUrl("../graphics/music-app-cover@30.png")
101+ }
102+ }
103+
104+ property var imageSource
105 }
106 }
107 }
108
109=== removed file 'app/components/MusicRow.qml'
110--- app/components/MusicRow.qml 2015-10-18 18:16:05 +0000
111+++ app/components/MusicRow.qml 1970-01-01 00:00:00 +0000
112@@ -1,79 +0,0 @@
113-/*
114- * Copyright (C) 2013, 2014, 2015
115- * Andrew Hayzen <ahayzen@gmail.com>
116- * Nekhelesh Ramananthan <krnekhelesh@gmail.com>
117- * Victor Thompson <victor.thompson@gmail.com>
118- *
119- * This program is free software; you can redistribute it and/or modify
120- * it under the terms of the GNU General Public License as published by
121- * the Free Software Foundation; version 3.
122- *
123- * This program is distributed in the hope that it will be useful,
124- * but WITHOUT ANY WARRANTY; without even the implied warranty of
125- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
126- * GNU General Public License for more details.
127- *
128- * You should have received a copy of the GNU General Public License
129- * along with this program. If not, see <http://www.gnu.org/licenses/>.
130- */
131-
132-import QtQuick 2.4
133-import Ubuntu.Components 1.3
134-
135-
136-Row {
137- height: units.gu(7)
138-
139- property alias column: columnComponent.sourceComponent
140- property real coverSize: styleMusic.common.albumSize
141- property var imageSource
142-
143- spacing: units.gu(2)
144-
145- Image {
146- id: image
147- anchors {
148- verticalCenter: parent.verticalCenter
149- }
150- asynchronous: true
151- fillMode: Image.PreserveAspectCrop
152- height: width
153- source: imageSource !== undefined && imageSource !== ""
154- ? (imageSource.art !== undefined
155- ? imageSource.art
156- : "image://albumart/artist=" + imageSource.author + "&album=" + imageSource.album)
157- : ""
158- sourceSize.height: height
159- sourceSize.width: width
160- width: units.gu(6)
161-
162- onStatusChanged: {
163- if (status === Image.Error) {
164- source = Qt.resolvedUrl("../graphics/music-app-cover@30.png")
165- }
166- }
167- visible: imageSource !== undefined
168- }
169-
170- Loader {
171- id: columnComponent
172- anchors {
173- verticalCenter: parent.verticalCenter
174- }
175- width: imageSource === undefined ? parent.width - parent.spacing
176- : parent.width - image.width - parent.spacing
177-
178- onSourceComponentChanged: {
179- for (var i=0; i < item.children.length; i++) {
180- item.children[i].elide = Text.ElideRight
181- item.children[i].height = units.gu(2)
182- item.children[i].maximumLineCount = 1
183- item.children[i].wrapMode = Text.NoWrap
184- item.children[i].verticalAlignment = Text.AlignVCenter
185-
186- // binds to width so it is updated when screen size changes
187- item.children[i].width = Qt.binding(function () { return width; })
188- }
189- }
190- }
191-}
192
193=== modified file 'app/components/Queue.qml'
194--- app/components/Queue.qml 2015-10-18 18:16:05 +0000
195+++ app/components/Queue.qml 2016-01-09 12:50:05 +0000
196@@ -1,5 +1,5 @@
197 /*
198- * Copyright (C) 2013, 2014, 2015
199+ * Copyright (C) 2013, 2014, 2015, 2016
200 * Andrew Hayzen <ahayzen@gmail.com>
201 * Daniel Holm <d.holmen@gmail.com>
202 * Victor Thompson <victor.thompson@gmail.com>
203@@ -42,23 +42,6 @@
204 delegate: MusicListItem {
205 id: queueListItem
206 color: player.currentIndex === index ? "#2c2c34" : styleMusic.mainView.backgroundColor
207- column: Column {
208- Label {
209- id: trackTitle
210- color: player.currentIndex === index ? UbuntuColors.blue : styleMusic.common.music
211- fontSize: "small"
212- objectName: "titleLabel"
213- text: model.title
214- }
215-
216- Label {
217- id: trackArtist
218- color: styleMusic.common.subtitle
219- fontSize: "x-small"
220- objectName: "artistLabel"
221- text: model.author
222- }
223- }
224 leadingActions: ListItemActions {
225 actions: [
226 Remove {
227@@ -69,6 +52,15 @@
228 multiselectable: true
229 objectName: "nowPlayingListItem" + index
230 reorderable: true
231+ subtitle {
232+ objectName: "artistLabel"
233+ text: model.author
234+ }
235+ title {
236+ color: player.currentIndex === index ? UbuntuColors.blue : styleMusic.common.music
237+ objectName: "titleLabel"
238+ text: model.title
239+ }
240 trailingActions: ListItemActions {
241 actions: [
242 AddToPlaylist {
243
244=== modified file 'app/ui/Songs.qml'
245--- app/ui/Songs.qml 2015-10-18 18:16:05 +0000
246+++ app/ui/Songs.qml 2016-01-09 12:50:05 +0000
247@@ -1,5 +1,5 @@
248 /*
249- * Copyright (C) 2013, 2014, 2015
250+ * Copyright (C) 2013, 2014, 2015, 2016
251 * Andrew Hayzen <ahayzen@gmail.com>
252 * Daniel Holm <d.holmen@gmail.com>
253 * Victor Thompson <victor.thompson@gmail.com>
254@@ -93,25 +93,15 @@
255 delegate: MusicListItem {
256 id: track
257 objectName: "tracksPageListItem" + index
258- column: Column {
259- Label {
260- id: trackTitle
261- color: styleMusic.common.music
262- fontSize: "small"
263- objectName: "tracktitle"
264- text: model.title
265- }
266-
267- Label {
268- id: trackArtist
269- color: styleMusic.common.subtitle
270- fontSize: "x-small"
271- text: model.author
272- }
273- }
274- height: units.gu(7)
275 imageSource: {"art": model.art}
276 multiselectable: true
277+ subtitle {
278+ text: model.author
279+ }
280+ title {
281+ objectName: "tracktitle"
282+ text: model.title
283+ }
284 trailingActions: AddToQueueAndPlaylist {
285 }
286
287
288=== modified file 'app/ui/SongsView.qml'
289--- app/ui/SongsView.qml 2015-10-28 01:05:33 +0000
290+++ app/ui/SongsView.qml 2016-01-09 12:50:05 +0000
291@@ -1,5 +1,5 @@
292 /*
293- * Copyright (C) 2013, 2014, 2015
294+ * Copyright (C) 2013, 2014, 2015, 2016
295 * Andrew Hayzen <ahayzen@gmail.com>
296 * Daniel Holm <d.holmen@gmail.com>
297 * Victor Thompson <victor.thompson@gmail.com>
298@@ -297,27 +297,17 @@
299 delegate: MusicListItem {
300 id: track
301 objectName: "songsPageListItem" + index
302- column: Column {
303- Label {
304- id: trackTitle
305- color: styleMusic.common.music
306- fontSize: "small"
307- objectName: "songspage-tracktitle"
308- text: model.title
309- }
310-
311- Label {
312- id: trackArtist
313- color: styleMusic.common.subtitle
314- fontSize: "x-small"
315- text: model.author
316- }
317- }
318- height: units.gu(6)
319 leadingActions: songStackPage.line1 === i18n.tr("Playlist")
320 ? playlistRemoveAction.item : null
321 multiselectable: true
322 reorderable: songStackPage.line1 === i18n.tr("Playlist")
323+ subtitle {
324+ text: model.author
325+ }
326+ title {
327+ objectName: "songspage-tracktitle"
328+ text: model.title
329+ }
330 trailingActions: AddToQueueAndPlaylist {
331 }
332
333
334=== modified file 'debian/changelog'
335--- debian/changelog 2015-12-23 02:07:46 +0000
336+++ debian/changelog 2016-01-09 12:50:05 +0000
337@@ -2,6 +2,7 @@
338
339 [ Andrew Hayzen ]
340 * Release 2.2ubuntu2 and start on 2.3
341+ * Use ListItemLayout for listitems to improve performance and match design guidelines (LP: #1526247).
342
343 [ Ken VanDine ]
344 * Install the content-hub json file in the correct place for peer registry

Subscribers

People subscribed via source and target branches