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.
Consider adding support for common task on FS instances like addFileChangeListener.
Created attachment 56438 [details] first draft
Created attachment 56505 [details] contract proposal
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).
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).
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.
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.
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.
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.
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
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.
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/?
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?
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)
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.
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.
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).
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.
Created attachment 57210 [details] api change - I would like to integrate tomorow
<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?!
Fixed: http://hg.netbeans.org/main/rev/1f347677128b http://hg.netbeans.org/main/rev/e47108d175e2 http://hg.netbeans.org/main/rev/240779bd6c8c http://hg.netbeans.org/main/rev/b7ae67e1f58b
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.
thanks: http://hg.netbeans.org/main/rev/53dce63798b3