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.
Currently the procedure for creating a lookup over a system filesystem folder is a bit compicated, since people often need to create registries of objects: try { new FolderLookup (DataFolder.findFolder(Repository.getDefault().getDefaultFileSystem().getRoot().getFileObject("com/foo/bar"))).getLookup() } catch (DataObjectNotFoundException donfe) { //do something } I'm attaching a patch which adds the method Lookups.forPath(String path). It defines an SPI interface which is implemented in DataSystems using FolderLookup. Benefits: - It's much more intuitive and simple - Nobody is directly depending on either the System Filesystem or DataSystems/FolderLookup - could be replaced if desired Objections? BTW, I'm having some problems getting the unit test working - using MockServices seems to get me a Lookup that some part of core someone tries to load can't use: java.lang.ClassCastException: org.openide.util.Lookup$DefLookup cannot be cast to org.netbeans.core.startup.MainLookup at org.netbeans.core.startup.MainLookup.moduleClassLoadersUp(MainLookup.java:86) at org.netbeans.core.startup.MainLookup.systemClassLoaderChanged(MainLookup.java:79) Otherwise I believe it should work - all very straightforward.
Created attachment 39735 [details] Patch
You are right, this is an important improvement. People should not depend on loaders in order to read some settings. Y01 Wrong licenses in the diff. Y02 I want a solution that will work in standalone mode. E.g. when only Utilities API is on classpath. I've created issue 98437 to record that request. Y03 I want a solution that works with filesystems, but without loaders - e.g. in http://dvbcentral.sf.net - imho a simple provider to read registration like in Y02, plus understand .instance files on system file system. Y04 I guess the full provider shall not be in loaders, but in core/settings Btw. As it seems that there might be more people working on this issue (http://www.netbeans.org/nonav/issues/showattachment.cgi/39744/NamedLookups1.diff), I suggest to create a branch and continue there.
Jarda, I'm not sure if I completely understand. It sounds like issue 98437 is about the ability to search META-INF/* in the classpath. That's definitely a useful thing, and I like the idea. It's also pretty different from lookup over a folder in the system filesystem (well, both are path-based lookup mechanisms). I'd like to solve the simple case for 6.0 - make it easy to get a lookup over a folder in the SFS w/o depending directly on loaders, etc. I agree it would be nice if there were a working default implementation even if you don't have loaders installed - but that's probably not a blocker for integrating the patch here - someone just has to implement it over core/settings or whatever. It could get confusing to have two path-based ways to get a Lookup - one over META-INF/*, another over the system filesystem. Both are valuable; I'm not sure both are the same thing. Are you proposing that the default implementation of Lookups.forPath() will read META-INF/*, and if Loaders is there, then it uses the system filesystem? Or are these orthagonal things (I think they are - at least, I can imagine wanting to use both in one application). I'm not completely clear on what you want to do, and "I suggest to create a branch and continue there" scares me that it will not happen for 6.0 (that's what I'm really worried about).
Y01 - easy to fix Y02 - you can have one, just implement it - once this is integrated, the API will be there. It seems to me issue 98437 can be fixed once this issue is integrated; they don't need to be integrated at the same time. Y03 - Same answer as Y02 Y04 - So you want me to move the implementation of the SPI to core/settings? I'm still a little unclear about whether there should not be some API for META-INF/* and perhaps the default implementation of this falls back to that if no provider is registered. META-INF/* is useful in its own right.
Y02, Y03 - Issue 98437 is just a tool, it is not fix for any of these problems. Imho these TCRs should be fixed before integration of anything. If you disagree with me that these are TCRs, consider trying full review and finding enough voices to turn my opinion into minority.
I would have no objection to this if it were located in openide/loaders. Then it would just be a straightforward convenience method. I don't understand the wisdom of creating a method which makes it look like you do not need Datasystems to look things up when in fact you do.
Ideally it provides a migration path to do without data systems; and with an impl such as Jarda suggests, it can even be used w/o DataSystems. All of that seems like goodness. We could easily enough provide a token that recommends that there be some impl of it. Generally, if someone wants a lookup over a folder in the SFS, they're dealing with FileObjects and Lookups and whatever they expect to find in said Lookups. DataSystems is an implementation detail they should not be confronted with - it adds no particular value. So, it's not about pretending ds is not needed, but about making doing something simple actually as simple as it should be and with minimum cognitive overhead.
The cognitive overhead of a method in openide/util and a method in openide/loaders is the same, I think - you call a method and get a Lookup either way. The use of Datasystems is not an implementation detail; that module is what defines and implements folder lookup, so if you want to do folder lookup you should use that module. If however the purpose is to at some point get the objects from some place other than layers, as is implied in issue #98437, then I think we should back up (certainly not fool with this for 6.0!) and decide what we want the NB registration mechanism to be in the long term. Adding lots of little separate ways to register things with no clear direction will just make things more confusing and intimidating. We need to *delete* (or harshly deprecate) bad ways of registering things as much as we need to add new ways. Let's not forget that we already have a simple and low-overhead system for registering objects in a namespace - which is used only by the debugger modules. Our story on lookup (with a lowercase 'l') is already too long to tell.
Re. "cognitive overhead" - well, the method is the same, but the difference is the amount of APIs one has to understand. In case of method in Lookups - one basically needs just two classes Lookup and Lookups. In case of anything in datasystems one has to be aware of filesystems, lookup, DataObject, etc. That is big difference. Re. "if it were located in openide/loaders" - well, the point is that in your application you do not want to deal with openide/loaders when reading configuration files. You want to read the configuration files. It unwise to force people to depend on openide/loaders just because they want to read content of a folder filled with .instance files. All they want is to create a "space" and read it. Lookups.forPath is ideal for that purpose. Re. "use of Datasystems is not an implementation detail" - in Tim's proposal it is not and I fully agree that if we should integrate Tim's patch, then the method should be somewhere in openide/loaders - however this goes against the "cognitivity" and also against the dependencies - I have no openide/loaders in http://dvbcentral.sf.net still I need Lookups.forPath Re. "purpose is to at some point get the objects from some place other than layers" - well, it is not purpose, it is necessity. In order for the new method to be useful, it has to be in openide/util and not in openide/loaders. However it is crazy to include new method in openide/util that only says "you need to include openide/loaders on classpath so i can work". This implies that it is necessary to include some default/fallback implementation. Imho the use of metaInfServices(prefix) is reasonably cheap fallback. I am not proud of it, but necessity is more important than pride. Re. "debugger" - if Lookups.forPath existed when debugger was created, the debugger would use them. Actually when I was working on this proposal I wanted Martin E. to migrate debugger to Lookups.forPath, basically it is compatible. However then I learned that debugger lookup is using its own style of masking, ordering, etc. Indeed introduced without any review or documentation. I was horrified and I see that as another reason why we need Lookups.forPath - we need standard way how to do these things and we do not want every module to do in their own way.
> Imho the use of metaInfServices(prefix) is reasonably cheap > fallback. I am not proud of it, > but necessity is more important than pride. Tell me if I am understanding everything correctly: You are okay with Lookups.forPath() and there will be a default implementation that is META-INF/* and if you have DS on the classpath then you will get a Lookup over a folder in the system filesystem instead. Is that accurate? I agree it's ugly to have two pretty diverse registries and the classpath will decide which one is live (I suppose one could proxy both, but that is ugly from the performance standpoint - replicating the performance problems with the services dir in SFS for everything else). I could live with it if it gets us the method on Lookups rather than somewhere in DS. Long-term that will be important, and it's much cleaner for anyone who has to understand it. My worry for such a scheme is that, at the risk of a proliferation of registration methods, it would be nice if the META-INF/* registration mechanism were available even with DS on the classpath - things like MimeLookup could be handled more efficiently that way. The only thing that you get from the system filesystem that you can't get from META-INF/* is live changes at runtime (for example, GenericNavigator lets you define a new regexp mapped to a random content type and writes the appropriate files to the SFS on the fly).
I have created a branch easylookup_98426 for openide and core and put there initial implementation that satisfies my taste (api in openide/util, primitive registration there as well, no-loaders impl in core/bootstrap, fully compatible impl in core/settings). It is not polished yet, but I'll attach the diff for easier review here. By end of the week I'd like to get objections about the overall approach and have then classified as TCR, TCA, advice, minor, etc.
Created attachment 39944 [details] cvs upd -r easylookup_98426 core openide && cvs diff -r easylookup_98426_root core openide
Please note the use of an "inheriting" test that guarantees that the behaviour defined for openide/util is satisfied by all implementations and that the additional behaviour of core/bootstrap is satisfied by the implementation of core/settings.
Btw. the naming that I used is from my original patch, not from Tim's one. If you prefer Lookups.forPath & co., tell me, I have no strong opinion on naming.
Slight preference for Lookups.forPath() over Lookups.namedService() Lookups.forPath() similar to Class.forName(), etc., and it is going to involve paths.
TB01: If I understand this patch correctly, if I run without core/settings present, I get a lookup over META-INF/*; but if core/settings or DS is present then I lose the contents of META-INF/* and only get SFS lookup. If that is correct, it seems a little worrisome - shouldn't I get a merge of both? Otherwise, if I have a module written for an app like dvbcentral, it will be broken if I try to reuse it inside an app that contains core/settings. Seems bad for reuse. Or maybe I'm not completely understanding the patch. TB02: Would prefer that NamedServices.find() be package-private and all access go through Lookups.forPath() - is there a use case for having both? TB03: Does this patch handle ordering attributes the same way FolderLookup over DS will? I don't see any reference to TopologicalSort in RecognizeInstanceFiles.OverFiles TB04: In FOItem, surely this code can't be right: private static String getClassName(FileObject fo) { ... attr = fo.getAttribute("instanceCreate"); if (attr != null) { return attr.getClass().getName(); } ... } Isn't this always going to return the type of the return value of fo.getAttribute("instanceCreate") - i.e. java.lang.String, rather than the type the string defined? Looks like it should be attr.toString()
TB1: META-INF/* works in all three cases. ensured by the inherited test. TB2: NamedServices is not in public package[1], so this should not matter. TB3: Good observation, will fix. TB4: Good observation, I'll investigate, however the code was copied from InstanceDataObject and is likely completely the same... [1] However access to non-public packages is not forbidden in org.openide.util
We have two ordering mechanisms - #position comments in service-style entries, and fs ordering attributes in fs-style registration. Probably it needs to be documented how the two registration mechanisms entries are ordered with respect to each other - i.e. if I have some objects registered by each method, do the services-style ones come before the fs-registered ones or after.
Created attachment 40640 [details] We can remove ~300 lines in our codebase, if we have this new API
TB04: The code in codebase is different than your sample and seems fine to me. I've just finished polishing documentation and unless there are unresolved TCRs I'd like to integrate this change on Mon Apr 17, 2007. Probably it would be good to integrate the fix in the all codebase, but of course, that is not required and it can wait.
Created attachment 40641 [details] Snapshot from the easylookup_98426 branch
April 17 is a Tuesday.
+ /** Interface for core/startup to provide lookup overt system filesystem. overt -> over
Thanks for your reviews, I'll integrate the API by tomorrow.
*** Issue 98437 has been marked as a duplicate of this issue. ***
IDE:------------------------------------------------- IDE: [16.4.07 22:46] Committing started Checking in core/settings/api/doc/changes/apichanges.xml; /shared/data/ccvs/repository/core/settings/api/doc/changes/apichanges.xml,v <-- apichanges.xml new revision: 1.9; previous revision: 1.8 done Checking in core/navigator/src/org/netbeans/modules/navigator/ProviderRegistry.java; /shared/data/ccvs/repository/core/navigator/src/org/netbeans/modules/navigator/ProviderRegistry.java,v <-- ProviderRegistry.java new revision: 1.6; previous revision: 1.5 done Checking in openide/util/src/org/openide/util/lookup/Lookups.java; /shared/data/ccvs/repository/openide/util/src/org/openide/util/lookup/Lookups.java,v <-- Lookups.java new revision: 1.8; previous revision: 1.7 done Checking in openide/util/src/org/openide/util/lookup/MetaInfServicesLookup.java; /shared/data/ccvs/repository/openide/util/src/org/openide/util/lookup/MetaInfServicesLookup.java,v <-- MetaInfServicesLookup.java new revision: 1.7; previous revision: 1.6 done Checking in core/navigator/nbproject/project.xml; /shared/data/ccvs/repository/core/navigator/nbproject/project.xml,v <-- project.xml new revision: 1.7; previous revision: 1.6 done Checking in core/startup/src/org/netbeans/core/startup/layers/RecognizeInstanceFiles.java; /shared/data/ccvs/repository/core/startup/src/org/netbeans/core/startup/layers/RecognizeInstanceFiles.java,v <-- RecognizeInstanceFiles.java new revision: 1.2; previous revision: 1.1 done Checking in openide/util/apichanges.xml; /shared/data/ccvs/repository/openide/util/apichanges.xml,v <-- apichanges.xml new revision: 1.25; previous revision: 1.24 done Checking in openide/util/test/unit/src/org/openide/util/lookup/NamedServicesLookupTest.java; /shared/data/ccvs/repository/openide/util/test/unit/src/org/openide/util/lookup/NamedServicesLookupTest.java,v <-- NamedServicesLookupTest.java new revision: 1.2; previous revision: 1.1 done Checking in openide/util/test/unit/src/org/openide/util/lookup/PrefixServicesLookupTest.java; /shared/data/ccvs/repository/openide/util/test/unit/src/org/openide/util/lookup/PrefixServicesLookupTest.java,v <-- PrefixServicesLookupTest.java new revision: 1.2; previous revision: 1.1 done Checking in openide/util/test/unit/src/org/openide/util/lookup/MetaInfServicesLookupTest.java; /shared/data/ccvs/repository/openide/util/test/unit/src/org/openide/util/lookup/MetaInfServicesLookupTest.java,v <-- MetaInfServicesLookupTest.java new revision: 1.5; previous revision: 1.4 done Checking in core/options/nbproject/project.xml; /shared/data/ccvs/repository/core/options/nbproject/project.xml,v <-- project.xml new revision: 1.10; previous revision: 1.9 done Checking in core/startup/test/unit/src/org/netbeans/core/startup/layers/NamedFSServicesLookupTest.java; /shared/data/ccvs/repository/core/startup/test/unit/src/org/netbeans/core/startup/layers/NamedFSServicesLookupTest.java,v <-- NamedFSServicesLookupTest.java new revision: 1.2; previous revision: 1.1 done Checking in openide/util/nbproject/project.properties; /shared/data/ccvs/repository/openide/util/nbproject/project.properties,v <-- project.properties new revision: 1.27; previous revision: 1.26 done Checking in core/startup/nbproject/project.xml; /shared/data/ccvs/repository/core/startup/nbproject/project.xml,v <-- project.xml new revision: 1.14; previous revision: 1.13 done Checking in core/settings/test/unit/src/org/netbeans/modules/settings/RecognizeInstanceObjectsTest.java; /shared/data/ccvs/repository/core/settings/test/unit/src/org/netbeans/modules/settings/RecognizeInstanceObjectsTest.java,v <-- RecognizeInstanceObjectsTest.java new revision: 1.2; previous revision: 1.1 done Checking in core/options/src/org/netbeans/modules/options/advanced/Model.java; /shared/data/ccvs/repository/core/options/src/org/netbeans/modules/options/advanced/Model.java,v <-- Model.java new revision: 1.15; previous revision: 1.14 done Checking in core/startup/src/META-INF/services/org.netbeans.modules.openide.util.NamedServicesProvider; /shared/data/ccvs/repository/core/startup/src/META-INF/services/org.netbeans.modules.openide.util.NamedServicesProvider,v <-- org.netbeans.modules.openide.util.NamedServicesProvider new revision: 1.2; previous revision: 1.1 done Checking in core/settings/manifest.mf; /shared/data/ccvs/repository/core/settings/manifest.mf,v <-- manifest.mf new revision: 1.20; previous revision: 1.19 done Checking in core/settings/src/org/netbeans/modules/settings/RecognizeInstanceObjects.java; /shared/data/ccvs/repository/core/settings/src/org/netbeans/modules/settings/RecognizeInstanceObjects.java,v <-- RecognizeInstanceObjects.java new revision: 1.2; previous revision: 1.1 done Checking in openide/util/src/org/netbeans/modules/openide/util/NamedServicesProvider.java; /shared/data/ccvs/repository/openide/util/src/org/netbeans/modules/openide/util/NamedServicesProvider.java,v <-- NamedServicesProvider.java new revision: 1.2; previous revision: 1.1 done Checking in core/settings/nbproject/project.xml; /shared/data/ccvs/repository/core/settings/nbproject/project.xml,v <-- project.xml new revision: 1.17; previous revision: 1.16 done Checking in ide/golden/impl-deps.txt; /shared/data/ccvs/repository/ide/golden/impl-deps.txt,v <-- impl-deps.txt new revision: 1.83; previous revision: 1.82 done Checking in core/settings/src/META-INF/services/org.netbeans.modules.openide.util.NamedServicesProvider; /shared/data/ccvs/repository/core/settings/src/META-INF/services/org.netbeans.modules.openide.util.NamedServicesProvider,v <-- org.netbeans.modules.openide.util.NamedServicesProvider new revision: 1.2; previous revision: 1.1 done Checking in core/src/org/netbeans/core/LookupCache.java; /shared/data/ccvs/repository/core/src/org/netbeans/core/LookupCache.java,v <-- LookupCache.java new revision: 1.14; previous revision: 1.13 done Checking in openide/loaders/src/org/openide/loaders/FolderLookup.java; /shared/data/ccvs/repository/openide/loaders/src/org/openide/loaders/FolderLookup.java,v <-- FolderLookup.java new revision: 1.23; previous revision: 1.22 done Checking in core/options/src/org/netbeans/modules/options/CategoryModel.java; /shared/data/ccvs/repository/core/options/src/org/netbeans/modules/options/CategoryModel.java,v <-- CategoryModel.java new revision: 1.5; previous revision: 1.4 done Checking in openide/util/src/org/openide/util/doc-files/api.html; /shared/data/ccvs/repository/openide/util/src/org/openide/util/doc-files/api.html,v <-- api.html new revision: 1.11; previous revision: 1.10 done Checking in xtest/instance/master-config.xml; /shared/data/ccvs/repository/xtest/instance/master-config.xml,v <-- master-config.xml new revision: 1.226; previous revision: 1.225 done IDE: [16.4.07 22:47] Committing finished