Bug 127121 - Support for common task on FS instances like addFileChangeListener
Support for common task on FS instances like addFileChangeListener
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Filesystems
6.x
All All
: P2 (vote)
: 6.x
Assigned To: apireviews
issues@platform
: API_REVIEW_FAST
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-11 13:30 UTC by rmatous
Modified: 2008-12-22 11:35 UTC (History)
3 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
first draft (22.76 KB, patch)
2008-02-11 13:34 UTC, rmatous
Details | Diff
contract proposal (11.72 KB, text/plain)
2008-02-12 10:47 UTC, rmatous
Details
Probably more simple counter proposal exposing MasterFileSystem, giving guarantee there is just one instance -> leading to easier API (can be implemented with no impact on performance). (24.71 KB, text/plain)
2008-02-19 18:25 UTC, rmatous
Details
api change - I would like to integrate tomorow (5.75 KB, text/plain)
2008-02-25 17:28 UTC, rmatous
Details

Note You need to log in before you can comment on or make changes to this bug.
Description rmatous 2008-02-11 13:30:03 UTC
Consider adding support for common task on FS instances like addFileChangeListener.
Comment 1 rmatous 2008-02-11 13:34:54 UTC
Created attachment 56438 [details]
first draft
Comment 2 rmatous 2008-02-12 10:47:47 UTC
Created attachment 56505 [details]
contract proposal
Comment 3 rmatous 2008-02-19 18:25:33 UTC
Created attachment 56885 [details]
Probably more simple counter proposal exposing MasterFileSystem, giving guarantee there is just one instance -> leading to  easier API (can be implemented with no impact on performance).
Comment 4 rmatous 2008-02-19 18:27:11 UTC
Probably more simple counter proposal exposing MasterFileSystem, giving guarantee there is just one instance -> leading
to  easier API (can be implemented with no impact on performance). 
Comment 5 Jesse Glick 2008-02-19 19:22:00 UTC
I think I liked the first patch better as it put fewer constraints on how the implementation should look. The semantics
of FileUtil.getMasterFileSystem seem very poorly defined to me. If all client modules need to do is

1. add/remove a listener to any files, as in ClassPath (which is anyway a BAD use case but that is covered in a separate
filesystems RFE)
2. refresh all files, as in Ant module

then just expose these methods directly.
Comment 6 Jaroslav Tulach 2008-02-20 16:06:54 UTC
The best thing on this proposal is that it removes an incompatibility introduced since 6.0 release - e.g. on all 
systems new File(FileObject.getPath()).exists() is true, for fileobjects backed by files. If that would not be the 
case, I might agree with Jesse. 

However I believe that the previous patch does solve the API problem described in this issue, moreover it does that in 
a compatible way with latest release. That are two positive points. That is why I vote for this solution.
Comment 7 rmatous 2008-02-20 16:41:56 UTC
From my point of view the motivation for doing it in arbitrary way is to stop forcing people to convert all listRoots
into FileObjects because it might be very slow if there is any mapped network drive with very slow access. 
Comment 8 Jesse Glick 2008-02-20 17:06:55 UTC
I guess I'm confused. Who is advocating which patch? They are anonymous so I don't know what is being argued about when
you say "previous patch" or "arbitrary way". To be clear, I am -1 on FileUtil.getMasterFileSystem; +1 on
FileUtil.getFileBasedFileSystems() or anything else supporting just a limited set of operations:
add/removeFileChangeListener, refresh.
Comment 9 rmatous 2008-02-20 17:28:26 UTC
1/current state -1
2/FileUtil.getMasterFileSystem; +1
3/FileUtil.getFileBasedFileSystems() +1

3/ or 2/ but important for me is to get rid of current state

