Thanks again for the patch!
Fri, Nov 8
Nice, will review soon.
A few additional changes based on Nate's comments.
Nice work. Just a few inline comments:
- Move connect to constructor
I've tested it and it works fine for me, thanks. One minor issue is noted below.
Tue, Nov 5
Mon, Nov 4
Ok. I would like to test this, will need a few days though.
- Remove unnecessary include
Thanks, I moved the reload() and added a signal readyForDirListerStart() to DocumentFactory, which is emitted when the document is loaded or failed. In ContextManager::setUrlToSelect() this signal is connected to the dirlister start.
- Move reload(), add DocumentFactory::readyForDirListerStart()
Sun, Nov 3
Sat, Nov 2
Thanks for the patch! The functionality seems to work fine. However it appears to cause a layout repression in the settings view:
The same determination is done in LoadingDocumentImpl::init().
Fri, Nov 1
Thu, Oct 31
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.
All right no problem, we can do this in another patch.
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".
Wed, Oct 30
Register as FileUtils::RenameFailed if the subfolder cannot be created.
The connects at ContextManager::setUrlToSelect() only work with the if (UrlUtils::urlIsFastLocalFile()) determination.
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.
Tue, Oct 29
That looks better, thanks. Not sure if it's the best place for the connect, but I'll leave that to Nate.
You are right, I changed it to use the signals from the document loaded (or not).
- Start dirlister on document signals
Mon, Oct 28
I'll certainly consider it.
Let's see if I can find the time. :-)
That makes sense to me!
Swap keep and delete buttons. This makes keep the default action.
make sure that an url passed on the command line is loaded and shown before the possibly long running dirlister