Merge lp:~jamesh/thumbnailer/no-fallback-albumart into lp:thumbnailer/devel

Proposed by James Henstridge
Status: Merged
Merged at revision: 343
Proposed branch: lp:~jamesh/thumbnailer/no-fallback-albumart
Merge into: lp:thumbnailer/devel
Diff against target: 606 lines (+152/-134)
19 files modified
data/CMakeLists.txt (+0/-8)
data/thumbnailer.in (+1/-6)
debian/control (+2/-11)
debian/thumbnailer-common.install (+0/-2)
debian/thumbnailer-service.install (+1/-0)
plugins/Ubuntu/Thumbnailer.0.1/albumartgenerator.cpp (+3/-11)
plugins/Ubuntu/Thumbnailer.0.1/artistartgenerator.cpp (+3/-11)
plugins/Ubuntu/Thumbnailer.0.1/thumbnailerimageresponse.cpp (+13/-19)
plugins/Ubuntu/Thumbnailer.0.1/thumbnailerimageresponse.h (+4/-10)
plugins/Ubuntu/Thumbnailer.0.1/thumbnailgenerator.cpp (+2/-29)
tests/image-provider/image-provider_test.cpp (+82/-0)
tests/qml/CMakeLists.txt (+4/-0)
tests/qml/Fixture.qml (+8/-10)
tests/qml/tst_albumart.qml (+13/-14)
tests/qml/tst_photo.qml (+1/-1)
tests/testsetup.h.in (+0/-1)
tests/utils/artserver.cpp (+0/-1)
tests/utils/testutils.cpp (+11/-0)
tests/utils/testutils.h (+4/-0)
To merge this branch: bzr merge lp:~jamesh/thumbnailer/no-fallback-albumart
Reviewer Review Type Date Requested Status
Michi Henning (community) Approve
PS Jenkins bot (community) continuous-integration Approve
James Henstridge Pending
Review via email: mp+263216@code.launchpad.net

This proposal supersedes a proposal from 2014-05-14.

Commit message

When a thumbnail can not be loaded, return an error from the image provider rather than substituting in a fallback image. Users can detect the problem via the status property of the QML Image component and provide their own fallback handling.

Description of the change

Rather than providing a fallback image from the albumart ImageProvider, return a null QImage object. When used with the QML Image component, this sets the status to Image.Error which allows applications to handle the fallback themselves.

This was requested by the music-app guys, but I haven't yet verified behaviour in the dash.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

FAILED: Continuous integration, rev:85
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~jamesh/thumbnailer/no-fallback-albumart/+merge/219460/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/thumbnailer-ci/59/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-utopic-amd64-ci/1
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-utopic-armhf-ci/1
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-utopic-armhf-ci/1/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-utopic-i386-ci/1

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-ci/59/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Jussi Pakkanen (jpakkane) wrote : Posted in a previous version of this proposal

Looks good.

review: Approve
Revision history for this message
James Henstridge (jamesh) wrote : Posted in a previous version of this proposal

It looks like this does cause problems for the dash: I get an empty space in the carousel where a song without album art sits.

We'll need some kind of fix for this on the Unity8 side before this can be merged.

review: Needs Fixing
Revision history for this message
James Henstridge (jamesh) wrote : Posted in a previous version of this proposal

I've filed a bug 1324142 on Unity8 about getting it to handle the error state correctly.

Revision history for this message
Jussi Pakkanen (jpakkane) wrote : Posted in a previous version of this proposal

This has lingered for quite a while. What shall we do with this?

89. By James Henstridge

Include the ID in the errors from album/artist art generators.

Revision history for this message
James Henstridge (jamesh) wrote :

This currently segfaults due to bug 1469611 in libQt5Quick.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
90. By James Henstridge

Merge from devel

91. By James Henstridge

Fix up tests for removal of fallback art.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
92. By James Henstridge

Remove fallback images.

93. By James Henstridge

Remove the default image override code from tests.

94. By James Henstridge

Remove the thumbnailer-common binary package:
 * Move the apport blacklist file to thumbnailer-service (only listing
   current architecture now, since it is arch specific).
 * Remove the fallback images entirely, since they are no longer used.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
95. By James Henstridge

Merge from devel, fixing massive conflicts.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Michi Henning (michihenning) wrote :

Looks good to me. A few minor quibbles:

In thumbnailerimageresponse.cpp, there is a stale comment in textureFactory().

The test writes to ~/.cache/unity-thumbnailer. It would be good to write to a different cache dir. (Setting XDG_CACHE_HOME for the test should take care of it.)

In Fixture.qml. line 113:

