Merge lp:~jamesh/ubuntu-ui-toolkit/albumart-dbus into lp:ubuntu-ui-toolkit

Proposed by James Henstridge on 2014-03-24
Status: Rejected
Rejected by: Tim Peeters on 2014-03-27
Proposed branch: lp:~jamesh/ubuntu-ui-toolkit/albumart-dbus
Merge into: lp:ubuntu-ui-toolkit
Diff against target: 230 lines (+142/-4)
6 files modified
debian/control (+2/-1)
modules/Ubuntu/Components/plugin/albumartgenerator.cpp (+90/-0)
modules/Ubuntu/Components/plugin/albumartgenerator.h (+36/-0)
modules/Ubuntu/Components/plugin/plugin.cpp (+6/-0)
modules/Ubuntu/Components/plugin/plugin.pro (+2/-0)
modules/Ubuntu/Components/plugin/thumbnailgenerator.cpp (+6/-3)
To merge this branch: bzr merge lp:~jamesh/ubuntu-ui-toolkit/albumart-dbus
Reviewer Review Type Date Requested Status
Tim Peeters 2014-03-24 Disapprove on 2014-03-27
PS Jenkins bot continuous-integration Needs Fixing on 2014-03-24
Jussi Pakkanen (community) Approve on 2014-03-24
Review via email: mp+212362@code.launchpad.net

Commit message

Add an image://albumart image provider that retrieves album art for a particular (artist, album) pair. Artwork is retrieved via a D-Bus service so it is possible for confined applications to take advantage of a centralised media art cache.

Description of the change

Add an image://albumart image provider that retrieves album art for a particular (artist, album) pair. Artwork is retrieved via a D-Bus service so it is possible for confined applications to take advantage of a centralised media art cache.

The D-Bus service is being added to the thumbnailer here:

https://code.launchpad.net/~jamesh/thumbnailer/dbus-service/+merge/212347

To post a comment you must log in.
Jussi Pakkanen (jpakkane) wrote :

Looks good.

review: Approve
974. By James Henstridge on 2014-03-24

Fix up constness of catch() clause.

Tim Peeters (tpeeters) wrote :

Why should an album art generator be part of the UITK? It seems specific for only a few apps to me and I don't like to add dependencies to the UITK because that gives problems with supporting and porting (to precise for example) the UITK. Could this be included in the apps that use it or as a separate module that they can use?

Jussi Pakkanen (jpakkane) wrote :

The album art image provider is conceptually the same as thumbnailer image provider. The latter says "get me a thumbnail for this file" and the former is "get me an image of this album". There is already code for this in the Dash (and I think duplicated in Unity8) but that is not the correct place for it for obvious reasons.

