wxFileSystemWatcher::AddTree and RemoveTree fixes

[dghart]

This is a continuation of #14480, which left FileSystemWatcherTestCase failing in wxMSW. The problem was that, unless there's a passed filespec, wxMSW adds just a single, recursively acting, watch on the 'trunk' folder. I've fixed the test to account for this.

That revealed a different problem with fswatcher in wxMSW: removing a watch used the generic RemoveTree(), which uses RemoveTraverser to remove those watches which were never created. That made the 'fswatcher' sample assert (well, unless it was built with g++/mingw). This is now 'fixed' as part of the solution to the next problem (but see below). After all this I noticed 12847#comment:19. I think I've inadvertently fixed item 2.

While doing this, I realised that there was a different problem with AddTree(): it takes an optional filespec parameter, watching (in theory) only matching files. This had two problems:

1) It still watched all subdirs. At least with inotify, that meant that all the contained files were watched, whatever their ext. I've fixed this by not watching dirs unless they too match the filespec (which would be possible though unusual). You may feel that it would be better never to watch dirs if there's a filespec; if so I'll change the patch.
2) RemoveTree() knew nothing of filespecs, so always removed too much and asserted. I've fixed this by storing any filespec in wxFSWatchInfo, so it's available on removal. This also made it easy to distinguish between the different wxMSW AddTree() methods.

With this patch wxGTK AddTree() and RemoveTree() now work correctly, whether or not there's a filespec (as ever, untested on wxMac). I've added a new section in FileSystemWatcherTestCase to test for this. I've also uploaded a temporary diff to the fswatcher sample if you wish to test that.

There's still a problem with wxMSW. If there's a passed filespec, wxMSW uses the generic AddTree() to add the matching files. However it doesn't currently _support_ watching files, so this will fail with a wxLogError from src/msw/fswatcher.cpp:144. I've therefore disabled that section of the FileSystemWatcherTestCase.

In summary: wxGTK now works (except for links). wxMSW is no worse than before, and one bug is partially fixed. FileSystemWatcherTestCase no longer fails on wxMSWin, but hits the "Unable to dequeue completion packet" issue mentioned in #13294.

I'll add a doc patch for wxFileSystemWatcher::AddTree once it's decided how it should deal with subdirs; I'll mention the wxMSW issues too.

[vadz]

I wonder if we shouldn't just remove the filespec argument. If it's really going to simplify the implementation a lot, I'd be ready to sacrifice it. After all, you always can easily filter out the files you're not interested in in your event handler and usually you'd want to watch all files anyhow, wouldn't you?

If we want to keep support for this feature, we'd need to add individual watches for all files matching it under the given tree. And possibly also internal watches for all directories that would be handled to add watches for any newly created files. This doesn't seem easy to do...

[dghart]

Replying to vadz:

I wonder if we shouldn't just remove the filespec argument. If it's really going to simplify the implementation a lot, I'd be ready to sacrifice it. After all, you always can easily filter out the files you're not interested in in your event handler and usually you'd want to watch all files anyhow, wouldn't you?

If we want to keep support for this feature, we'd need to add individual watches for all files matching it under the given tree. And possibly also internal watches for all directories that would be handled to add watches for any newly created files. This doesn't seem easy to do...

Fortunately the latest patch, #14544, changes how a filespec is handled and solves these issues.

[PB]

