gallery-app

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

  1. thumbnail-mtime
  2. Merge into trunk
Proposed by James Henstridge
Status: Merged
Approved by: Arthur Mello
Approved revision: 1236
Merged at revision: 1257
Proposed branch: lp://qastaging/~jamesh/gallery-app/thumbnail-mtime
Merge into: lp://qastaging/gallery-app
Diff against target: 176 lines (+30/-39)
6 files modified
rc/qml/AlbumViewer/AlbumInternals/FramePortrait.qml (+2/-18)
rc/qml/Components/MediaGrid.qml (+1/-10)
rc/qml/MediaViewer/MediaViewer.qml (+4/-1)
rc/qml/OrganicView/OrganicMediaList.qml (+1/-10)
src/media/media-source.cpp (+18/-0)
src/media/media-source.h (+4/-0)
To merge this branch: bzr merge lp://qastaging/~jamesh/gallery-app/thumbnail-mtime
Reviewer Review Type Date Requested Status
Arthur Mello (community) Approve
PS Jenkins bot continuous-integration Needs Fixing
Review via email: mp+268185@code.qastaging.launchpad.net

Commit message

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

Description of the change

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.

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

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)
Revision history for this message
Arthur Mello (artmello) wrote :

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://qastaging/~jamesh/gallery-app/thumbnail-mtime updated
1235. By James Henstridge

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

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

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.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)
Revision history for this message
Arthur Mello (artmello) wrote :

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://qastaging/~jamesh/gallery-app/thumbnail-mtime updated
1236. By James Henstridge

Merge from trunk

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)
Revision history for this message
James Henstridge (jamesh) wrote :

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.

Revision history for this message
Arthur Mello (artmello) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches