Improve handling of file spec in wxFileSystemWatcher::AddTree().

Fix watching too many files (i.e. even those not matching the provided spec)
and asserts when removing a recursive watch with a file spec in wxMSW.

Closes #14488.

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

diff --git a/include/wx/fswatcher.h b/include/wx/fswatcher.h
index 9792bce12b4eab66c2d43661d53fb606241c266d..7c6b63ef39db17584f47af9ff130482c239cfdc9 100644 (file)
--- a/include/wx/fswatcher.h
+++ b/include/wx/fswatcher.h
@@ -200,8 +200,11 @@ public:
     {
     }
 
     {
     }
 

-    wxFSWatchInfo(const wxString& path, int events, wxFSWPathType type) :
-        m_path(path), m_events(events), m_type(type)
+    wxFSWatchInfo(const wxString& path,
+                  int events,
+                  wxFSWPathType type,
+                  const wxString& filespec = wxString()) :
+        m_path(path), m_filespec(filespec), m_events(events), m_type(type)

     {
     }
 
     {
     }
 


@@ -210,6 +213,8 @@ public:
         return m_path;
     }
 
         return m_path;
     }
 

+    const wxString& GetFilespec() const { return m_filespec; }
+

     int GetFlags() const
     {
         return m_events;
     int GetFlags() const
     {
         return m_events;


@@ -222,6 +227,7 @@ public:
 
 protected:
     wxString m_path;
 
 protected:
     wxString m_path;

+    wxString m_filespec;      // For tree watches, holds any filespec to apply

     int m_events;
     wxFSWPathType m_type;
 };
     int m_events;
     wxFSWPathType m_type;
 };


