Bug 391606 - Undoing file removal causes an "Internal error in copyOrMove, should never happen" error
Summary: Undoing file removal causes an "Internal error in copyOrMove, should never ha...
Status: RESOLVED FIXED
Alias: None
Product: plasmashell
Classification: Plasma
Component: Folder (show other bugs)
Version: 5.12.2
Platform: Manjaro Linux
: NOR normal
Target Milestone: 1.0
Assignee: Eike Hein
URL:
Keywords:
: 393021 398169 404731 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-03-09 12:12 UTC by Alexey Boltenko
Modified: 2019-10-21 08:28 UTC (History)
11 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.62


Attachments
GIF recording of the bug. (2.33 MB, image/gif)
2018-03-09 12:12 UTC, Alexey Boltenko
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Boltenko 2018-03-09 12:12:48 UTC
Created attachment 111276 [details]
GIF recording of the bug.

KDE Plasma Version: 5.12.2
KDE Frameworks Version: 5.43.0
Qt Version: 5.10.1
Kernel Version: 4.15.7-1-MANJARO

Steps to reproduce:
1. Delete any file/folder
2. Attempt to undo

This does not happen anywhere else.
Comment 1 Kai Uwe Broulik 2018-03-09 12:44:57 UTC
I bet that's an issue with desktop:/ KIO slave URL rewriting. I can only reproduce in FolderView when pointed at desktop:/ but not at e.g. $HOME

In Dolphin you don't get the option to Trash (but only Delete Permanently) for desktop:/ likely because of that. (However, I think you should be able to trash and restore files on the desktop, so I'd rather like to see that fixed than disable trashing)
Comment 2 Kai Uwe Broulik 2018-03-09 12:57:44 UTC
Bug seems to be in FileUndoManager. 

FolderModel does:
KIO::FileUndoManager::self()->recordJob(KIO::FileUndoManager::Trash, urls, QUrl(QStringLiteral("trash:/")), job);

When I trash from destkop:/ and try to restore, FileUndoManager tries to do

m_currentJob = KIO::rename(op.m_dst, op.m_src, KIO::HideProgressInfo);

with m_dst being trash:/foo and src being desktop:/foo but KIO::rename explicitly states that it would fail when "direct renaming" isn't possible which here it clearly isn't as trash and desktop are two different protocols. CC'ing David Faure
Comment 3 David Faure 2018-03-13 14:42:24 UTC
Try replacing rename() with file_move(), which is higher level and supports remote protocols?

That would be a more generic solution than trying to resolve to local paths in special cases.
Comment 4 jhana2017 2018-04-06 13:27:41 UTC
I am also affected by this.
Comment 5 Kai Uwe Broulik 2018-04-06 13:46:29 UTC
> Try replacing rename() with file_move(), which is higher level and supports remote protocols?

Further down there's a codepath m_current.isMoveCommand() || m_current.m_type == FileUndoManager::Trash using file_move which seems to be for this particular usecase. I'l see if shuffling the if statements around helps.
Comment 6 jhana2017 2018-04-06 13:51:24 UTC
> I'l see if shuffling the if statements around helps.

Sound like a great debugging strategy. xD
Comment 7 Kai Uwe Broulik 2018-04-06 15:03:13 UTC
Patch: https://phabricator.kde.org/D11987

(If I start growing gray hair you know why ;)
Comment 8 Kai Uwe Broulik 2018-04-12 11:50:48 UTC
*** Bug 393021 has been marked as a duplicate of this bug. ***
Comment 9 Kai Uwe Broulik 2018-09-03 06:51:10 UTC
*** Bug 398169 has been marked as a duplicate of this bug. ***
Comment 10 Patrick Silva 2018-10-28 14:29:14 UTC
Bug persists.

Operating System: Arch Linux 
KDE Plasma Version: 5.14.2
Qt Version: 5.12.0 beta3
KDE Frameworks Version: 5.51.0
Comment 11 Patrick Silva 2019-01-18 00:55:03 UTC
plasma 5.15 beta has the same bug.

Operating System: Arch Linux 
KDE Plasma Version: 5.14.90
KDE Frameworks Version: 5.54.0
Qt Version: 5.12.0
Comment 12 Patrick Silva 2019-02-23 12:36:23 UTC
*** Bug 404731 has been marked as a duplicate of this bug. ***
Comment 13 Ash 2019-09-02 01:46:27 UTC
This is still happening, any movement on that patch? Might also suggest a new error message.

OS: Arch Linux
KDE Plasma Version: 5.16.4
KDE Frameworks Version: 5.61.0
Qt Version: 5.13.0
Comment 14 David Faure 2019-09-07 13:54:32 UTC
Git commit 994d9030d2e3f60764338a69e67270a5f1905fa8 by David Faure.
Committed on 07/09/2019 at 13:54.
Pushed by dfaure into branch 'master'.

[CopyJob] Use resolved URL in copyingDone so undo can rename back using local file

Summary:
In case of desktop:/ KIO which rewrites URLs, looking up the URL the trashed file got didn't work.
This resulted in trash:/filename being recorded by the undo manager which then
failed to restore the file as it was actually trashed to e.g. trash:/0-filename.

In addition, the source URL must be resolved too, since kio_trash
doesn't support renaming from trash to desktop URLs.
FIXED-IN: 5.62

CHANGELOG: Undoing trashing files on the desktop has been fixed

Thanks: to Kai-Uwe for the debugging and the initial patch; to Nate for pinging me a few times for this to happen ;)

Test Plan:
Extended kio_desktop's unittest: http://www.davidfaure.fr/2019/test_trash_and_undo.diff
Did not test as user.

Reviewers: broulik, ngraham

Reviewed By: ngraham

Subscribers: kde-frameworks-devel

Tags: #frameworks

Differential Revision: https://phabricator.kde.org/D23758

M  +3    -1    src/core/copyjob.cpp

https://commits.kde.org/kio/994d9030d2e3f60764338a69e67270a5f1905fa8
Comment 15 David Faure 2019-10-21 08:28:20 UTC
Git commit 4d216b0a019e22940593435d80aa4c0bd7cd429b by David Faure.
Committed on 21/10/2019 at 08:28.
Pushed by dfaure into branch 'master'.

Add unittest for KIO fix for undoing trashing files from the desktop

See KIO commit 994d9030d2e3f60 in KF 5.62

M  +31   -0    kioslave/desktop/tests/kio_desktop_test.cpp

https://commits.kde.org/plasma-workspace/4d216b0a019e22940593435d80aa4c0bd7cd429b