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.
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.
Created attachment 36931 [details] Patch adding AsynchChildren to the Nodes API
<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).
Created attachment 36933 [details] Wait.gif missing from patch
Created attachment 36934 [details] Revised patch
Created attachment 36935 [details] Revised patch which also handles batching of key creation
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.
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.
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.
> 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.
> static Children create(Provider/*renamed*/ p); // create synchronous children I'm fine with that, but doesn't it make Children.Keys superfluous?
Any time frame for integration?
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.
Dear selectively blind Tim, I see Y01, Y02, Y03, Y04, Y05. Please lower your selective blindness and notice them as objections as well.
Created attachment 39732 [details] Revised patch
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?
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?
> 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?
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.
Created attachment 39745 [details] Yet another version of the patch
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.
Reassigning to myself.
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
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