Merge lp:~jamesh/thumbnailer/no-fallback-albumart into lp:thumbnailer/devel
- no-fallback-albumart
- Merge into devel
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 | ||||
Related bugs: |
|
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.
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
Jussi Pakkanen (jpakkane) wrote : Posted in a previous version of this proposal | # |
Looks good.
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.
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.
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.
James Henstridge (jamesh) wrote : | # |
This currently segfaults due to bug 1469611 in libQt5Quick.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:89
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 90. By James Henstridge
-
Merge from devel
- 91. By James Henstridge
-
Fix up tests for removal of fallback art.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:91
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 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.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:94
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 95. By James Henstridge
-
Merge from devel, fixing massive conflicts.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:95
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
Michi Henning (michihenning) wrote : | # |
Looks good to me. A few minor quibbles:
In thumbnailerimag
The test writes to ~/.cache/
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?
- 96. By James Henstridge
-
Fix up QML tests.
- 97. By James Henstridge
-
Add direct image provider tests for error conditions.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:97
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
James Henstridge (jamesh) wrote : | # |
> Looks good to me. A few minor quibbles:
>
> In thumbnailerimag
Fixed.
>
> The test writes to ~/.cache/
> 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://
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.
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!
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_
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.
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/
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?
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.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:99
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
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.
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.
James Henstridge (jamesh) wrote : | # |
The My Music scope seems to be working with this unity-scope-
https:/
I'm seeing the fallback images occasionally disappear though, for which I can't determine the cause.
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.
Preview Diff
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' |
21 | Binary 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' |
35 | Binary 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); |
FAILED: Continuous integration, rev:85 /code.launchpad .net/~jamesh/ thumbnailer/ no-fallback- albumart/ +merge/ 219460/ +edit-commit- message
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:/
http:// jenkins. qa.ubuntu. com/job/ thumbnailer- ci/59/ jenkins. qa.ubuntu. com/job/ thumbnailer- utopic- amd64-ci/ 1 jenkins. qa.ubuntu. com/job/ thumbnailer- utopic- armhf-ci/ 1 jenkins. qa.ubuntu. com/job/ thumbnailer- utopic- armhf-ci/ 1/artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ thumbnailer- utopic- i386-ci/ 1
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/thumbnailer -ci/59/ rebuild
http://