(In #14544) Replying to dghart:

(In #14488) Replying to vadz:

I wonder if we shouldn't just remove the filespec argument. If it's really going to simplify the implementation a lot, I'd be ready to sacrifice it. After all, you always can easily filter out the files you're not interested in in your event handler and usually you'd want to watch all files anyhow, wouldn't you? If we want to keep support for this feature, we'd need to add individual watches for all files matching it under the given tree. And possibly also internal watches for all directories that would be handled to add watches for any newly created files. This doesn't seem easy to do...

Fortunately the latest patch, #14544, changes how a filespec is handled and solves these issues.

Vadim may have meant it generally: underlying native MSW API doesn't support watching individual files so there's still that... :( I believe that the documentation should mention that watching individual files and using filter is not supported on MSW.

[dghart]

Replying to PB:

Vadim may have meant it generally: underlying native MSW API doesn't support watching individual files so there's still that... :(

I hope I've worked-around that in #14544 by doing the filtering in wxFSWatcherImpl::ProcessNativeEvent. However I have very little wxMSW knowledge and, if you're still using wxFileSystemWatcher, it would be great if you could test these patches :)

[vadz]

(In #14544) I'll try to look at this series soon although to be honest I'm a bit intimidated. It might be better to redo a series of patches (0001-Something, 0002-SomethingElse, ...) if parts of the later patches change something done by the earlier ones as this should make reviewing them simpler.

I.e. perhaps it would be more convenient for both of us if you created a new branch, cherry pick all your commits there and used git rebase -i to reorganize them in the optimal way and then just used git format-patch?

If not, I'll try to look at this stuff starting from #14488 on Thursday.

[dghart]

(In #14544) Replying to vadz:

I'll try to look at this series soon although to be honest I'm a bit intimidated. It might be better to redo a series of patches (0001-Something, 0002-SomethingElse, ...) if parts of the later patches change something done by the earlier ones as this should make reviewing them simpler.

I.e. perhaps it would be more convenient for both of us if you created a new branch, cherry pick all your commits there and used git rebase -i to reorganize them in the optimal way and then just used git format-patch?

If not, I'll try to look at this stuff starting from #14488 on Thursday.

I tried to keep the patches clean and simple. With the exception of the unit-test, there are only minor 'internal' reversions in the series; by minor I mean just a couple of lines. So I think you'll find it easier that you expect.

If not, I'll redo them differently.

[vadz]

To reply to the original question, I actually think the only behaviour that would really make sense would be to watch all files with the given filespec (only) in all the subdirectories. In particular, applying the filespec check to the directory doesn't make much sense to me and I'd rather not watch any subdirectories at all for now, this is at least more predictable, so I'll modify the patch to do it like this but if you can implement the above logic, it would be much better.

Another question is about the error messages I now reliably get when running the unit test:

Test program for wxWidgets non-GUI features
build: 2.9.5 (wchar_t,Visual C++ 1500,wx containers,compatible with 2.8)
....17:11:27: Error: Unable to dequeue completion packet (error 5: access is denied.)
17:11:27: Error: Unable to dequeue completion packet (error 5: access is denied.)
..

OK (6)

They come from wxIOCPService::GetStatus() but I have no idea why do we get them :-( Would you by chance?

Thanks in advance!

[vadz]

Sorry, I realized that we do already watch the files in the subdirectories if they match the filespec because we iterate over all of them recursively, please disregard the first part of the previous comment.

[vadz]

(In #14544) Thanks, I've applied everything here, I think, but I had to do it by combining some patches to avoid breaking unit tests.

The only change I intentionally did (as opposed to any number of bugs I could have introduced unintentionally...) was to change the filespec check to use the full file name instead of just its extension, I don't see any reason why we shouldn't allow watching for e.g. "syslog*".

Please let me know if I broke anything else and thanks again for all your work on this!

[VZ]

(In [72678]) 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.

[VZ]

(In #14544) (In [72681]) Check for filespec when generating events in wxFileSystemWatcher.

Instead of setting watches on individual files when a non-empty filespec is
given, always watch all the files but just ignore the events from the ones not
matching the filespec. This makes the code simpler and fixes several bugs.

See #14544.

[VZ]

(In #14544) (In [72682]) Fix bug in Unix wxFileSystemWatcher implementation when watch is deleted.

Don't assert when removing a watch descriptor from the stale descriptors list.

See #14544.

[VZ]

(In #14544) (In [72683]) Handle deletion of watched directories in wxFileSystemWatcher sample.

Don't assert when trying to stop watching a directory that doesn't exist any
more later.

See #14544.

[dghart]

Replying to vadz:

Another question is about the error messages I now reliably get when running the unit test:
{{{
Test program for wxWidgets non-GUI features
build: 2.9.5 (wchar_t,Visual C++ 1500,wx containers,compatible with 2.8)
....17:11:27: Error: Unable to dequeue completion packet (error 5: access is denied.)
17:11:27: Error: Unable to dequeue completion packet (error 5: access is denied.)
..

OK (6)
}}}

They come from wxIOCPService::GetStatus() but I have no idea why do we get them :-( Would you by chance?

I have no MSW knowledge, but these errors were reported 18 months ago in #13294, and are (presumably) the ones mentioned in issue 4 of #12847 comments 13 and 18.

[dghart]

(In #14544) Replying to vadz:

Thanks, I've applied everything here, I think, but I had to do it by combining some patches to avoid breaking unit tests.

Sorry to reopen this, but unless I'm missing something (which is quite possible; the complex series of patches confuses me too) I don't think you applied CreateDelete.diff

[vadz]

(In #14544) Oops, sorry about this and thanks for noticing it. Hopefully nothing else got forgotten, so re-closing it, thanks again.

[VZ]

(In #14544) (In [72689]) Improve inotify()-based wxFileSystemWatcher to handle creation/deletion.

Handle creation and deletion of directories under the watched path better.

See #14544.