Create additional subfolders if needed (e.g. when extra slashes in FileNameFormater).
ClosedPublic

Authored by bdevries on Oct 28 2019, 8:40 PM.

Details

Summary

The importer will (recursively) create additional subfolders if the autoformatter pattern
contains slashes.
Add additional unit tests to importertest.
BUG: 237114
FEATURE: 336039
FIXED-IN: 19.12.0

Test Plan

Added additional unit tests to check the creation of subfolders when slashes are present
in the autoformatter pattern.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bdevries created this revision.Oct 28 2019, 8:40 PM
Restricted Application added a project: Gwenview. · View Herald TranscriptOct 28 2019, 8:40 PM
bdevries requested review of this revision.Oct 28 2019, 8:40 PM
ngraham edited the summary of this revision. (Show Details)Oct 29 2019, 4:50 PM
ngraham added a subscriber: ngraham.
ngraham added inline comments.
importer/importer.cpp
139

We should probably show something to the user here, or else the operation will have silently failed and who knows what kind of mischief can happen!

You're absolutely right that there needs to be a GUI warning/report of some kind. However, the current importer does not have the infrastructure to handle this: it does nothing with failed imports (of any kind) except print a warning message to stdout. See e.g. the FIXME/warning on lines 208 and 255.

On the other hand, with the current implementation no real misschief can/will happen, because the FileUtils::rename call on line 142 will always fail with result = FileUtils::RenameFailed in case the subfolder could not be created. The file will simply be skipped (but nothing will be reported), which is the current behavior of the importer for any kind of failed import.
What I can already do to slightly improve on this, is to make the failed subfolder creation more explicit by directly setting

result = FileUtils::RenameFailed

and then skipping the FileUtils::rename call (that will fail) altogether. What do you think? If so, I will update the diff.

Taking a step back (yet again :-) ), I would very much like to improve the error reporting if time permits.
Should we hold back this improvement until the infrastructure for reporting failed imports has been implemented, or not?

PS: Sorry, I couldn't figure out how to reply inline. It kept saying that my reply was unsubmitted...

bdevries updated this revision to Diff 69061.Oct 30 2019, 3:33 PM

Register as FileUtils::RenameFailed if the subfolder cannot be created.

If the subfolder cannot be created, it will immediately register
as FileUtils::RenameFailed and no attempt to actually rename the
file will be made.
The FileUtils::RenameFailed handle can then later on be used to
warn the user about which files failed to import. (This is
currently not properly implemented.)

bdevries marked an inline comment as done.Oct 31 2019, 8:24 AM

Register as FileUtils::RenameFailed if the subfolder cannot be created.

If the subfolder cannot be created, it will immediately register
as FileUtils::RenameFailed and no attempt to actually rename the
file will be made.
The FileUtils::RenameFailed handle can then later on be used to
warn the user about which files failed to import. (This is
currently not properly implemented.)

You wanna do that in this patch, or in another?

For traceability's sake, I would separate the proper error handling into a separate patch/commit.
It's completely missing at the moment, so it seems strange to mix it in with a "new feature".

I don't have a strong opinion about the order that the patches would then applied.

ngraham accepted this revision.Oct 31 2019, 6:47 PM

All right no problem, we can do this in another patch.

Nothing bad happens if we commit this now, right?

This revision is now accepted and ready to land.Oct 31 2019, 6:47 PM

No, nothing bad will happen. The importer will try to create a subfolder if needed. If it somehow doesn't succeed, it will result in a FileUtils::RenameFailed.
In that case the file will simply be skipped altogether. It will remain untouched on the source device, and it will also not be deleted if the user tells the importer to delete the imported files. Deleting is only done for successfully imported files.
I'm already working on the other patch. Luckily, it seems like it might be relatively simple to implement some basic UI warnings for failed items.

This revision was automatically updated to reflect the committed changes.