Merge lp:~vthompson/music-app/fix-1515067 into lp:music-app

[Victor Thompson]

* Remove repeater for getting album titles in Artists.qml

* Remove repeater for getting album titles in Artists.qml

This makes the artistart queries the following: image://artistart/artist=Atmosphere&album=

There are 2 main issues/questions concerning this change:
1. Is there a benefit to providing the "album" in this query? Ie, will we get better results?
2. When I remove the album parameter, such that the query is "image://artistart/artist=Atmosphere", none of the artist thumbnails work. This seems to suggest to me that providing an album would be desired.

That being said, the results for providing a blank album seem to be the same as before--just fewer queries.

[Michi Henning (michihenning)]

Thanks for the quick response, Victor!

The changes look good to me. But I don't know that much about the backend semantics, so I added Facundo to the review. Facundo, could you let us know please? We are trying to avoid hitting the server twice for each album in order to get artist art.

What are the semantics of setting only the artist, but not the album?

If I set both, such as "Beatles/Revolver" for one request, and then "Beatles/Help!", is it possible that different artist art would be returned? But maybe the question is academic because the music app does not have the concept of "different artist art depending on the album"...

If the album is needed for disambiguation, no problem. Victor, in that case, I'd just modify the change to always send a single request that specifies both album and artist, and not send the request with the empty album.

The point is to avoid hitting the server twice when once is enough.

review: Needs Information

[Jenkins Bot (ubuntu-core-apps-jenkins-bot)]

PASSED: Continuous integration, rev:939
https://core-apps-jenkins.ubuntu.com/job/music-app-ci/8/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/141/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/music-app-ci/8/rebuild

review: Approve (continuous-integration)

[lp:~vthompson/music-app/fix-1515067]

940. By Victor Thompson on 2015-12-04

update changelog

[Jenkins Bot (ubuntu-core-apps-jenkins-bot)]

PASSED: Continuous integration, rev:940
https://core-apps-jenkins.ubuntu.com/job/music-app-ci/23/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/233/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/music-app-ci/23/rebuild

review: Approve (continuous-integration)

[Victor Thompson (vthompson)]

I've added Facundo Batista to the review.

[Michi Henning (michihenning)]

For what it's worth, the latest version of the thumbnailer (currently in silo 26) now errors on artist art if no artist is provided, and on album art if no album is provided.

When querying for artist art, leaving the album empty is fine (as it has always been). Similarly, when querying for album art, leaving the artist empty is fine, too. I don't know whether the results will be better with both fields present. (I would expect so, seeing that there are quite likely albums with the same title from different artists.)

[Andrew Hayzen (ahayzen)]

LGTM :-)

When testing this branch, the artist/album art was correctly created.

I noticed that the local music scope has fallback cover art for where we have art and empty items for where we have fallback, I wonder if this is known or a bug ?

As stated the other comments, it is unclear if specifying the album for artistart helps with accuracy.

For now, I'm going to approve as this reduces the number of calls, which is what it is trying to fix, and we can add sending the album later if required.

review: Approve

[Michi Henning (michihenning)]

I would strongly recommend to test with the thumbnailer in silo 26, if you haven't done that already.

[Victor Thompson (vthompson)]

Michi, can you confirm if that silo has landed in rc-proposed or not? I assume it has.

review: Needs Information

[Michi Henning (michihenning)]

Yes, that landed ages ago.

[lp:~vthompson/music-app/fix-1515067]

941. By Victor Thompson on 2016-01-30

Merge trunk and test.

942. By Victor Thompson on 2016-01-31

Fix changelog

[Jenkins Bot (ubuntu-core-apps-jenkins-bot)]

PASSED: Continuous integration, rev:941
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/220/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/511/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/220/rebuild

review: Approve (continuous-integration)

[Victor Thompson (vthompson)]

Testing against rc-proposed seems to work as intended.

[Jenkins Bot (ubuntu-core-apps-jenkins-bot)]

PASSED: Continuous integration, rev:942
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/221/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/512/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/221/rebuild

review: Approve (continuous-integration)

[Michi Henning (michihenning)]

Can't we land this? This fix seems to be doing the job.

=== modified file 'app/ui/Artists.qml'
--- app/ui/Artists.qml	2016-01-12 00:30:08 +0000
+++ app/ui/Artists.qml	2016-01-31 00:01:15 +0000
@@ -81,31 +81,11 @@
         }
         delegate: Card {
             id: artistCard
-            coverSources: [{art: "image://artistart/artist=" + model.artist + "&album=" + artistCard.album}]
+            coverSources: [{art: "image://artistart/artist=" + model.artist + "&album="}]
             objectName: "artistsPageGridItem" + index
             primaryText: model.artist != "" ? model.artist : i18n.tr("Unknown Artist")
             secondaryTextVisible: false
 
-            property string album: ""
-
-            AlbumsModel {
-                id: albumArtistModel
-                albumArtist: model.artist
-                store: musicStore
-            }
-
-            Repeater {
-                id: albumArtistModelRepeater
-                model: albumArtistModel
-                delegate: Item {
-                    property string album: model.title
-                }
-
-                onItemAdded: {
-                    artistCard.album = item.album
-                }
-            }
-
             onClicked: {
                 mainPageStack.push(Qt.resolvedUrl("ArtistView.qml"),
                                    {

=== modified file 'debian/changelog'
--- debian/changelog	2016-01-28 01:43:54 +0000
+++ debian/changelog	2016-01-31 00:01:15 +0000
@@ -2,6 +2,7 @@
 
   [ Victor Thompson ]
   * Release 2.3 and start 2.4
+  * Remove repeater for getting album titles in Artists.qml (LP: #1515067).
 
  -- Victor <victor@victor-virtual-machine>  Wed, 27 Jan 2016 19:40:55 -0600