Changeset View
Standalone View
lib/abstractimageoperation.cpp
| Show First 20 Lines • Show All 46 Lines • ▼ Show 20 Line(s) | 46 | { | |||
|---|---|---|---|---|---|
| 47 | delete mOp; | 47 | delete mOp; | ||
| 48 | } | 48 | } | ||
| 49 | 49 | | |||
| 50 | void undo() Q_DECL_OVERRIDE | 50 | void undo() Q_DECL_OVERRIDE | ||
| 51 | { | 51 | { | ||
| 52 | mOp->undo(); | 52 | mOp->undo(); | ||
| 53 | } | 53 | } | ||
| 54 | 54 | | |||
| 55 | void redo() override | ||||
| 56 | { | ||||
| 57 | mOp->redo(); | ||||
| 58 | } | ||||
| 59 | | ||||
| 55 | private: | 60 | private: | ||
| 56 | AbstractImageOperation* mOp; | 61 | AbstractImageOperation* mOp; | ||
| 57 | }; | 62 | }; | ||
| 58 | 63 | | |||
| 59 | struct AbstractImageOperationPrivate | 64 | struct AbstractImageOperationPrivate | ||
| 60 | { | 65 | { | ||
| 61 | QString mText; | 66 | QString mText; | ||
| 62 | QUrl mUrl; | 67 | QUrl mUrl; | ||
| 68 | ImageOperationCommand* mCommand; | ||||
| 63 | }; | 69 | }; | ||
| 64 | 70 | | |||
| 65 | AbstractImageOperation::AbstractImageOperation() | 71 | AbstractImageOperation::AbstractImageOperation() | ||
| 66 | : d(new AbstractImageOperationPrivate) | 72 | : d(new AbstractImageOperationPrivate) | ||
| 67 | { | 73 | { | ||
| 68 | } | 74 | } | ||
| 69 | 75 | | |||
| 70 | AbstractImageOperation::~AbstractImageOperation() | 76 | AbstractImageOperation::~AbstractImageOperation() | ||
| 71 | { | 77 | { | ||
| 72 | delete d; | 78 | delete d; | ||
| 73 | } | 79 | } | ||
| 74 | 80 | | |||
| 75 | void AbstractImageOperation::applyToDocument(Document::Ptr doc) | 81 | void AbstractImageOperation::applyToDocument(Document::Ptr doc) | ||
| 76 | { | 82 | { | ||
| 77 | d->mUrl = doc->url(); | 83 | d->mUrl = doc->url(); | ||
| 78 | redo(); | 84 | | ||
| 85 | d->mCommand = new ImageOperationCommand(this); | ||||
| 86 | d->mCommand->setText(d->mText); | ||||
| 87 | // QUndoStack::push() executes command by calling its redo() function | ||||
| 88 | doc->undoStack()->push(d->mCommand); | ||||
| 79 | } | 89 | } | ||
| 80 | 90 | | |||
| 81 | Document::Ptr AbstractImageOperation::document() const | 91 | Document::Ptr AbstractImageOperation::document() const | ||
| 82 | { | 92 | { | ||
| 83 | Document::Ptr doc = DocumentFactory::instance()->load(d->mUrl); | 93 | Document::Ptr doc = DocumentFactory::instance()->load(d->mUrl); | ||
| 84 | doc->startLoadingFullImage(); | 94 | doc->startLoadingFullImage(); | ||
| 85 | return doc; | 95 | return doc; | ||
| 86 | } | 96 | } | ||
| 87 | 97 | | |||
| 88 | void AbstractImageOperation::finish(bool ok) | 98 | void AbstractImageOperation::finish(bool ok) | ||
| 89 | { | 99 | { | ||
| 90 | if (ok) { | 100 | if (ok) { | ||
| 91 | ImageOperationCommand* command = new ImageOperationCommand(this); | 101 | // Give QUndoStack time to update in case the redo/undo is executed immediately | ||
| 92 | command->setText(d->mText); | 102 | // (e.g. undo crop just sets the previous image) | ||
| 93 | document()->undoStack()->push(command); | 103 | QTimer::singleShot(0, document().data(), &Document::imageOperationCompleted); | ||
rkflx: Any reason you are moving this around? To me checking for `ok` makes sense (but did not test… | |||||
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. muhlenpfordt: `QUndoStack::push()` and redoing by {key Ctrl Shift Z} both call `ImageOperationCommand::redo… | |||||
| 94 | document()->imageOperationCompleted(); | | |||
| 95 | } else { | 104 | } else { | ||
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()). muhlenpfordt: Currently all image operations use a queued job which gives QUndoStack time to update. Only… | |||||
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 ;) rkflx: Makes sense, but this change made me also scratch my head a bit before I realized you just… | |||||
Ok, should have mentioned this. I removed these changes and do a cleanup patch after this one is completed. muhlenpfordt: Ok, should have mentioned this. I removed these changes and do a cleanup patch after this one… | |||||
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? rkflx: I'm still confused: `document()->imageOperationCompleted();` was also part of the other commit… | |||||
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. I can't do the cleanup before this patch. muhlenpfordt: Moving the `imageOperationCompleted()` call to a delayed timer is needed because of… | |||||
Would it make sense to call finishUndoJob here, and inline that function only in the cleanup patch? rkflx: Would it make sense to call `finishUndoJob` here, and inline that function only in the cleanup… | |||||
Ok, logically it's not the finish of an undo here, but technically it's the same action and easier to read this way. muhlenpfordt: Ok, logically it's not the finish of an undo here, but technically it's the same action and… | |||||
| 96 | deleteLater(); | 105 | // Remove command from undo stack without executing undo() | ||
| 106 | d->mCommand->setObsolete(true); | ||||
"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? rkflx: "This function was introduced in Qt 5.9."
As we cannot bump dependencies on `stable` and I… | |||||
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). muhlenpfordt: Rats! In earlier versions of `QUndoStack` it does not seem to be possible to remove a command. | |||||
| 107 | document()->undoStack()->undo(); | ||||
| 97 | } | 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? muhlenpfordt: To test this part I simulated an error with `KJob::setError()` passing an errorCode != 0 in e.g. | |||||
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…) rkflx: > BTW, just noticed a `FailureOperation` in `DocumentTest` which seems to be happy with the… | |||||
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? muhlenpfordt: `DocumentTest::testUndoStackPush()` already tests a failing image operation which is executed… | |||||
| 98 | } | 109 | } | ||
| 99 | 110 | | |||
| 100 | void AbstractImageOperation::finishFromKJob(KJob* job) | 111 | void AbstractImageOperation::finishFromKJob(KJob* job) | ||
| 101 | { | 112 | { | ||
| 102 | finish(job->error() == KJob::NoError); | 113 | finish(job->error() == KJob::NoError); | ||
| 103 | } | 114 | } | ||
| 104 | 115 | | |||
| 105 | void AbstractImageOperation::finishUndoJob() | | |||
| 106 | { | | |||
| 107 | // Give QUndoStack time to update in case the undo is executed immediately | | |||
| 108 | // (e.g. undo crop just sets the previous image) | | |||
| 109 | QTimer::singleShot(0, document().data(), &Document::imageOperationCompleted); | | |||
| 110 | } | | |||
| 111 | | ||||
| 112 | void AbstractImageOperation::setText(const QString& text) | 116 | void AbstractImageOperation::setText(const QString& text) | ||
| 113 | { | 117 | { | ||
| 114 | d->mText = text; | 118 | d->mText = text; | ||
| 115 | } | 119 | } | ||
| 116 | 120 | | |||
| 117 | void AbstractImageOperation::redoAsDocumentJob(DocumentJob* job) | 121 | void AbstractImageOperation::redoAsDocumentJob(DocumentJob* job) | ||
| 118 | { | 122 | { | ||
| 119 | connect(job, SIGNAL(result(KJob*)), SLOT(finishFromKJob(KJob*))); | 123 | connect(job, SIGNAL(result(KJob*)), SLOT(finishFromKJob(KJob*))); | ||
| 120 | document()->enqueueJob(job); | 124 | document()->enqueueJob(job); | ||
| 121 | } | 125 | } | ||
| 122 | 126 | | |||
| 123 | } // namespace | 127 | } // namespace | ||
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.