Merge lp:~aacid/thumbnailer/fix_factory_leak into lp:thumbnailer/devel

[Albert Astals Cid]

Only create the QQuickTextureFactory when needed

Fixes leak when the response is cancelled

Only create the QQuickTextureFactory when needed

[Albert Astals Cid (aacid)]

A question i just realized. Why are you guys calling finished() from the constructor?

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:254
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/404/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/214/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/219/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/213/console

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

review: Needs Fixing (continuous-integration)

[Michi Henning (michihenning)]

Needs a trivial fix on line 90 of thumbnailerimageresponse.cpp to compile. (Missing internal namespace qualifier.) Otherwise, looks good to me!

review: Needs Fixing

[James Henstridge (jamesh)]

Albert: we are calling finished from the constructor when we know we've hit an error condition. The signal emission is delayed via invokeMethod() to give the caller an opportunity to connect to the signal.

[lp:~aacid/thumbnailer/fix_factory_leak]

255. By Albert Astals Cid on 2015-08-17

compile++

[Albert Astals Cid (aacid)]

> Needs a trivial fix on line 90 of thumbnailerimageresponse.cpp to compile.
> (Missing internal namespace qualifier.) Otherwise, looks good to me!

Done.

[Albert Astals Cid (aacid)]

> Albert: we are calling finished from the constructor when we know we've hit an
> error condition. The signal emission is delayed via invokeMethod() to give
> the caller an opportunity to connect to the signal.

Ah, ThumbnailerImageResponse::ThumbnailerImageResponse(QSize const& requested_size, QString const& default_image) always gives out the default image, ok.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:255
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/408/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/218
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/223
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/223/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/217

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

review: Approve (continuous-integration)

[Michi Henning (michihenning)]

Cool, thank you!

review: Approve

=== modified file 'plugins/Ubuntu/Thumbnailer.0.1/thumbnailerimageresponse.cpp'
--- plugins/Ubuntu/Thumbnailer.0.1/thumbnailerimageresponse.cpp	2015-07-29 18:24:18 +0000
+++ plugins/Ubuntu/Thumbnailer.0.1/thumbnailerimageresponse.cpp	2015-08-17 08:29:34 +0000
@@ -47,7 +47,6 @@
     : requested_size_(requested_size)
     , default_image_(default_image)
 {
-    loadDefaultImage();
     // Queue the signal emission so there is time for the caller to connect.
     QMetaObject::invokeMethod(this, "finished", Qt::QueuedConnection);
 }
@@ -56,7 +55,14 @@
 
 QQuickTextureFactory* ThumbnailerImageResponse::textureFactory() const
 {
-    return texture_;
+    if (!image_.isNull()) {
+        return QQuickTextureFactory::textureFactoryForImage(image_);
+    } else {
+        char const* env_default = getenv("THUMBNAILER_TEST_DEFAULT_IMAGE");
+        QImage aux;
+        aux.load(env_default ? QString(env_default) : default_image_);
+        return QQuickTextureFactory::textureFactoryForImage(aux);
+    }
 }
 
 void ThumbnailerImageResponse::cancel()
@@ -68,21 +74,12 @@
     watcher_.reset();
 }
 
-void ThumbnailerImageResponse::loadDefaultImage()
-{
-    char const* env_default = getenv("THUMBNAILER_TEST_DEFAULT_IMAGE");
-    QImage result;
-    result.load(env_default ? QString(env_default) : default_image_);
-    texture_ = QQuickTextureFactory::textureFactoryForImage(result);
-}
-
 void ThumbnailerImageResponse::dbusCallFinished()
 {
     QDBusPendingReply<QDBusUnixFileDescriptor> reply = *watcher_.get();
     if (!reply.isValid())
     {
         qWarning() << "ThumbnailerImageResponse::dbusCallFinished(): D-Bus error: " << reply.error().message();
-        loadDefaultImage();
         Q_EMIT finished();
         return;
     }
@@ -90,8 +87,7 @@
     try
     {
         QSize realSize;
-        QImage image = internal::imageFromFd(reply.value().fileDescriptor(), &realSize, requested_size_);
-        texture_ = QQuickTextureFactory::textureFactoryForImage(image);
+        image_ = internal::imageFromFd(reply.value().fileDescriptor(), &realSize, requested_size_);
         Q_EMIT finished();
         return;
     }
@@ -105,7 +101,6 @@
         qWarning() << "ThumbnailerImageResponse::dbusCallFinished(): unknown exception";
     }
 
-    loadDefaultImage();
     Q_EMIT finished();
     // LCOV_EXCL_STOP
 }

=== modified file 'plugins/Ubuntu/Thumbnailer.0.1/thumbnailerimageresponse.h'
--- plugins/Ubuntu/Thumbnailer.0.1/thumbnailerimageresponse.h	2015-06-12 23:52:13 +0000
+++ plugins/Ubuntu/Thumbnailer.0.1/thumbnailerimageresponse.h	2015-08-17 08:29:34 +0000
@@ -52,11 +52,9 @@
     void dbusCallFinished();
 
 private:
-    void loadDefaultImage();
-
     QString id_;
     QSize requested_size_;
-    QQuickTextureFactory * texture_ = nullptr;
+    QImage image_;
     QString default_image_;
     std::unique_ptr<QDBusPendingCallWatcher> watcher_;
 };