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.
DataObject should have a getLookup() method, for a number of reasons: - Consistency with other APIs (node, etc.) - Any explanation of the APIs involves a lengthy discussion of "what are cookies", why getCookie() is really the same thing as getLookup(), getLookup() is preferred/more modern/etc. Then the user actually has to use getCookie() on DataObject - this doesn't make sense Ideally the end goal should be to deprecate Node.Cookie, Node.getCookie(), DataObject.getCookie(); but even if we can't do all of those things right away, having more self-consistency in the APIs is an important goal right now.
So the question is how to add it while keeping backward compatibility for existing DataObject impls that (1) use CookieSet, (2) override getCookie, (3) both.
Hmm...probably have to do the same override checks as Node; for CookieSet... first use of CookieSet swaps lookup for ProxyLookup with an AbstractLookup + constructor lookup, and CookieSet now writes into the InstanceContent that backs the AbstractLookup?
Important thing to add: every DataObject and its DataNode should have all FileObject's it represents in its lookup. That way we will be able to write actions that act on FileObjects and get one step closer to independence on DataObjects. I guess the time for this is 4.2 +1, I am affraid I cannot handle this by feature freeze.
> I am affraid I cannot handle this by > feature freeze. Ah, but it's not a feature :-)
"every DataObject and its DataNode should have all FileObject's it represents in its lookup" - ah, good point.
This issue is going to be solved as part of the actions rewrite (issue 70280). Beyond already stated goals, one additional is to put the instance InstanceDataObject represents into its lookup.
Yet another possible problem that needs to be addressed: The SaveCookie behaviour needs to work as described at http://openide.netbeans.org/servlets/ReadMsg?list=dev&msgNo=19716
Created attachment 35647 [details] Fix that enhances CookieSet and uses this functionality in DataNode
I have a patch that improves DataNode to always put FileObject into its own node.getLookup(). I'd like to integrate it in a week or so. The changes in DataNode are minimal, the most of the work is done in CookieSet. There is a new factory method to create "general" cookieset that can hold not just cookies, but any object. Each cookie set now implements Lookup.Provider and thus can be used to query for any object including FileObject. The only possibly incompatible change is that DataNode now uses the "generic" cookie set, everything else shall remain compatible. The javadocs shall be more or less written, the change of apichanges.xml and spec versions and dependencies is still tbd.
OwnData*.java got put in the main source tree. Looks like an accident - should have been in the unit test source tree. obj.toArray(new FileObject[0]) should be obj.toArray(new FileObject[obj.size()]). Otherwise looks OK from an API perspective. Can't comment on the implementation since it is nearly incomprehensible to me. I am not really sure how this issue evolved, however. The original request was for DataObject to implement Lookup.Provider. But the patch does not appear to make that change.
Created attachment 35733 [details] The diff to integrate on Monday
Here is a new diff with API changes, module dependencies and a cleanup of Own* classes. I kept new FileObject[0], as that is more safe in multithreaded environment. Re. "...how this issue evolved..." - I'd like to change apisupport file type template to generate the code shown in the OwnDataLoader and OwnDataObject classes - e.g. DataObject would be default implement Lookup.Provider and pass its getCookieSet().getLookup() as a lookup to its OwnDataNode. I can do it as a template enabled only when one codes against org.openide.loaders > 7.0, ok?
toArray(new FileObject[0]) is also unsafe in a multithreaded environment. If updateFilesInCookieSet could potentially be called while obj is being updated from a different thread, then you must e.g. synchronize on obj in all places where it might be used. Looks like by default DataObject.files produces a new set each time, but DO subclasses which override this might try to cache the set. "DataObject would be default implement Lookup.Provider and pass its getCookieSet().getLookup() as a lookup to its OwnDataNode" - oh boy. Good thing we have apisupport now, because no one would ever figure that out without a template.
I'll integrate today as I promised.
"#62707: FileObject(s) are now in DataNode.getLookup()" Checking in openide/loaders/manifest.mf; /shared/data/ccvs/repository/openide/loaders/manifest.mf,v <-- manifest.mf new revision: 1.29; previous revision: 1.28 done Checking in openide/loaders/api/apichanges.xml; /shared/data/ccvs/repository/openide/loaders/api/apichanges.xml,v <-- apichanges.xml new revision: 1.23; previous revision: 1.22 done Checking in openide/loaders/nbproject/project.xml; /shared/data/ccvs/repository/openide/loaders/nbproject/project.xml,v <-- project.xml new revision: 1.23; previous revision: 1.22 done Checking in openide/loaders/src/org/openide/loaders/DataNode.java; /shared/data/ccvs/repository/openide/loaders/src/org/openide/loaders/DataNode.java,v <-- DataNode.java new revision: 1.32; previous revision: 1.31 done Checking in openide/loaders/src/org/openide/loaders/MultiDataObject.java; /shared/data/ccvs/repository/openide/loaders/src/org/openide/loaders/MultiDataObject.java,v <-- MultiDataObject.java new revision: 1.25; previous revision: 1.24 done Checking in openide/loaders/test/unit/src/org/openide/loaders/DataLoaderOrigTest.java; /shared/data/ccvs/repository/openide/loaders/test/unit/src/org/openide/loaders/DataLoaderOrigTest.java,v <-- DataLoaderOrigTest.java new revision: 1.4; previous revision: 1.3 done RCS file: /shared/data/ccvs/repository/openide/loaders/test/unit/src/org/openide/loaders/DataShadowLookupTest.java,v done Checking in openide/loaders/test/unit/src/org/openide/loaders/DataShadowLookupTest.java; /shared/data/ccvs/repository/openide/loaders/test/unit/src/org/openide/loaders/DataShadowLookupTest.java,v <-- DataShadowLookupTest.java initial revision: 1.1 done RCS file: /shared/data/ccvs/repository/openide/loaders/test/unit/src/org/openide/loaders/FileObjectInLookupTest.java,v done Checking in openide/loaders/test/unit/src/org/openide/loaders/FileObjectInLookupTest.java; /shared/data/ccvs/repository/openide/loaders/test/unit/src/org/openide/loaders/FileObjectInLookupTest.java,v <-- FileObjectInLookupTest.java initial revision: 1.1 done Checking in openide/nodes/apichanges.xml; /shared/data/ccvs/repository/openide/nodes/apichanges.xml,v <-- apichanges.xml new revision: 1.8; previous revision: 1.7 done Checking in openide/nodes/nbproject/project.properties; /shared/data/ccvs/repository/openide/nodes/nbproject/project.properties,v <-- project.properties new revision: 1.10; previous revision: 1.9 done Checking in openide/nodes/src/org/openide/nodes/AbstractNode.java; /shared/data/ccvs/repository/openide/nodes/src/org/openide/nodes/AbstractNode.java,v <-- AbstractNode.java new revision: 1.11; previous revision: 1.10 done Checking in openide/nodes/src/org/openide/nodes/CookieSet.java; /shared/data/ccvs/repository/openide/nodes/src/org/openide/nodes/CookieSet.java,v <-- CookieSet.java new revision: 1.6; previous revision: 1.5 done RCS file: /shared/data/ccvs/repository/openide/nodes/src/org/openide/nodes/CookieSetLkp.java,v done Checking in openide/nodes/src/org/openide/nodes/CookieSetLkp.java; /shared/data/ccvs/repository/openide/nodes/src/org/openide/nodes/CookieSetLkp.java,v <-- CookieSetLkp.java initial revision: 1.1 done Checking in openide/nodes/src/org/openide/nodes/Node.java; /shared/data/ccvs/repository/openide/nodes/src/org/openide/nodes/Node.java,v <-- Node.java new revision: 1.13; previous revision: 1.12 done Checking in openide/nodes/src/org/openide/nodes/NodeLookup.java; /shared/data/ccvs/repository/openide/nodes/src/org/openide/nodes/NodeLookup.java,v <-- NodeLookup.java new revision: 1.5; previous revision: 1.4 done Checking in openide/nodes/test/unit/src/org/openide/nodes/CookieSetTest.java; /shared/data/ccvs/repository/openide/nodes/test/unit/src/org/openide/nodes/CookieSetTest.java,v <-- CookieSetTest.java new revision: 1.4; previous revision: 1.3 done Checking in openide/nodes/test/unit/src/org/openide/nodes/NodeLookupTest.java; /shared/data/ccvs/repository/openide/nodes/test/unit/src/org/openide/nodes/NodeLookupTest.java,v <-- NodeLookupTest.java new revision: 1.5; previous revision: 1.4
Here are the associated changes in API Support: IDE: [6.11.06 10:15] Committing started cvs server: scheduling file `templateDataObjectWithLookup.javx' for addition cvs server: use 'cvs commit' to add this file permanently RCS file: /shared/data/ccvs/repository/apisupport/project/src/org/netbeans/modules/apisupport/project/ui/wizard/loader/templateDataObjectWithLookup.javx,v done Checking in templateDataObjectWithLookup.javx; /shared/data/ccvs/repository/apisupport/project/src/org/netbeans/modules/apisupport/project/ui/wizard/loader/templateDataObjectWithLookup.javx,v <-- templateDataObjectWithLookup.javx initial revision: 1.1 done Checking in NewLoaderIterator.java; /shared/data/ccvs/repository/apisupport/project/src/org/netbeans/modules/apisupport/project/ui/wizard/loader/NewLoaderIterator.java,v <-- NewLoaderIterator.java new revision: 1.27; previous revision: 1.26 done Checking in templateDataNode.javx; /shared/data/ccvs/repository/apisupport/project/src/org/netbeans/modules/apisupport/project/ui/wizard/loader/templateDataNode.javx,v <-- templateDataNode.javx new revision: 1.5; previous revision: 1.4
I confess I still do not understand why DataObject itself does not implement Lookup.Provider. Without that, we are not really free of cookies at all, because we have to do for (DataObject d : ...) { // uncompilable: OpenCookie oc = d.getLookup().lookup(OpenCookie.class); OpenCookie oc = d.getCookie(OpenCookie.class); } or (worse) ask d.getNodeDelegate().getLookup().lookup(OpenCookie.class).
I see. I am concerned about backward compatibility. Especially about the consistency between lookup of DataObject and lookup of its Node delegate. But your example is valid. It would be better if DataObject had lookup by default. I can try.
Created attachment 35822 [details] Adding DataObject.getLookup
I propose to add DataObject.getLookup that delegates to getNodeDelegate().getLookup. Basically this seems like the only safe solution that can guarantee that people who do override DataObject.getCookie to return something special, will continue to find the "specials" in the DataObject.getLookup. Moreover this does not require any special reflection tricks (those are done in NodeLookup). Also this solution simplifies Jesse's example. And last but not least, everyone who creates new "File Type" will get more effective implementation that relies on MultiDataObject.getCookieSet().getLookup. Unless there are major objections I add this to trunk tomorrow.
Not too bad. Will be weird in case the DataNode contains e.g. Index cookie but for the most part DataNode and DataObject cookie sets should match anyway. I am concerned about the threading, though: Children.MUTEX may be acquired on a call that does not look like it has anything to do with Nodes. This may have odd effects which DataObject.getCookie does not suffer from. Safer would be to duplicate the kind of logic used in adding Node.getLookup but I have no idea how much work that would be.
DataObject.getLookup integrated. Re: Threading. Good comment. It can happen. I hope it is not going to be that often - e.g. it is good one can do dataObject.getLookup(), but still the most common way is to get Lookup from Utilities.actionGlobalContext and that will be safe. The trouble with getCookie and getLookup (as shown in Nodes) is that there may be subclasses that override one or the other and then it is not easy to keep some consistency. I am happy I can reuse all the hack in NodeLookup, if I had to do the same tricks in DataObject itself, it would be too adventurous. If a threading problem appears then I suggest to fix the actual DataObject subclass by overriding getLookup as the apisupport template shows. Checking in loaders/api/apichanges.xml; /shared/data/ccvs/repository/openide/loaders/api/apichanges.xml,v <-- apichanges.xml new revision: 1.24; previous revision: 1.23 done Checking in loaders/src/org/openide/loaders/DataObject.java; /shared/data/ccvs/repository/openide/loaders/src/org/openide/loaders/DataObject.java,v <-- DataObject.java new revision: 1.32; previous revision: 1.31 done Checking in loaders/test/unit/src/org/openide/loaders/BasicDataObjectTest.java; /shared/data/ccvs/repository/openide/loaders/test/unit/src/org/openide/loaders/BasicDataObjectTest.java,v <-- BasicDataObjectTest.java new revision: 1.6; previous revision: 1.5 done Checking in loaders/test/unit/src/org/openide/loaders/DataNodeTest.java; /shared/data/ccvs/repository/openide/loaders/test/unit/src/org/openide/loaders/DataNodeTest.java,v <-- DataNodeTest.java new revision: 1.8; previous revision: 1.7 done Checking in loaders/test/unit/src/org/openide/loaders/MultiDataObjectTest.java; /shared/data/ccvs/repository/openide/loaders/test/unit/src/org/openide/loaders/MultiDataObjectTest.java,v <-- MultiDataObjectTest.java new revision: 1.6; previous revision: 1.5 done