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

include/wx/fswatcher.h

@@ -200,8 +200,11 @@ public:
     {
     }
 
-    wxFSWatchInfo(const wxString& path, int events, wxFSWPathType type) :
+    wxFSWatchInfo(const wxString& path,
-        m_path(path), m_events(events), m_type(type)
+                  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;
     }
 
+    const wxString& GetFilespec() const { return m_filespec; }
+
     int GetFlags() const
     {
         return m_events;
@@ -222,6 +227,7 @@ public:
 
 protected:
     wxString m_path;
+    wxString m_filespec;      // For tree watches, holds any filespec to apply
     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,
-                         const wxString& filter = wxEmptyString);
+                         const wxString& filespec = wxEmptyString);
 
     /**
      * 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.
-    bool AddAny(const wxFileName& path, int events, wxFSWPathType type);
+    bool AddAny(const wxFileName& path, int events, wxFSWPathType type,
+                const wxString& filespec = wxString());
 
 protected:
 

src/common/fswatchercmn.cpp

@@ -101,7 +101,8 @@ bool wxFileSystemWatcherBase::Add(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())
@@ -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
-    wxFSWatchInfo watch(canonical, events, type);
+    wxFSWatchInfo watch(canonical, events, type, filespec);
     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,
-                                      const wxString& filter)
+                                      const wxString& filespec)
 {
     if (!path.DirExists())
         return false;
@@ -150,8 +151,9 @@ bool wxFileSystemWatcherBase::AddTree(const wxFileName& path, int events,
     class AddTraverser : public wxDirTraverser
     {
     public:
-        AddTraverser(wxFileSystemWatcherBase* watcher, int events) :
+        AddTraverser(wxFileSystemWatcherBase* watcher, int events,
-            m_watcher(watcher), m_events(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)
         {
-            wxLogTrace(wxTRACE_FSWATCHER,
+            if ( m_watcher->AddAny(wxFileName::FileName(filename),
+                                   m_events, wxFSWPath_File) )
+            {
+                wxLogTrace(wxTRACE_FSWATCHER,
                        "--- AddTree adding file '%s' ---", filename);
-            m_watcher->AddAny(wxFileName::FileName(filename),
+            }
-                             m_events, wxFSWPath_File);
             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);
-            // 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;
-        wxString m_filter;
+        wxString m_filespec;
     };
 
     wxDir dir(path.GetFullPath());
-    AddTraverser traverser(this, events);
+    AddTraverser traverser(this, events, filespec);
-    dir.Traverse(traverser, filter);
+    dir.Traverse(traverser, filespec);
 
     // Add the path itself explicitly as Traverse() doesn't return it.
-    Add(path.GetPathWithSep(), events);
+    AddAny(path.GetPathWithSep(), events, wxFSWPath_Dir, filespec);
 
     return true;
 }
@@ -204,8 +215,9 @@ bool wxFileSystemWatcherBase::RemoveTree(const wxFileName& path)
     class RemoveTraverser : public wxDirTraverser
     {
     public:
-        RemoveTraverser(wxFileSystemWatcherBase* watcher) :
+        RemoveTraverser(wxFileSystemWatcherBase* watcher,
-            m_watcher(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)
         {
-            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;
+        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());
-    RemoveTraverser traverser(this);
+    RemoveTraverser traverser(this, filespec);
-    dir.Traverse(traverser);
+    dir.Traverse(traverser, filespec);
 
     // As in AddTree() above, handle the path itself explicitly.
     Remove(path);

tests/fswatcher/fswatchertest.cpp

@@ -417,11 +417,9 @@ private:
     CPPUNIT_TEST_SUITE( FileSystemWatcherTestCase );
         CPPUNIT_TEST( TestEventCreate );
         CPPUNIT_TEST( TestEventDelete );
-
+#if !defined(__VISUALC__) || wxCHECK_VISUALC_VERSION(7)
-        // FIXME: Currently this test fails under Windows.
-#ifndef __WINDOWS__
         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
@@ -446,10 +444,9 @@ private:
     void TestEventRename();
     void TestEventModify();
     void TestEventAccess();
-#ifndef __WINDOWS__
+#if !defined(__VISUALC__) || wxCHECK_VISUALC_VERSION(7)
-    void TestTrees();
+    void TestTrees();   // Visual C++ 6 can't build this
-#endif // __WINDOWS__
+#endif
-
     void TestNoEventsAfterRemove();
 
     DECLARE_NO_COPY_CLASS(FileSystemWatcherTestCase)
@@ -641,7 +638,8 @@ void FileSystemWatcherTestCase::TestEventAccess()
 // ----------------------------------------------------------------------------
 // TestTrees
 // ----------------------------------------------------------------------------
-#ifndef __WINDOWS__
+
+#if !defined(__VISUALC__) || wxCHECK_VISUALC_VERSION(7)
 void FileSystemWatcherTestCase::TestTrees()
 {
     class TreeTester : public EventHandler
@@ -664,10 +662,11 @@ void FileSystemWatcherTestCase::TestTrees()
                 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.
-                    wxFile(prefix + wxString::Format("file%u", f+1),
+                    wxFile(prefix + wxString::Format("file%u", f+1) + ext[f],
                            wxFile::write);
                 }
             }
@@ -707,8 +706,13 @@ void FileSystemWatcherTestCase::TestTrees()
         {
             CPPUNIT_ASSERT(m_watcher);
 
-            const size_t
+            size_t treeitems = 1; // the trunk
-                treeitems = (subdirs*files) + subdirs + 1; // +1 for 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();
@@ -724,6 +728,28 @@ void FileSystemWatcherTestCase::TestTrees()
             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);
@@ -750,6 +776,12 @@ void FileSystemWatcherTestCase::TestTrees()
 
             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()
@@ -781,7 +813,8 @@ void FileSystemWatcherTestCase::TestTrees()
     TreeTester tester;
     tester.Run();
 }
-#endif // __WINDOWS__
+#endif // !defined(__VISUALC__) || wxCHECK_VISUALC_VERSION(7)
+
 
 namespace
 {