Jesse pointed, as part of discussion of issue 168237, that the proposed fix (to hold all folders and listen on them)
is in fact an inefficient way to implement his #3b comment from issue 33162:
#3b (assuming #2). I ask to listen on "file:/prjs/foo/" recursively. Somehow a
file /prjs/foo/nbproject/something.properties gets created - not necessarily
using the Filesystems API, maybe by Ant. A general file refresh is triggered
(e.g. main window exposed). I expect to get a CREATED event.
While I was working on issue 170727 I realized that an explicit method call to trigger such mode (obtaining all
timestamps of all child objects) would be really helpful. Having a
FileObject.addFileChangeListenerRecursively(FileChangeListener l) with a dumb implementation in FileObject and more
efficient one in masterfs (which could handle external changes too) would server as good enough "trigger".
Created attachment 86626 [details]
The change in openide.filesystems
Dear reviewers, consider this change in FileObject API. It provides default API for listening on subtrees, while it
opens doors to more efficient implementation in masterfs. For example the external detection of changes (as proposed
by issue 170727) would be enabled only if one uses addRecursiveListener. In future the implementation behind this
method can use native events as proposed by issue 26230.
the implementation seems to be flawed to me and won't keep up to the promise of the api signature. You will only get
some events, not all. AFAIK with deep structures, you need to keep all the FileObjects in the tree you listen to in
order to receive events reliably.
Therefore raising confusion among the API users and randomizing errors in other areas..
-1 on adding this without a bulletproof implementation that can be relied on..
VS1: I assume it makes sense to listen recursively only on folders. Maybe this should somehow be enforced.
VS2: The patch does not really allow to 'listen on "file:/prjs/foo/" recursively' in the sense of #3b (I believe). What
is needed is a recursive version of FileUtil.addFileChangeListener(FCL l, File f). Specifically, we need the way for
attaching a listener to a non-existing folder or for that matter a listener that would survive deletion and re-creation
of a folder, which it was attached to.
[JG01] Along the same lines as mkleint, I think it is acceptable to add this only if the masterfs impl does indeed work
with external changes from the beginning (even if inefficiently, by retaining a tree of FileObject's). The nonnormative
section in the Javadoc claims that it works but there is no masterfs implementation shown in the patch.
I would also probably rather see the default impl throw UnsupportedOperationException than work only if you happen to be
retaining child FOs independently.
[JG02] It looks confusing for this to be a method on FileObject when the closest analogue seems to be
FileUtil.add/removeFileChangeListener(FileChangeListener,File) - you could just add a 'recursive' parameter to this.
There are trade-offs to both styles:
1. As currently proposed, you cannot ask for recursive changes from a folder unless the folder currently exists and is
expected to not be deleted & recreated during the lifetime of the listener.
2. The FileUtil signatures do not support listening to e.g. HTTP resources (since it was originally decided to use File
rather than URI/URL).
3. addRecursiveListener can be overridden directly without requiring a separate method in FileSystem or separate service
for masterfs to inject an implementation.
From a client's perspective, #1 seems most significant.
VS2 ~ JG02 (did not see vstejskal's comments)
I agree with comments. Without reliable detection of external changes (issue 170727) it does not make sense.
Created attachment 86673 [details]
Combo patch with API change and implementation in masterfs
Re. VS1: In case of plain files, the behaviour of "recursive" listening shall collapse to regular listening. Shall I
somehow change the implementation to get closer to such desired state?
JG01 and Miloš and Jirka and co.: Indeed, masterfs needs to be modified to detect external changes. The good thing is
that the new patch is in summary simpler than the one for issue 170727
http://www.netbeans.org/nonav/issues/showattachment.cgi/86516/X.diff and Jan's for issue 168237
Other good thing is that right now the parsing API can simplify their code by use of the addRecursiveListener. Morever
once, when time for issue 26230 comes, parsing API will not need to change anything, just the internal masterfs
implementation can be optimized.
I'll comment on VS2 and JG02 later.
Reiterating JG01 that it would be preferable for the openide.filesystems impl to simply throw an error, rather than
pretend to work but not really work correctly.
[JG03] ExternalTouchTest.testRecursiveListener does not look very convincing. new File(FileUtil.toFile(obj.getParent()),
"sibling.java").createNewFile() is not how external changes would really be made. I would expect to see a FileObject
only ever explicitly created for the root, and the new files in deep hierarchy to be created only using java.io.File calls.
Created attachment 86721 [details]
Add-on patch introducing FileUtil.addRecursiveListener
Re. VS2 and JG02: The above patch shall address your needs. I am not really sure why Parsing API shall listen on
non-existing source roots, but de gustibus non est disputandum.
Re. 'pretend to work but not ... correctly': I am not aware of a case where it would not work correctly (except
LocalFS and its external changes, but it is not used in NetBeans right now). If you could provide a unit test showing
such incorrectness I would address it.
Re. JG03 I have changed the test to use FileUtil only initially in my local repository.
Changes visible at
JG01 - working incorrectly on LocalFS rather than throwing an UOE seems like a bug to me. IIRC you would not be likely
to get such a FS just by forgetting to include masterfs in your classpath/suite, but better safe than sorry. We do not
intend for anyone to use the default impl in openide.filesystems, and know that if they did it would not work as
expected, so why have one at all? Change the Javadoc to say that either UOE is thrown or the listener is guaranteed to
receive all changes.
JG02 - we should not have *both* methods on FileObject and in FileUtil; that is just confusing. (BTW "@see
FileObject#removeFileChangeListener" is probably wrong.)
BTW - unlike CVS, Hg branch names can contain '-' which is a bit easier to type.
Re. "throwing an UOE" - I do not want to complicate API use by throwing UOE. That just puts all the load on API user.
I do not think that wrong behaviour on LocalFileSystem is a problem, but to make everyone feel better:
Re. "both methods" - I do not see it as a problem and propose to integrate with both of them. In case this would be an
integration blocker, I am ready to remove the utility methods in FileUtil class.
JS01: Events should be delivered just once. I added a test with two level file tree which may discover other inaccuracies.
Re. JS01: It is perfect to receive comments in form of patches! Fixed in
http://hg.netbeans.org/prototypes/rev/f0eff38c343f but please verify, the test was not absolutely correct.
Re. JS01: Thank you for fixing the test. But I think it is not correct that when I create 2 sub folders and 3 files
externally, only 1 DATA_CREATED and 1 FOLDER_CREATED events are fired. I marked such places with TODO.
Also I added a new test case for FileUtil.addRecursiveListener(FCL, File) - not folder. There are still redundant events
Issue 168237 is supposed to be fixed without addRecursiveListener (at least for 6.8). Removing the dependency and
changing the target milestone.
Created attachment 87782 [details]
Integration ready patch. All (Jirka's) tests passing.
The PHP and version control guys agreed that the simplest way to address issue 172254 is to empower
addRecursiveListener. Thus I want us to reconsider our decision to include this API in 6.8 release. It shall be used
only by PHP projects with enabled "copy on save" feature.
The patch http://www.netbeans.org/nonav/issues/showattachment.cgi/87782/X.diff solves all issues presented by Jirka S.
The only difference compared to his suggestions is that if a folder is created and has some content, then it is not
guaranteed to receive individual file creation events for each subfile. However this does not block PHP guys to use
this API correctly neither (in future) the parsing API guys to do the same. Quite opposite, the less events the
better, imho. Otherwise we would spend time on generating the correct events and immediately those events would be
filtered out as unimportant.
Tentative integration day is Sep 18, 2009
Merged as core-main#07f49eb8e934
Integrated into 'main-golden', will be available in build *200909181401* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
User: Jaroslav Tulach <email@example.com>
Log: #170862: Introducing FileObject.addRecursiveListener(...) and its efficient implementation in masterfs