Fix data loss in PropertyFileIncluded

src/App/Document.cpp

@@ -894,13 +894,6 @@ bool Document::saveAs(const char* file)
     if (this->FileName.getStrValue() != file) {
         this->FileName.setValue(file);
         this->Label.setValue(fi.fileNamePure());
-
-        //FIXME: At the moment PropertyFileIncluded doesn't work when renaming this directory.
-        //       PropertyFileIncluded shouldn't store the transient path
-#if 0
-        Base::Uuid id;
-        this->Uid.setValue(id);
-#endif
     }
 
     return save();

src/App/Property.h

@@ -61,7 +61,7 @@ public:
      */
     virtual unsigned int getMemSize (void) const {
         // you have to implement this method in all property classes!
-        return Base::Persistence::getMemSize() + sizeof(father) + sizeof(StatusBits);
+        return sizeof(father) + sizeof(StatusBits);
     }
 
     /// get the name of this property in the belonging container

src/App/PropertyFile.cpp

@@ -24,6 +24,7 @@
 #include "PreCompiled.h"
 
 #ifndef _PreComp_
+# include <algorithm>
 # include <sstream>
 #endif
 
@@ -35,6 +36,7 @@
 #include <Base/Stream.h>
 #include <Base/Console.h>
 #include <Base/PyObjectBase.h>
+#include <Base/Uuid.h>
 
 #include "PropertyFile.h"
 #include "Document.h"
@@ -53,6 +55,7 @@ using namespace std;
 
 TYPESYSTEM_SOURCE(App::PropertyFileIncluded , App::Property);
 
+
 PropertyFileIncluded::PropertyFileIncluded()
 {
 
@@ -69,11 +72,24 @@ PropertyFileIncluded::~PropertyFileIncluded()
 
 std::string PropertyFileIncluded::getDocTransientPath(void) const
 {
+    std::string path;
     PropertyContainer *co = getContainer();
-    if (co->isDerivedFrom(DocumentObject::getClassTypeId()))
+    if (co->isDerivedFrom(DocumentObject::getClassTypeId())) {
-        return dynamic_cast<DocumentObject*>(co)->getDocument()->TransientDir.getValue();
+        path = dynamic_cast<DocumentObject*>(co)->getDocument()->TransientDir.getValue();
+        std::replace(path.begin(), path.end(), '\\', '/');
+    }
+    return path;
+}
 
-    return std::string();
+std::string PropertyFileIncluded::getUniqueFileName(const std::string& path, const std::string& filename) const
+{
+    Base::Uuid uuid;
+    Base::FileInfo fi(path + "/" + filename);
+    while (fi.exists()) {
+        fi.setFile(path + "/" + filename + "." + uuid.getValue());
+    }
+
+    return fi.filePath();
 }
 
 std::string PropertyFileIncluded::getExchangeTempFile(void) const
@@ -97,7 +113,7 @@ void PropertyFileIncluded::setValue(const char* sFile, const char* sName)
             throw Base::Exception(str.str());
         }
 
-        aboutToSetValue(); // undo redo by move the file away with temp name
+        aboutToSetValue(); // undo/redo by moving the file away with temp name
 
         // remove old file (if not moved by undo)
         Base::FileInfo value(_cValue);
@@ -133,10 +149,9 @@ void PropertyFileIncluded::setValue(const char* sFile, const char* sName)
             _BaseFileName = file.fileName();
         }
 
-        // if the files is already in transient dir of the document, just use it
+        // if the file is already in transient dir of the document, just use it
         if (path == pathTrans) {
             bool done = file.renameFile(_cValue.c_str());
-            //assert(done);
             if (!done) {
                 std::stringstream str;
                 str << "Cannot rename file " << file.filePath() << " to " << _cValue;
@@ -160,7 +175,6 @@ void PropertyFileIncluded::setValue(const char* sFile, const char* sName)
             }
 
             bool done = file.copyTo(_cValue.c_str());
-            //assert(done); 
             if (!done) {
                 std::stringstream str;
                 str << "Cannot copy file from " << file.filePath() << " to " << _cValue;
@@ -180,7 +194,7 @@ const char* PropertyFileIncluded::getValue(void) const
 PyObject *PropertyFileIncluded::getPyObject(void)
 {
     PyObject *p = PyUnicode_DecodeUTF8(_cValue.c_str(),_cValue.size(),0);
-    if (!p) throw Base::Exception("UTF8 conversion failure at PropertyString::getPyObject()");
+    if (!p) throw Base::Exception("PropertyFileIncluded: UTF-8 conversion failure");
     return p;
 }
 
@@ -201,7 +215,7 @@ void PropertyFileIncluded::setPyObject(PyObject *value)
     }
     else if (PyTuple_Check(value)) {
         if (PyTuple_Size(value) != 2)
-            throw Py::TypeError("Tuple need size of (filePath,newFileName)"); 
+            throw Py::TypeError("Tuple needs size of (filePath,newFileName)"); 
         PyObject* file = PyTuple_GetItem(value,0);
         PyObject* name = PyTuple_GetItem(value,1);
 
@@ -220,7 +234,7 @@ void PropertyFileIncluded::setPyObject(PyObject *value)
             fileStr = PyString_AsString(FileName);
         }
         else {
-            std::string error = std::string("first in tuple must be a file or string");
+            std::string error = std::string("First item in tuple must be a file or string");
             error += value->ob_type->tp_name;
             throw Py::TypeError(error);
         }
@@ -235,7 +249,7 @@ void PropertyFileIncluded::setPyObject(PyObject *value)
             nameStr = PyString_AsString(FileName);
         }
         else {
-            std::string error = std::string("second in tuple must be a string");
+            std::string error = std::string("Second item in tuple must be a string");
             error += value->ob_type->tp_name;
             throw Py::TypeError(error);
         }
@@ -245,7 +259,7 @@ void PropertyFileIncluded::setPyObject(PyObject *value)
 
     }
     else {
-        std::string error = std::string("type must be str or file");
+        std::string error = std::string("Type must be string or file");
         error += value->ob_type->tp_name;
         throw Py::TypeError(error);
     }
@@ -256,29 +270,40 @@ void PropertyFileIncluded::setPyObject(PyObject *value)
 
 void PropertyFileIncluded::Save (Base::Writer &writer) const
 {
+#if 0
+    // when saving a document under a new file name the transient directory
+    // name changes and thus the stored file name doesn't work any more.
+    if (!_cValue.empty() && !Base::FileInfo(_cValue).exists()) {
+        Base::FileInfo fi(getDocTransientPath() + "/" + _BaseFileName);
+        if (fi.exists())
+            _cValue = fi.filePath();
+    }
+#endif
     if (writer.isForceXML()) {
         if (!_cValue.empty()) {
             Base::FileInfo file(_cValue.c_str());
-            writer.Stream() << writer.ind() << "<FileIncluded data=\"" << 
+            writer.Stream() << writer.ind() << "<FileIncluded data=\""
-            file.fileName() << "\">" << std::endl;
+                            << file.fileName() << "\">" << std::endl;
             // write the file in the XML stream
             writer.incInd();
             writer.insertBinFile(_cValue.c_str());
             writer.decInd();
             writer.Stream() << writer.ind() <<"</FileIncluded>" << endl;
         }
-        else
+        else {
             writer.Stream() << writer.ind() << "<FileIncluded data=\"\"/>" << std::endl;
+        }
     }
     else {
         // instead initiate an extra file 
         if (!_cValue.empty()) {
             Base::FileInfo file(_cValue.c_str());
-            writer.Stream() << writer.ind() << "<FileIncluded file=\"" << 
+            writer.Stream() << writer.ind() << "<FileIncluded file=\""
-            writer.addFile(file.fileName().c_str(), this) << "\"/>" << std::endl;
+                            << writer.addFile(file.fileName().c_str(), this) << "\"/>" << std::endl;
         }
-        else
+        else {
             writer.Stream() << writer.ind() << "<FileIncluded file=\"\"/>" << std::endl;
+        }
     }
 }
 
@@ -315,9 +340,12 @@ void PropertyFileIncluded::Restore(Base::XMLReader &reader)
 void PropertyFileIncluded::SaveDocFile (Base::Writer &writer) const
 {
     Base::ifstream from(Base::FileInfo(_cValue.c_str()));
-    if (!from)
+    if (!from) {
-        throw Base::Exception("PropertyFileIncluded::SaveDocFile() "
+        std::stringstream str;
-        "File in document transient dir deleted");
+        str << "PropertyFileIncluded::SaveDocFile(): "
+            << "File '" << _cValue << "' in transient directory doesn't exist.";
+        throw Base::Exception(str.str());
+    }
 
     // copy plain data
     unsigned char c;
@@ -330,9 +358,12 @@ void PropertyFileIncluded::SaveDocFile (Base::Writer &writer) const
 void PropertyFileIncluded::RestoreDocFile(Base::Reader &reader)
 {
     Base::ofstream to(Base::FileInfo(_cValue.c_str()));
-    if (!to) 
+    if (!to) {
-        throw Base::Exception("PropertyFileIncluded::RestoreDocFile() "
+        std::stringstream str;
-        "File in document transient dir deleted");
+        str << "PropertyFileIncluded::RestoreDocFile(): "
+            << "File '" << _cValue << "' in transient directory doesn't exist.";
+        throw Base::Exception(str.str());
+    }
 
     // copy plain data
     aboutToSetValue();
@@ -351,18 +382,23 @@ Property *PropertyFileIncluded::Copy(void) const
     // remember the base name
     prop->_BaseFileName = _BaseFileName;
 
-    if (!_cValue.empty()) {
+    Base::FileInfo file(_cValue);
-        Base::FileInfo file(_cValue);
+    if (file.exists()) {
-
         // create a new name in the document transient directory
-        Base::FileInfo NewName(Base::FileInfo::getTempFileName(file.fileName().c_str(),file.dirPath().c_str()));
+        Base::FileInfo newName(getUniqueFileName(file.dirPath(), file.fileName()));
-        NewName.deleteFile();
+        // copy the file
-        // move the file 
+        bool done = file.copyTo(newName.filePath().c_str());
-        bool done = file.renameFile(NewName.filePath().c_str());
+        if (!done) {
-        assert(done);
+            std::stringstream str;
+            str << "PropertyFileIncluded::Copy(): "
+                << "Copying the file '" << file.filePath() << "' to '"
+                << newName.filePath() << "' failed.";
+            throw Base::Exception(str.str());
+        }
+
         // remember the new name for the Undo
-        Base::Console().Log("Copy this=%p Before=%s After=%s\n",prop,prop->_cValue.c_str(),NewName.filePath().c_str());
+        Base::Console().Log("Copy '%s' to '%s'\n",_cValue.c_str(),newName.filePath().c_str());
-        prop->_cValue = NewName.filePath().c_str();
+        prop->_cValue = newName.filePath().c_str();
     }
 
     return prop;
@@ -371,26 +407,45 @@ Property *PropertyFileIncluded::Copy(void) const
 void PropertyFileIncluded::Paste(const Property &from)
 {
     aboutToSetValue();
-    Base::FileInfo file(_cValue);
+    const PropertyFileIncluded &prop = dynamic_cast<const PropertyFileIncluded&>(from);
-    // delete old file (if still there)
+    // make sure that source and destination file are different
-    file.deleteFile();
+    if (_cValue != prop._cValue) {
-    const PropertyFileIncluded &fileInc = dynamic_cast<const PropertyFileIncluded&>(from);
+        // delete old file (if still there)
+        Base::FileInfo(_cValue).deleteFile();
 
-    // set the base name
+        // get path to destination which can be the transient directory
-    _BaseFileName = fileInc._BaseFileName;
+        // of another document
+        std::string path = getDocTransientPath();
+        Base::FileInfo fiSrc(prop._cValue);
+        Base::FileInfo fiDst(path + "/" + prop._BaseFileName);
+        if (fiSrc.exists()) {
+            fiDst.setFile(getUniqueFileName(fiDst.dirPath(), fiDst.fileName()));
+            if (!fiSrc.copyTo(fiDst.filePath().c_str())) {
+                std::stringstream str;
+                str << "PropertyFileIncluded::Paste(): "
+                    << "Copying the file '" << fiSrc.filePath() << "' to '"
+                    << fiDst.filePath() << "' failed.";
+                throw Base::Exception(str.str());
+            }
+            _cValue = fiDst.filePath();
+        }
+        else {
+            _cValue.clear();
+        }
 
-    if (!fileInc._cValue.empty()) {
+        // set the base name
-        // move the saved files back in place
+        _BaseFileName = prop._BaseFileName;
-        Base::FileInfo NewFile(fileInc._cValue);
-        _cValue = NewFile.dirPath() + "/" + fileInc._BaseFileName;
-        bool done = NewFile.renameFile(_cValue.c_str());
-        assert(done);
     }
-    else
-        _cValue.clear();
     hasSetValue();
 }
 
+unsigned int PropertyFileIncluded::getMemSize (void) const
+{
+    unsigned int mem = Property::getMemSize();
+    mem += _cValue.size();
+    mem += _BaseFileName.size();
+    return mem;
+}
 
 //**************************************************************************
 // PropertyFile

src/App/PropertyFile.h

@@ -95,9 +95,7 @@ public:
 
     virtual Property *Copy(void) const;
     virtual void Paste(const Property &from);
-
+    virtual unsigned int getMemSize (void) const;
-    // get the transient path if the property is in a DocumentObject
-    std::string getDocTransientPath(void) const;
 
     /** get a temp file name in the transient path of the document.
       * Using this file for new Version of the file and set 
@@ -107,8 +105,13 @@ public:
     std::string getExchangeTempFile(void) const;
 
 protected:
-    std::string _cValue;
+    // get the transient path if the property is in a DocumentObject
-    std::string _BaseFileName;
+    std::string getDocTransientPath(void) const;
+    std::string getUniqueFileName(const std::string&, const std::string&) const;
+
+protected:
+    mutable std::string _cValue;
+    mutable std::string _BaseFileName;
 };
 
 

src/Base/FileInfo.cpp

@@ -26,6 +26,7 @@
 #include "PreCompiled.h"
 
 #ifndef _PreComp_
+# include <algorithm>
 # include <cassert>
 # include <cstdio>
 # include <cstdlib>
@@ -188,22 +189,13 @@ std::string FileInfo::getTempFileName(const char* FileName, const char* Path)
 
 void FileInfo::setFile(const char* name)
 {
-    std::string result;
+    if (!name) {
-    const char *It=name;
+        FileName.clear();
-
+        return;
-    while(*It != '\0') {
-        switch(*It)
-        {
-        case '\\':
-            result += "/";
-            break;
-        default:
-            result += *It;
-        }
-        It++;
     }
 
-    FileName = result;
+    FileName = name;
+    std::replace(FileName.begin(), FileName.end(), '\\', '/');
 }
 
 std::string FileInfo::filePath () const

src/Mod/Test/Document.py

@@ -180,7 +180,6 @@ class DocumentSaveRestoreCases(unittest.TestCase):
   def testSaveAndRestore(self):
     # saving and restoring
     SaveName = self.TempPath + os.sep + "SaveRestoreTests.FCStd"
-    self.Doc.FileName = SaveName
     self.failUnless(self.Doc.Label_1.TypeTransient == 4711)
     self.Doc.Label_1.TypeTransient = 4712
     # setup Linking
@@ -189,7 +188,7 @@ class DocumentSaveRestoreCases(unittest.TestCase):
     self.Doc.Label_1.LinkSub = (self.Doc.Label_2,["Sub1","Sub2"])
     self.Doc.Label_2.LinkSub = (self.Doc.Label_1,["Sub3","Sub4"])
     # save the document
-    self.Doc.save()
+    self.Doc.saveAs(SaveName)
     FreeCAD.closeDocument("SaveRestoreTests")
     self.Doc = FreeCAD.open(SaveName)
     self.failUnless(self.Doc.Label_1.Integer == 4711)
@@ -207,8 +206,8 @@ class DocumentSaveRestoreCases(unittest.TestCase):
     Doc = FreeCAD.newDocument("RestoreTests")
     Doc.addObject("App::FeatureTest","Label_1")
     # saving and restoring
-    Doc.FileName = self.TempPath + os.sep + "Test2.FCStd"
+    FileName = self.TempPath + os.sep + "Test2.FCStd"
-    Doc.save()
+    Doc.saveAs(FileName)
     # restore must first clear the current content
     Doc.restore()
     self.failUnless(len(Doc.Objects) == 1)
@@ -563,13 +562,12 @@ class DocumentPlatformCases(unittest.TestCase):
     self.Doc.addObject("App::FeatureTest", "Test")
     self.TempPath = tempfile.gettempdir()
     self.DocName = self.TempPath + os.sep + "PlatformTests.FCStd"
-    self.Doc.FileName = self.DocName
 
   def testFloatList(self):
     self.Doc.Test.FloatList = [-0.05, 2.5, 5.2]
 
     # saving and restoring
-    self.Doc.save()
+    self.Doc.saveAs(self.DocName)
     FreeCAD.closeDocument("PlatformTests")
     self.Doc = FreeCAD.open(self.DocName)
 
@@ -581,7 +579,7 @@ class DocumentPlatformCases(unittest.TestCase):
     self.Doc.Test.ColourList = [(1.0,0.5,0.0),(0.0,0.5,1.0)]
 
     # saving and restoring
-    self.Doc.save()
+    self.Doc.saveAs(self.DocName)
     FreeCAD.closeDocument("PlatformTests")
     self.Doc = FreeCAD.open(self.DocName)
 
@@ -598,7 +596,7 @@ class DocumentPlatformCases(unittest.TestCase):
     self.Doc.Test.VectorList = [(-0.05, 2.5, 5.2),(-0.05, 2.5, 5.2)]
 
     # saving and restoring
-    self.Doc.save()
+    self.Doc.saveAs(self.DocName)
     FreeCAD.closeDocument("PlatformTests")
     self.Doc = FreeCAD.open(self.DocName)
 
@@ -609,7 +607,7 @@ class DocumentPlatformCases(unittest.TestCase):
       self.Doc.addObject("Points::Feature", "Points")
 
       # saving and restoring
-      self.Doc.save()
+      self.Doc.saveAs(self.DocName)
       FreeCAD.closeDocument("PlatformTests")
       self.Doc = FreeCAD.open(self.DocName)
 
@@ -679,10 +677,10 @@ class DocumentFileIncludeCases(unittest.TestCase):
     self.failUnless(file.read()=="test No2")
     file.close()
     # Save restore test
-    self.Doc.FileName = self.TempPath+"/FileIncludeTest.fcstd"
+    FileName = self.TempPath+"/FileIncludeTests.fcstd"
-    self.Doc.save()
+    self.Doc.saveAs(FileName)
     FreeCAD.closeDocument("FileIncludeTests")
-    self.Doc = FreeCAD.open(self.TempPath+"/FileIncludeTest.fcstd")
+    self.Doc = FreeCAD.open(self.TempPath+"/FileIncludeTests.fcstd")
     # check if the file is still there
     self.L1 = self.Doc.getObject("FileObject1")
     file = open(self.L1.File,"r")
@@ -715,10 +713,20 @@ class DocumentFileIncludeCases(unittest.TestCase):
     self.failUnless(file.read()=="test No2")
     file.close()
 
+    # create a second document, copy a file and close the document
+    # the test is about to put the file to the correct transient dir
+    doc2 = FreeCAD.newDocument("Doc2")
+    L4 = doc2.addObject("App::DocumentObjectFileIncluded","FileObject")
+    L4.File = (L3.File,"Test.txt")
+    FreeCAD.closeDocument("FileIncludeTests")
+    self.Doc = FreeCAD.open(self.TempPath+"/FileIncludeTests.fcstd")
+    self.failUnless(os.path.exists(L4.File))
+    FreeCAD.closeDocument("Doc2")
+
 
   def tearDown(self):
     #closing doc
-    FreeCAD.closeDocument("FileIncludeTest")
+    FreeCAD.closeDocument("FileIncludeTests")
 
 
 class DocumentPropertyCases(unittest.TestCase):
@@ -733,8 +741,7 @@ class DocumentPropertyCases(unittest.TestCase):
         self.Obj.addProperty(i,i)
     tempPath = tempfile.gettempdir()
     tempFile = tempPath + os.sep + "PropertyTests.FCStd"
-    self.Doc.FileName = tempFile
+    self.Doc.saveAs(tempFile)
-    self.Doc.save()
     FreeCAD.closeDocument("PropertyTests")
     self.Doc = FreeCAD.open(tempFile)