Bug 153347 - Add removeNotify() equivalent to ChildFactory
Add removeNotify() equivalent to ChildFactory
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Nodes
6.x
All All
: P3 (vote)
: 6.x
Assigned To: _ tboudreau
issues@platform
: API, API_REVIEW_FAST
Depends on:
Blocks: 138228 151716
  Show dependency treegraph
 
Reported: 2008-11-17 22:23 UTC by _ tboudreau
Modified: 2009-02-19 22:53 UTC (History)
2 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
Patch (8.41 KB, patch)
2008-11-17 22:43 UTC, _ tboudreau
Details | Diff
Revised patch (10.81 KB, patch)
2008-11-26 00:06 UTC, _ tboudreau
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description _ tboudreau 2008-11-17 22:23:11 UTC
I've noticed a number of cases in the last year where, in using ChildFactory, I am forced to use a weak listener when I
could explicitly remove the listener if my ChildFactory were notified nothing is interested in it anymore.

I'm attaching a backward-compatible patch which adds a ChildFactory.Detachable subclass which contains such a
removeNotify() method.

Concrete use-case:  I am implementing NodeFactory support for mobility projects;  their children are based on
ChildFactory.  It would be a much smaller diff to simply use NodeFactory directly, rather than rewrite all of the
children  classes to be layer-based;  also, the resulting Children classes would probably perform worse than the current
code.

I'm happy to commit this if we can give it a quick review.  Diff attached.
Comment 1 _ tboudreau 2008-11-17 22:43:18 UTC
Attaching patch with test.  Includes an addNotify() method with similar semantics to that of Children.  Use case: 
simplifies logic for subclasses which would otherwise have to do this work in createKeys() but would be forced to test
if it was really the first call to createKeys().  There are already some examples of doing that in mobility and elsewhere.
Comment 2 _ tboudreau 2008-11-17 22:43:42 UTC
Created attachment 73843 [details]
Patch
Comment 3 _ tboudreau 2008-11-17 23:19:36 UTC
Note: Nd to update spec version and correct it in apichanges doc before applying patch.
Comment 4 Jesse Glick 2008-11-20 16:47:58 UTC
While you're at it, please let ChildFactory.createNodesForKey return null rather than new Node[0], changing

return n == null ? new Node[0] : new Node[] { n };

to

return n == null ? null : new Node[] { n };

Probably nothing else needs to change besides Javadoc, since {Asynch,Synch}Children just uses it for
Children.Keys.createNodes, which already accepts null in place of new Node[0].
Comment 5 _ wadechandler 2008-11-20 17:04:18 UTC
Why would you want null returned versus an empty array? I'm asking because if someones code is relying on that empty
array it will break now with a null pointer exception, and also by returning empty arrays code can be made simpler when
all you'll do is loop over it because you can just

Node[] nodes = ...
for(Node n: nodes){
   ...
}

or

for(int i=0;i<nodes.length;i++){
   ...
}

and you don't need a null check or anything. You do create an empty object to return when there really is nothing to do
however. What would the main gains be versus the other besides those I've mentioned?
Comment 6 Jesse Glick 2008-11-20 19:38:46 UTC
Null is good here simply to avoid constructing a new array object, since that is a very common case, and this code is
performance-critical. The method is part of a protected interface so no else could be calling it. As I said, the return
value would only be used by Children.Keys, which already accepts null from createNodes for the same reason.
Comment 7 _ wadechandler 2008-11-20 20:11:33 UTC
Thanks for the clarifications.
Comment 8 Jesse Glick 2008-11-20 22:14:37 UTC
My request for createNodesForKey to be able to return null was not really related to this patch, I just thought it would
be good to bundle it in. Anyway, I will retroactively label that [JG01].


[JG02] The <summary> in apichanges.xml is bogus.


[JG03] I believe "@param T" is correct, not "@param <T>". (Not sure, because the javadoc tool does not seem to actually
display docs for type parameters anyway.)


[JG04] It looks like AsynchChildren.remoteNotify will call factory.removeNotify() twice. ChildFactoryTest.DetachableImpl
should check this: addNotify should first assertFalse(added) and removeNotify should first assertFalse(removed).


[JG05] Do you have any patches prepared showing .Detachable in use? In mobility.* or wherever?
Comment 9 _ tboudreau 2008-11-20 22:32:00 UTC
> Do you have any patches prepared showing .Detachable in use? In mobility.* or wherever?

http://www.netbeans.org/nonav/issues/showattachment.cgi/73844/mproject.diff
Comment 10 _ tboudreau 2008-11-26 00:05:30 UTC
> I believe "@param T" is correct, not "@param <T>"

Hmm, our source editor generates @param <T> FWIW;  doesn't mean it's correct :-/

The patch I'm about to attach should address all of the comments.  If there are no further objections, I'd like to
commit this in the next few days.
Comment 11 _ tboudreau 2008-11-26 00:06:34 UTC
Created attachment 74150 [details]
Revised patch
Comment 12 _ tboudreau 2008-12-01 13:49:14 UTC
Integrated in e24a1479f40c


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