@@ -260,7 +266,7 @@ public:
      * of particular type.
      */
     virtual bool AddTree(const wxFileName& path, int events = wxFSW_EVENT_ALL,
      * of particular type.
      */
     virtual bool AddTree(const wxFileName& path, int events = wxFSW_EVENT_ALL,

-                         const wxString& filter = wxEmptyString);
+                         const wxString& filespec = wxEmptyString);

 
     /**
      * Removes path from the list of watched paths.
 
     /**
      * Removes path from the list of watched paths.


@@ -310,7 +316,8 @@ public:
     //
     // Delegates the real work of adding the path to wxFSWatcherImpl::Add() and
     // updates m_watches if the new path was successfully added.
     //
     // Delegates the real work of adding the path to wxFSWatcherImpl::Add() and
     // updates m_watches if the new path was successfully added.

-    bool AddAny(const wxFileName& path, int events, wxFSWPathType type);
+    bool AddAny(const wxFileName& path, int events, wxFSWPathType type,
+                const wxString& filespec = wxString());

 
 protected:
 
 
 protected:
 

diff --git a/src/common/fswatchercmn.cpp b/src/common/fswatchercmn.cpp
index 7382c2f6fb52abbbbe2809bbdf7a5957b35617cc..28aeffdcb68dc9cb7c4988008b3fee75000cc631 100644 (file)
--- a/src/common/fswatchercmn.cpp
+++ b/src/common/fswatchercmn.cpp
@@ -101,7 +101,8 @@ bool wxFileSystemWatcherBase::Add(const wxFileName& path, int events)
 bool
 wxFileSystemWatcherBase::AddAny(const wxFileName& path,
                                 int events,
 bool
 wxFileSystemWatcherBase::AddAny(const wxFileName& path,
                                 int events,

-                                wxFSWPathType type)
+                                wxFSWPathType type,
+                                const wxString& filespec)

 {
     wxString canonical = GetCanonicalPath(path);
     if (canonical.IsEmpty())
 {
     wxString canonical = GetCanonicalPath(path);
     if (canonical.IsEmpty())


@@ -111,7 +112,7 @@ wxFileSystemWatcherBase::AddAny(const wxFileName& path,
                 wxString::Format("Path '%s' is already watched", canonical));
 
     // adding a path in a platform specific way
                 wxString::Format("Path '%s' is already watched", canonical));
 
     // adding a path in a platform specific way

-    wxFSWatchInfo watch(canonical, events, type);
+    wxFSWatchInfo watch(canonical, events, type, filespec);

     if ( !m_service->Add(watch) )
         return false;
 
     if ( !m_service->Add(watch) )
         return false;
 


@@ -140,7 +141,7 @@ bool wxFileSystemWatcherBase::Remove(const wxFileName& path)
 }
 
 bool wxFileSystemWatcherBase::AddTree(const wxFileName& path, int events,
 }
 
 bool wxFileSystemWatcherBase::AddTree(const wxFileName& path, int events,

-                                      const wxString& filter)
+                                      const wxString& filespec)

 {
     if (!path.DirExists())
         return false;
 {
     if (!path.DirExists())
         return false;


@@ -150,8 +151,9 @@ bool wxFileSystemWatcherBase::AddTree(const wxFileName& path, int events,
     class AddTraverser : public wxDirTraverser
     {
     public:
     class AddTraverser : public wxDirTraverser
     {
     public:

-        AddTraverser(wxFileSystemWatcherBase* watcher, int events) :
-            m_watcher(watcher), m_events(events)
+        AddTraverser(wxFileSystemWatcherBase* watcher, int events,
+                     const wxString& filespec) :
+            m_watcher(watcher), m_events(events), m_filespec(filespec)

         {
         }
 
         {
         }
 


@@ -161,35 +163,44 @@ bool wxFileSystemWatcherBase::AddTree(const wxFileName& path, int events,
         // about that, but Add() should also behave well then
         virtual wxDirTraverseResult OnFile(const wxString& filename)
         {
         // about that, but Add() should also behave well then
         virtual wxDirTraverseResult OnFile(const wxString& filename)
         {

-            wxLogTrace(wxTRACE_FSWATCHER,
+            if ( m_watcher->AddAny(wxFileName::FileName(filename),
+                                   m_events, wxFSWPath_File) )
+            {
+                wxLogTrace(wxTRACE_FSWATCHER,

                        "--- AddTree adding file '%s' ---", filename);
                        "--- AddTree adding file '%s' ---", filename);

-            m_watcher->AddAny(wxFileName::FileName(filename),
-                             m_events, wxFSWPath_File);
+            }

             return wxDIR_CONTINUE;
         }
 
         virtual wxDirTraverseResult OnDir(const wxString& dirname)
         {
             return wxDIR_CONTINUE;
         }
 
         virtual wxDirTraverseResult OnDir(const wxString& dirname)
         {

-            wxLogTrace(wxTRACE_FSWATCHER,
+            // We can't currently watch only the files with the given filespec
+            // in the subdirectories so we only watch subdirectories at all if
+            // we want to watch everything.
+            if ( m_filespec.empty() )
+            {
+                if ( m_watcher->AddAny(wxFileName::DirName(dirname),
+                                       m_events, wxFSWPath_Dir) )
+                {
+                    wxLogTrace(wxTRACE_FSWATCHER,

                        "--- AddTree adding directory '%s' ---", dirname);
                        "--- AddTree adding directory '%s' ---", dirname);

-            // we add as much as possible and ignore errors
-            m_watcher->AddAny(wxFileName::DirName(dirname),
-                             m_events, wxFSWPath_Dir);
+                }
+            }

             return wxDIR_CONTINUE;
         }
 
     private:
         wxFileSystemWatcherBase* m_watcher;
         int m_events;
             return wxDIR_CONTINUE;
         }
 
     private:
         wxFileSystemWatcherBase* m_watcher;
         int m_events;

-        wxString m_filter;
+        wxString m_filespec;

     };
 
     wxDir dir(path.GetFullPath());
     };
 
     wxDir dir(path.GetFullPath());

-    AddTraverser traverser(this, events);
-    dir.Traverse(traverser, filter);
+    AddTraverser traverser(this, events, filespec);
+    dir.Traverse(traverser, filespec);

 
     // Add the path itself explicitly as Traverse() doesn't return it.
 
     // Add the path itself explicitly as Traverse() doesn't return it.

-    Add(path.GetPathWithSep(), events);
+    AddAny(path.GetPathWithSep(), events, wxFSWPath_Dir, filespec);

 
     return true;
 }
 
     return true;
 }


@@ -204,8 +215,9 @@ bool wxFileSystemWatcherBase::RemoveTree(const wxFileName& path)
     class RemoveTraverser : public wxDirTraverser
     {
     public:
     class RemoveTraverser : public wxDirTraverser
     {
     public:

-        RemoveTraverser(wxFileSystemWatcherBase* watcher) :
-            m_watcher(watcher)
+        RemoveTraverser(wxFileSystemWatcherBase* watcher,
+                        const wxString& filespec) :
+            m_watcher(watcher), m_filespec(filespec)

         {
         }
 
         {
         }
 


@@ -217,17 +229,45 @@ bool wxFileSystemWatcherBase::RemoveTree(const wxFileName& path)
 
         virtual wxDirTraverseResult OnDir(const wxString& dirname)
         {
 
         virtual wxDirTraverseResult OnDir(const wxString& dirname)
         {

-            m_watcher->Remove(wxFileName::DirName(dirname));
+            // Currently the subdirectories would have been added only if there
+            // is no filespec.
+            //
+            // Notice that we still need to recurse into them even if we're
+            // using a filespec because they can contain files matching it.
+            if ( m_filespec.empty() )
+            {
+                m_watcher->Remove(wxFileName::DirName(dirname));
+            }
+

             return wxDIR_CONTINUE;
         }
 
     private:
         wxFileSystemWatcherBase* m_watcher;
             return wxDIR_CONTINUE;
         }
 
     private:
         wxFileSystemWatcherBase* m_watcher;

+        wxString m_filespec;

     };
 
     };
 

+    // If AddTree() used a filespec, we must use the same one
+    wxString canonical = GetCanonicalPath(path);
+    wxFSWatchInfoMap::iterator it = m_watches.find(canonical);
+    wxCHECK_MSG( it != m_watches.end(), false,
+                 wxString::Format("Path '%s' is not watched", canonical) );
+    wxFSWatchInfo watch = it->second;
+    const wxString filespec = watch.GetFilespec();
+
+#if defined(__WINDOWS__)
+    // When there's no filespec, the wxMSW AddTree() would have set a watch
+    // on only the passed 'path'. We must therefore remove only this
+    if (filespec.empty())
+    {
+        return Remove(path);
+    }
+    // Otherwise fall through to the generic implementation
+#endif // __WINDOWS__
+

     wxDir dir(path.GetFullPath());
     wxDir dir(path.GetFullPath());

-    RemoveTraverser traverser(this);
-    dir.Traverse(traverser);
+    RemoveTraverser traverser(this, filespec);
+    dir.Traverse(traverser, filespec);

 
     // As in AddTree() above, handle the path itself explicitly.
     Remove(path);
 
     // As in AddTree() above, handle the path itself explicitly.
     Remove(path);

diff --git a/tests/fswatcher/fswatchertest.cpp b/tests/fswatcher/fswatchertest.cpp
index 2f9ff84b853eab840f1f2d705e46dac4ae0dfb75..2cf5554dbbe4774ae7dd6a8a4acf312e203aa01c 100644 (file)
--- a/tests/fswatcher/fswatchertest.cpp
+++ b/tests/fswatcher/fswatchertest.cpp
@@ -417,11 +417,9 @@ private:
     CPPUNIT_TEST_SUITE( FileSystemWatcherTestCase );
         CPPUNIT_TEST( TestEventCreate );
         CPPUNIT_TEST( TestEventDelete );
     CPPUNIT_TEST_SUITE( FileSystemWatcherTestCase );
         CPPUNIT_TEST( TestEventCreate );
         CPPUNIT_TEST( TestEventDelete );

-
-        // FIXME: Currently this test fails under Windows.
-#ifndef __WINDOWS__
+#if !defined(__VISUALC__) || wxCHECK_VISUALC_VERSION(7)

         CPPUNIT_TEST( TestTrees );
         CPPUNIT_TEST( TestTrees );

-#endif // __WINDOWS__
+#endif

 
         // kqueue-based implementation doesn't collapse create/delete pairs in
         // renames and doesn't detect neither modifications nor access to the
 
         // kqueue-based implementation doesn't collapse create/delete pairs in
         // renames and doesn't detect neither modifications nor access to the


@@ -446,10 +444,9 @@ private:
     void TestEventRename();
     void TestEventModify();
     void TestEventAccess();
     void TestEventRename();
     void TestEventModify();
     void TestEventAccess();

-#ifndef __WINDOWS__
-    void TestTrees();
-#endif // __WINDOWS__
-
+#if !defined(__VISUALC__) || wxCHECK_VISUALC_VERSION(7)
+    void TestTrees();   // Visual C++ 6 can't build this
+#endif

     void TestNoEventsAfterRemove();
 
     DECLARE_NO_COPY_CLASS(FileSystemWatcherTestCase)
     void TestNoEventsAfterRemove();
 
     DECLARE_NO_COPY_CLASS(FileSystemWatcherTestCase)


@@ -641,7 +638,8 @@ void FileSystemWatcherTestCase::TestEventAccess()
 // ----------------------------------------------------------------------------
 // TestTrees
 // ----------------------------------------------------------------------------
 // ----------------------------------------------------------------------------
 // TestTrees
 // ----------------------------------------------------------------------------

-#ifndef __WINDOWS__
+
+#if !defined(__VISUALC__) || wxCHECK_VISUALC_VERSION(7)

 void FileSystemWatcherTestCase::TestTrees()
 {
     class TreeTester : public EventHandler
 void FileSystemWatcherTestCase::TestTrees()
 {
     class TreeTester : public EventHandler


@@ -664,10 +662,11 @@ void FileSystemWatcherTestCase::TestTrees()
                 CPPUNIT_ASSERT(dir.Mkdir());
 
                 const wxString prefix = dir.GetPathWithSep();
                 CPPUNIT_ASSERT(dir.Mkdir());
 
                 const wxString prefix = dir.GetPathWithSep();

+                const wxString ext[] = { ".txt", ".log", "" };

                 for ( unsigned f = 0; f < files; ++f )
                 {
                     // Just create the files.
                 for ( unsigned f = 0; f < files; ++f )
                 {
                     // Just create the files.

-                    wxFile(prefix + wxString::Format("file%u", f+1),
+                    wxFile(prefix + wxString::Format("file%u", f+1) + ext[f],

                            wxFile::write);
                 }
             }
                            wxFile::write);
                 }
             }


@@ -707,8 +706,13 @@ void FileSystemWatcherTestCase::TestTrees()
         {
             CPPUNIT_ASSERT(m_watcher);
 
         {
             CPPUNIT_ASSERT(m_watcher);
 

-            const size_t
-                treeitems = (subdirs*files) + subdirs + 1; // +1 for the trunk
+            size_t treeitems = 1; // the trunk
+#ifndef __WINDOWS__
+            // When there's no file mask, wxMSW sets a single watch
+            // on the trunk which is implemented recursively.
+            // wxGTK always sets an additional watch for each file/subdir
+            treeitems += (subdirs*files) + subdirs;
+#endif // __WINDOWS__

 
             // Store the initial count; there may already be some watches
             const int initial = m_watcher->GetWatchedPathsCount();
 
             // Store the initial count; there may already be some watches
             const int initial = m_watcher->GetWatchedPathsCount();


@@ -724,6 +728,28 @@ void FileSystemWatcherTestCase::TestTrees()
             CPPUNIT_ASSERT_EQUAL(initial, m_watcher->GetWatchedPathsCount());
         }
 
             CPPUNIT_ASSERT_EQUAL(initial, m_watcher->GetWatchedPathsCount());
         }
 

+        void WatchTreeWithFilespec(const wxFileName& dir)
+        {
+            CPPUNIT_ASSERT(m_watcher);
+            CPPUNIT_ASSERT(dir.DirExists()); // Was built in WatchTree()
+
+            // Store the initial count; there may already be some watches
+            const int initial = m_watcher->GetWatchedPathsCount();
+
+            // When we use a filter, both wxMSW and wxGTK implementations set
+            // an additional watch for each file/subdir. Test by passing *.txt
+            // We expect the dirs and the other 2 files to be skipped
+            const size_t treeitems = subdirs + 1;
+            m_watcher->AddTree(dir, wxFSW_EVENT_ALL, "*.txt");
+
+            const int plustree = m_watcher->GetWatchedPathsCount();
+            CPPUNIT_ASSERT_EQUAL(initial + treeitems, plustree);
+
+            // RemoveTree should try to remove only those files that were added
+            m_watcher->RemoveTree(dir);
+            CPPUNIT_ASSERT_EQUAL(initial, m_watcher->GetWatchedPathsCount());
+        }
+

         void RemoveAllWatches()
         {
             CPPUNIT_ASSERT(m_watcher);
         void RemoveAllWatches()
         {
             CPPUNIT_ASSERT(m_watcher);


@@ -750,6 +776,12 @@ void FileSystemWatcherTestCase::TestTrees()
 
             WatchDir(singledir);
             WatchTree(treedir);
 
             WatchDir(singledir);
             WatchTree(treedir);

+            // Now test adding and removing a tree using a filespec
+            // wxMSW uses the generic method to add matching files; which fails
+            // as it doesn't support adding files :/ So disable the test
+#ifndef __WINDOWS__
+            WatchTreeWithFilespec(treedir);
+#endif // __WINDOWS__

 
             RemoveSingleWatch(singledir);
             // Add it back again, ready to test RemoveAll()
 
             RemoveSingleWatch(singledir);
             // Add it back again, ready to test RemoveAll()


@@ -781,7 +813,8 @@ void FileSystemWatcherTestCase::TestTrees()
     TreeTester tester;
     tester.Run();
 }
     TreeTester tester;
     tester.Run();
 }

-#endif // __WINDOWS__
+#endif // !defined(__VISUALC__) || wxCHECK_VISUALC_VERSION(7)
+

 
 namespace
 {
 
 namespace
 {