Suggested compromise solution (let's say 4/):
modification of 3/: 
- do not expose FileSystem in FileUtil, just expose the methods directly (add/removeFileChangeListener) - as Jesse suggest
- as an implementation detail (not much detail if we keep bacward compatibility) switch impl. of masterfs on windows to
have just one instance (and revert change
http://bits.netbeans.org/dev/javadoc/org-netbeans-modules-masterfs/apichanges.html#one-drive-one-instance-of-filesystem-on-windows)-
as Yarda suggest 
Comment 10 Jesse Glick 2008-02-20 17:37:41 UTC
So this is mainly about Windows drives? That was not clear to me before.

Perhaps we should not even try to list all Windows drives. Continue having separate FS instance per drive. Inside impl
code in FileUtil (or whatever), keep a WeakSet<FileSystem> of master filesystems which have already been created anyway,
and only refresh these. Would still cause problems if you worked on a Windows ZIP drive, removed the ZIP drive, then ran
an Ant build somewhere else before garbage collection... but I guess this is not too likely.
Comment 11 rmatous 2008-02-20 17:52:20 UTC
Not only about Windows drives, but also about missing/misty API if you want to have registered listener and get all
notifications(so listen on all possible fileobjects). 

No WeakSet<FileSystem> - too static! Drives can be added/removed during IDE seesion, which should be solved by 3/.
So 2/,3/,4/?

Comment 12 rmatous 2008-02-20 19:46:06 UTC
OK, Jesse, Yarda - sorry if I submitted this issue in unclear way, forget my previous comments and diffs
Please let start from scratch, please comment. I didn't intend to discourage you from next comments, thanks for your time.

Motivation:
I want add 2 methods FileUtil.add/removeFileChangeListener - for users of the API not to care about how many instances
of FileSystem exists or will exist. Maybe could be added complementary method FileUtil.refreshAll that could be used
instead of calling FileUtil.refreshFor(File.listRoots())

Proposed API (javadoc will be added after principal agreement):
API.1: add FileUtil.addFileChangeListener
API.2: add FileUtil.removeFileChangeListener 
API.3: maybe add method FileUtil.refreshAll

Any comments, agreement, disagreement?

Implementation details (for WINDOWS):
Impl.1: continue having separate FS instance per drive 
Impl.2: having one FS instance for all drives

My comments:
I'm able to implement all API.1, API.2, API.3 without additional contract between openide.filesystem and masterfs. I was
able to achieve it for method refreshFor, which uses not nice setAttibute trick.

Yarda supports Impl.2  - 6.0 solution because of backward compatibility. Jesse, strong opinion whether Impl.1/Impl.2?
Comment 13 rmatous 2008-02-20 20:09:45 UTC
Alhough I'm able to implement it without additional contract between openide.filesystem and masterfs somehow, maybe it
would be clear if there was some defined contract (the same for mentioned setAttribute in FileUtil.refreshFor)
Comment 14 Jesse Glick 2008-02-20 20:13:02 UTC
API.{1,2,3} sound fine to me. Note that some clients could in fact offer a hint about which file roots they might care
about, e.g. after running an Ant process it would be reasonable to only refresh FileObject's from the same drive; but I
think the code run when the main window receives focus has no context and so probably needs to refresh everything
(unless we e.g. have it only refresh drives mentioned by roots in the Files tab, i.e. generic source groups of open
projects).

Impl.1 sounds safer and better to me. Windows drives are really separate roots. I don't see any backward compatibility
issue here; FileObject.path was for years clearly documented to not correspond to File.absolutePath - in Javadoc, FAQs,
and other developer documentation.
Comment 15 Jaroslav Tulach 2008-02-21 08:14:53 UTC
I have strong preference for Impl2. Because that is how it worked in 6.0. The only reason why current version behaves 
differently is that we've been told that this behaviour is not achievable while improving performance. I am glad to 
hear it is. Please mimic as much of behaviour from 6.0 as possible, unless you have good reason not to[1].

I do not care about any compatible changes in API that you do, if they are sane. All your proposed changes are. 
Although I do not really like that the name of the methods pretends to be taken from JavaBeans pattern and in fact it 
is not. It is static. pu. st. void registerAll(FileChangeListener fcl) where the listener is known to be held with 
weak references would imho be more honest. However as I said naming is not really important to me, if the behaviour 
tested and documented.


[1] I'd like to define what is not "good reason" - if you have a subjective feeling how some things should look like , 
and you cannot describe any objective benefit why it should be so, then it is not good reason.
Comment 16 rmatous 2008-02-21 12:01:12 UTC
Thanks for your comments. 

My comments:
My goal is very simple and transparent: deliver proposed API + improve naming of proposed methods. Implementation
details like Impl.{1,2} seems to me loosely coupled with it, which I see just implementation detail how to achieve it.
Whatever, but not too late (especially for Impl2). 
Comment 17 Jesse Glick 2008-02-21 16:21:05 UTC
Use FileUtil.addFileChangeListener rather than FileUtil.registerAll if the listeners will be held strongly, which is the
normal design - then you can use FileUtil.weakFileChangeListener if that is what you want, in which case
FileUtil.removeFileChangeListener will be called when the real listener is collected since the naming convention is
obeyed. If you really want to hold the listeners weakly (a surprise since this is not how event sources normally work)
then you should use a nonstandard registration method name such as registerAll.
Comment 18 rmatous 2008-02-25 17:28:35 UTC
Created attachment 57210 [details]
api change - I would like to integrate tomorow
Comment 19 Jesse Glick 2008-02-25 18:51:19 UTC
<code>FileSystems</code> -> <code>FileSystem</code>s

Typos: FileUtil.addFileChangesListener, addFileChangesListener, removeFileChangesListener, 
FileChangeListeners

Having methods getDiskFileSystem() and getDiskFileSystem(File...) makes a call "getDiskFileSystem()" ambiguous. javac
ought to warn about this, I guess? Suggest changing the name of the new method.

Anyway I don't understand this loop:

for (File file : files) {
  FileObject fo = toFileObject(file);
  fs = getDiskFileSystem();
  if (fs != null) break;
}

You're not using the loop variable! So what is it doing?!
Comment 21 Jesse Glick 2008-02-28 17:08:29 UTC
When newly using an API in other modules, you need to adjust the dependencies. In this case, o.apache.tools.ant.module
really needs openide.filesystems 7.7, yet its project.xml still only asks for 6.2. Please fix.
Comment 22 rmatous 2008-02-29 10:49:18 UTC
thanks:
http://hg.netbeans.org/main/rev/53dce63798b3


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