Don't keep entries for XRC resources that failed to load in wxXmlResource.
authorVadim Zeitlin <vadim@wxwidgets.org>
Sat, 20 Nov 2010 23:53:34 +0000 (23:53 +0000)
committerVadim Zeitlin <vadim@wxwidgets.org>
Sat, 20 Nov 2010 23:53:34 +0000 (23:53 +0000)
Attempting to load a resource that couldn't be loaded resulted in
wxXmlResource::Load() returning false for this and _all_the_subsequent_ calls
to it because each call to Load() reattempted to reload all resources,
including the one(s) that failed to load initially.

Instead, try to load just the resource(s) that we should load right now and
ignore all the other ones. Also, don't add entries for the one(s) that we fail
to load.

This fixes the unit test failures in the XRC test case which was affected by
the test checking that XRC couldn't be loaded from garbage that ran before it.
It also makes the code simpler by ensuring that wxXmlResourceDataRecords
elements always have a valid wxXmlDocument associated with them.

Also clean up the code: use wxScopedPtr instead of manually deleting pointers
and reorganize #if checks to be easier to follow.

git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@66219 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775

include/wx/xrc/xmlres.h
src/xrc/xmlres.cpp

index 1530fab993670cacd44e9e9820017ed94f29ccfc..f7c6867929aeef9594a20eb2e0351419aec702ab 100644 (file)
@@ -299,6 +299,10 @@ protected:
     virtual void DoReportError(const wxString& xrcFile, const wxXmlNode *position,
                                const wxString& message);
 
+    // Load the contents of a single file and returns its contents as a new
+    // wxXmlDocument (which will be owned by caller) on success or NULL.
+    wxXmlDocument *DoLoadFile(const wxString& file);
+
     // Scans the resources list for unloaded files and loads them. Also reloads
     // files that have been modified since last loading.
     bool UpdateResources();
index eebc913691f3b996ea4dcbced1222ca0ee6371d7..1b62dbc3dcfe6a78e7099f748c5309d2ff1ed48c 100644 (file)
 #include "wx/dir.h"
 #include "wx/xml/xml.h"
 #include "wx/hashset.h"
+#include "wx/scopedptr.h"
 
