Enable redo for undone image operations
ClosedPublic

Authored by muhlenpfordt on Apr 11 2018, 8:50 AM.

Details

Summary

Redoing an undone image operation (e.g. crop, rotate) does not work.
This is caused by a missing redo() override in the derived
QUndoCommand class.
The patch adds the redo() and moves/adjusts some calls to make
undo/redo work properly.
Additionally adds an autotest for undo/redo in DocumentTest.

FIXED-IN: 18.04.2

Test Plan
  • Use different image operations to edit one or more image(s)
  • Undo/redo operations

Diff Detail

Repository
R260 Gwenview
Branch
redo (branched from Applications/18.04)
Lint
Lint OK
Unit
No Unit Test Coverage
muhlenpfordt requested review of this revision.Apr 11 2018, 8:50 AM
muhlenpfordt created this revision.

Is this a fix for 18.04.0 or better master only?

lib/abstractimageoperation.cpp
101–104

Currently all image operations use a queued job which gives QUndoStack time to update. Only TestOperation::redo() immediately executes finish() at the moment (see DocumentTest::testModifiedAndSavedSignals()).
I moved the timer here to also catch future non-queue (re)do operations in addition to undo.

105–108

To test this part I simulated an error with KJob::setError() passing an errorCode != 0 in e.g. TransformJob. Are there any real errors that could happen and should be tested?

rkflx added a subscriber: rkflx.Apr 11 2018, 9:05 AM

Is this a fix for 18.04.0 or better master only?

From the sound of it I'd say this is for stable, but as tagging is tomorrow already, it might only end up in 18.04.1.

muhlenpfordt edited the summary of this revision. (Show Details)

Rebase to Applications/18.04

huoni added a subscriber: huoni.Apr 12 2018, 12:49 AM

Redo works perfectly, and unit tests succeed. LTGM.

rkflx added a comment.Apr 12 2018, 1:15 PM

No issues found when testing, but did not try to understand how the code works yet.

Meanwhile I hope you don't hate me for saying this, but to me it looks like that you should add an autotest or extend an existing autotest, so that Redo does not regress in the future… Could you try to work on that?

Meanwhile I hope you don't hate me for saying this, but to me it looks like that you should add an autotest or extend an existing autotest, so that Redo does not regress in the future… Could you try to work on that?

No problem - I'll take a look at it.
BTW, just noticed a FailureOperation in DocumentTest which seems to be happy with the error handling in D12105#inline-61018. :)

muhlenpfordt edited the summary of this revision. (Show Details)

Added DocumentTest::testUndoRedo()

Added DocumentTest::testUndoRedo()

Hope the test isn't too paranoid? ;)

rkflx requested changes to this revision.Apr 14 2018, 11:16 PM

Added DocumentTest::testUndoRedo()

Hope the test isn't too paranoid? ;)

Hm, for me the testUndoRedo test does not fail when I have not applied the rest of your patch yet. It should fail, because how should it catch a regression otherwise? Your test should basically automate (in terms of Gwenview internals) what you wrote in your test plan, and show that while there was a bug before, your patch is now fixing the problem.

lib/abstractimageoperation.cpp
91–93

Any reason you are moving this around? To me checking for ok makes sense (but did not test the consequences), also possibly avoiding the need to remove the command from the stack again on failure.

101–104

Makes sense, but this change made me also scratch my head a bit before I realized you just reverted parts of f78e5e4b05b7.

Could you do this in a separate patch instead? Thanks ;)

105–108

BTW, just noticed a FailureOperation in DocumentTest which seems to be happy with the error handling in D12105#inline-61018.

This also seems like a candidate for an autotest ;)

(I'm not saying you absolutely must add an autotest, I'm just pointing out how one could go about testing such things with code instead of "simulating" an error by modifying regular code…)

106

"This function was introduced in Qt 5.9."