return Qt.rgba(data[pos] / 255, data[pos+1] / 255, data[pos+2] / 255, data[pos+3] / 255);

Might this be behind the failure on big-ending machines?

review: Needs Fixing
96. By James Henstridge

Fix up QML tests.

97. By James Henstridge

Add direct image provider tests for error conditions.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
James Henstridge (jamesh) wrote :

> Looks good to me. A few minor quibbles:
>
> In thumbnailerimageresponse.cpp, there is a stale comment in textureFactory().

Fixed.

>
> The test writes to ~/.cache/unity-thumbnailer. It would be good to write to a
> different cache dir. (Setting XDG_CACHE_HOME for the test should take care of
> it.)

Which test are you referring to?

> In Fixture.qml. line 113:
>
> return Qt.rgba(data[pos] / 255, data[pos+1] / 255, data[pos+2] / 255,
> data[pos+3] / 255);
>
> Might this be behind the failure on big-ending machines?

The data is documented to be in RGBA order, which matches the order of the arguments to Qt.rgba():

http://doc.qt.io/qt-5/qml-qtquick-canvasimagedata.html#data-prop

Even this data was in an endian dependent format, it wouldn't explain why we're reading out black pixels on the problem architectures.

98. By James Henstridge

Remove obsolete comment from textureFactory() method.

99. By James Henstridge

Remove error logging in requestFinished() method.

We now expose the error message via the errorString() method, which Qt
itself will log.

Revision history for this message
Michi Henning (michihenning) wrote :

Oops, it's the QML test that writes to $HOME.

OK, I wasn't sure of this, so endian-ness is not the problem here.

I compiled this on the powerpc porter box, and we are getting 0,0,0,0 back each and every time, so the problem is below the qml level somewhere. I'll have another look tomorrow.

This MR looks fine to me, thanks!

review: Approve
Revision history for this message
James Henstridge (jamesh) wrote :

So, the QML tests are already setting XDG_CACHE_HOME.

I was having some trouble with those tests failing locally while passing in Jenkins, which I think I have solved since you made your original comment. In my environment I had QT_QPA_PLATFORMTHEME=appmenu-qt5, which seemed to be initialising the D-Bus connection prior to me replacing it with libqtdbustest. This meant the test was running against the installed copy of thumbnailer-service.

With the updates in the branch, ctest will blank out this environment variable when running the test, which made those tests pass reliably for me.

Revision history for this message
Michi Henning (michihenning) wrote :

Hmmm... So, when I tried running your test, it was still failing due to the incorrect signal spy. I fixed the offending source line (spy vs statusSpy, from memory), and ran the test again. This time, the test failed with lots of FAILED PREVIOUSLY errors. (I have no thumbnailer installed locally.) When I looked, I found cache files under ~/.cache/unity-thumbnailer. After deleting that directory, I ran the test again, and the FAILED_PREVIOUSLY messages disappeared, but the ~/.cache/unity-thumbnailer directory re-appeared.

It looks like some is indeed creating files under $HOME/.cache. Hmmm... Aargh...

The environment variably problem affects only the client-side, not the server side?

Revision history for this message
Michi Henning (michihenning) wrote :

Ah, no, I take that back. I was looking at the wrong VM. This happened on a VM where I had the thumbnailer installed locally.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
James Henstridge (jamesh) wrote :

Some results from testing:

* Gallery seems to be unaffected (not surprising since it should be able to load all the images/videos camera-app writes).

* music-app shows new fallback art with the change (white music note on orange/purple background).

* My Music scope is a mixed bag.

   * Tracks dept correctly shows the music note fallback

   * Albums dept shows blank square for fallbacks

   * Artists dept shows an Ubuntu logo fallback (not to be confused with the grey silhouette sometimes returned by 7digital).

So the scope might require some additional work before this can be merged. Nothing crashed and burned though, so that's promising.

Revision history for this message
James Henstridge (jamesh) wrote :

Also, we don't seem to be showing fallback images in the my music previews. The results showing fallback images just got a cross in the preview.

Revision history for this message
James Henstridge (jamesh) wrote :

The My Music scope seems to be working with this unity-scope-mediascanner branch applied:

https://code.launchpad.net/~jamesh/unity-scope-mediascanner/fallback-art-fixes/+merge/282396

I'm seeing the fallback images occasionally disappear though, for which I can't determine the cause.

Revision history for this message
Michi Henning (michihenning) wrote :

Disappearing fallback images? How? How can I reproduce?

Seeing that you set the commit message, I take that this is good to go for the time being?

I'm happy with what is there now. I suggest that we merge to devel, but hold off with the silo until after OTA-9. That'll give us time to sort out what's going with the disappearing fallback images, and also give us time for more testing, seeing that lots of other components are affected by this change. (A few hours ago, there were still comments flying past regarding fallback images in the dash, for example.)

