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

2018-03-11 Thread Elvis Angelaccio 
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:f6a2bb0b6836: FileUndoManager: dont delete 
non-existing local files (authored by elvisangelaccio).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10312?vs=29183=29210

REVISION DETAIL
  https://phabricator.kde.org/D10312

AFFECTED FILES
  autotests/fileundomanagertest.cpp
  autotests/fileundomanagertest.h
  src/widgets/fileundomanager.cpp

To: elvisangelaccio, dfaure
Cc: ngraham, #frameworks, michaelh

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

2018-03-10 Thread David Faure 
dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  undo-deleted-file (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D10312

To: elvisangelaccio, dfaure
Cc: ngraham, #frameworks, michaelh

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

2018-03-10 Thread Elvis Angelaccio 
elvisangelaccio updated this revision to Diff 29183.
elvisangelaccio added a comment.


  - QLatin1String for concat

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10312?vs=29182=29183

BRANCH
  undo-deleted-file (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D10312

AFFECTED FILES
  autotests/fileundomanagertest.cpp
  autotests/fileundomanagertest.h
  src/widgets/fileundomanager.cpp

To: elvisangelaccio, dfaure
Cc: ngraham, #frameworks, michaelh

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

2018-03-10 Thread Elvis Angelaccio 
elvisangelaccio updated this revision to Diff 29182.
elvisangelaccio added a comment.


  - Address comments

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10312?vs=27940=29182

BRANCH
  undo-deleted-file (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D10312

AFFECTED FILES
  autotests/fileundomanagertest.cpp
  autotests/fileundomanagertest.h
  src/widgets/fileundomanager.cpp

To: elvisangelaccio, dfaure
Cc: ngraham, #frameworks, michaelh

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

2018-03-10 Thread Elvis Angelaccio 
elvisangelaccio marked 3 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10312

To: elvisangelaccio, dfaure
Cc: ngraham, #frameworks, michaelh

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

2018-03-04 Thread David Faure 
dfaure added a comment.


  Looks good, just minor issues left.

INLINE COMMENTS

> fileundomanagertest.cpp:735
> +FileUndoManager::self()->recordCopyJob(copyJob);
> +const bool ok = copyJob->exec();
> +QVERIFY(ok);

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

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

> fileundomanagertest.cpp:744
> +const bool ok = deleteJob->exec();
> +QVERIFY(ok);
> +QVERIFY(!QFileInfo::exists(dest.toLocalFile()));

same here

> fileundomanagertest.cpp:753
> +FileUndoManager::self()->undo();
> +QCOMPARE(spyUndoAvailable.count(), 1);
> +QVERIFY(!FileUndoManager::self()->undoAvailable());

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

  QVERIFY(!spyUndoAvailable.at(0).at(0).toBool());

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10312

To: elvisangelaccio, dfaure
Cc: ngraham, #frameworks, michaelh

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

2018-02-24 Thread Elvis Angelaccio 
elvisangelaccio updated this revision to Diff 27940.
elvisangelaccio added a comment.


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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10312?vs=26542=27940

BRANCH
  undo-deleted-file (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D10312

AFFECTED FILES
  autotests/fileundomanagertest.cpp
  autotests/fileundomanagertest.h
  src/widgets/fileundomanager.cpp

To: elvisangelaccio, dfaure
Cc: ngraham, #frameworks, michaelh

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

2018-02-19 Thread David Faure 
dfaure added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10312

To: elvisangelaccio, dfaure
Cc: ngraham, #frameworks, michaelh

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

2018-02-18 Thread Elvis Angelaccio 
elvisangelaccio added inline comments.

INLINE COMMENTS

> dfaure wrote in fileundomanager.cpp:404
> Yep.

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10312

To: elvisangelaccio, dfaure
Cc: ngraham, #frameworks, michaelh

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

2018-02-11 Thread David Faure 
dfaure added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in fileundomanager.cpp:404
> You mean just an `emit undoAvailable(false);` ?

Yep.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10312

To: elvisangelaccio, dfaure
Cc: ngraham, #frameworks, michaelh

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

2018-02-10 Thread Elvis Angelaccio 
elvisangelaccio added inline comments.

INLINE COMMENTS

> dfaure wrote in 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.

You mean just an `emit undoAvailable(false);` ?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10312

To: elvisangelaccio, dfaure
Cc: ngraham, #frameworks, michaelh

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

2018-02-05 Thread David Faure 
dfaure added inline comments.

INLINE COMMENTS

> fileundomanager.cpp:404
> +} else if (commandType == FileUndoManager::Copy) {
> +d->slotLock();
> +return;

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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10312

To: elvisangelaccio, dfaure
Cc: ngraham, #frameworks, michaelh

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

2018-02-04 Thread Elvis Angelaccio 
elvisangelaccio edited the summary of this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10312

To: elvisangelaccio, dfaure
Cc: #frameworks, michaelh, ngraham

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

2018-02-04 Thread Elvis Angelaccio 
elvisangelaccio created this revision.
elvisangelaccio added a reviewer: dfaure.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
elvisangelaccio requested review of this revision.

REVISION 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 undo operation is
  not available anymore.

TEST PLAN
  From Dolphin:
  
  - Copy foo.txt to bar.txt
  - Shift+Del bar.txt
  - CTRL+Z

REPOSITORY
  R241 KIO

BRANCH
  undo-deleted-file (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D10312

AFFECTED FILES
  autotests/fileundomanagertest.cpp
  autotests/fileundomanagertest.h
  src/widgets/fileundomanager.cpp

To: elvisangelaccio, dfaure
Cc: #frameworks, michaelh, ngraham