+namespace
+{
+
+// Helper function to get modification time of either a wxFileSystem URI or
+// just a normal file name, depending on the build.
+#if wxUSE_DATETIME
+
+wxDateTime GetXRCFileModTime(const wxString& filename)
+{
+#if wxUSE_FILESYSTEM
+    wxFileSystem fsys;
+    wxScopedPtr<wxFSFile> file(fsys.OpenFile(filename));
+
+    return file ? file->GetModificationTime() : wxDateTime();
+#else // wxUSE_FILESYSTEM
+    return wxDateTime(wxFileModificationTime(filename));
+#endif // wxUSE_FILESYSTEM
+}
+
+#endif // wxUSE_DATETIME
+
+} // anonymous namespace
 
 class wxXmlResourceDataRecord
 {
 public:
-    wxXmlResourceDataRecord() : Doc(NULL) {
+    // Ctor takes ownership of the document pointer.
+    wxXmlResourceDataRecord(const wxString& File_,
+                            wxXmlDocument *Doc_
+                           )
+        : File(File_), Doc(Doc_)
+    {
 #if wxUSE_DATETIME
-        Time = wxDateTime::Now();
+        Time = GetXRCFileModTime(File);
 #endif
     }
+
     ~wxXmlResourceDataRecord() {delete Doc;}
 
     wxString File;
@@ -65,6 +94,8 @@ public:
 #if wxUSE_DATETIME
     wxDateTime Time;
 #endif
+
+    wxDECLARE_NO_COPY_CLASS(wxXmlResourceDataRecord);
 };
 
 class wxXmlResourceDataRecords : public wxVector<wxXmlResourceDataRecord*>
@@ -314,6 +345,8 @@ bool wxXmlResource::Load(const wxString& filemask_)
 {
     wxString filemask = ConvertFileNameToURL(filemask_);
 
+    bool allOK = true;
+
 #if wxUSE_FILESYSTEM
     wxFileSystem fsys;
 #   define wxXmlFindFirst  fsys.FindFirst(filemask, wxFILE)
@@ -335,14 +368,16 @@ bool wxXmlResource::Load(const wxString& filemask_)
         if ( IsArchive(fnd) )
         {
             if ( !Load(fnd + wxT("#zip:*.xrc")) )
-                return false;
+                allOK = false;
         }
         else // a single resource URL
 #endif // wxUSE_FILESYSTEM
         {
-            wxXmlResourceDataRecord *drec = new wxXmlResourceDataRecord;
-            drec->File = fnd;
-            Data().push_back(drec);
+            wxXmlDocument * const doc = DoLoadFile(fnd);
+            if ( !doc )
+                allOK = false;
+            else
+                Data().push_back(new wxXmlResourceDataRecord(fnd, doc));
         }
 
         fnd = wxXmlFindNext;
@@ -350,7 +385,7 @@ bool wxXmlResource::Load(const wxString& filemask_)
 #   undef wxXmlFindFirst
 #   undef wxXmlFindNext
 
-    return UpdateResources();
+    return allOK;
 }
 
 bool wxXmlResource::Unload(const wxString& filename)
@@ -613,133 +648,133 @@ static void PreprocessForIdRanges(wxXmlNode *rootnode)
 bool wxXmlResource::UpdateResources()
 {
     bool rt = true;
-    bool modif;
-#   if wxUSE_FILESYSTEM
-    wxFSFile *file = NULL;
-    wxUnusedVar(file);
-    wxFileSystem fsys;
-#   endif
-
-    wxString encoding(wxT("UTF-8"));
-#if !wxUSE_UNICODE && wxUSE_INTL
-    if ( (GetFlags() & wxXRC_USE_LOCALE) == 0 )
-    {
-        // In case we are not using wxLocale to translate strings, convert the
-        // strings GUI's charset. This must not be done when wxXRC_USE_LOCALE
-        // is on, because it could break wxGetTranslation lookup.
-        encoding = wxLocale::GetSystemEncodingName();
-    }
-#endif
 
     for ( wxXmlResourceDataRecords::iterator i = Data().begin();
           i != Data().end(); ++i )
     {
         wxXmlResourceDataRecord* const rec = *i;
 
-        modif = (rec->Doc == NULL);
+        // Check if we need to reload this one.
 
-        if (!modif && !(m_flags & wxXRC_NO_RELOADING))
+        // We never do it if this flag is specified.
+        if ( m_flags & wxXRC_NO_RELOADING )
+            continue;
+
+        // Otherwise check its modification time if we can.
+#if wxUSE_DATETIME
+        const wxDateTime lastModTime = GetXRCFileModTime(rec->File);
+
+        if ( lastModTime.IsValid() && lastModTime <= rec->Time )
+#else // !wxUSE_DATETIME
+        // Never reload the file contents: we can't know whether it changed or
+        // not in this build configuration and it would be unexpected and
+        // counter-productive to get a performance hit (due to constant
+        // reloading of XRC files) in a minimal wx build which is presumably
+        // used because of resource constraints of the current platform.
+#endif // wxUSE_DATETIME/!wxUSE_DATETIME
         {
-#           if wxUSE_FILESYSTEM
-            file = fsys.OpenFile(rec->File);
-#           if wxUSE_DATETIME
-            modif = file && file->GetModificationTime() > rec->Time;
-#           else // wxUSE_DATETIME
-            modif = true;
-#           endif // wxUSE_DATETIME
-            if (!file)
-            {
-                wxLogError(_("Cannot open file '%s'."), rec->File);
-                rt = false;
-            }
-            wxDELETE(file);
-            wxUnusedVar(file);
-#           else // wxUSE_FILESYSTEM
-#           if wxUSE_DATETIME
-            modif = wxDateTime(wxFileModificationTime(rec->File)) > rec->Time;
-#           else // wxUSE_DATETIME
-            modif = true;
-#           endif // wxUSE_DATETIME
-#           endif // wxUSE_FILESYSTEM
+            // No need to reload, the file wasn't modified since we did it
+            // last.
+            continue;
         }
 
-        if (modif)
+        wxXmlDocument * const doc = DoLoadFile(rec->File);
+        if ( !doc )
         {
-            wxLogTrace(wxT("xrc"), wxT("opening file '%s'"), rec->File);
+            // Notice that we keep the old XML document: it seems better to
+            // preserve it instead of throwing it away if we have nothing to
+            // replace it with.
+            rt = false;
+            continue;
+        }
 
-            wxInputStream *stream = NULL;
+        // Replace the old resource contents with the new one.
+        delete rec->Doc;
+        rec->Doc = doc;
 
-#           if wxUSE_FILESYSTEM
-            file = fsys.OpenFile(rec->File);
-            if (file)
-                stream = file->GetStream();
-#           else
-            stream = new wxFileInputStream(rec->File);
-#           endif
+        // And, now that we loaded it successfully, update the last load time.
+#if wxUSE_DATETIME
+        rec->Time = lastModTime.IsValid() ? lastModTime : wxDateTime::Now();
+#endif // wxUSE_DATETIME
+    }
 
-            if (stream)
-            {
-                delete rec->Doc;
-                rec->Doc = new wxXmlDocument;
-            }
-            if (!stream || !stream->IsOk() || !rec->Doc->Load(*stream, encoding))
-            {
-                wxLogError(_("Cannot load resources from file '%s'."),
-                           rec->File);
-                wxDELETE(rec->Doc);
-                rt = false;
-            }
-            else if (rec->Doc->GetRoot()->GetName() != wxT("resource"))
-            {
-                ReportError
-                (
-                    rec->Doc->GetRoot(),
-                    "invalid XRC resource, doesn't have root node <resource>"
-                );
-                wxDELETE(rec->Doc);
-                rt = false;
-            }
-            else
-            {
-                long version;
-                int v1, v2, v3, v4;
-                wxString verstr = rec->Doc->GetRoot()->GetAttribute(
-                                      wxT("version"), wxT("0.0.0.0"));
-                if (wxSscanf(verstr.c_str(), wxT("%i.%i.%i.%i"),
-                    &v1, &v2, &v3, &v4) == 4)
-                    version = v1*256*256*256+v2*256*256+v3*256+v4;
-                else
-                    version = 0;
-                if (m_version == -1)
-                    m_version = version;
-                if (m_version != version)
-                {
-                    wxLogError("Resource files must have same version number.");
-                    rt = false;
-                }
+    return rt;
+}
+
+wxXmlDocument *wxXmlResource::DoLoadFile(const wxString& filename)
+{
+    wxLogTrace(wxT("xrc"), wxT("opening file '%s'"), filename);
+
+    wxInputStream *stream = NULL;
 
-                ProcessPlatformProperty(rec->Doc->GetRoot());
-                PreprocessForIdRanges(rec->Doc->GetRoot());
-                wxIdRangeManager::Get()->FinaliseRanges(rec->Doc->GetRoot());
-#if wxUSE_DATETIME
 #if wxUSE_FILESYSTEM
-                rec->Time = file->GetModificationTime();
-#else // wxUSE_FILESYSTEM
-                rec->Time = wxDateTime(wxFileModificationTime(rec->File));
-#endif // wxUSE_FILESYSTEM
-#endif // wxUSE_DATETIME
-            }
+    wxFileSystem fsys;
+    wxScopedPtr<wxFSFile> file(fsys.OpenFile(filename));
+    if (file)
+    {
+        // Notice that we don't have ownership of the stream in this case, it
+        // remains owned by wxFSFile.
+        stream = file->GetStream();
+    }
+#else // !wxUSE_FILESYSTEM
+    wxFileInputStream fstream(filename);
+    stream = &fstream;
+#endif // wxUSE_FILESYSTEM/!wxUSE_FILESYSTEM
 
-#           if wxUSE_FILESYSTEM
-                wxDELETE(file);
-                wxUnusedVar(file);
-#           else
-                wxDELETE(stream);
-#           endif
-        }
+    if ( !stream || !stream->IsOk() )
+    {
+        wxLogError(_("Cannot open resources file '%s'."), filename);
+        return NULL;
     }
 
-    return rt;
+    wxString encoding(wxT("UTF-8"));
+#if !wxUSE_UNICODE && wxUSE_INTL
+    if ( (GetFlags() & wxXRC_USE_LOCALE) == 0 )
+    {
+        // In case we are not using wxLocale to translate strings, convert the
+        // strings GUI's charset. This must not be done when wxXRC_USE_LOCALE
+        // is on, because it could break wxGetTranslation lookup.
+        encoding = wxLocale::GetSystemEncodingName();
+    }
+#endif
+
+    wxScopedPtr<wxXmlDocument> doc(new wxXmlDocument);
+    if (!doc->Load(*stream, encoding))
+    {
+        wxLogError(_("Cannot load resources from file '%s'."), filename);
+        return NULL;
+    }
+
+    wxXmlNode * const root = doc->GetRoot();
+    if (root->GetName() != wxT("resource"))
+    {
+        ReportError
+        (
+            root,
+            "invalid XRC resource, doesn't have root node <resource>"
+        );
+        return NULL;
+    }
+
+    long version;
+    int v1, v2, v3, v4;
+    wxString verstr = root->GetAttribute(wxT("version"), wxT("0.0.0.0"));
+    if (wxSscanf(verstr, wxT("%i.%i.%i.%i"), &v1, &v2, &v3, &v4) == 4)
+        version = v1*256*256*256+v2*256*256+v3*256+v4;
+    else
+        version = 0;
+    if (m_version == -1)
+        m_version = version;
+    if (m_version != version)
+    {
+        wxLogWarning("Resource files must have same version number.");
+    }
+
+    ProcessPlatformProperty(root);
+    PreprocessForIdRanges(root);
+    wxIdRangeManager::Get()->FinaliseRanges(root);
+
+    return doc.release();
 }
 
 wxXmlNode *wxXmlResource::DoFindResource(wxXmlNode *parent,