If usecase 5 is important, wouldn't something like
    Object NbEditorUtilities.getDataKey(FileObject fileObject)
which returns the DataObject simply as an Object for use in maps satisfy things?

#5 is much less common than #1-#4, and when it's used it's usually in some kind of cache hashmap.

Using the FileObject instead of the DataObject for the map key might be enough in most situations, with the downside that on file renames the cache will be invalidated. This doesn't happen with DataObject because they handle renames transparently.

NbEditorUtilities.getDataKey(FileObject fileObject) isn't a bad idea as long as we document the properties of the key so people know where they fit and where they don't. Right now the implementation could return the DataObject, but we could relax that further on to return the FileObject or even something lighter.

If some given data cache does not already track rename, then using anything less than the DataObject would seem to be a hassle. If you use FileObject, then the cache will simply be wasting space until the entry is invalidated (things could be worse than simply wasted space depending on how the cache is used).

> Right now the implementation could return the DataObject, but we could
> relax that further on to return the FileObject or even something lighter.

I don't see how that would ever be possible unless you are guaranteed that there could not be a rename.

Yes, if rename is important and recomputing the cache value expensive, DataObject is the way to go. It just has to be a conscious decision.

If the only penalty for switching from DataObject to FileObject is a cheap cache value re-computation for renamed files (which, generally, happens rarely) then switching map keys might be a worthy tradeoff as it could remove the whole dependency on DataSystems API.

The API proposal doesn't impact #5, only #1-#4.

Re case #5: I am afraid that some of the places which are using DOs as keys to a map actually do that for correctness. In fact, the AnnotationHolder in the spi.editor.hints and the AnnotationsHolder in java.editor have been written to use FOs, and have been changed to use DOs later to fix the move bugs (part of bug #131403 for spi.editor.hints and #145606 for java.editor).

