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.

Bug 98426 - Provide easier way to create a Lookup over an SFS folder
Summary: Provide easier way to create a Lookup over an SFS folder
Status: RESOLVED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: -- Other -- (show other bugs)
Version: 6.x
Hardware: All All
: P2 blocker (vote)
Assignee: Jaroslav Tulach
URL:
Keywords: API, API_REVIEW_FAST
: 98437 (view as bug list)
Depends on: 104658
Blocks:
  Show dependency tree
 
Reported: 2007-03-21 07:23 UTC by _ tboudreau
Modified: 2008-12-22 10:58 UTC (History)
6 users (show)

See Also:
Issue Type: ENHANCEMENT
Exception Reporter:


Attachments
Patch (16.17 KB, patch)
2007-03-21 07:42 UTC, _ tboudreau
Details | Diff
cvs upd -r easylookup_98426 core openide && cvs diff -r easylookup_98426_root core openide (45.00 KB, patch)
2007-03-26 13:38 UTC, Jaroslav Tulach
Details | Diff
We can remove ~300 lines in our codebase, if we have this new API (61.88 KB, patch)
2007-04-09 21:43 UTC, Jaroslav Tulach
Details | Diff
Snapshot from the easylookup_98426 branch (78.86 KB, patch)
2007-04-09 21:58 UTC, Jaroslav Tulach
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description _ tboudreau 2007-03-21 07:23:48 UTC
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.
Comment 1 _ tboudreau 2007-03-21 07:42:50 UTC
Created attachment 39735 [details]
Patch
Comment 2 Jaroslav Tulach 2007-03-21 12:16:09 UTC
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.
Comment 3 _ tboudreau 2007-03-21 20:21:56 UTC
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).
Comment 4 _ tboudreau 2007-03-21 20:45:02 UTC
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.
Comment 5 Jaroslav Tulach 2007-03-21 22:01:16 UTC
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.
Comment 6 Jesse Glick 2007-03-21 22:21:15 UTC
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.
Comment 7 _ tboudreau 2007-03-21 22:26:38 UTC
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.
Comment 8 Jesse Glick 2007-03-21 22:58:21 UTC
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.
Comment 9 Jaroslav Tulach 2007-03-23 11:23:59 UTC
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.
Comment 10 _ tboudreau 2007-03-24 12:10:38 UTC
> 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).
Comment 11 Jaroslav Tulach 2007-03-26 13:07:35 UTC
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.
Comment 12 Jaroslav Tulach 2007-03-26 13:38:19 UTC
Created attachment 39944 [details]
cvs upd -r easylookup_98426 core openide && cvs diff -r easylookup_98426_root core openide
Comment 13 Jaroslav Tulach 2007-03-26 13:40:06 UTC
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.
Comment 14 Jaroslav Tulach 2007-03-26 13:42:36 UTC
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.
Comment 15 _ tboudreau 2007-03-26 22:46:02 UTC
Slight preference for Lookups.forPath() over Lookups.namedService()
Lookups.forPath() similar to Class.forName(), etc., and it is going to involve
paths.
Comment 16 _ tboudreau 2007-03-27 17:37:29 UTC
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()


Comment 17 Jaroslav Tulach 2007-03-27 18:25:00 UTC
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
Comment 18 _ tboudreau 2007-03-27 19:00:13 UTC
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.
Comment 19 Jaroslav Tulach 2007-04-09 21:43:46 UTC
Created attachment 40640 [details]
We can remove ~300 lines in our codebase, if we have this new API
Comment 20 Jaroslav Tulach 2007-04-09 21:57:23 UTC
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.
Comment 21 Jaroslav Tulach 2007-04-09 21:58:42 UTC
Created attachment 40641 [details]
Snapshot from the easylookup_98426 branch
Comment 22 Jesse Glick 2007-04-09 22:08:38 UTC
April 17 is a Tuesday.
Comment 23 _ tboudreau 2007-04-10 00:29:29 UTC
+ /** Interface for core/startup to provide lookup overt system filesystem.

overt -> over

Comment 24 Jaroslav Tulach 2007-04-16 09:59:10 UTC
Thanks for your reviews, I'll integrate the API by tomorrow.
Comment 25 Jaroslav Tulach 2007-04-16 21:48:00 UTC
*** Issue 98437 has been marked as a duplicate of this issue. ***
Comment 26 Jaroslav Tulach 2007-04-16 21:49:11 UTC
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