As far as dependencies go, this does not add any extra dependencies. The UITK part is just a dbus call (the actual generation happens in a service provided by libthumbnailer). If the album art service is not running (as it probably won't on precise), the code returns the default image. The dependency on >=1.1 of thumbnailer and thumbnailer-service is just to declare this and can be changed to just libthumbnailer>=1.0 if that makes supporting precise easier (note that neither AlbumArtGenerator.cpp nor AlbumArtGenerator.h includes anything from libthumbnnailer so they will compile and link just fine).

We proposed this for the UITK because we feel that it should be as widely available as the thumbnailing image provider is. That being said the current users are the shell and the music player app. If you feel that this code has a better location somewhere else then we are glad to work with you to find one. It is a very small change, after all.

Tim Peeters (tpeeters) wrote :

Yes it is similar to the thumbnailer image provider. I thought there were plans for the thumbnailer also to move it out of the UITK (cf. https://bugs.launchpad.net/ubuntu-ui-toolkit/+bug/1237045/comments/17) but I don't know the status of those plans. I would not mind if the thumbnailer was also moved out of the UITK, that would prevent possible issues with porting such as https://bugs.launchpad.net/ubuntu-ui-toolkit/+bug/1237045/

Jussi Pakkanen (jpakkane) wrote :

We need a QQuickImageProvider that can be used by all applications, the shell and so directly from their QML files. This is the requirement we were given. Where should this functionality live, then, if it needs to be moved out of uitk?

James Henstridge (jamesh) wrote :

So what is the way forward here? Do we move the QML image providers to a new QML plugin in thumbnailer? If we do add a thumbnailer QML plugin, should the other image provider be moved to it? What effect does that have on application compatibility?

Also, its worth noting that the "new dependency" here is still part of the thumbnailer source package. It is a new binary package in order to satisfy multi-arch (we only want one copy of the D-Bus service on the system, as opposed to the library, where each architecture needs its own copy).

James Henstridge (jamesh) wrote :

Wherever the code ends up, the thumbnailer-service dependency should probably be a Recommends:

18:36 <cjwatson> jamesh: what do thumbnailer-service's own Depends look like? I'm concerned that this will cause a problem for cross-installation in development chroots
18:37 <cjwatson> jamesh: and it's pretty questionable whether e.g. the d-bus service should be mandatorily installed during development
18:37 <cjwatson> those are usually run-time facilities that tests would have to mock anyway
18:42 <jamesh> cjwatson: it depends on libthumbnailer + its dependencies (which is already a dep for ubuntu-ui-toolkit)
18:44 <cjwatson> jamesh: right, but in cross-install situations that'll involve following the dep trees on both architectures
18:44 <cjwatson> best avoided if we don't have to
18:44 <jamesh> cjwatson: if the service isn't available, the image provider should present a fallback image and log the problem
18:45 <jamesh> I guess it could be a "Recommends" in that case
18:45 <cjwatson> jamesh: ok, so that sounds like it should be Recommends not Depends, and we should take other measures if necessary to ensure that it's on our images

Tim Peeters (tpeeters) wrote :

After some discussion with James and Jussi on irc we decided to move the thumbnailer QML bindings out of the UITK into a new package in the thumbnailer proejct.

review: Disapprove

Unmerged revisions

974. By James Henstridge on 2014-03-24

Fix up constness of catch() clause.

973. By James Henstridge on 2014-03-24

Make qtdeclarative5-ubuntu-ui-toolkit-plugin depend on
thumbnailer-service, since the album art image provider has a runtime
dependency on this D-Bus service.

972. By James Henstridge on 2014-03-24

Small cleanups.

971. By James Henstridge on 2014-03-24

Make album art image provider call out to the D-Bus service, and follow
the URI scheme used by Unity.

970. By Jussi Pakkanen on 2014-03-10

Now with xlarge goodness.

969. By Jussi Pakkanen on 2014-03-10

Basic work on album art downloader.

968. By Launchpad Translations on behalf of ubuntu-sdk-team on 2014-03-09

Launchpad automatic translations update.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/control'
2--- debian/control 2014-03-17 09:02:58 +0000
3+++ debian/control 2014-03-24 10:25:09 +0000
4@@ -33,7 +33,7 @@
5 libqt5organizer5,
6 qtpim5-dev,
7 language-pack-en-base,
8- libthumbnailer-dev,
9+ libthumbnailer-dev (>=1.1),
10 libdbus-1-dev,
11 libnih-dbus-dev,
12 xvfb,
13@@ -56,6 +56,7 @@
14 qtdeclarative5-window-plugin,
15 qtdeclarative5-qtfeedback-plugin,
16 qtdeclarative5-unity-action-plugin (>= 1.1.0),
17+ thumbnailer-service,
18 ttf-ubuntu-font-family,
19 ubuntu-ui-toolkit-theme,
20 ${misc:Depends},
21
22=== added file 'modules/Ubuntu/Components/plugin/albumartgenerator.cpp'
23--- modules/Ubuntu/Components/plugin/albumartgenerator.cpp 1970-01-01 00:00:00 +0000
24+++ modules/Ubuntu/Components/plugin/albumartgenerator.cpp 2014-03-24 10:25:09 +0000
25@@ -0,0 +1,90 @@
26+/*
27+ * Copyright 2014 Canonical Ltd.
28+ *
29+ * This program is free software; you can redistribute it and/or modify
30+ * it under the terms of the GNU Lesser General Public License as published by
31+ * the Free Software Foundation; version 3.
32+ *
33+ * This program is distributed in the hope that it will be useful,
34+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
35+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
36+ * GNU Lesser General Public License for more details.
37+ *
38+ * You should have received a copy of the GNU Lesser General Public License
39+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
40+ *
41+ * Authors: Jussi Pakkanen <jussi.pakkanen@canonical.com>
42+ * James Henstridge <james.henstridge@canonical.com>
43+*/
44+
45+#include "albumartgenerator.h"
46+#include <stdexcept>
47+#include <QDebug>
48+#include <QFile>
49+#include <QUrlQuery>
50+#include <QDBusUnixFileDescriptor>
51+#include <QDBusReply>
52+
53+static const char DEFAULT_ALBUM_ART[] = "/usr/share/unity/icons/album_missing.png";
54+
55+static const char BUS_NAME[] = "com.canonical.Thumbnailer";
56+static const char BUS_PATH[] = "/com/canonical/Thumbnailer";
57+static const char THUMBNAILER_IFACE[] = "com.canonical.Thumbnailer";
58+static const char GET_ALBUM_ART[] = "GetAlbumArt";
59+
60+AlbumArtGenerator::AlbumArtGenerator()
61+ : QQuickImageProvider(QQuickImageProvider::Image, QQmlImageProviderBase::ForceAsynchronousImageLoading),
62+ iface(BUS_NAME, BUS_PATH, THUMBNAILER_IFACE) {
63+}
64+
65+static QImage fallbackImage(QSize *realSize) {
66+ QImage fallback;
67+ fallback.load(DEFAULT_ALBUM_ART);
68+ *realSize = fallback.size();
69+ return fallback;
70+}
71+
72+QImage AlbumArtGenerator::requestImage(const QString &id, QSize *realSize,
73+ const QSize &requestedSize) {
74+ QUrlQuery query(id);
75+ if (!query.hasQueryItem("artist") || !query.hasQueryItem("album")) {
76+ qWarning() << "Invalid albumart uri:" << id;
77+ return fallbackImage(realSize);
78+ }
79+
80+ const QString artist = query.queryItemValue("artist", QUrl::FullyDecoded);
81+ const QString album = query.queryItemValue("album", QUrl::FullyDecoded);
82+
83+ QString desiredSize = "original";
84+ int size = requestedSize.width() > requestedSize.height() ? requestedSize.width() : requestedSize.height();
85+ if (size < 128) {
86+ desiredSize = "small";
87+ } else if (size < 256) {
88+ desiredSize = "large";
89+ } else if (size < 512) {
90+ desiredSize = "xlarge";
91+ }
92+
93+ // perform dbus call
94+ QDBusReply<QDBusUnixFileDescriptor> reply = iface.call(
95+ GET_ALBUM_ART, artist, album, desiredSize);
96+ if (!reply.isValid()) {
97+ qWarning() << "D-Bus error: " << reply.error().message();
98+ return fallbackImage(realSize);
99+ }
100+
101+ try {
102+ QFile file;
103+ file.open(reply.value().fileDescriptor(), QIODevice::ReadOnly);
104+ QImage image;
105+ image.load(&file, NULL);
106+ *realSize = image.size();
107+ return image;
108+ } catch (const std::exception &e) {
109+ qDebug() << "Album art loader failed: " << e.what();
110+ } catch (...) {
111+ qDebug() << "Unknown error when generating image.";
112+ }
113+
114+ return fallbackImage(realSize);
115+}
116
117=== added file 'modules/Ubuntu/Components/plugin/albumartgenerator.h'
118--- modules/Ubuntu/Components/plugin/albumartgenerator.h 1970-01-01 00:00:00 +0000
119+++ modules/Ubuntu/Components/plugin/albumartgenerator.h 2014-03-24 10:25:09 +0000
120@@ -0,0 +1,36 @@
121+/*
122+ * Copyright 2014 Canonical Ltd.
123+ *
124+ * This program is free software; you can redistribute it and/or modify
125+ * it under the terms of the GNU Lesser General Public License as published by
126+ * the Free Software Foundation; version 3.
127+ *
128+ * This program is distributed in the hope that it will be useful,
129+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
130+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
131+ * GNU Lesser General Public License for more details.
132+ *
133+ * You should have received a copy of the GNU Lesser General Public License
134+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
135+ *
136+ * Authors: Jussi Pakkanen <jussi.pakkanen@canonical.com>
137+ * James Henstridge <james.henstridge@canonical.com>
138+*/
139+
140+#ifndef ALBUMART_GENERATOR_H
141+#define ALBUMART_GENERATOR_H
142+
143+#include <QDBusInterface>
144+#include <QQuickImageProvider>
145+
146+class AlbumArtGenerator: public QQuickImageProvider
147+{
148+private:
149+ QDBusInterface iface;
150+
151+public:
152+ AlbumArtGenerator();
153+ QImage requestImage(const QString &id, QSize *size, const QSize &requestedSize);
154+};
155+
156+#endif
157
158=== modified file 'modules/Ubuntu/Components/plugin/plugin.cpp'
159--- modules/Ubuntu/Components/plugin/plugin.cpp 2014-02-13 17:16:06 +0000
160+++ modules/Ubuntu/Components/plugin/plugin.cpp 2014-03-24 10:25:09 +0000
161@@ -37,6 +37,7 @@
162 #include "qquickmimedata.h"
163 #include "bottombarvisibilitycommunicator.h"
164 #include "thumbnailgenerator.h"
165+#include "albumartgenerator.h"
166 #include "ucubuntuanimation.h"
167 #include "ucfontutils.h"
168 #include "ucarguments.h"
169@@ -237,6 +238,11 @@
170 } catch(std::runtime_error &e) {
171 qDebug() << "Could not create thumbnailer: " << e.what();
172 }
173+ try {
174+ engine->addImageProvider(QLatin1String("albumart"), new AlbumArtGenerator);
175+ } catch(std::runtime_error &e) {
176+ qDebug() << "Could not create albumart: " << e.what();
177+ }
178
179 // Necessary for Screen.orientation (from import QtQuick.Window 2.0) to work
180 QGuiApplication::primaryScreen()->setOrientationUpdateMask(
181
182=== modified file 'modules/Ubuntu/Components/plugin/plugin.pro'
183--- modules/Ubuntu/Components/plugin/plugin.pro 2014-02-13 09:38:23 +0000
184+++ modules/Ubuntu/Components/plugin/plugin.pro 2014-03-24 10:25:09 +0000
185@@ -52,6 +52,7 @@
186 ucalarmmodel.h \
187 unitythemeiconprovider.h \
188 thumbnailgenerator.h \
189+ albumartgenerator.h \
190 alarmrequest_p.h \
191 alarmrequest_p_p.h \
192 adapters/alarmsadapter_p.h \
193@@ -86,6 +87,7 @@
194 ucalarmmodel.cpp \
195 unitythemeiconprovider.cpp \
196 thumbnailgenerator.cpp \
197+ albumartgenerator.cpp \
198 alarmrequest_p.cpp \
199 ucstatesaver.cpp \
200 statesaverbackend_p.cpp \
201
202=== modified file 'modules/Ubuntu/Components/plugin/thumbnailgenerator.cpp'
203--- modules/Ubuntu/Components/plugin/thumbnailgenerator.cpp 2014-02-25 09:14:23 +0000
204+++ modules/Ubuntu/Components/plugin/thumbnailgenerator.cpp 2014-03-24 10:25:09 +0000
205@@ -22,8 +22,8 @@
206 #include <QMimeDatabase>
207 #include <QUrl>
208
209-const char *DEFAULT_VIDEO_ART = "/usr/share/unity/icons/video_missing.png";
210-const char *DEFAULT_ALBUM_ART = "/usr/share/unity/icons/album_missing.png";
211+static const char *DEFAULT_VIDEO_ART = "/usr/share/unity/icons/video_missing.png";
212+static const char *DEFAULT_ALBUM_ART = "/usr/share/unity/icons/album_missing.png";
213
214 ThumbnailGenerator::ThumbnailGenerator() : QQuickImageProvider(QQuickImageProvider::Image,
215 QQmlImageProviderBase::ForceAsynchronousImageLoading) {
216@@ -45,10 +45,13 @@
217 std::string tgt_path;
218 try {
219 ThumbnailSize desiredSize;
220+ const int xlarge_cutoff = 512;
221 const int large_cutoff = 256;
222 const int small_cutoff = 128;
223- if(requestedSize.width() > large_cutoff || requestedSize.height() > large_cutoff) {
224+ if(requestedSize.width() > xlarge_cutoff || requestedSize.height() > xlarge_cutoff) {
225 desiredSize = TN_SIZE_ORIGINAL;
226+ } if(requestedSize.width() > large_cutoff || requestedSize.height() > large_cutoff) {
227+ desiredSize = TN_SIZE_XLARGE;
228 } else if(requestedSize.width() > small_cutoff || requestedSize.height() > small_cutoff) {
229 desiredSize = TN_SIZE_LARGE;
230 } else {

Subscribers

People subscribed via source and target branches

to status/vote changes: