#14544: wxFileSystemWatcher: More fixes

41 views
Skip to first unread message

wxTrac

unread,
Jul 31, 2012, 8:15:43 AM7/31/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14544>

#14544: wxFileSystemWatcher: More fixes
---------------------------------+------------------------------------------
Reporter: dghart | Owner:
Type: defect | Status: new
Priority: normal | Milestone: 2.9.5
Component: base | Version: 2.9-svn
Keywords: wxFileSystemWatcher | Blockedby: 14488, 14490, 14542, 14543
Patch: 1 | Blocking:
---------------------------------+------------------------------------------
This is (I hope) the last of a series of wxFileSystemWatcher patches. The
patches should be applied in this order, and on top of those from #14488,
#14490, #14542 and #14543. (If you prefer I could instead create a Grand
Unified Patch...)

First a reversion. In an earlier patch I tried to fix
wxFileSystemWatcher:AddTree by having it watch files as well as dirs; this
was partly because RemoveTree() was trying to remove those watches.
This was a mistake. Doing so is 1) unnecessary: the watch on the parent
dir can manage its contents perfectly well by itself; and 2) Add() doesn't
do this, and the difference in behaviour caused problems in dealing with
creation/deletion in a watched tree. RevertAddTreeFiles.diff reverts this,
and also removes from RemoveTree() the attempt to remove the unwatched
files.

Next a correction to the #14480/[72049] fix. If a watched file is deleted,
the first event to arrive is likely to be IN_IGNORED. At that stage the wd
won't be in m_staleDescriptors, so trying to remove it causes an assert.
StaleDescriptors.diff fixes this.

#14488 tried to make passing a file-mask happen at AddTree() level. This
no longer works now that files themselves aren't watched. Instead
Filespec.diff devolves it down to wxFSWatcherImpl::ProcessNativeEvent,
which is much more flexible. This also avoids the issue raised in #14488
about whether to watch non-matching dirs. Filespecs now work well in
wxGTK, and afaict also in wxMSW.

The current code lets the user add and remove watches, but ignores the
possibility that a watched dir may be added/removed by the filesystem
itself. CreateDelete.diff fixes this.

Finally, SampleAndTest.diff makes the sample notice if the directory it's
watching is deleted or renamed, notifies the user, and removes it from the
wxListView. This prevents the sample asserting (at least in wxGTK) when it
tries to remove it later. It also updates FileSystemWatcherTestCase to
work with the new code. I don't know if this will now fail on wxMac; if
so, #if defined(__UNIX__) will need to be changed.

This patch series make wxFileSystemWatcher work well in wxGTK, without
adversely affecting wxMSW.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14544>

wxTrac

unread,
Jul 31, 2012, 9:18:10 AM7/31/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:2>

#14544: wxFileSystemWatcher: More fixes
---------------------------------+------------------------------------------
Reporter: dghart | Owner:
Type: defect | Status: new
Priority: normal | Milestone: 2.9.5
Component: base | Version: 2.9-svn
Keywords: wxFileSystemWatcher | Blockedby: 14488, 14488, 14490, 14542, 14543
Patch: 1 | Blocking:
---------------------------------+------------------------------------------

Comment(by PB):

Replying to [comment:1 dghart]:

> (In #14488) Replying to [comment:1 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.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:2>

wxTrac

unread,
Jul 31, 2012, 12:53:10 PM7/31/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:5>

#14544: wxFileSystemWatcher: More fixes
---------------------------------+------------------------------------------
Reporter: dghart | Owner:
Type: defect | Status: new
Priority: normal | Milestone: 2.9.5
Component: base | Version: 2.9-svn
Keywords: wxFileSystemWatcher | Blockedby: 14488, 14488, 14488, 14490, 14542, 14542, 14543
Patch: 1 | Blocking:
---------------------------------+------------------------------------------

Comment(by 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.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:5>

wxTrac

unread,
Jul 31, 2012, 2:28:07 PM7/31/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:6>

#14544: wxFileSystemWatcher: More fixes
---------------------------------+------------------------------------------
Reporter: dghart | Owner:
Type: defect | Status: new
Priority: normal | Milestone: 2.9.5
Component: base | Version: 2.9-svn
Keywords: wxFileSystemWatcher | Blockedby: 14488, 14488, 14488, 14490, 14542, 14542, 14543
Patch: 1 | Blocking:
---------------------------------+------------------------------------------

Comment(by dghart):

Replying to [comment:5 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.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:6>

wxTrac

unread,
Oct 14, 2012, 9:06:16 PM10/14/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:10>

#14544: wxFileSystemWatcher: More fixes
-----------------------------------------------------------------------------------+
Reporter: dghart | Owner:
Type: defect | Status: closed
Priority: normal | Milestone: 2.9.5
Component: base | Version: 2.9-svn
Resolution: fixed | Keywords: wxFileSystemWatcher
Blockedby: 14488, 14488, 14488, 14488, 14490, 14542, 14542, 14542, 14542, 14543 | Patch: 1
Blocking: |
-----------------------------------------------------------------------------------+
Changes (by vadz):

* status: new => closed
* resolution: => fixed
* blockedby: 14488, 14488, 14488, 14488, 14488, 14490, 14542, 14542,
14542, 14542, 14542, 14543, 14543 => 14488,
14488, 14488, 14488, 14490, 14542, 14542,
14542, 14542, 14543


Comment:

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!


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:10>

wxTrac

unread,
Oct 14, 2012, 9:09:27 PM10/14/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:17>

#14544: wxFileSystemWatcher: More fixes
--------------------------------------------------------------------------------------------------------+
Reporter: dghart | Owner:
Type: defect | Status: closed
Priority: normal | Milestone: 2.9.5
Component: base | Version: 2.9-svn
Resolution: fixed | Keywords: wxFileSystemWatcher
Blockedby: 14488, 14488, 14488, 14488, 14488, 14490, 14490, 14542, 14542, 14542, 14542, 14542, 14543 | Patch: 1
Blocking: |
--------------------------------------------------------------------------------------------------------+

Comment(by VZ):

(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.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:17>

wxTrac

unread,
Oct 14, 2012, 9:09:51 PM10/14/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:18>

#14544: wxFileSystemWatcher: More fixes
--------------------------------------------------------------------------------------------------------+
Reporter: dghart | Owner:
Type: defect | Status: closed
Priority: normal | Milestone: 2.9.5
Component: base | Version: 2.9-svn
Resolution: fixed | Keywords: wxFileSystemWatcher
Blockedby: 14488, 14488, 14488, 14488, 14488, 14490, 14490, 14542, 14542, 14542, 14542, 14542, 14543 | Patch: 1
Blocking: |
--------------------------------------------------------------------------------------------------------+

Comment(by VZ):

(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.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:18>

wxTrac

unread,
Oct 14, 2012, 9:10:14 PM10/14/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:19>

#14544: wxFileSystemWatcher: More fixes
--------------------------------------------------------------------------------------------------------+
Reporter: dghart | Owner:
Type: defect | Status: closed
Priority: normal | Milestone: 2.9.5
Component: base | Version: 2.9-svn
Resolution: fixed | Keywords: wxFileSystemWatcher
Blockedby: 14488, 14488, 14488, 14488, 14488, 14490, 14490, 14542, 14542, 14542, 14542, 14542, 14543 | Patch: 1
Blocking: |
--------------------------------------------------------------------------------------------------------+

Comment(by VZ):

(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.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:19>

wxTrac

unread,
Oct 16, 2012, 8:47:03 AM10/16/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:22>

#14544: wxFileSystemWatcher: More fixes
----------------------------------------------------------------------------------------------------------------------+
Reporter: dghart | Owner:
Type: defect | Status: reopened
Priority: normal | Milestone: 2.9.5
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxFileSystemWatcher
Blockedby: 14488, 14488, 14488, 14488, 14488, 14488, 14490, 14490, 14490, 14542, 14542, 14542, 14542, 14542, 14543 | Patch: 1
Blocking: |
----------------------------------------------------------------------------------------------------------------------+
Changes (by dghart):

* status: closed => reopened
* resolution: fixed =>


Comment:

Replying to [comment:10 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


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:22>

wxTrac

unread,
Oct 16, 2012, 10:01:51 AM10/16/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:23>

#14544: wxFileSystemWatcher: More fixes
----------------------------------------------------------------------------------------------------------------------+
Reporter: dghart | Owner:
Type: defect | Status: closed
Priority: normal | Milestone: 2.9.5
Component: base | Version: 2.9-svn
Resolution: fixed | Keywords: wxFileSystemWatcher
Blockedby: 14488, 14488, 14488, 14488, 14488, 14488, 14490, 14490, 14490, 14542, 14542, 14542, 14542, 14542, 14543 | Patch: 1
Blocking: |
----------------------------------------------------------------------------------------------------------------------+
Changes (by vadz):

* status: reopened => closed
* resolution: => fixed


Comment:

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


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:23>

wxTrac

unread,
Oct 16, 2012, 10:02:43 AM10/16/12
to wx-...@googlegroups.com
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:24>

#14544: wxFileSystemWatcher: More fixes
----------------------------------------------------------------------------------------------------------------------+
Reporter: dghart | Owner:
Type: defect | Status: closed
Priority: normal | Milestone: 2.9.5
Component: base | Version: 2.9-svn
Resolution: fixed | Keywords: wxFileSystemWatcher
Blockedby: 14488, 14488, 14488, 14488, 14488, 14488, 14490, 14490, 14490, 14542, 14542, 14542, 14542, 14542, 14543 | Patch: 1
Blocking: |
----------------------------------------------------------------------------------------------------------------------+

Comment(by VZ):

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

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

See #14544.


--
Ticket URL: <http://trac.wxwidgets.org/ticket/14544#comment:24>
Reply all
Reply to author
Forward
0 new messages