dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
Thanks for working on this, here's my review.
INLINE COMMENTS
> jobtest.cpp:1724
>
> +void JobTest::safeOverwrite()
> +{
This test is called Overwrite, and passes the Overwrite flag, but it's not
actually overwriting anything (destFile doesn't exist).
I think both cases should be tested --> make a _data() method with two rows,
destFileExists=false and destFileExists=true.
Apart from creating destFile is that bool is true, I think all the code of the
test remains the same.
Another issue: the whole test needs to be skipped on Windows (use QSKIP in
#ifdef), since you only implemented it in file_unix.cpp
> file_unix.cpp:288
> + if (!_dest_backup.isEmpty() && !orig_delete_attempted) {
> + int btnCode =
> messageBox(SlaveBase::WarningContinueCancel,
> + i18n("Proceeding further
> will destroy the destination file. Do you want to continue?")); //TODO better
> message
This is all in the context of KIO::Overwrite being set, programatically. So the
whole point *is* to overwrite without asking (this is often used after the user
has already approved overwriting, or for cases where the user should not be
involved like application-internal files).
So it makes zero sense to me to ask the user about overwriting yet again. In
fact the thing you're asking here is "do you agree that canceling before
completion will no longer preserve the existing destination file, as it would
do otherwise". In my opinion, this is too much detail, we shouldn't be asking a
question "just in case the user wants to cancel". In all the cases where the
user isn't going to cancel, this is really just an annoyance.
I view the feature (no data loss during cancel-of-overwrite) as a nice bonus,
but it's not a *bug* if an overwritten file is lost during an overwrite
operation, by definition.
So... I would suggest to proceed without asking.
> file_unix.cpp:422
> + if (::unlink(_dest_backup.data()) == -1) {
> + qCWarning(KIO_FILE) << QStringLiteral("Couldn't remove original
> dest '%1'").arg(QString::fromLocal8Bit(_dest_backup));
> + }
qCWarning supports <<, no need for arg().
The use of a QString will add double quotes automatically, so having double
quotes around the whole message will look strange.
Just do the usual
qCWarning() << "Couldn't ..." << _dest_backup;
> file_unix.cpp:426
> + if (::rename(_dest.data(), _dest_backup.data()) == -1) {
> + qCWarning(KIO_FILE) << QStringLiteral("Couldn't rename '%1' to
> '%2'").arg(dest).arg(QString::fromLocal8Bit(_dest_backup));
> + }
(same)
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D19080
To: chinmoyr, dfaure, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
