Bug 170862 - FileObject.addRecursiveListener
FileObject.addRecursiveListener
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Filesystems
6.x
All All
: P2 (vote)
: 6.x
Assigned To: Jaroslav Tulach
issues@platform
: API, API_REVIEW_FAST
Depends on:
Blocks: 172182 172254
  Show dependency treegraph
 
Reported: 2009-08-25 12:15 UTC by Jaroslav Tulach
Modified: 2009-09-18 22:29 UTC (History)
7 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
The change in openide.filesystems (14.22 KB, patch)
2009-08-25 14:16 UTC, Jaroslav Tulach
Details | Diff
Combo patch with API change and implementation in masterfs (40.95 KB, patch)
2009-08-26 10:53 UTC, Jaroslav Tulach
Details | Diff
Add-on patch introducing FileUtil.addRecursiveListener (18.29 KB, patch)
2009-08-27 09:37 UTC, Jaroslav Tulach
Details | Diff
Integration ready patch. All (Jirka's) tests passing. (93.29 KB, patch)
2009-09-16 15:36 UTC, Jaroslav Tulach
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaroslav Tulach 2009-08-25 12:15:34 UTC
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".
Comment 1 Jaroslav Tulach 2009-08-25 14:16:39 UTC
Created attachment 86626 [details]
The change in openide.filesystems
Comment 2 Jaroslav Tulach 2009-08-25 14:20:03 UTC
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.
Comment 3 Milos Kleint 2009-08-25 15:12:15 UTC
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..
Comment 4 Vitezslav Stejskal 2009-08-25 15:22:08 UTC
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.
Comment 5 Jesse Glick 2009-08-25 15:34:45 UTC
[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.
Comment 6 Jesse Glick 2009-08-25 16:43:25 UTC
VS2 ~ JG02 (did not see vstejskal's comments)
Comment 7 Jiri Skrivanek 2009-08-26 09:18:45 UTC
I agree with comments. Without reliable detection of external changes (issue 170727) it does not make sense.
Comment 8 Jaroslav Tulach 2009-08-26 10:53:15 UTC
Created attachment 86673 [details]
Combo patch with API change and implementation in masterfs
Comment 9 Jaroslav Tulach 2009-08-26 11:03:58 UTC
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.
Comment 10 Jesse Glick 2009-08-26 15:07:20 UTC
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.
Comment 11 Jaroslav Tulach 2009-08-27 09:37:06 UTC
Created attachment 86721 [details]
Add-on patch introducing FileUtil.addRecursiveListener
Comment 12 Jaroslav Tulach 2009-08-27 09:50:18 UTC
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.
Comment 14 Jesse Glick 2009-08-27 14:59:59 UTC
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.
Comment 15 Jaroslav Tulach 2009-08-28 10:16:14 UTC
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.
Comment 16 Jiri Skrivanek 2009-08-28 10:47:35 UTC
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
Comment 17 Jaroslav Tulach 2009-08-28 16:55:15 UTC
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.
Comment 18 Jiri Skrivanek 2009-08-31 12:37:02 UTC
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
Comment 19 Jaroslav Tulach 2009-08-31 16:30:43 UTC
Issue 168237 is supposed to be fixed without addRecursiveListener (at least for 6.8). Removing the dependency and 
changing the target milestone.
Comment 20 Jaroslav Tulach 2009-09-16 15:36:11 UTC
Created attachment 87782 [details]
Integration ready patch. All (Jirka's) tests passing.
Comment 21 Jaroslav Tulach 2009-09-16 15:43:03 UTC
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
Comment 22 Jaroslav Tulach 2009-09-18 06:59:21 UTC
Merged as core-main#07f49eb8e934
Comment 23 Quality Engineering 2009-09-18 22:29:54 UTC
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


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo