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.

Bug 206556 - Asynch ChildFactory has poor support for incremental display
Summary: Asynch ChildFactory has poor support for incremental display
Status: RESOLVED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: Nodes (show other bugs)
Version: 7.1
Hardware: All All
: P3 normal (vote)
Assignee: Jesse Glick
URL:
Keywords: API, API_REVIEW_FAST
Depends on: 206958
Blocks:
  Show dependency tree
 
Reported: 2011-12-19 20:51 UTC by Jesse Glick
Modified: 2012-02-25 20:14 UTC (History)
2 users (show)

See Also:
Issue Type: ENHANCEMENT
Exception Reporter:


Attachments
Does not work as you might want - pauses for 10s (1.27 KB, text/x-java)
2011-12-19 20:51 UTC, Jesse Glick
Details
Workaround - manually translated to CPS (1.32 KB, text/x-java)
2011-12-19 20:52 UTC, Jesse Glick
Details
Proposed patch (16.32 KB, patch)
2011-12-19 21:40 UTC, Jesse Glick
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Glick 2011-12-19 20:51:33 UTC
Created attachment 114324 [details]
Does not work as you might want - pauses for 10s

In an asynch ChildFactory, createNodeForKey is supposed to be fast, and createKeys often just runs through one loop (according to an underlying model) and then returns true. The problem with this style is that the explorer just shows a "Please wait..." node until the list is fully populated, which can take a long time.

It also supports returning false to "create more keys", but this does not work very well. In order to add more keys on the end of the list, you need to know what position in the list you are in when createKeys is called. This means you need to manually convert a natural loop into continuation-passing style.

But it is even worse than that, because you cannot just store the current index (say) in a field in the factory - AsynchChildren.run might halt the computation in response to an interrupt and restart it later, without giving any indication to the factory that this has happened. The only way to be sure you are continuing at the right place is to ensure that each loop iteration produces a predictable number of keys (one, even if createNodeForKey will return null later) and count the size of the existing list; or otherwise inspect the last element in the existing list.

This gets very awkward, and can only work if the setup for the loop is very fast and creating each key on a loop iterator is the only significant job. f19dc2ad222e shows how this can be done (for a Maven POM project's submodule list); the final patch looks short but getting there involved a lot of trial and error. bdf59cfe5ce5 is an example where it is necessary to create keys which will never be displayed, just so that the list size matches the iterator index, in some cases even requiring a new key type just to hold this information (FileKey). 5ebd3e1ba751 is an example that is probably wrong (will not handle interruption and restart correctly), though it is hard to know how to test this.
Comment 1 Jesse Glick 2011-12-19 20:52:07 UTC
Created attachment 114325 [details]
Workaround - manually translated to CPS
Comment 2 Jesse Glick 2011-12-19 21:03:02 UTC
Another minor issue with the known workaround is that the Please wait... node disappears after the first key is produced, which leaves no indication other than the wait cursor that more keys are coming.
Comment 3 Jesse Glick 2011-12-19 21:40:47 UTC
Created attachment 114327 [details]
Proposed patch
Comment 4 Jesse Glick 2011-12-19 21:41:11 UTC
Please review.
Comment 5 Vladimir Voskresensky 2011-12-20 05:59:11 UTC
decreasing version number looks a little bit strange, although I understand that module itself was changed 7.26->7.27, how it worked with 7.9 (7.5 in other changed place) at all?
 <run-dependency>
-    <specification-version>7.9</specification-version>
+    <specification-version>7.27</specification-version>
 </run-dependency>
Comment 6 Jaroslav Tulach 2011-12-20 08:59:52 UTC
Y01 Stop class could be STOP instance, I think.

Y02 What about addAll? Should not it be atomic operation?

Y03 Write a bunch of tests. Otherwise I don't want such code in module that I maintain.
Comment 7 Jesse Glick 2011-12-20 15:49:32 UTC
(In reply to comment #5)
> decreasing version number looks a little bit strange

Sorry, I have no idea what you meant by this comment. What is decreasing? openide.nodes is going from 7.26 to 7.27, and all modules using ChildFactory and now taking advantage of the new behavior are updating their dependency accordingly.


Y01 - OK.


Y02 - you mean, should addAll immediately display the newly added keys? It could, I just did not bother since known usages would be calling add(E) on one key at a time.

(Typically you would use addAll if you had a bunch of keys ready-made from some part of the model, in which case createKeys would be ending after this call anyway, and you would not care about this RFE. In principle some factory might somehow slowly generate a collection of keys, addAll, then slowly generate another batch, and so on; seems pretty unlikely but could be implemented easily enough if you care.)

Currently with the patch addAll will work as before - the keys will be appended to the list but the display will not change until you return from createKeys.


Y03 - will try.
Comment 8 Jesse Glick 2011-12-20 22:57:04 UTC
Also need to handle nulls being temporarily added to the list (even if subsequently removed):

java.lang.NullPointerException
	at org.openide.nodes.Children$Keys.setKeys(Children.java:1487)
	at org.openide.nodes.AsynchChildren$1.add(AsynchChildren.java:187)
	at org.netbeans.modules.maven.nodes.ProjectFilesNode$ProjectFilesChildren.createKeys(ProjectFilesNode.java:178)
	at org.openide.nodes.AsynchChildren.run(AsynchChildren.java:204)
Comment 9 Vladimir Voskresensky 2011-12-24 15:21:58 UTC
(In reply to comment #7)
> (In reply to comment #5)
> > decreasing version number looks a little bit strange
> 
> Sorry, I have no idea what you meant by this comment. What is decreasing?
> openide.nodes is going from 7.26 to 7.27, and all modules using ChildFactory
> and now taking advantage of the new behavior are updating their dependency
> accordingly.
Sorry, it was my fault (I misinterpret numbers) and your change is correct.
Comment 10 Jesse Glick 2012-01-03 16:59:31 UTC
(In reply to comment #6)
> Y01 Stop class could be STOP instance, I think.

It cannot, because I need to catch it by class name.
Comment 11 Jesse Glick 2012-01-03 17:53:21 UTC
core-main #625b915ff8ab
Comment 12 Quality Engineering 2012-01-04 15:42:47 UTC
Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/625b915ff8ab
User: Jesse Glick <jglick@netbeans.org>
Log: #206556: asynch ChildFactory has poor support for incremental display.