FileUndoManager: don't delete non-existing local files
ClosedPublic

Authored by elvisangelaccio on Feb 4 2018, 10:19 PM.

Details

Summary

After a CopyJob the FileUndoManager records the file that was copied.
If this file is deleted before the Undo operation is triggered, the
File UndoManager will still try to undo the copy by deleting it.

This patch fixes this issue by validating the recorded files with
QFileInfo::exists(). If the FileUndoManager realizes that is no longer
possible to undo the CopyJob, it will signal that the undo operation is
not available anymore.

Test Plan

From Dolphin:

  • Copy foo.txt to bar.txt
  • Shift+Del bar.txt
  • CTRL+Z

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 4 2018, 10:19 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
elvisangelaccio requested review of this revision.Feb 4 2018, 10:19 PM
elvisangelaccio edited the summary of this revision. (Show Details)Feb 4 2018, 10:22 PM
ngraham added a subscriber: ngraham.Feb 5 2018, 1:36 AM
dfaure added inline comments.Feb 5 2018, 2:52 PM
src/widgets/fileundomanager.cpp
404

Where's the corresponding call to slotUnlock?

I think you only wanted to update the state of the action, maybe better to do that directly.

src/widgets/fileundomanager.cpp
404

You mean just an emit undoAvailable(false); ?

dfaure added inline comments.Feb 11 2018, 6:54 PM
src/widgets/fileundomanager.cpp
404

Yep.

src/widgets/fileundomanager.cpp
404

Ok, that's enough to fix the dolphin side, but now the new unit test doesn't pass...

Then the hardcoded false is wrong, the equivalent line would have been emit undoAvailable(q->undoAvailable()) instead. But I didn't dig into whether it makes sense for it to be true sometimes.

  • Improved unit test
  • Replaced slotUnlock() with slotPop()

Looks good, just minor issues left.

autotests/fileundomanagertest.cpp
735

This isn't Q_ASSERT, it's ok to perform the operation inside the macro.

QVERIFY2(job->exec(), qPrintable(job->errorString()));
744

same here

753

This is missing a check for the value of the bool emitted.

QVERIFY(!spyUndoAvailable.at(0).at(0).toBool());
  • Address comments
elvisangelaccio marked 3 inline comments as done.Mar 10 2018, 6:42 PM
  • QLatin1String for concat
dfaure accepted this revision.Mar 10 2018, 7:37 PM
This revision is now accepted and ready to land.Mar 10 2018, 7:37 PM
This revision was automatically updated to reflect the committed changes.