Bug 62707 - DataObject.getLookup()
DataObject.getLookup()
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Data Systems
5.x
All All
: P2 (vote)
: 6.x
Assigned To: Jaroslav Tulach
issues@platform
: API, API_REVIEW_FAST
Depends on: 73015 98197 146377
Blocks: 61824 70280
  Show dependency treegraph
 
Reported: 2005-08-19 23:05 UTC by _ tboudreau
Modified: 2008-12-22 11:55 UTC (History)
6 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
Fix that enhances CookieSet and uses this functionality in DataNode (63.30 KB, patch)
2006-10-30 16:59 UTC, Jaroslav Tulach
Details | Diff
The diff to integrate on Monday (76.40 KB, patch)
2006-11-02 16:09 UTC, Jaroslav Tulach
Details | Diff
Adding DataObject.getLookup (9.03 KB, patch)
2006-11-07 07:44 UTC, Jaroslav Tulach
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description _ tboudreau 2005-08-19 23:05:59 UTC
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.
Comment 1 Jesse Glick 2005-08-20 00:04:36 UTC
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.
Comment 2 _ tboudreau 2005-08-20 00:37:26 UTC
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?
Comment 3 Jaroslav Tulach 2005-08-20 15:37:03 UTC
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. 
Comment 4 _ tboudreau 2005-08-20 23:17:06 UTC
> I am affraid I cannot handle this by 
> feature freeze. 

Ah, but it's not a feature :-)
Comment 5 Jesse Glick 2005-08-22 21:47:09 UTC
"every DataObject and its DataNode should have all FileObject's it represents in
its lookup" - ah, good point.
Comment 6 Jaroslav Tulach 2006-01-31 15:45:33 UTC
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. 
Comment 7 Jaroslav Tulach 2006-02-22 08:47:41 UTC
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 
Comment 8 Jaroslav Tulach 2006-10-30 16:59:54 UTC
Created attachment 35647 [details]
Fix that enhances CookieSet and uses this functionality in DataNode
Comment 9 Jaroslav Tulach 2006-10-30 17:05:15 UTC
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. 
Comment 10 Jesse Glick 2006-10-30 18:18:50 UTC
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.
Comment 11 Jaroslav Tulach 2006-11-02 16:09:35 UTC
Created attachment 35733 [details]
The diff to integrate on Monday
Comment 12 Jaroslav Tulach 2006-11-02 16:14:03 UTC
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?
Comment 13 Jesse Glick 2006-11-02 17:26:52 UTC
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.
Comment 14 Jaroslav Tulach 2006-11-06 08:22:47 UTC
I'll integrate today as I promised.
Comment 15 Jaroslav Tulach 2006-11-06 08:24:26 UTC
"#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
Comment 16 Jaroslav Tulach 2006-11-06 09:19:44 UTC
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
Comment 17 Jesse Glick 2006-11-06 16:29:27 UTC
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).
Comment 18 Jaroslav Tulach 2006-11-07 07:03:32 UTC
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.
Comment 19 Jaroslav Tulach 2006-11-07 07:44:11 UTC
Created attachment 35822 [details]
Adding DataObject.getLookup
Comment 20 Jaroslav Tulach 2006-11-07 07:48:20 UTC
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.
Comment 21 Jesse Glick 2006-11-07 17:18:42 UTC
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.
Comment 22 Jaroslav Tulach 2006-11-08 12:20:37 UTC
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


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo