Bug 91529 - Support for asynchronously computed child nodes
Support for asynchronously computed child nodes
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Nodes
6.x
All All
: P3 (vote)
: TBD
Assigned To: _ tboudreau
issues@platform
API_USABILITY
: API, API_REVIEW
Depends on:
Blocks: 105608
  Show dependency treegraph
 
Reported: 2006-12-24 08:43 UTC by _ tboudreau
Modified: 2008-12-22 10:52 UTC (History)
6 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
Patch adding AsynchChildren to the Nodes API (12.56 KB, patch)
2006-12-24 08:53 UTC, _ tboudreau
Details | Diff
Wait.gif missing from patch (881 bytes, image/gif)
2006-12-24 19:15 UTC, _ tboudreau
Details
Revised patch (12.51 KB, text/plain)
2006-12-24 19:20 UTC, _ tboudreau
Details
Revised patch which also handles batching of key creation (16.76 KB, patch)
2006-12-24 22:00 UTC, _ tboudreau
Details | Diff
Revised patch (37.29 KB, patch)
2007-03-21 05:04 UTC, _ tboudreau
Details | Diff
Yet another version of the patch (38.64 KB, patch)
2007-03-21 12:08 UTC, _ tboudreau
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description _ tboudreau 2006-12-24 08:43:28 UTC
As we've discussed elsewhere, it would be nice if there were direct support for
creating child Nodes asynchronously - currently everyone has to roll their own
threading code for this.

Attached is a patch which adds this to the Nodes API.

BTW, re failing tests we discussed elsewhere:  It appears that getNodes(true)
may return the old Node list after a call to setKeys().  I'm not sure if this is
by design or not, but it's the behavior I've observed (hence the loop to check
for child node count in AsynchChildrenTest).  Perhaps that is the source of the
occasionally failing tests for asynch children elsewhere in the IDE which Jarda
and Jesse mentioned.
Comment 1 _ tboudreau 2006-12-24 08:53:29 UTC
Created attachment 36931 [details]
Patch adding AsynchChildren to the Nodes API
Comment 2 Jesse Glick 2006-12-24 17:56:38 UTC
<T extends Object> is better written simply as <T>.


@SuppressWarnings("unchecked") is gratuitous in removeNotify(). Use emptyList().


I don't exactly see the point of the final class + interface pattern here in a
support SPI; would probably be simpler to make it an abstract class with most or
all methods either final or abstract, and probably all protected - in particular
update(boolean) should not be public I guess.


R.cancelled is only used in the outer class, so should be moved there.


wait.gif not included in patch.


I thought Yarda had made some change to Nodes so that a wait cursor would
automatically appear over the explorer component if some node was being expanded?


PackageView cannot use this class because it displays nodes incrementally as
they are computed, whereas AC only displays the full set of nodes at the end.


Using Thread.sleep in the unit test makes it inherently unreliable. Should use
monitors and checkpoints (or Yarda's synchronized logging technique).
Comment 3 _ tboudreau 2006-12-24 19:15:07 UTC
Created attachment 36933 [details]
Wait.gif missing from patch
Comment 4 _ tboudreau 2006-12-24 19:20:54 UTC
Created attachment 36934 [details]
Revised patch
Comment 5 _ tboudreau 2006-12-24 22:00:04 UTC
Created attachment 36935 [details]
Revised patch which also handles batching of key creation
Comment 6 _ tboudreau 2006-12-24 22:03:58 UTC
Revised patch attached, addressing r.cancelled, Collections.<Object>emptyList()
and <T extends Object>, and the issues with being able to batch child creation

Re "Using Thread.sleep in the unit test makes it inherently unreliable. Should use
monitors and checkpoints (or Yarda's synchronized logging technique)."  I've
made some improvements there.

Re the tests of existing lazy Children implementations that fail, which Jarda
and Jesse have mentioned:  Given the difficulty I had writing the unit tests in
the current patch, I strongly suspect any such intermittently failing test is
actually testing a race condition not the behavior of a Children implementation.
 Almost anything can trigger an additional call to addNotify() and it's easy to
write a test that looks like it passes (it triggers a call to setKeys() and
behaves as if the nodes array will be updated synchronously, and looks like it
works, or it triggers an additional unexpected call to addNotify() and once in a
while tests the nodes array before the nodes have been updated).

Re whether to use an inner interface or just make the class non-final, it's
probably mostly a matter of aesthetics - I don't have a strong opinion either way.
Comment 7 Jaroslav Tulach 2006-12-25 14:33:22 UTC
I guess this is a request for review of an API that Tim wants to integrate. I 
am marking it as fast track. However because we are all on holidays, please 
let the timer expire on Jan 5, 2007. This way Petr will have enough time to 
comment and object. 
Comment 8 Jaroslav Tulach 2006-12-25 14:48:53 UTC
Y01 hide the AsynchChildren class, it is an implementation detail and put  
factory method somewhere, possibly NodeOp, preferably (in my opinion)  
Children.create(AsynchChildren, ...). As a result there needs to be an 
additional method like  
"addChangeListener(ChangeListener l) throws TooManyListenerException" in the 
Provider interface to give it access to the update method. 
  
Y02 Make the Provider interface top level class, give it some more concrete  
name  
  
Y03 Re. "wait cursor': I suggest the new factory method to have more 
parameters that can customize the behaviour, or that there could be more 
methods: 
static Children create(Provider/*renamed*/ p); // create synchronous children 
static Children createAsynchronous(Provider p, boolean waitNode); 
as Jesse is right, if the creation is asynch, the wait cursor should be there 
by default. This suggestion requires you to rename the asynchCreateKeys method 
to be renamed into createKeys/populateKeys. 
 
Y04 Why is testGetNodesWaits directly calling addNotify? Imho it should not - 
it is a protected method. The same for justComputeNodes in the other test. 
 
Y05 Given Milos K.'s experience with output window tests, I strongly suggest 
to remove any "time specification" (like wait(2000), Thread.sleep(200) & co.) 
from the test. If it was on me, I would reject the patch due to "too random 
and unstable tests", but at the end this is up to  Petr, as he is the one that 
will have to live with the tests like these.  
 
 
Comment 9 _ tboudreau 2006-12-26 04:35:16 UTC
> Y05 Given Milos K.'s experience with output window tests, I strongly suggest 
> to remove any "time specification" (like wait(2000), Thread.sleep(200) & co.) 
> from the test...

I would happily remove them if someone can tell me a reliable and
*side-effect-free* way to be notified when the child nodes have been updated
after a call to setKeys() - i.e. something that can't trigger another call to
addNotify().  (NodeListener is not side-effect-free).

Re the longer wait()'s, they are only so as not to torture someone (aka me)
waiting for the test to timeout if it is going to fail.

Comment 10 _ tboudreau 2006-12-26 04:37:24 UTC
> static Children create(Provider/*renamed*/ p); // create synchronous children

I'm fine with that, but doesn't it make Children.Keys superfluous?
Comment 11 sreimers 2007-02-16 19:19:34 UTC
Any time frame for integration?
Comment 12 _ tboudreau 2007-02-16 23:31:31 UTC
No objections from fast api review, so I suppose Real Soon Now[tm].  If anybody
on cc feels like going ahead and integrating it before I get to it, that would
be fine too.
Comment 13 Jaroslav Tulach 2007-02-17 06:54:37 UTC
Dear selectively blind Tim, I see Y01, Y02, Y03, Y04, Y05. Please lower your 
selective blindness and notice them as objections as well.
Comment 14 _ tboudreau 2007-03-21 05:04:16 UTC
Created attachment 39732 [details]
Revised patch
Comment 15 _ tboudreau 2007-03-21 05:09:44 UTC
I've attached a revised patch that should address all of the mentioned issues. 
To wit:
 - AsynchChildren is now hidden
 - AsynchChildren.Provider -> ChildFactory
 - ChildFactory is now an abstract class with a protected refresh() method to
cause the children object to refresh its keys.  All methods protected as Jesse
suggested.
 - All sleeps removed from unit tests.  There are some timed waits, but in all
of these cases the test will pass whether the wait times out or gets a notification
 - Children.create() added, per Jarda's suggestion.  Also per Jarda's suggestion
a boolean parameter specifies whether the ChildFactory should be called
asynchronously or as Children.Keys normally would (in which case the only
benefit to using ChildFactory is that you can easily switch it over to
asynchronous operation if you need to).

Further comments?
Comment 16 Jaroslav Tulach 2007-03-21 10:38:46 UTC
Y06 There is too many usages of @SuppressWarning, why? Imho the code should be 
correctly generified instead...

Y07 useWaitNode in the create method - ok, I know what that does when set to 
true - it shows the wait node, but there is no documentation for what it does 
when set to false. Reading the code seems to indicate that the behaviour is 
synchronous. If that is the intention then the parameter should be renamed 
to "asynchronous". 

Y08 However there is also 3rd kind of work: asynchronous computation and no 
wait node, do not you want to support that as well? This is used when 
expanding folders - initialization is asynchronous and explorer window shows 
wait icon

Y09 Should not the create method be in the ChildFactory itself?
Comment 17 _ tboudreau 2007-03-21 11:17:02 UTC
> Y06 There is too many usages of @SuppressWarning, why? Imho the code should be 
> correctly generified instead...

I hadn't noticed that Children.Keys has been generified.  Nice.

> If that is the intention then the parameter should be renamed 
> to "asynchronous".

Funny, I started out with "asynchronous" and changed it.  For a beginner,
useWaitNode is probably clearer.

> Y08 However there is also 3rd kind of work: asynchronous computation and no 
> wait node, do not you want to support that as well?

I thought about it, but wasn't sure the added complexity was worth it - how
often is a Please Wait node actually worse than no response?  Probably the
solution would be an overridable method on ChildFactory that returns the wait
node and is allowed to return null;  the default implementation will return the
wait node currently used.  Worth adding?

> Y09 Should not the create method be in the ChildFactory itself?
Thought about that as well.  Either choice is compatible (we can guarantee
nobody has an existing Children subclass with a create method that takes a
ChildFactory, so adding it to Children is fine).  I suspect more people will
find it and use it if it's on Children itself, so I slightly prefer that, but I
agree that it's all a bit more self-contained on ChildFactory.  How badly do you
want me to move it?

Comment 18 _ tboudreau 2007-03-21 12:07:37 UTC
Attaching new diff.  The only thing not addressed is Y09, moving the creation
method to ChildFactory.

I made one change that seemed like a good idea, remembering how annoyed I always
am at having to return a 1-item Node array from Children.createNodes().  So
there are now two protected methods for creating Nodes on ChildFactory - 

protected Node createNodeForKey (T key) {
    return null;
}

and 
protected Node[] createNodesForKey (T key) {
    Node n = createNodeForKey (key);
    return n == null ? new Node[0] : new Node[] { n };
}

(there needs to be a default impl of createNodeForKey() otherwise anyone who
only wants to use createNodesForKey() would still have to do an empty impl of
createNodeForKey()).

All in all, it seems like a very convenience for eliminating a bit boilerplate
everyone has to type when almost all Children are 1:1 keys:nodes.
Comment 19 _ tboudreau 2007-03-21 12:08:18 UTC
Created attachment 39745 [details]
Yet another version of the patch
Comment 20 _ tboudreau 2007-03-27 02:46:14 UTC
No further comments on this in a while; are there any further objections to its
integration?  The one outstanding question is whether the create() method should
be on Children or ChildFactory.
Comment 21 _ tboudreau 2007-06-02 02:24:21 UTC
Reassigning to myself.
Comment 22 _ tboudreau 2007-06-02 04:48:04 UTC
IDE:-------------------------------------------------
IDE: [6/1/07 8:46 PM] Committing "Nodes API" started
cvs server: scheduling file `src/org/openide/nodes/SynchChildren.java' for addition
cvs server: scheduling file `src/org/openide/nodes/ChildFactory.java' for addition
cvs server: scheduling file `src/org/openide/nodes/AsynchChildren.java' for addition
cvs server: scheduling file
`test/unit/src/org/openide/nodes/ChildFactoryTest.java' for addition
cvs server: use 'cvs commit' to add these files permanently
cvs server: scheduling file `wait.gif' for addition
cvs server: use 'cvs commit' to add this file permanently
RCS file: /cvs/openide/nodes/src/org/openide/nodes/AsynchChildren.java,v
done
Checking in src/org/openide/nodes/AsynchChildren.java;
/cvs/openide/nodes/src/org/openide/nodes/AsynchChildren.java,v  <-- 
AsynchChildren.java
initial revision: 1.1
done
Checking in src/org/openide/nodes/Children.java;
/cvs/openide/nodes/src/org/openide/nodes/Children.java,v  <--  Children.java
new revision: 1.21; previous revision: 1.20
done
RCS file: /cvs/openide/nodes/src/org/openide/nodes/SynchChildren.java,v
done
Checking in src/org/openide/nodes/SynchChildren.java;
/cvs/openide/nodes/src/org/openide/nodes/SynchChildren.java,v  <-- 
SynchChildren.java
initial revision: 1.1
done
RCS file: /cvs/openide/nodes/src/org/openide/nodes/ChildFactory.java,v
done
Checking in src/org/openide/nodes/ChildFactory.java;
/cvs/openide/nodes/src/org/openide/nodes/ChildFactory.java,v  <--  ChildFactory.java
initial revision: 1.1
done
Checking in src/org/openide/nodes/Bundle.properties;
/cvs/openide/nodes/src/org/openide/nodes/Bundle.properties,v  <--  Bundle.properties
new revision: 1.4; previous revision: 1.3
done
RCS file: /cvs/openide/nodes/src/org/openide/nodes/wait.gif,v
done
Checking in src/org/openide/nodes/wait.gif;
/cvs/openide/nodes/src/org/openide/nodes/wait.gif,v  <--  wait.gif
initial revision: 1.1
done
RCS file: /cvs/openide/nodes/test/unit/src/org/openide/nodes/ChildFactoryTest.java,v
done
Checking in test/unit/src/org/openide/nodes/ChildFactoryTest.java;
/cvs/openide/nodes/test/unit/src/org/openide/nodes/ChildFactoryTest.java,v  <--
 ChildFactoryTest.java
initial revision: 1.1
done
Checking in nbproject/project.properties;
/cvs/openide/nodes/nbproject/project.properties,v  <--  project.properties
new revision: 1.11; previous revision: 1.10
done
Checking in apichanges.xml;
/cvs/openide/nodes/apichanges.xml,v  <--  apichanges.xml
new revision: 1.9; previous revision: 1.8
done
IDE: [6/1/07 8:47 PM] Committing "Nodes API" finished
Comment 23 Jesse Glick 2007-06-03 22:08:34 UTC
Some minor tweaks:

Checking in src/org/openide/nodes/AsynchChildren.java;
/shared/data/ccvs/repository/openide/nodes/src/org/openide/nodes/AsynchChildren.java,v
 <--  AsynchChildren.java
new revision: 1.2; previous revision: 1.1
done
Checking in src/org/openide/nodes/SynchChildren.java;
/shared/data/ccvs/repository/openide/nodes/src/org/openide/nodes/SynchChildren.java,v
 <--  SynchChildren.java
new revision: 1.2; previous revision: 1.1
done
Checking in src/org/openide/nodes/Children.java;
/shared/data/ccvs/repository/openide/nodes/src/org/openide/nodes/Children.java,v
 <--  Children.java
new revision: 1.22; previous revision: 1.21
done
Checking in test/unit/src/org/openide/nodes/ChildFactoryTest.java;
/shared/data/ccvs/repository/openide/nodes/test/unit/src/org/openide/nodes/ChildFactoryTest.java,v
 <--  ChildFactoryTest.java
new revision: 1.2; previous revision: 1.1
done
Checking in nbproject/project.properties;
/shared/data/ccvs/repository/openide/nodes/nbproject/project.properties,v  <-- 
project.properties
new revision: 1.12; previous revision: 1.11
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