This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
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 http://www.netbeans.org/nonav/issues/showattachment.cgi/86507/indexing-keep-fileobject 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 http://hg.netbeans.org/prototypes/rev/c01c2c6e19af and http://hg.netbeans.org/prototypes/rev/3601a8c7c910
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: http://hg.netbeans.org/prototypes/rev/4daa46ed0149 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. http://hg.netbeans.org/prototypes/rev/41467e9c38b6
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 fired. http://hg.netbeans.org/prototypes/rev/b5e31bb54f70
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) Changeset: http://hg.netbeans.org/main-golden/rev/c01c2c6e19af User: Jaroslav Tulach <jtulach@netbeans.org> Log: #170862: Introducing FileObject.addRecursiveListener(...) and its efficient implementation in masterfs