With KDE Applications 17.04, Gwenview Importer was reintroduced into the KF5 world (yay!). However, using it to import pictures e.g. from a memory card, camera device or local directory does not work. At the same time, choosing to "delete the <N> imported documents from the device" after the import "finished", works rather well. This leaves us with no pictures on both the device and the import destination, potentially calling for a lengthy recovery session with PhotoRec. Note: This not only affects users manually calling the importer, but everyone using the import action provided by the device notifier popup when inserting external media. Steps to reproduce: - create backup copy of <folder with pictures> - $ gwenview_importer <folder with pictures> - choose <destination> - click on "Import All" - (notice how <destination> is still empty) - click on "Delete" - (notice how <folder with pictures> is now empty, too) - dismiss warning ("Failed to delete document: Unknown error code 0"), which actually means "Failed to import, succeeded to delete" Expected result: Pictures moved (or copied, when choosing "Keep" instead of "Delete") from <folder with pictures> to <destination>.
Proposal for fix: https://phabricator.kde.org/D5749
Git commit 6ce5d2f8d82f83c5a3d726ee5807ebaf7a6e3c6c by Luigi Toscano, on behalf of Henrik Fehlauer. Committed on 11/05/2017 at 20:43. Pushed by ltoscano into branch 'Applications/17.04'. Avoid data loss when importing pictures Summary: Fix porting regressions, which left users of Gwenview Importer with: - failed import (import destination still empty) - additionally (when choosing "Delete" instead of "Keep" after import): pictures also removed from import source, with no way to recover Correct additional problems remaining after fixing the import failure: - hang on duplicate filenames - identically named files with different content are never imported - error dialog when deleting pictures from import source In detail: 1st problem (introduced in 017b4fe5dc7f4b6e876cfd7b108dcebbf609ae94): Initially, pictures are copied to a temporary folder (e.g. "foo/.gwenview_importer-IcQqvo/"), before being moved/renamed to the final destination (e.g. "foo/"), which is determined by calling "cd .." on the temporary folder. However, mistakenly this path contains a superfluous '/' (e.g. "foo/.gwenview_importer-IcQqvo//"), resulting in the final destination reading "foo/.gwenview_importer-IcQqvo/" instead of "foo/". On cleanup, the temporary folder is removed, simultaneously deleting all new pictures. Fix: Omit '/' where appropriate. 2nd problem (broken by a3262e9b197ee97323ef8bf3a9dca1e13f61a74c): When trying to find a unique filename, the while loop "stat"ing the file repeats forever. Fix: Call "KIO::stat" and "KJobWidgets::setWindow" also inside the loop. 3rd problem (introduced in 017b4fe5dc7f4b6e876cfd7b108dcebbf609ae94): If the destination directory already contains an identically named file, we try to find a different name by appending a number. For this, we need to strip the old filename from the full path. Unfortunately, only calling "path()" is incorrect, giving "foo/pict0001.jpg/pict0001_1.jpg", rather than "foo/pict0001_1.jpg". Fix: Use "adjusted(QUrl::RemoveFilename)". 4th problem (broken by a3262e9b197ee97323ef8bf3a9dca1e13f61a74c): Although deletion works, we show a warning dialog stating failure. Fix: Invert the logic of "job->exec()", as the error handling should be skipped by "break"ing out of the while loop. Test Plan: Steps to reproduce (see https://bugs.kde.org/show_bug.cgi?id=379615) no longer result in data loss. Autotests for importer (separate review request in D5750) pass. Hopefully, this helps catching any future porting regressions. Reviewers: ltoscano, sandsmark, gateau Reviewed By: ltoscano Differential Revision: https://phabricator.kde.org/D5749 M +4 -1 importer/fileutils.cpp M +1 -1 importer/importdialog.cpp M +2 -2 importer/importer.cpp https://commits.kde.org/gwenview/6ce5d2f8d82f83c5a3d726ee5807ebaf7a6e3c6c