[krita/video-export-rebased] libs/ui: Save to a temporary file, then copy the result over on success (fwd)

[Boudewijn Rempt]

Hi,

This patch should make saving/exporting super-safe but it does
need testing... 

We're now saving to a temporary file, then we
rename the original file to a new name, then copy the temporary
file to the original name, then remove the renamed original.

Theoretically, nothing can go wrong anymore and saving cannot
corrupt an existing file anymore.

-- 
Boudewijn Rempt | http://www.krita.org, http://www.valdyas.org

---------- Forwarded message ----------
Date: Tue, 16 Aug 2016 13:57:31 +0000
From: Boudewijn Rempt <boud at valdyas.org>
Reply-To: kde-commits at kde.org
To: kde-commits at kde.org
Subject: [krita/video-export-rebased] libs/ui: Save to a temporary file,
    then copy the result over on success

Git commit 189daca58291cf6d3443fdc53eac6d579c66086e by Boudewijn Rempt.
Committed on 16/08/2016 at 13:46.
Pushed by rempt into branch 'video-export-rebased'.

Save to a temporary file, then copy the result over on success

I thought we were already doing this, but apparently that only
was done for remote urls, back in the days we still supported that.

The temporary file is saved in the tmp dir, and then copied over.
We even make a safety copy of the original file if it exists before
doing the copy. This should make saving over network drives faster
and more reliable, as well as saving in general more robust.

BUG:366765,357132,355726

M  +63   -17   libs/ui/KisDocument.cpp

http://commits.kde.org/krita/189daca58291cf6d3443fdc53eac6d579c66086e

diff --git a/libs/ui/KisDocument.cpp b/libs/ui/KisDocument.cpp
index 7421873..b33a6f5 100644
--- a/libs/ui/KisDocument.cpp
+++ b/libs/ui/KisDocument.cpp
@@ -649,8 +649,6 @@ bool KisDocument::exportDocument(const QUrl &_url, KisPropertiesConfigurationSP
 
 bool KisDocument::saveFile(KisPropertiesConfigurationSP exportConfiguration)
 {
-    dbgUI << "doc=" << url().url();
-
     // Save it to be able to restore it after a failed save
     const bool wasModified = isModified();
 
@@ -674,12 +672,19 @@ bool KisDocument::saveFile(KisPropertiesConfigurationSP exportConfiguration)
 
     setFileProgressUpdater(i18n("Saving Document"));
 
-    if (!isNativeFormat(outputMimeType)) {
-        dbgUI << "Saving to format" << outputMimeType << "in" << localFilePath();
+    QFileInfo fi(localFilePath());
+    QString tempororaryFileName;
+    {
+        QTemporaryFile tf(QDir::tempPath() + "/XXXXXX" + fi.baseName() + "." + fi.completeSuffix());
+        tf.open();
+        tempororaryFileName = tf.fileName();
+    }
+    Q_ASSERT(!tempororaryFileName.isEmpty());
 
+    if (!isNativeFormat(outputMimeType)) {
         Private::SafeSavingLocker locker(d);
         if (locker.successfullyLocked()) {
-            status = d->importExportManager->exportDocument(localFilePath(), outputMimeType, exportConfiguration);
+            status = d->importExportManager->exportDocument(tempororaryFileName, outputMimeType, exportConfiguration);
         } else {
             status = KisImportExportFilter::UsageError;
         }
@@ -689,11 +694,9 @@ bool KisDocument::saveFile(KisPropertiesConfigurationSP exportConfiguration)
         dbgFile << "Export status was" << status;
     } else {
         // Native format => normal save
-        Q_ASSERT(!localFilePath().isEmpty());
-        ret = saveNativeFormat(localFilePath());
+        ret = saveNativeFormat(tempororaryFileName);
     }
 
-
     if (ret) {
         if (!d->suppressProgress) {
             QPointer<KoUpdater> updater = d->progressUpdater->startSubtask(1, "clear undo stack");
@@ -703,15 +706,58 @@ bool KisDocument::saveFile(KisPropertiesConfigurationSP exportConfiguration)
         } else {
             d->undoStack->setClean();
         }
-        removeAutoSaveFiles();
+
+        QFile tempFile(tempororaryFileName);
+        QString s = localFilePath();
+        QFile dstFile(s);
+        while (QFileInfo(s).exists()) {
+            s.append("_");
+        }
+        bool r;
+        if (s != localFilePath()) {
+            r = dstFile.rename(s);
+            if (!r) {
+               setErrorMessage(i18n("Could not rename original file to %1: %2", dstFile.fileName(), dstFile. errorString()));
+            }
+         }
+
+        if (tempFile.exists()) {
+
+            r = tempFile.copy(localFilePath());
+            if (!r) {
+                setErrorMessage(i18n("Copying the temporary file failed: %1 to %2: %3", tempFile.fileName(), dstFile.fileName(), tempFile.errorString()));
+            }
+            else {
+                r = tempFile.remove();
+                if (!r) {
+                    setErrorMessage(i18n("Could not remove temporary file %1: %2", tempFile.fileName(), tempFile.errorString()));
+                }
+                else if (s != localFilePath()) {
+                    r = dstFile.remove();
+                    if (!r) {
+                        setErrorMessage(i18n("Could not remove saved original file: %1", dstFile.errorString()));
+                    }
+                }
+            }
+        }
+        else {
+            setErrorMessage(i18n("The temporary file %1 is gone before we could copy it!", tempFile.fileName()));
+        }
+
+        if (errorMessage().isEmpty()) {
+            removeAutoSaveFiles();
+        }
+        else {
+            qWarning() << "Error while saving:" << errorMessage();
+        }
         // Restart the autosave timer
         // (we don't want to autosave again 2 seconds after a real save)
         setAutoSave(d->autoSaveDelay);
-    }
-    clearFileProgressUpdater();
 
-    QApplication::restoreOverrideCursor();
-    if (!ret) {
+        d->mimeType = outputMimeType;
+        setConfirmNonNativeSave(isExporting(), false);
+    }
+    else {
         if (!suppressErrorDialog) {
 
             if (errorMessage().isEmpty()) {
@@ -742,10 +788,9 @@ bool KisDocument::saveFile(KisPropertiesConfigurationSP exportConfiguration)
         setModified(wasModified);
     }
 
-    if (ret) {
-        d->mimeType = outputMimeType;
-        setConfirmNonNativeSave(isExporting(), false);
-    }
+    clearFileProgressUpdater();
+
+    QApplication::restoreOverrideCursor();
 
     return ret;
 }
@@ -2125,6 +2170,7 @@ bool KisDocument::saveAs(const QUrl &kurl, KisPropertiesConfigurationSP exportCo
 
 bool KisDocument::save(KisPropertiesConfigurationSP exportConfiguration)
 {
+    qDebug() << "Saving!";
     d->m_saveOk = false;
     if ( d->m_file.isEmpty() ) { // document was created empty
         d->prepareSaving();