As we cannot bump dependencies on stable and I don't think we should depend on that version just yet, how about making this conditional?

This revision now requires changes to proceed.Apr 14 2018, 11:16 PM
muhlenpfordt marked 2 inline comments as done.

Remove 'cleanup' changes - TODO for other patch
Added Qt version check for QUndoCommand::setObsolete()

Hm, for me the testUndoRedo test does not fail when I have not applied the rest of your patch yet.

I added DocumentTest::testUndoRedo() to branches master and Applications/18.04 without any other part of this diff. In both versions the test fails:

********* Start testing of DocumentTest *********
Config: Using QtTest library 5.9.1, Qt 5.9.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 7.2.0)
PASS   : DocumentTest::initTestCase()
FAIL!  : DocumentTest::testUndoRedo() 'modifiedSpy.wait()' returned FALSE. ()
   Loc: [/home/muehle/workspace/gwenview/gwenview-master/tests/auto/documenttest.cpp(877)]
PASS   : DocumentTest::cleanupTestCase()
Totals: 2 passed, 1 failed, 0 skipped, 0 blacklisted, 5011ms
********* Finished testing of DocumentTest *********

Even if I don't use signal waits but delays it fails at several other places.
Sorry, don't have any idea why it doesn't fail for you.

lib/abstractimageoperation.cpp
91–93

QUndoStack::push() and redoing by Ctrl++Z both call ImageOperationCommand::redo() which calls the operations redo(). The operations redo() ends with calling this finish() and we get a loop.
I tried to keep the push() here to call only after successfully executing the operation but found no working way - in one or another situation the undo stack get's corrupted.

101–104

Ok, should have mentioned this. I removed these changes and do a cleanup patch after this one is completed.

105–108

DocumentTest::testUndoStackPush() already tests a failing image operation which is executed in the operations redo() function and handled here. Or what do you mean should be tested?

106

Rats! In earlier versions of QUndoStack it does not seem to be possible to remove a command. But pushing only after success does not work (see other comment).
If setObsolete() is not enabled (Qt < 5.9) the failed operation will be undone but resists on the undo stack and could be redone. Since all operations actually set only NoError it should be a theoretical problem for now.

(Will look at the rest later.)

lib/abstractimageoperation.cpp
101–104

I'm still confused: document()->imageOperationCompleted(); was also part of the other commit yet you still remove it, and QTimer::singleShot is now duplicated?

Maybe it would make sense to do the cleanup first, so it would be more clear what you are actually changing here?

muhlenpfordt added inline comments.Apr 16 2018, 6:23 AM
lib/abstractimageoperation.cpp
101–104

Moving the imageOperationCompleted() call to a delayed timer is needed because of DocumentTest::testModifyAndSaveAs() (and the new test) which immediately calls finish() and will fail without this modification.
The duplication of this part was my intention to do the cleanup. ;)

I can't do the cleanup before this patch.
Calling AbstractImageOperation::finish() without this patch will do a undoStack()->push() and call redo() immediately after an undo().

rkflx accepted this revision.EditedMay 9 2018, 4:59 PM

Very sorry this took so long, and thanks for your additional explanations. Reading those and d597c7081756, I think my confusion has now been cleared up.

Sorry, don't have any idea why it doesn't fail for you.

Trying it again, it now fails for me as expected. Not sure what went wrong the first time around.

lib/abstractimageoperation.cpp
101–104

Would it make sense to call finishUndoJob here, and inline that function only in the cleanup patch?

This revision is now accepted and ready to land.May 9 2018, 4:59 PM
rkflx edited the summary of this revision. (Show Details)May 9 2018, 4:59 PM
muhlenpfordt marked an inline comment as done.

Call finishUndoJob instead of duplicate QTimer code

lib/abstractimageoperation.cpp
101–104

Ok, logically it's not the finish of an undo here, but technically it's the same action and easier to read this way.

This revision was automatically updated to reflect the committed changes.