Importer: convert all file operations to allow remote URLs
ClosedPublic

Authored by bdevries on Oct 22 2019, 8:47 PM.

Details

Summary

All file operations are updated to allow remote URLs, both on the source
and destination side. This consists mainly of converting local file/dir
operations to KIO calls.
Remote source was already mostly covered.
To support remote destinations, a few adaptations were needed:

  • If destination is remote or a remote server mounted locally, then do not create the temporary directory as a subdir of the destination, but create it locally using default QTemporaryDir().
  • When comparing file contents, if destination is remote, use KIO::storedGet to get the file contents in one go. This is much simpler and more robust than using asynchronous KIO::open calls, but has the drawback that the file is read in one go instead of in chunks.
Test Plan

Added unit tests for remote source and destination.
Also tested manually by running gwenview_importer with a combination
of remote and local destinations, covering smb and sftp protocols, as
well as remote server mounted locally (e.g. cifs mount).
Also tested on multiple files with the same name existing in source
directory; both identical and different in content.

Diff Detail

Repository
R260 Gwenview
Branch
importer
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18638
Build 18656: arc lint + arc unit
bdevries requested review of this revision.Oct 22 2019, 8:47 PM
bdevries created this revision.
bdevries updated this revision to Diff 68686.Oct 24 2019, 2:47 PM

Use QScopedPointer for QIODevice pointers

Use QScopedPointer for pointers to QIODevices used for comparing file
contents. This is to avoid that the files remain open and memory leaks
at the end of this routine.

bdevries updated this revision to Diff 68715.Oct 24 2019, 8:28 PM

Add unit tests and solve bug when mulitiple files of same name exist.

Added unit tests for remote source and destination.
Solved a bug when multiple files of the same name exist on the source side.

bdevries edited the summary of this revision. (Show Details)Oct 24 2019, 8:31 PM
bdevries edited the test plan for this revision. (Show Details)
bdevries set the repository for this revision to R260 Gwenview.
ngraham added a subscriber: ngraham.Nov 8 2019, 4:09 PM

Nice work. Just a few inline comments:

importer/fileutils.cpp
62

uncomment or remove

77

uncomment or remove

86

uncomment or remove

87

misplaced comma

161

What's the reason for this change?

importer/importer.cpp
78

job, mAuthWindow

117

Seems unrelated; let's do this in a separate patch

bdevries updated this revision to Diff 69487.EditedNov 8 2019, 9:16 PM
bdevries marked 7 inline comments as done.

A few additional changes based on Nate's comments.

  • Remove commented qDebug statements
  • Fix commas and whitespace
  • Take out fix for unrelated bug (when importing two files with the same name when there is also a file with that name already in the destination) --> see D25224
bdevries added inline comments.Nov 8 2019, 9:17 PM
importer/fileutils.cpp
87

Wow, nice catch!

161

KIO::rename will only do simple renaming. E.g. it will fail if the file needs to be moved across partitions or over the network.
Since we can't assume anymore that the temporary directory is on the same partition/machine than the destination folder, we need to use KIO::moveAs (or KIO::move).
It should be just as efficient as KIO::rename, since the documentation mentions that KIO::move(As) will first try to do a simple rename. Only if that fails it will copy-and-delete.

importer/importer.cpp
78

Another nice catch!

117

Yes, it's is indeed not directly related. This solves a bug that already existed before this patch. I'll take it out and re-submit.

ngraham retitled this revision from Importer: convert all file operations to allow remote URLs. to Importer: convert all file operations to allow remote URLs.Dec 3 2019, 6:34 PM
ngraham accepted this revision.Dec 3 2019, 6:37 PM
This revision is now accepted and ready to land.Dec 3 2019, 6:37 PM
This revision was automatically updated to reflect the committed changes.