Another similar, but a bit different usecase, is actually listening on a file move, as done in java.source's OpenedEditors (converted from FOs to DOs as partial fix for bug #131403). parsing.api's Scheduler is probably another candidate for a similar FO->DO conversion (see bug #199357), although the matters are slightly more complex there.

> switching from DataObject to FileObject ... remove the
> whole dependency on DataSystems API.

With 
    Object NbEditorUtilities.getDataKey(FileObject fileObject)
there is no inherent dependency on DataSystem.

Though in some (most?) cases where you need a key for the file you might need to work with a DataObject; I just don't know.

> The API proposal doesn't impact #5, only #1-#4.

The idea was that by adding getDataKey to the proposal then #5 could be handled without needing to depend on DataSystem. But if the cases where one would use getDataKey will also require DataSystem API, then there is no advantage to this additional method.

@Jan Lahoda : Interesting. Good to see DataObjects are used on purpose there.

#5 was caused by my idea that in some situations using DataObject as a map key might be too much if you don't need the DataObject, but just some uniqueness.

This uniqueness might be perhaps satisfied by something else, say the primary FileObject.

Or, as @err suggested, we could generate an Object / Key somehow (NbEditorUtilities.getDataKey(FileObject) ?) and just guarantee that this Key will have some properties. 

Even Jesse mentioned on nbdev@ the possibility of 'something in org.openide.filesystems defining a "file key" preserved after a rename.'

Regarding #1-#4, what do you guys think of the suggested API (http://wiki.netbeans.org/FilesystemsLoadersBridge#API_Proposal )? To me, the only big API change is FileObject becoming a Lookup.Provider.

Question in the proposal: http://wiki.netbeans.org/wiki/index.php?title=FilesystemsLoadersBridge&diff=52364&oldid=52260

ERR01 - Is the word "Primary" needed? Why not just NbEditorUtilities.getFileObject(...)? Can a Document and/or JTextComponent have more than one FileObject?
ERR02 - Why have NbEditorUtilities.getPrimaryFileObject() for both doc and JTextComponent?
ERR03 - Can NbEditorUtilities.getPrimaryFileObject(doc) return null?
        If so, for convenience, would something like:
            NbEditorUtilities.lookupFileObject(doc, SomeCookie.class)
        be worth having?
ERR04 - Why not provide method to get a unique key? Do most use cases that need a key require the DataSystem API anyway?

I have removed all references to use case #5 in the wiki. The API proposal remains unchanged: http://wiki.netbeans.org/FilesystemsLoadersBridge#API_Proposal

>ERR01 - Is the word "Primary" needed? Why not just
NbEditorUtilities.getFileObject(...)? Can a Document and/or JTextComponent have
more than one FileObject?

DataObjects might represent multiple FileObjects but only one primary FileObject (see http://bits.netbeans.org/dev/javadoc/org-openide-loaders/org/openide/loaders/MultiDataObject.html ).

By extension a Document will only have a single FileObject (the "primary" one).

I don't mind changing the API here, I was just using "primary" to be explicit, but the javadoc should suffice.

>ERR02 - Why have NbEditorUtilities.getPrimaryFileObject() for both doc and
JTextComponent?

No particular purpose. It's just a simple overload.

>ERR03 - Can NbEditorUtilities.getPrimaryFileObject(doc) return null?
        If so, for convenience, would something like:
            NbEditorUtilities.lookupFileObject(doc, SomeCookie.class)
        be worth having?

This being the equivalent of NbEditorUtilities.getPrimaryFileObject(doc).getLookup().lookup(SomeCookie.class) with some null checks?

It does make sense, because some code casts Document.StreamDescriptionProperty directly to a DataObject and tries to get a Cookie. In practice it doesn't seem to be a problem because there doesn't exist a Document for non-existing DataObjects in the editor infrastructure (they would see NPEs otherwise).

We could add NbEditorUtilities.lookup(doc, SomeCookie.class). I'll update the proposal API.

>ERR04 - Why not provide method to get a unique key? Do most use cases that need a key require the DataSystem API anyway?

Jan Lahoda seems to say DataObjects are on purpose in my examples. I've removed use case #5 from this proposal. There is still something to debate here but since it doesn't impact the API, I'll leave it for some future proposal.

(In reply to comment #8)
> Question in the proposal:
> http://wiki.netbeans.org/wiki/index.php?title=FilesystemsLoadersBridge&diff=52364&oldid=52260

Y01 The above question has been deleted from the proposal without providing an acceptable answer.

There was/is nothing in the API impacting identity, only a discussion related to use-case #5 which I have also removed.

I think it's nice to debate about uniqueness but I also don't want this proposal to drag forever for something that isn't in the suggested code changes.

Since the proposal is about "... modules ... don't actually need DataObjects". The issue is relevant.

> >ERR04 - Why not provide method to get a unique key? Do most use cases that need a key require the DataSystem API anyway?
> 
> Jan Lahoda seems to say DataObjects are on purpose 

Right, but are the DataObjects only for uniqueness or are the DataObjects used in other ways as well in those situations?

If it is only for uniqueness, then providing a method that supplies a unique key for a file object means in those cases you don't need the DataSystems API. 

On the other hand, if you still need the DataObject then a method for unique key is of no use. And it serves no purpose in the proposal.

I didn't mean that nobody needs DataObject. Only that a subset of the editor modules that have a dependency on DataSystems API don't really need it if this proposal is implemented.

I haven't inspected all the modules, but at least in spi.editor.hints, DataObjects are used just for uniqueness. I'm pretty sure there are other modules where the DataObjects are used for some other reasons too: perhaps listeners, etc.

So, use-case #5 wasn't pointless, only that using the primary FileObject instead wasn't a solution if we want to be rename-transparent. So I removed all references to #5 in the wiki to slim-down the proposal and, presumably, fasten the review process.

What we need in order to get rid of DataObject which are used only as map keys is either what you suggested:

  Object NbEditorUtilities.getDataKey(FileObject fileObject)

or something like

  Object FileObject.getRenameAwareKey();  (or perhaps return a Key marker interface so we don't use a pure Object).

If we update the FileObject public API then we need another private bridge to DataSystems API and something like

public interface FileObjectKeyFactory {
   Object getKey(FileObject fo);
 }

which just returns DataObject.find(fo) in the default service implementation.

I wouldn't mind changing the FileObject API but I gather that's more sensitive. Jaroslav ?

(In reply to comment #14)
> I removed all
> references to #5 in the wiki to slim-down the proposal and, presumably, fasten
> the review process.

Probably a good idea. You might want to track a rename-friendly key as a separate API review, since it does not seem strongly related.

> or perhaps return a Key marker
> interface so we don't use a pure Object

Maybe necessary anyway so you can listen to renames (comment #5).

So, considering use-case #5 is excluded from the current proposal and deferred to another one, do we have an agreement?

(In reply to comment #12)
> There was/is nothing in the API impacting identity

The proposal suggests to replace DataObject with FileObject. DataObjects are "logical" wrapper around FileObjects and they also maintain identity while renamed, moved. From that I conclude that the proposal does not provide full replacement of DataObject. I am not willing to accept an API change which is known at the time of creation to be unsufficient into Filesystem API.

If FileObject is supposed to implement Lookup.Provider, then we need to know how the content of lookup helps to preserve identity.

I think this ( http://wiki.netbeans.org/wiki/index.php?title=FilesystemsLoadersBridge&diff=52395&oldid=52364 ) was a valid fix to your initial question. I effectively removed the uniqueness issue from this proposal.

The proposal was never meant to provide a full replacement of DataObject / DataSystems API. It was just supposed to reduce the DataSystems API dependency for *some* of the editor modules, which only needed use-case #1-#4.

I don't see how this API change is not "sufficient". The API change stands on its own and doesn't have anything to do with uniqueness. Actually, this whole review never touched anything from the API proposal, except uniqueness which was a secondary remark.

> If FileObject is supposed to implement Lookup.Provider, then we need to know
> how the content of lookup helps to preserve identity.

Why?

BTW, I'm also open to a Skype meeting about this.

The uniqueness discussion seems to be this proposal Achilles' heel and even after removing it from the proposal it still lingers on for some reason. So, either it's something I'm missing or it's something not clear about the proposal.

Filesystem API and DataSystem API are general APIs used by many clients, not only by editor. I am not willing to accept new  concept into Filesystem API that works only for few usecases in editor (as is even known to not satisfy all the editor needs - see the deleted parts in http://wiki.netbeans.org/wiki/index.php?title=FilesystemsLoadersBridge&diff=52395&oldid=52364).

I fully agree that it does not make sense for many editor modules to depend on DataSystem API and that it is desirable to replace the current API usage with simpler dependency. Associating DataObject's lookup with FileObject is a way to achieve that. Howeveris not 100% replacement - especially due to lost identity during move, rename. FileSystem API is not a place for half-baken solutions.

I am not particularly keen on changing the FileObject API per-se, all I want is to reach an agreement on a change that allows us to get rid of the DataSystems API dependency and hides it beween a simpler API.

So, one suggestion is to have FileObjectLookupFactory.getLookup(FileObject).

FileObjectLookupFactory could either be a *private* service and be used by FileObject.getLookup() as we have been discussing so far or a *public* one.

If FileObjectLookupFactory is a public service, then modules could be using it directly by doing Lookup.getDefault().lookup(FileObjectLookupFactory.class).getLookup(fo).lookup(Cookie.class) themselves. This avoids the FileObject API change but introduces FileObjectLookupFactory direct usage into modules.

To me, even introducing such a global service would be an improvement!

Now, if you want to discuss about uniqueness, this could be fixed with a new interface

interface FileObject.RenameAwareKey {}

which will could get by calling FileObject.getLookup().lookup(FileObject.RenameAwareKey.class);

The lookup returned by the FileObjectLookupFactory @ServiceProvider in DataSystemsAPI would just return a simple wrapper, something like

class DataObjectKey implements FileObject.RenameAwareKey {

  private final DataObject obj;
  DataObjectKey(DataObject obj){
   this.obj = obj;
  }

  public boolean equals(Object o){
    if(o instanceof DataObjectKey) {
      return this.obj.equals( ((DataObjectKey) o).obj);
    }
    return super.equals(o);
  }
}

RenameAwareKey is useful for knowing a FileObject has been renamed and getting access to the renamed FileObject. If its only method is equals, then one can as well store FileObject.getPath() and compare it.

However I am more interested in behavior of Lookup in case of move. E.g.

Lookup old = fo.getLookup();
moved = fo.move(someOtherFolder);
Lookup now = moved.getLookup();

what is the relation between old and now? In case the same operationo is done on DataObjects they would be ==.

> RenameAwareKey is useful for knowing a FileObject has been renamed and getting
> access to the renamed FileObject. If its only method is equals, then one can as
> well store FileObject.getPath() and compare it.

No, it was meant to be used as a key in a Map instead of a DataObject (use-case #5). So, it's a Map key that's rename-aware or rename-transparent. What's missing in my example is the hashCode implementation.

> However I am more interested in behavior of Lookup in case of move. E.g.
> 
> Lookup old = fo.getLookup();
> moved = fo.move(someOtherFolder);
> Lookup now = moved.getLookup();
> 
> what is the relation between old and now? In case the same operationo is done
> on DataObjects they would be ==.

With the current proposal API it would still be ==.

(But why is == relevant?)

(In reply to comment #24)
> (But why is == relevant?)

Well, I wanted to say that the content of the two lookups should be the same. If they are ==, then it is guaranteed.

> No, it was meant to be used as a key in a Map instead of a DataObject (use-case
> #5). So, it's a Map key that's rename-aware or rename-transparent. What's
> missing in my example is the hashCode implementation.

Actually, if we have lookups that are ==, then one can use IdentityHashMap<Lookup,Whatever>. 

> With the current proposal API it would still be ==.

OK, then I am satisfied and you can proceed with patch. Please include a test that guarantees == after move and rename.

Ok, I'll start working on a patch and attach it here (I assume?).

But I don't really like the idea of IdentityHashMap<Lookup, Whatever> because replacing the DataObject with the Lookup as a map key is a bit better, but not by much :-) (Speaking in light of use-case #5).

Here is a patch against jet-main revision 32dda4ed3d72 (Mar 27 01:33:44 2012): 01-api.patch

Some notes:

* because FileObject implements Lookup.Provider, any module that depends on Filesystems API needs to also add a dependency on Lookup API otherwise you get a compile-time error.

A few modules were using Filesystems API without Lookup API and had to be patched for a build [1]. Use 02-project.xml.patch

* because I wanted to be strict and implement == on getLookup() even when no FileObjectLookupFactory exists, I introduced the only field into FileObject. It's a cheap Lookups.singleton but it felt odd.

* I introduced a new public package, org.openide.filesystems.implspi, but tried to restrict it to DataSystems only as suggested by http://wiki.netbeans.org/NbmPackageStability#Mixing_public_and_friend_packages

I accordance to Murphy's law I tried finding a quick module to patch with the new behavior but I only could find the ones that actually needed DataObjects. Finally, I found html.editor.lib which had a single case where the DataSystems API dependency was needed and patched it: see 03-html.editor.lib.patch



1. ant.browsetask/nbproject/project.xml apisupport.crudsample/nbproject/project.xml apisupport.osgidemo/nbproject/project.xml apisupport.restsample/nbproject/project.xml css.lib/nbproject/project.xml java.hints.declarative.test/nbproject/project.xml mobility.project.bridge/nbproject/project.xml openide.filesystems/nbproject/project.xml parsing.ui/nbproject/project.xml profiler.freeform/nbproject/project.xml spellchecker.bindings.htmlxml/nbproject/project.xml spellchecker.bindings.properties/nbproject/project.xml wag.codegen.j2ee/nbproject/project.xml wag.codegen.java/nbproject/project.xml wag.codegen.php/nbproject/project.xml wag.codegen/nbproject/project.xml wag.manager/nbproject/project.xml

Created attachment 117348 [details]
API proposal

Created attachment 117349 [details]
project.xml patching (add Lookup API if FileSystems API was a dependency)

Created attachment 117350 [details]
html.editor.lib with the new API

Any feedback on this?

Y02 I don't like the new org.openide.filesystems.implspi - I don't mind having new class with protected abstract methods in org.openide.filesystems.

Y03 I'd prefer to initialize selfLookup once. Either by provided lookup or fallback. And then hold on it.

Y04 apichanges.xml should mention the change.

Y05 the tests should be executed without and also with openide.loaders on classpath. That will ensure the behavior is same with and without default implementation of FileObjectLookupFactory

Y06 I don't see the change in openide.loaders to provide the FileObjectLookupFactory in the diff. Feel free to include that in the same diff.

Otherwise OK on filesystems side. Remind me after 7.2 is branched to integrate your change.

(In reply to comment #32)
> Y02 I don't like the new org.openide.filesystems.implspi - I don't mind having
> new class with protected abstract methods in org.openide.filesystems.

Ok, I've moved it to org.openide.filesystems.

> Y03 I'd prefer to initialize selfLookup once. Either by provided lookup or
> fallback. And then hold on it.

selfLookup is already initialized only once. Are you talking about the FileObjectLookupFactory.getLookup(FileObject) call instead? Do you want a single call there?

I guess that should be doable but I would have to synchronize the code since getLookup might be called from anywhere.

Also, the current code works if we switch the FileObjectLookupFactory at runtime. It won't if we keep a fixed reference.

> Y04 apichanges.xml should mention the change.

Done

> Y05 the tests should be executed without and also with openide.loaders on
> classpath. That will ensure the behavior is same with and without default
> implementation of FileObjectLookupFactory

I think the existing tests cover these situations. I don't really know offhand how to change the classpath just for a given test.

I guess I could alter the global Lookup somehow and look for the FileObjectLookupFactory instance?

> Y06 I don't see the change in openide.loaders to provide the
> FileObjectLookupFactory in the diff. Feel free to include that in the same
> diff.

It's in the patch: FileObjectDataObjectBridge.java with a @ServiceProvider.

Created attachment 118724 [details]
API proposal

Jaroslav, I see a release72_beta_base branch in the repository. Is it too early to integrate the proposed changes?

The real branching will happen when release72_base tag appears in the golden repository. Anyway we should develop the changes on a branch first. I've created 
FileObjectGetLookup209231 in core-main repository and set a builder
http://deadlock.netbeans.org/hudson/job/prototypes-FileObjectGetLookup209231 up.

Ok, I'll start committing the patch on that branch.

PS: The Hudson build failed for some reason.

(In reply to comment #37)
> Ok, I'll start committing the patch on that branch.
> 
> PS: The Hudson build failed for some reason.

Right the hudson build fails when executing validation tests. Somebody needs to update dependencies of all modules using openide.filesystems API to also depend on openide.lookup. Do you want to do so?

Yes, I'll start committing to that branch.

Well, apparently my 'emi' user is only allowed to push to jet-main and not core-main.

I've sent an email now to ask permission on core-main too.

Meanwhile attach your hg bundle ~/improvements.hg here, I can apply them.

Here are my (not yet well documented) changes:
http://hg.netbeans.org/core-main/rev/6c1ea3400216
Please review the concept.

Hy, I'll look into this next week, after I return from holiday.

I've updated the javadoc
http://deadlock.netbeans.org/hudson/job/prototypes-FileObjectGetLookup209231/javadoc/org-openide-filesystems/apichanges.html#fileobject-lookup
and I'd like to integrate on Wednesday.

BTW use @linkplain for "move operation".

(In reply to comment #42)
> Here are my (not yet well documented) changes:
> http://hg.netbeans.org/core-main/rev/6c1ea3400216
> Please review the concept.

Interesting: so you are trying to get closer to a named lookup and the 'bridge' module becomes 'settings'.

FileObjectLkp.reassign looks similar to a create operation. So you only care about equality in terms of ==, not the actual lookup content?

Why NamedServicesProvider.lookupFor(Object) and not lookupFor(FileObject)?

Other than that it looks ok (but different than what I thought of).

> Why NamedServicesProvider.lookupFor(Object) and not lookupFor(FileObject)?

openide.util.lookup cannot see FileObject type. That would imply cyclic dependency between openide.filesystems and openide.util.lookup.

> Interesting: so you are trying to get closer to a named lookup and the 'bridge'
> module becomes 'settings'.

There already was a bridge between FileUtil.getConfigObject and settings module. I prefer to re-use it, rather then expose a new SPI interface in openide.filesystems. 

> Other than that it looks ok (but different than what I thought of).

Yeah, adding the method to NamedServicesProvider is not the nicest thing around, but from a point of user of the FileObject.getLookup() it should not matter. Thus who cares?

> FileObjectLkp.reassign looks similar to a create operation. So you only care
> about equality in terms of ==, not the actual lookup content?

Right, you can put the FileObject.getLookup() into IdentifyHashMap to track identity of the FileObject. I've just tried to improve javadoc: http://hg.netbeans.org/core-main/rev/75dae001a3b5

Merged as ergonomics#03c4d8750398

(In reply to comment #47)
> I prefer to re-use it, rather then expose a new SPI interface in
> openide.filesystems. [...]
> from a point of user of the FileObject.getLookup() it should not matter

There is a user-visible drawback to this scheme: you cannot (meaningfully) use FileObject.getLookup from a unit test until you manually add o.n.m.settings as a runtime test dep, which is not intuitive - you would expect that merely having o.o.loaders would suffice for DataObject.getLookup() to be used, but it does not.

> you can put the FileObject.getLookup() into IdentifyHashMap to track
> identity of the FileObject

Or more to the point, attach a listener to the lookup and be assured that it will fire events even if the file gets renamed, right?

(In reply to comment #49)
> (In reply to comment #47)
> > I prefer to re-use it, rather then expose a new SPI interface in
> > openide.filesystems. [...]
> > from a point of user of the FileObject.getLookup() it should not matter
> 
> There is a user-visible drawback to this scheme: you cannot (meaningfully) use
> FileObject.getLookup from a unit test until you manually add o.n.m.settings as
> a runtime test dep, which is not intuitive - you would expect that merely
> having o.o.loaders would suffice for DataObject.getLookup() to be used, but it
> does not.

I am aware of that. It is the same situation as with Lookups.forPath & FolderLookup. You need the settings module and people sometimes run into this problem. There is a note in non-normative section of javadoc to attract their attention.

I was considering to revert the dependency between openide.loaders and settings. Make openide.loaders depend on settings (as settings API is data system independent) and move the bridge code from settings module to openide.loaders. But the change was huge and I decided to leave it outside of scope of this issue.

> > you can put the FileObject.getLookup() into IdentifyHashMap to track
> > identity of the FileObject
> 
> Or more to the point, attach a listener to the lookup and be assured that it
> will fire events even if the file gets renamed, right?

http://hg.netbeans.org/ergonomics/rev/ac5e15f85396

Integrated into 'main-golden', will be available in build *201208230001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/03c4d8750398
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: Merge of #209231 to default branch