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
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
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
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
elvisangelaccio marked 3 inline comments as done.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D10312
To: elvisangelaccio, dfaure
Cc: ngraham, #frameworks, michaelh
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
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
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
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
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
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);` ?
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
elvisangelaccio edited the summary of this revision.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D10312
To: elvisangelaccio, dfaure
Cc: #frameworks, michaelh, ngraham
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
14 matches
Mail list logo
