Merge lp:~jamesh/gallery-app/thumbnail-mtime into lp:gallery-app

[James Henstridge]

Use the file's last modified date for the "at" parameter when invoking the thumbnailer to avoid needlessly reloading thumbnails.

Currently gallery-app appends an "?at=" parameter to the image://thumbnailer/ URI in order to work around the fact that QML's image cache isn't invalidated when the underlying file is changed.

At the moment it sets this parameter to Date.now(), which achieves this goal but also results in the thumbnail being reloaded when the file _hasn't_ been changed. What we really want is something that will change when the files contents change. The file's mtime is an obvious candidate here, so that's what I've done here:

1. Add a "lastModified" property to the MediaSource object.

2. set the "at" parameter to mediaSource.lastModified instead of Date.now()

3. Remove the onDataChanged signal connections in the QML, since I've set the dataChanged signal as the notifier for the lastModified property.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1234
http://jenkins.qa.ubuntu.com/job/gallery-app-ci/1175/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/gallery-app-vivid-amd64-ci/103
    SUCCESS: http://jenkins.qa.ubuntu.com/job/gallery-app-vivid-armhf-ci/103
        deb: http://jenkins.qa.ubuntu.com/job/gallery-app-vivid-armhf-ci/103/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/gallery-app-vivid-i386-ci/103
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-click-autopilot-vivid-touch/256
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-click-autopilot-runner-mako/908
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-click-builder-vivid-armhf/722
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22609

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/gallery-app-ci/1175/rebuild

review: Needs Fixing (continuous-integration)

[Arthur Mello (artmello)]

Thanks a lot for looking on that. I found one functional issue when testing it on krillin (r99):

1. Open one photo from "Events" tab;
2. Start editing it and crop the image;
3. Return to the opened photo and confirm it was cropped;
4. Return to the "Events" tab.

Expected result:
The thumbnail is updated with the new cropped image.

Current result:
The thumbnail is not updated.

See that I am not moving the event list and so the thumbnail keeps in the visible area all the time. If I move the event list in a way that the thumbnail leaves the visible area it is correctly refreshed when I return to it.

review: Needs Fixing

[lp:~jamesh/gallery-app/thumbnail-mtime]

1235. By James Henstridge on 2015-08-18

Refresh the FileInfo before emitting dataChanged() so the new
modification time is seen.

[James Henstridge (jamesh)]

I think I've got this working now. I've added a refresh() call that clears the cache of the QFileInfo embedded in the MediaSource object, and called it prior to emitting the signal.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1235
http://jenkins.qa.ubuntu.com/job/gallery-app-ci/1176/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/gallery-app-vivid-amd64-ci/104
    SUCCESS: http://jenkins.qa.ubuntu.com/job/gallery-app-vivid-armhf-ci/104
        deb: http://jenkins.qa.ubuntu.com/job/gallery-app-vivid-armhf-ci/104/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/gallery-app-vivid-i386-ci/104
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-click-autopilot-vivid-touch/257
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-click-autopilot-runner-mako/909
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-click-builder-vivid-armhf/723
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22633

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/gallery-app-ci/1176/rebuild

review: Needs Fixing (continuous-integration)

[Arthur Mello (artmello)]

Sorry for the long time without a review. Once again, thx for your work on this. It is definitely a improve from the previous review. But I am seeing on issue while testing:

Steps:
1. Take a couple of photos with camera-app;
2. Open gallery-app and confirm that taken photos are visible;
3. Click one photo to start navigating between them;
4. Edit one of the photos;
5. Return (top left arrow) to exit editing view;

Expected result:
Return to photo viewer displaying the result of the edited photo

Actual result:
Return to photo viewer displaying another photo of the collection. If I navigate back I can see that the edited photo was correctly modified

review: Needs Fixing

[lp:~jamesh/gallery-app/thumbnail-mtime]

1236. By James Henstridge on 2015-11-27

Merge from trunk

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1236
http://jenkins.qa.ubuntu.com/job/gallery-app-ci/1210/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/gallery-app-vivid-amd64-ci/138
    SUCCESS: http://jenkins.qa.ubuntu.com/job/gallery-app-vivid-armhf-ci/138
        deb: http://jenkins.qa.ubuntu.com/job/gallery-app-vivid-armhf-ci/138/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/gallery-app-vivid-i386-ci/138
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-click-autopilot-vivid-touch/335
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-click-autopilot-runner-touch/997
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-click-builder-vivid-armhf/848
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/25658

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/gallery-app-ci/1210/rebuild