If you agree, please top-approve yourself.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'data/CMakeLists.txt'
2--- data/CMakeLists.txt 2015-05-22 03:13:47 +0000
3+++ data/CMakeLists.txt 2016-01-12 09:29:54 +0000
4@@ -4,14 +4,6 @@
5 execute_process (COMMAND ${PKG_CONFIG_EXECUTABLE} gio-2.0 --variable glib_compile_schemas OUTPUT_VARIABLE _glib_comple_schemas OUTPUT_STRIP_TRAILING_WHITESPACE)
6 execute_process (COMMAND ${_glib_comple_schemas} ${CMAKE_CURRENT_SOURCE_DIR} --targetdir=${CMAKE_CURRENT_BINARY_DIR} OUTPUT_STRIP_TRAILING_WHITESPACE)
7
8-install(
9- FILES
10- album_missing.png
11- video_missing.png
12- DESTINATION
13- ${CMAKE_INSTALL_DATADIR}/thumbnailer/icons
14-)
15-
16 configure_file(
17 thumbnailer.in
18 thumbnailer
19
20=== removed file 'data/album_missing.png'
21Binary files data/album_missing.png 2014-09-25 09:47:05 +0000 and data/album_missing.png 1970-01-01 00:00:00 +0000 differ
22=== modified file 'data/thumbnailer.in'
23--- data/thumbnailer.in 2014-09-26 11:24:27 +0000
24+++ data/thumbnailer.in 2016-01-12 09:29:54 +0000
25@@ -1,6 +1,1 @@
26-@CMAKE_INSTALL_PREFIX@/lib/x86_64-linux-gnu/thumbnailer/vs-thumb
27-@CMAKE_INSTALL_PREFIX@/lib/i386-linux-gnu/thumbnailer/vs-thumb
28-@CMAKE_INSTALL_PREFIX@/lib/powerpc-linux-gnu/thumbnailer/vs-thumb
29-@CMAKE_INSTALL_PREFIX@/lib/powerpc64le-linux-gnu/thumbnailer/vs-thumb
30-@CMAKE_INSTALL_PREFIX@/lib/arm-linux-gnueabihf/thumbnailer/vs-thumb
31-@CMAKE_INSTALL_PREFIX@/lib/aarch64-linux-gnu/thumbnailer/vs-thumb
32+@CMAKE_INSTALL_FULL_LIBDIR@/thumbnailer/vs-thumb
33
34=== removed file 'data/video_missing.png'
35Binary files data/video_missing.png 2014-09-25 09:47:05 +0000 and data/video_missing.png 1970-01-01 00:00:00 +0000 differ
36=== modified file 'debian/control'
37--- debian/control 2015-12-21 07:07:46 +0000
38+++ debian/control 2016-01-12 09:29:54 +0000
39@@ -47,8 +47,8 @@
40 Pre-Depends: ${misc:Pre-Depends},
41 Depends: ${misc:Depends},
42 ${shlibs:Depends},
43-Conflicts: libthumbnailer0
44-Replaces: libthumbnailer0
45+Conflicts: libthumbnailer0, thumbnailer-common
46+Replaces: libthumbnailer0, thumbnailer-common
47 Description: D-Bus service for out of process thumbnailing
48 This package provides a D-Bus service that can provide thumbnails on
49 behalf of another process.
50@@ -73,7 +73,6 @@
51 Depends: ${misc:Depends},
52 ${shlibs:Depends},
53 libthumbnailer-qt1.0 (= ${binary:Version}),
54- thumbnailer-common (= ${source:Version}),
55 Provides: ubuntu-thumbnailer-impl,
56 ubuntu-thumbnailer-impl-0,
57 Recommends: thumbnailer-service (= ${binary:Version}),
58@@ -83,14 +82,6 @@
59 This package provides image providers that allow access to the
60 thumbnailer from Qt Quick 2 / QML applications.
61
62-Package: thumbnailer-common
63-Architecture: all
64-Multi-Arch: foreign
65-Depends: ${misc:Depends},
66-Description: Common files for the thumbnailer service.
67- This package provides common files for the thumbnailer service, such
68- as default icons.
69-
70 Package: libthumbnailer-qt1.0
71 Architecture: any
72 Multi-Arch: same
73
74=== removed file 'debian/thumbnailer-common.install'
75--- debian/thumbnailer-common.install 2014-09-26 11:24:27 +0000
76+++ debian/thumbnailer-common.install 1970-01-01 00:00:00 +0000
77@@ -1,2 +0,0 @@
78-usr/share/thumbnailer/icons/*
79-etc/apport/blacklist.d/thumbnailer
80
81=== modified file 'debian/thumbnailer-service.install'
82--- debian/thumbnailer-service.install 2015-08-06 05:08:47 +0000
83+++ debian/thumbnailer-service.install 2016-01-12 09:29:54 +0000
84@@ -1,3 +1,4 @@
85+etc/apport/blacklist.d/thumbnailer
86 usr/bin/thumbnailer-admin
87 usr/lib/*/thumbnailer/thumbnailer-service
88 usr/lib/*/thumbnailer/vs-thumb
89
90=== modified file 'plugins/Ubuntu/Thumbnailer.0.1/albumartgenerator.cpp'
91--- plugins/Ubuntu/Thumbnailer.0.1/albumartgenerator.cpp 2015-12-09 02:39:32 +0000
92+++ plugins/Ubuntu/Thumbnailer.0.1/albumartgenerator.cpp 2016-01-12 09:29:54 +0000
93@@ -24,13 +24,6 @@
94
95 #include "thumbnailerimageresponse.h"
96
97-namespace
98-{
99-
100-const char DEFAULT_ALBUM_ART[] = "/usr/share/thumbnailer/icons/album_missing.png";
101-
102-} // namespace
103-
104 namespace unity
105 {
106
107@@ -48,19 +41,18 @@
108
109 QQuickImageResponse* AlbumArtGenerator::requestImageResponse(const QString& id, const QSize& requestedSize)
110 {
111- QSize size = requestedSize;
112 QUrlQuery query(id);
113 if (!query.hasQueryItem(QStringLiteral("artist")) || !query.hasQueryItem(QStringLiteral("album")))
114 {
115 qWarning() << "AlbumArtGenerator::requestImageResponse(): Invalid albumart uri:" << id;
116- return new ThumbnailerImageResponse(requestedSize, DEFAULT_ALBUM_ART);
117+ return new ThumbnailerImageResponse("Invalid albumart ID: " + id);
118 }
119
120 const QString artist = query.queryItemValue(QStringLiteral("artist"), QUrl::FullyDecoded);
121 const QString album = query.queryItemValue(QStringLiteral("album"), QUrl::FullyDecoded);
122
123- auto request = thumbnailer->getAlbumArt(artist, album, size);
124- return new ThumbnailerImageResponse(size, DEFAULT_ALBUM_ART, request);
125+ auto request = thumbnailer->getAlbumArt(artist, album, requestedSize);
126+ return new ThumbnailerImageResponse(request);
127 }
128
129 } // namespace qml
130
131=== modified file 'plugins/Ubuntu/Thumbnailer.0.1/artistartgenerator.cpp'
132--- plugins/Ubuntu/Thumbnailer.0.1/artistartgenerator.cpp 2015-11-04 07:16:12 +0000
133+++ plugins/Ubuntu/Thumbnailer.0.1/artistartgenerator.cpp 2016-01-12 09:29:54 +0000
134@@ -25,13 +25,6 @@
135
136 #include "thumbnailerimageresponse.h"
137
138-namespace
139-{
140-
141-const char DEFAULT_ARTIST_ART[] = "/usr/share/thumbnailer/icons/album_missing.png";
142-
143-} // namespace
144-
145 namespace unity
146 {
147
148@@ -49,19 +42,18 @@
149
150 QQuickImageResponse* ArtistArtGenerator::requestImageResponse(const QString& id, const QSize& requestedSize)
151 {
152- QSize size = requestedSize;
153 QUrlQuery query(id);
154 if (!query.hasQueryItem(QStringLiteral("artist")) || !query.hasQueryItem(QStringLiteral("album")))
155 {
156 qWarning() << "ArtistArtGenerator::requestImageResponse(): Invalid artistart uri:" << id;
157- return new ThumbnailerImageResponse(requestedSize, DEFAULT_ARTIST_ART);
158+ return new ThumbnailerImageResponse("Invalid artistart ID: " + id);
159 }
160
161 const QString artist = query.queryItemValue(QStringLiteral("artist"), QUrl::FullyDecoded);
162 const QString album = query.queryItemValue(QStringLiteral("album"), QUrl::FullyDecoded);
163
164- auto request = thumbnailer->getArtistArt(artist, album, size);
165- return new ThumbnailerImageResponse(size, DEFAULT_ARTIST_ART, request);
166+ auto request = thumbnailer->getArtistArt(artist, album, requestedSize);
167+ return new ThumbnailerImageResponse(request);
168 }
169
170 } // namespace qml
171
172=== modified file 'plugins/Ubuntu/Thumbnailer.0.1/thumbnailerimageresponse.cpp'
173--- plugins/Ubuntu/Thumbnailer.0.1/thumbnailerimageresponse.cpp 2015-12-17 00:25:10 +0000
174+++ plugins/Ubuntu/Thumbnailer.0.1/thumbnailerimageresponse.cpp 2016-01-12 09:29:54 +0000
175@@ -31,21 +31,15 @@
176 namespace qml
177 {
178
179-ThumbnailerImageResponse::ThumbnailerImageResponse(QSize const& requested_size,
180- QString const& default_image,
181- QSharedPointer<thumb_qt::Request> const& request)
182- : requested_size_(requested_size)
183- , request_(request)
184- , default_image_(default_image)
185+ThumbnailerImageResponse::ThumbnailerImageResponse(QSharedPointer<thumb_qt::Request> const& request)
186+ : request_(request)
187 {
188 Q_ASSERT(request);
189 connect(request_.data(), &thumb_qt::Request::finished, this, &ThumbnailerImageResponse::requestFinished);
190 }
191
192-ThumbnailerImageResponse::ThumbnailerImageResponse(QSize const& requested_size,
193- QString const& default_image)
194- : requested_size_(requested_size)
195- , default_image_(default_image)
196+ThumbnailerImageResponse::ThumbnailerImageResponse(QString const& error_message)
197+ : error_message_(error_message)
198 {
199 // Queue the signal emission so there is time for the caller to connect.
200 QMetaObject::invokeMethod(this, "finished", Qt::QueuedConnection);
201@@ -58,22 +52,22 @@
202
203 QQuickTextureFactory* ThumbnailerImageResponse::textureFactory() const
204 {
205- // TODO: Once we remove fallback image support, this test for request_ != nullptr
206- // (here and elsewhere) needs to be removed because request_ is nullptr
207- // only if the default image constructor above was called.
208 if (request_ && request_->isValid())
209 {
210 return QQuickTextureFactory::textureFactoryForImage(request_->image());
211 }
212 else
213 {
214- char const* env_default = getenv("THUMBNAILER_TEST_DEFAULT_IMAGE");
215- QImage aux;
216- aux.load(env_default ? QString(env_default) : default_image_);
217- return QQuickTextureFactory::textureFactoryForImage(aux);
218+ qWarning() << "ThumbnailerImageResponse::textureFactory(): method called without valid request.";
219+ return nullptr;
220 }
221 }
222
223+QString ThumbnailerImageResponse::errorString() const
224+{
225+ return error_message_;
226+}
227+
228 void ThumbnailerImageResponse::cancel()
229 {
230 if (request_ && !request_->isFinished() && !request_->isCancelled())
231@@ -84,9 +78,9 @@
232
233 void ThumbnailerImageResponse::requestFinished()
234 {
235- if (!request_->isValid() && !request_->isCancelled())
236+ if (!request_->isValid())
237 {
238- qWarning() << "ThumbnailerImageResponse::dbusCallFinished(): D-Bus error: " << request_->errorMessage();
239+ error_message_ = request_->errorMessage();
240 }
241 Q_EMIT finished();
242 }
243
244=== modified file 'plugins/Ubuntu/Thumbnailer.0.1/thumbnailerimageresponse.h'
245--- plugins/Ubuntu/Thumbnailer.0.1/thumbnailerimageresponse.h 2015-09-30 10:08:20 +0000
246+++ plugins/Ubuntu/Thumbnailer.0.1/thumbnailerimageresponse.h 2016-01-12 09:29:54 +0000
247@@ -20,8 +20,6 @@
248
249 #include <QQuickImageProvider>
250
251-#include <memory>
252-
253 #include <unity/thumbnailer/qt/thumbnailer-qt.h>
254
255 namespace unity
256@@ -46,24 +44,20 @@
257 public:
258 Q_DISABLE_COPY(ThumbnailerImageResponse)
259
260- ThumbnailerImageResponse(QSize const& requested_size,
261- QString const& default_image,
262- QSharedPointer<unity::thumbnailer::qt::Request> const& request);
263- ThumbnailerImageResponse(QSize const& requested_size,
264- QString const& default_image);
265+ ThumbnailerImageResponse(QSharedPointer<unity::thumbnailer::qt::Request> const& request);
266+ ThumbnailerImageResponse(QString const& error_message);
267 ~ThumbnailerImageResponse();
268
269 QQuickTextureFactory* textureFactory() const override;
270+ QString errorString() const override;
271 void cancel() override;
272
273 private Q_SLOTS:
274 void requestFinished();
275
276 private:
277- QString id_;
278- QSize requested_size_;
279 QSharedPointer<unity::thumbnailer::qt::Request> request_;
280- QString default_image_;
281+ QString error_message_;
282 };
283
284 } // namespace qml
285
286=== modified file 'plugins/Ubuntu/Thumbnailer.0.1/thumbnailgenerator.cpp'
287--- plugins/Ubuntu/Thumbnailer.0.1/thumbnailgenerator.cpp 2015-11-04 07:16:12 +0000
288+++ plugins/Ubuntu/Thumbnailer.0.1/thumbnailgenerator.cpp 2016-01-12 09:29:54 +0000
289@@ -19,36 +19,11 @@
290 #include "thumbnailgenerator.h"
291
292 #include <QDebug>
293-#include <QMimeDatabase>
294 #include <QUrl>
295
296 #include <settings.h>
297 #include "thumbnailerimageresponse.h"
298
299-namespace
300-{
301-
302-const char* DEFAULT_VIDEO_ART = "/usr/share/thumbnailer/icons/video_missing.png";
303-const char* DEFAULT_ALBUM_ART = "/usr/share/thumbnailer/icons/album_missing.png";
304-
305-QString default_image_based_on_mime(QString const &id)
306-{
307- QMimeDatabase db;
308- QMimeType mime = db.mimeTypeForFile(id);
309-
310- if (mime.name().contains(QStringLiteral("audio")))
311- {
312- return DEFAULT_ALBUM_ART;
313- }
314- else if (mime.name().contains(QStringLiteral("video")))
315- {
316- return DEFAULT_VIDEO_ART; // LCOV_EXCL_LINE // Being lazy here: default art is about to go away.
317- }
318- return DEFAULT_ALBUM_ART;
319-}
320-
321-} // namespace
322-
323 namespace unity
324 {
325
326@@ -66,8 +41,6 @@
327
328 QQuickImageResponse* ThumbnailGenerator::requestImageResponse(const QString& id, const QSize& requestedSize)
329 {
330- QSize size = requestedSize;
331-
332 /* Allow appending a query string (e.g. ?something=timestamp)
333 * to the id and then ignore it.
334 * This is workaround to force reloading a thumbnail when it has
335@@ -79,8 +52,8 @@
336 * is the only way around the issue for now. */
337 QString src_path = QUrl(id).path();
338
339- auto request = thumbnailer->getThumbnail(src_path, size);
340- return new ThumbnailerImageResponse(size, default_image_based_on_mime(id), request);
341+ auto request = thumbnailer->getThumbnail(src_path, requestedSize);
342+ return new ThumbnailerImageResponse(request);
343 }
344
345 } // namespace qml
346
347=== modified file 'tests/image-provider/image-provider_test.cpp'
348--- tests/image-provider/image-provider_test.cpp 2015-12-23 03:04:01 +0000
349+++ tests/image-provider/image-provider_test.cpp 2016-01-12 09:29:54 +0000
350@@ -99,6 +99,7 @@
351 ASSERT_EQ("", response->errorString());
352
353 unique_ptr<QQuickTextureFactory> factory(response->textureFactory());
354+ ASSERT_NE(nullptr, factory.get());
355 QImage image = factory->image();
356
357 EXPECT_EQ(128, image.width());
358@@ -118,6 +119,87 @@
359 provider.requestImageResponse(filename, QSize(128, 128)));
360 response->cancel();
361 wait(response.get());
362+ EXPECT_EQ("Request cancelled", response->errorString());
363+}
364+
365+TEST_F(ProviderTest, thumbnail_missing)
366+{
367+ const char* filename = TESTDATADIR "/no-such-file.jpg";
368+
369+ ThumbnailGenerator provider(thumbnailer_);
370+ unique_ptr<QQuickImageResponse> response(
371+ provider.requestImageResponse(filename, QSize(128, 128)));
372+ wait(response.get());
373+ string error = response->errorString().toStdString();
374+ EXPECT_NE(string::npos, error.find("No such file or directory")) << error;
375+}
376+
377+TEST_F(ProviderTest, albumart)
378+{
379+ const char* id = "artist=metallica&album=load";
380+
381+ AlbumArtGenerator provider(thumbnailer_);
382+ unique_ptr<QQuickImageResponse> response(
383+ provider.requestImageResponse(id, QSize(128, 128)));
384+ wait(response.get());
385+ EXPECT_EQ("", response->errorString());
386+
387+ unique_ptr<QQuickTextureFactory> factory(response->textureFactory());
388+ ASSERT_NE(nullptr, factory.get());
389+ QImage image = factory->image();
390+
391+ EXPECT_EQ(48, image.width());
392+ EXPECT_EQ(48, image.height());
393+ EXPECT_EQ(QColor("#C80000").rgb(), image.pixel(0, 0));
394+ EXPECT_EQ(QColor("#00D200").rgb(), image.pixel(47, 0));
395+ EXPECT_EQ(QColor("#0000DC").rgb(), image.pixel(0, 47));
396+ EXPECT_EQ(QColor("#646E78").rgb(), image.pixel(47, 47));
397+}
398+
399+TEST_F(ProviderTest, albumart_missing)
400+{
401+ const char* id = "artist=no-such-artist&album=no-such-album";
402+
403+ AlbumArtGenerator provider(thumbnailer_);
404+ unique_ptr<QQuickImageResponse> response(
405+ provider.requestImageResponse(id, QSize(128, 128)));
406+ wait(response.get());
407+ string error = response->errorString().toStdString();
408+ EXPECT_NE(string::npos, error.find("could not get thumbnail for album")) << error;
409+}
410+
411+TEST_F(ProviderTest, artistart)
412+{
413+ const char* id = "artist=beck&album=odelay";
414+
415+ ArtistArtGenerator provider(thumbnailer_);
416+ unique_ptr<QQuickImageResponse> response(
417+ provider.requestImageResponse(id, QSize(128, 128)));
418+ wait(response.get());
419+ EXPECT_EQ("", response->errorString());
420+
421+ unique_ptr<QQuickTextureFactory> factory(response->textureFactory());
422+ ASSERT_NE(nullptr, factory.get());
423+ QImage image = factory->image();
424+
425+ EXPECT_EQ(128, image.width());
426+ EXPECT_EQ(96, image.height());
427+ EXPECT_EQ(QColor("#FE0000").rgb(), image.pixel(0, 0));
428+ EXPECT_EQ(QColor("#FFFF00").rgb(), image.pixel(127, 0));
429+ EXPECT_EQ(QColor("#0000FE").rgb(), image.pixel(0, 95));
430+ EXPECT_EQ(QColor("#00FF01").rgb(), image.pixel(127, 95));
431+}
432+
433+TEST_F(ProviderTest, artistart_missing)
434+{
435+ const char* id = "artist=no-such-artist&album=no-such-album";
436+
437+ ArtistArtGenerator provider(thumbnailer_);
438+ unique_ptr<QQuickImageResponse> response(
439+ provider.requestImageResponse(id, QSize(128, 128)));
440+ wait(response.get());
441+ string error = response->errorString().toStdString();
442+ EXPECT_NE(string::npos, error.find("could not get thumbnail for artist")) << error;
443 }
444
445 int main(int argc, char **argv)
446
447=== modified file 'tests/qml/CMakeLists.txt'
448--- tests/qml/CMakeLists.txt 2015-12-18 02:09:49 +0000
449+++ tests/qml/CMakeLists.txt 2016-01-12 09:29:54 +0000
450@@ -9,3 +9,7 @@
451 )
452 add_dependencies(qml_test thumbnailer-service thumbnailer-qml)
453 add_test(qml ${CMAKE_SOURCE_DIR}/tools/run-xvfb.sh ./qml_test -import ${CMAKE_BINARY_DIR}/plugins)
454+# Clear some environment variables that can interfere with the tests
455+set_tests_properties(qml PROPERTIES
456+ ENVIRONMENT "DBUS_SESSION_BUS_ADDRESS=;QT_QPA_PLATFORMTHEME="
457+)
458
459=== modified file 'tests/qml/Fixture.qml'
460--- tests/qml/Fixture.qml 2015-12-12 00:25:57 +0000
461+++ tests/qml/Fixture.qml 2016-01-12 09:29:54 +0000
462@@ -69,16 +69,6 @@
463 encodeURIComponent(album));
464 }
465
466- function loadBadAlbumUrl(artist) {
467- load("image://albumart/artist=" +
468- encodeURIComponent(artist));
469- }
470-
471- function loadBadArtistUrl(album) {
472- load("image://artistart/album=" +
473- encodeURIComponent(album));
474- }
475-
476 function load(url) {
477 // Have the image component load the image
478 image.source = url;
479@@ -109,6 +99,14 @@
480 imageData = canvas.context.createImageData(dumpfile);
481 }
482
483+ function expectLoadError(url) {
484+ image.source = url;
485+ while (image.status === Image.Loading) {
486+ statusSpy.wait();
487+ }
488+ compare(image.status, Image.Error);
489+ }
490+
491 function pixel(x, y) {
492 var pos = (imageData.width * y + x) * 4;
493 var data = imageData.data;
494
495=== modified file 'tests/qml/tst_albumart.qml'
496--- tests/qml/tst_albumart.qml 2015-12-09 02:32:38 +0000
497+++ tests/qml/tst_albumart.qml 2016-01-12 09:29:54 +0000
498@@ -22,22 +22,21 @@
499 }
500
501 function test_artistart_not_found() {
502- loadArtistArt("beck2", "odelay")
503- compare(size.width, 96);
504- compare(size.height, 96);
505+ expectLoadError("image://artistart/artist=beck2&album=odelay");
506 }
507
508 function test_albumart_not_found() {
509- loadAlbumArt("beck2", "odelay")
510- compare(size.width, 96);
511- compare(size.height, 96);
512- }
513-
514- function test_bad_artist_art_url() {
515- loadBadArtistUrl("load")
516- }
517-
518- function test_bad_album_art_url() {
519- loadBadAlbumUrl("metallica")
520+ expectLoadError("image://albumart/artist=beck2&album=odelay");
521+ }
522+
523+ function test_missing_parameter() {
524+ expectLoadError("image://albumart/");
525+ expectLoadError("image://albumart/foo");
526+ expectLoadError("image://albumart/artist=X");
527+ expectLoadError("image://albumart/album=Y");
528+ expectLoadError("image://artistart/");
529+ expectLoadError("image://artistart/foo");
530+ expectLoadError("image://artistart/artist=X");
531+ expectLoadError("image://artistart/album=Y");
532 }
533 }
534
535=== modified file 'tests/qml/tst_photo.qml'
536--- tests/qml/tst_photo.qml 2015-12-12 00:25:57 +0000
537+++ tests/qml/tst_photo.qml 2016-01-12 09:29:54 +0000
538@@ -54,6 +54,6 @@
539 }
540
541 function test_no_such_photo() {
542- loadThumbnail("no_such_photo.jpg");
543+ expectLoadError("image://thumbnailer/file:///no_such_photo.jpg");
544 }
545 }
546
547=== modified file 'tests/testsetup.h.in'
548--- tests/testsetup.h.in 2015-10-14 03:57:29 +0000
549+++ tests/testsetup.h.in 2016-01-12 09:29:54 +0000
550@@ -29,6 +29,5 @@
551
552 #define THUMBNAILER_SERVICE "@CMAKE_BINARY_DIR@/src/service/thumbnailer-service"
553 #define FAKE_ART_SERVER "@CMAKE_CURRENT_SOURCE_DIR@/server/server.py"
554-#define THUMBNAILER_TEST_DEFAULT_IMAGE "@CMAKE_SOURCE_DIR@/data/album_missing.png"
555
556 #define THUMBNAILER_ADMIN "@CMAKE_BINARY_DIR@/src/thumbnailer-admin/thumbnailer-admin"
557
558=== modified file 'tests/utils/artserver.cpp'
559--- tests/utils/artserver.cpp 2015-12-12 02:06:41 +0000
560+++ tests/utils/artserver.cpp 2016-01-12 09:29:54 +0000
561@@ -69,7 +69,6 @@
562 }
563 blocked_server_url_ = "http://127.0.0.1:" + std::to_string(addr.sin_port);
564
565- setenv("THUMBNAILER_TEST_DEFAULT_IMAGE", THUMBNAILER_TEST_DEFAULT_IMAGE, true);
566 update_env();
567 }
568
569
570=== modified file 'tests/utils/testutils.cpp'
571--- tests/utils/testutils.cpp 2015-12-14 01:27:29 +0000
572+++ tests/utils/testutils.cpp 2016-01-12 09:29:54 +0000
573@@ -19,6 +19,7 @@
574 #include "testutils.h"
575
576 #include <QString>
577+#include <ostream>
578
579 using namespace std;
580
581@@ -26,3 +27,13 @@
582 {
583 return stream << s.toUtf8().constData();
584 }
585+
586+ostream& operator<<(ostream& stream, const char* s)
587+{
588+ return std::operator<<(stream, s);
589+}
590+
591+void PrintTo(const QString& s, ostream* stream)
592+{
593+ *stream << s;
594+}
595
596=== modified file 'tests/utils/testutils.h'
597--- tests/utils/testutils.h 2015-12-14 01:27:29 +0000
598+++ tests/utils/testutils.h 2016-01-12 09:29:54 +0000
599@@ -24,3 +24,7 @@
600
601 // Helper for gtest to allow us to insert QString into a stream.
602 std::ostream& operator<<(std::ostream& stream, const QString& s);
603+// Needed because the QString version will take precedence over the std:: one
604+std::ostream& operator<<(std::ostream& stream, const char* s);
605+
606+void PrintTo(const QString& s, std::ostream* stream);

Subscribers

People subscribed via source and target branches

to all changes: