D19080: [WIP] Make file overwrite a bit safer

[David Faure]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190216/d968985b/attachment-0001.html>