Fix thumbnail update after undo
ClosedPublic

Authored by muhlenpfordt on Mar 26 2018, 1:19 PM.

Details

Summary

After editing an image multiple times (e.g. rotate) and then undoing,
the thumbnail displays a previous edit state between the undos.
This is caused by connecting to QUndoStack signal indexChanged
which emits before some undo operations are finished and the new
thumbnail is created too early.
This patch emits the modified/saved signals for a document only
after the undo image operation is finished. It also solves the problem
displaying a wrong thumbnail in another mode after undoing all edits.

Depends on D11714
BUG: 356998
FIXED-IN: 18.04.0

Test Plan
  • Open image in Gwenview (View or Browse Mode)
  • Edit image at least 3 times (e.g. rotate with Ctrl+R)
  • Undo all edits (Ctrl+Z)
  • Thumbnail should always correspond to undo step
  • Thumbnail should be identical in View, Browse and Fullscreen Mode

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
muhlenpfordt requested review of this revision.Mar 26 2018, 1:19 PM
muhlenpfordt created this revision.
muhlenpfordt added inline comments.
lib/thumbnailview/thumbnailview.cpp
879

After undoing the last edit isModified() is false but the document changed and the thumbnail needs an update.

huoni added a subscriber: huoni.Mar 28 2018, 12:43 AM

Nice, I noticed this problem recently! Tested and working for me.
Again, I'm not qualified to code review :)

Just noticed that this patch breaks test #4 documenttest. :(
Seems to be an undo problem but I'll have to take a closer look what's going wrong here.
For current master sometimes #11 urlutilstest fails too.

muhlenpfordt added a comment.EditedMar 28 2018, 12:08 PM

Found the problem. :)
Since the modified and saved signals are not emitted directly but after finishing the undo job the test just needed some more delay.

Test #11 failure maybe a config problem on my side? Error log + further comment: P179.

Updated test to wait for signals

Excluded D11714 from diff again

rkflx added a subscriber: rkflx.Mar 28 2018, 8:44 PM

Could not look at your patches properly yet, sorry for that. However, one thing caught my eye: QTest::qWait(100) – Is there anything in Qt where you could wait for a condition or a signal instead? The timeout seems brittle and looks a bit weird ;)

one thing caught my eye: QTest::qWait(100) – Is there anything in Qt where you could wait for a condition or a signal instead? The timeout seems brittle and looks a bit weird ;)

Undoing a rotation is not just switching back to a stored image (unlike e.g. crop does), it's implemented as rotating to the opposite direction. That's why I used the same delay as above.
But we could change each of the modifiedSpy and savedSpy checks (maybe with a longer timeout) from

QTest::qWait(100);
QCOMPARE(modifiedSpy.count(), 1);

to

QVERIFY(modifiedSpy.wait(100));

Should I change these 4 occurrences from DocumentTest::testModifiedAndSavedSignals() in this patch and/or create a different one for updating the whole documenttest (not sure what's possible to change)?

rkflx added a comment.Mar 29 2018, 1:11 PM

That's why I used the same delay as above.

Ah, I should have read the surrounding code… "33 matches found". I guess just use the timeout for now.

Nevertheless, figuring out whether there are alternatives for those timeouts might a good long-term task.

Should I change these 4 occurrences from DocumentTest::testModifiedAndSavedSignals() in this patch and/or create a different one for updating the whole documenttest (not sure what's possible to change)?

QSignalSpy::wait was only available since Qt 5.0, so it makes sense that the old code used the timeout. I guess you could do a separate patch refactoring to that function where it makes sense. I don't see why you'd have to keep the 100, though, because wait will start an event loop, so it will wait for the signal emission instead of waiting for a fixed amount of time and checking whether the signal was emitted only afterwards.

rkflx added a comment.Apr 7 2018, 10:41 PM

Works and looks just as well as the other patch. Some questions, but otherwise your patch is as good as accepted.

Error log + further comment: P179.

Made a note to look into this later.


BTW, Redo is quite broken (unrelated to your patch, though)…

lib/abstractimageoperation.cpp
107–108

I don't really understand what you mean with "is not executed asynchronous(ly)". Could you provide some steps to reproduce some kind of problem if we called the function directly instead of using the timer?

lib/thumbnailview/thumbnailview.cpp
881–884

It's probably better to keep the else branch as-is, but in case you know (without spending too much time researching), I'd be interested in how we could enter the else, i.e. in which situations mDocumentInfoProvider is null.

BTW, Redo is quite broken (unrelated to your patch, though)…

Thanks for the reminder - I noticed this but forgot about it. ;)

lib/abstractimageoperation.cpp
107–108
  • Replace the QTimer line with the direct call document()->imageOperationCompleted();
  • Crop an image and undo
  • The SaveBar keeps showing since the undo stack is not yet updated.
lib/thumbnailview/thumbnailview.cpp
881–884

I think this should not happen and found no way to provoke this. But I'm not 100% sure it is really impossible.

rkflx accepted this revision.Apr 8 2018, 8:24 AM
rkflx added inline comments.
lib/abstractimageoperation.cpp
107–108

Ah, I see. Maybe it would make sense then to reword the comment a bit, e.g.:

  • not asynchronous(ly) → immediately (?)
  • add crop as an example
lib/thumbnailview/thumbnailview.cpp
881–884

Ok, let's just keep it then.

This revision is now accepted and ready to land.Apr 8 2018, 8:24 AM
muhlenpfordt edited the summary of this revision. (Show Details)

Rebase to Applications/18.04
Changed comment

lib/abstractimageoperation.cpp
107–108

Is this comment more understandable? Better suggestions always welcome. ;)

rkflx accepted this revision.Apr 8 2018, 4:37 PM
rkflx added inline comments.
lib/abstractimageoperation.cpp
107–108

Works for me, thanks for the update ;)

This revision was automatically updated to reflect the committed changes.

Since April 5th the PlaceTreeModelTest on Jenkins fails. I can't reproduce this, on my system the test is ok.
Is this something we should worry about?

rkflx added a comment.Apr 9 2018, 8:54 AM

Since April 5th the PlaceTreeModelTest on Jenkins fails. I can't reproduce this, on my system the test is ok.
Is this something we should worry about?

I know, noticed this on Friday already. It's not Silas' fault though, but caused by a change Nate did in KIO. Did not get around to this yet, but I'll hope to have something in time for 18.04.0. Don't worry about it ;)

ngraham added a subscriber: ngraham.Apr 9 2018, 1:15 PM

Since April 5th the PlaceTreeModelTest on Jenkins fails. I can't reproduce this, on my system the test is ok.
Is this something we should worry about?

I know, noticed this on Friday already. It's not Silas' fault though, but caused by a change Nate did in KIO. Did not get around to this yet, but I'll hope to have something in time for 18.04.0. Don't worry about it ;)

Oops, sorry. Was it the "Recently Saved" change?