review: Needs Fixing (continuous-integration)

[James Henstridge (jamesh)]

Sorry for not getting back to you on this. I did manage to reproduce your finding, but it wasn't consistent: sometimes it would advance to the next photo and other times it wasn't. So that sounds like a race condition.

I tried instrumenting the QML side code and don't see the view index being changed from that side.

At this point, my guess is that you're seeing a pre-existing race condition. I see that the code is using QFileSystemWatcher to track file system changes, and this feeds into the QAbstractListModel. If an item is being removed then added, this could presumably cause the view index to move forward (i.e. row 2 deleted, causing view to point at new row 2. New row inserted between rows 1 and 2 causes index to move forward to 3).

My patch doesn't even touch this code, so I don't know how it could be responsible for this.

[Arthur Mello (artmello)]

Issue mentioned above does happen on gallery trunk and is probably not related with this MR, bug #1524973 was created to track this issue.
Thanks a lot for looking at this.

review: Approve

=== modified file 'rc/qml/AlbumViewer/AlbumInternals/FramePortrait.qml'
--- rc/qml/AlbumViewer/AlbumInternals/FramePortrait.qml	2015-11-05 18:10:29 +0000
+++ rc/qml/AlbumViewer/AlbumInternals/FramePortrait.qml	2015-12-01 02:38:05 +0000
@@ -44,25 +44,9 @@
         anchors.fill: parent
         asynchronous: true
         visible: fullImage.opacity < 1
-        source: load && mediaSource ? "image://thumbnailer/" + mediaSource.path + "?at=" + Date.now() : ""
+        source: load && mediaSource ? "image://thumbnailer/" + mediaSource.path + "?at=" + mediaSource.lastModified : ""
         fillMode: fullImage.fillMode
         sourceSize.width: 256
-
-        Connections {
-            target: mediaSource ? mediaSource : null
-            onDataChanged: {
-                // data changed but filename didn't, so we need to bypass the qml image
-                // cache by tacking a timestamp to the filename so sees it as different.
-                preview.source = "image://thumbnailer/" + mediaSource.path + "?at=" + Date.now()
-
-                // reload full image
-                var src = fullImage.source;
-                fullImage.asynchronous = false;
-                fullImage.source = "";
-                fullImage.asynchronous = true;
-                fullImage.source = src;
-            }
-        }
     }
     Image {
         id: fullImage
@@ -71,7 +55,7 @@
         cache: false
         fillMode: Image.PreserveAspectCrop
         source: (preview.status === Image.Ready && !isPreview) ?
-                "image://thumbnailer/" + mediaSource.path : ""
+                "image://thumbnailer/" + mediaSource.path + "?at=" + mediaSource.lastModified : ""
 
         property int maxSize: Math.max(width, height)
         sourceSize.width: maxSize

=== modified file 'rc/qml/Components/MediaGrid.qml'
--- rc/qml/Components/MediaGrid.qml	2015-11-05 18:10:29 +0000
+++ rc/qml/Components/MediaGrid.qml	2015-12-01 02:38:05 +0000
@@ -85,22 +85,13 @@
             sourceFillMode: UbuntuShape.PreserveAspectCrop
             source: Image {
                 id: thumbImage
-                source: "image://thumbnailer/" + mediaSource.path + "?at=" + Date.now()
+                source: "image://thumbnailer/" + mediaSource.path + "?at=" + mediaSource.lastModified
                 asynchronous: true
                 fillMode: Image.PreserveAspectCrop
                 sourceSize {
                     width: photosGrid.thumbnailSize
                     height: photosGrid.thumbnailSize
                 }
-
-                Connections {
-                    target: mediaSource
-                    onDataChanged: {
-                        // data changed but filename didn't, so we need to bypass the qml image
-                        // cache by tacking a timestamp to the filename so sees it as different.
-                        thumbImage.source = "image://thumbnailer/" + mediaSource.path + "?at=" + Date.now()
-                    }
-                }
             }
 
             Image {

=== modified file 'rc/qml/MediaViewer/MediaViewer.qml'
--- rc/qml/MediaViewer/MediaViewer.qml	2015-11-05 18:10:29 +0000
+++ rc/qml/MediaViewer/MediaViewer.qml	2015-12-01 02:38:05 +0000
@@ -332,7 +332,10 @@
                         editor = overview.pushPage(Qt.resolvedUrl("GalleryPhotoEditorPage.qml"), { photo: path });
                     }
                     editor.done.connect(function(photoWasModified) {
-                        if (photoWasModified) galleryPhotoViewer.media.dataChanged();
+                        if (photoWasModified) {
+                            galleryPhotoViewer.media.refresh();
+                            galleryPhotoViewer.media.dataChanged();
+                        }
                         overview.popPage();
                     });
                 }

=== modified file 'rc/qml/OrganicView/OrganicMediaList.qml'
--- rc/qml/OrganicView/OrganicMediaList.qml	2015-11-06 14:05:24 +0000
+++ rc/qml/OrganicView/OrganicMediaList.qml	2015-12-01 02:38:05 +0000
@@ -174,7 +174,7 @@
                 sourceFillMode: UbuntuShape.PreserveAspectCrop
                 source: Image {
                     id: thumbImage
-                    source: "image://thumbnailer/" + model.mediaSource.path + "?at=" + Date.now()
+                    source: "image://thumbnailer/" + model.mediaSource.path + "?at=" + model.mediaSource.lastModified
                     asynchronous: true
 
                     /* The SDK thumbnailer respects the freedesktop.org standard and uses 128 for the small
@@ -188,15 +188,6 @@
                     fillMode: Image.PreserveAspectCrop
                 }
 
-                Connections {
-                    target: model && model.mediaSource ? model.mediaSource : null
-                    onDataChanged: {
-                        // data changed but filename didn't, so we need to bypass the qml image
-                        // cache by tacking a timestamp to the filename so sees it as different.
-                        thumbImage.source = "image://thumbnailer/" + model.mediaSource.path + "?at=" + Date.now()
-                    }
-                }
-
                 Image {
                     // Display a play icon if the thumbnail is from a video
                     source: "../../img/icon_play.png"

=== modified file 'src/media/media-source.cpp'
--- src/media/media-source.cpp	2014-11-30 18:44:05 +0000
+++ src/media/media-source.cpp	2015-12-01 02:38:05 +0000
@@ -77,6 +77,15 @@
 }
 
 /*!
+ * \brief MediaSource::lastModified
+ * \return
+ */
+qint64 MediaSource::lastModified() const
+{
+    return m_file.lastModified().toMSecsSinceEpoch();
+}
+
+/*!
  * \brief MediaSource::Image
  * \param respectOrientation
  * \return
@@ -242,6 +251,15 @@
 }
 
 /*!
+ * \brief MediaSource::refresh
+ * \return
+ */
+void MediaSource::refresh()
+{
+    m_file.refresh();
+}
+
+/*!
  * \brief MediaSource::set_busy
  * \param busy
  */

=== modified file 'src/media/media-source.h'
--- src/media/media-source.h	2014-11-30 18:44:05 +0000
+++ src/media/media-source.h	2015-12-01 02:38:05 +0000
@@ -47,6 +47,7 @@
     Q_OBJECT
     Q_PROPERTY(MediaType type READ type NOTIFY typeChanged)
     Q_PROPERTY(QUrl path READ path NOTIFY pathChanged)
+    Q_PROPERTY(qint64 lastModified READ lastModified NOTIFY dataChanged)
     Q_PROPERTY(int orientation READ orientation NOTIFY orientationChanged)
     Q_PROPERTY(QDate exposureDate READ exposureDate NOTIFY exposureDateTimeChanged)
     Q_PROPERTY(QTime exposureTimeOfDay READ exposureTimeOfDay NOTIFY exposureDateTimeChanged)
@@ -79,6 +80,7 @@
 
     QFileInfo file() const;
     QUrl path() const;
+    qint64 lastModified() const;
 
     virtual QImage image(bool respectOrientation = true, const QSize &scaleSize=QSize());
     virtual Orientation orientation() const;
@@ -101,6 +103,8 @@
 
     void setMediaTable(MediaTable *mediaTable);
 
+    Q_INVOKABLE void refresh();
+
 public Q_SLOTS:
     void setSize(const QSize& size);