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.
This API review covers one component from http://www.netbeans.org/issues/show_bug.cgi?id=72091 ...and that is the ability to insert nodes into a pre-existing project UI, next to those provided by the project's LogicalViewProvider. The idea is that a client module could have a layer entry like: <folder name="Projects"> <folder name="Nodes"> <folder name="Web"> <file name="com-foo-MyNodeFactory.instance"/> </folder> <folder name="J2SE"> <file name="com-bar-MyNodeFactory.instance"/> </folder> <folder name="Freeform"> <file name="com-baz-MyNodeFactory.instance"/> </folder> </folder> </folder> The NodeFactory interface would be: public interface NodeFactory { Node[] createNodes(Project p); Node findPath(Project p, Node root, Object target); } Attached is a patch which implements this behavior. This is a first pass at getting this working, and I welcome feedback. The patch includes support for web/project, java/j2seproject, and ant/freeform, which I thought was a pretty good sampling. My focus on this implementation was to make the code change in those modules as small as possible. This led to one ugliness in the API itself, which is the DefaultLogicalViewProvider.ChildrenDelegate interface. Conceivably, if all the children of the project root nodes were refactored into NodeFactories, this could go away. But, that would be a major rewrite.
Created attachment 28804 [details] contains a diff file and 2 new files (NodeFactory.java and DefaultLogicalViewProvider.java)
Wouldn't it be better to use the module name as the name of the folder which contains the NodeFactory instance? This would prevent name clashes and make it easier to find them.
I have not tried this out yet, but what I would like to have are: 1) The ability to describe an order between nodes. 2) The ability to exclude nodes from the LogicalView. For 1): I simply want a way to specify the layout;-) For 2): Suppose in a `derived' WebProject, I create a more specialized Node (-Tree) for some part of the LogicalView. Now I would end up with both, the Node from the WebProject and my own specialized Node:-( Does this make sense? Thanks, -- Marco
Marco-- Thanks for commenting! 1. The ordering of nodes would be controlled by the ordering of the NodeFactory objects in the sysFs. So, this would be done the same way that, say, menu items are ordered. One caveat though: this only applies to nodes declared through the API. Nodes from the original project type come in their original order, after any API-declared nodes. The original nodes could be refactored into NodeFactories, but this patch does not do that yet, because I wanted to limit my changes in this initial pass. 2. Nodes could be excluded using the "_hidden" technique common to all layer file-based APIs. I wouldn't imagine that an IDE plugin would do such a thing, but certainly an RCP app might. Again, you could not hide nodes from the original project type until they are refactored into NodeFactories.
Rich, the ultimate problem with ordering is the NodeFactory's method creteNodes() and specifically the return type. Once you return an array of node it's pretty hard to do any ordering and insert other items inbetween. Which ultimately leads to a situation where Node[] arrays returned from the method contain just one Node instance and thus each node has it's own factory. I think we want to avoid that. I can see a solution to the Node/NodeFactory coupling. Have layer registration for NodeFactories as you suggest, additionally have a similar structure for individual nodes. Rather than these to be .instances, these would be simple tokens. These would be than passed to the NodeFactory's createNode(Project p, String token). The first NodeFactory that recognizes the token returns an instance and we're done. Hiding of Nodes can be then done using the _hidden layer technique or by registering your Nodefactory to be the first one and return some "empty" value (that's rather dynamic approach) the con is possibly multiple node factories will be asked for the node.
Suggested token system seems weird to me, and I don't know of any precedent for it in our APIs. What's the problem with a NodeFactory returning one Node, exactly? I would expect one factory to produce all source roots, another for all test roots, another for Libraries, etc.
Jesse, nothing wrong with it, I actually wanted to suggest that but the 1:1 ratio between the Node and NodeFactories seemed a bit over the top to me. Isn't that a bit too many classes? On the other hand we could reduce the actual number of Factories by having a parametrized layer instances there.. (implementation detail I guess).
One NodeFactory can produce zero or more Node's, right? So it is not a 1:1 relationship of Node to NodeFactory. Anyway as you noted you can use static factory methods to avoid an excessive number of classes, though I think this issue would not actually come up in practice.
Jesse, not sure we understand each other. I do express my concerns about the "more" part in the sentence "One NodeFactory can produce zero or more Node's".
What I disagree with (well, this is all just guessing because we have not seen any concrete impls yet to judge) is your evaluation "Once you return an array of node it's pretty hard to do any ordering and insert other items inbetween. Which ultimately leads to a situation where Node[] arrays returned from the method contain just one Node instance and thus each node has it's own factory." I think rather there will (or should) be one NodeFactory for each distinct top-level node or node grouping. If a factory produces >1 node, that is probably because it is actually dynamically checking some info in the project and producing some number of nodes accordingly - e.g. one node per source group. In such a case I see no problem; another module might want to insert some nodes before the source groups, or after them, but inserting in the middle does not make much sense. Compare the existing situation with DynamicContextMenu, which I think you worked on - an "action" in a popup menu can produce zero or more physical menu items. We still permit actions to be configured and ordered via layer in many situations. There are no complaints that you cannot insert something into the middle of two JMenuItem's produced by one action. Right? On another note, the actual signature Node[] createNodes(Project p); is not usable because it does not permit the factory to change the list of nodes on the fly. Possible solution: return Children. More cumbersome for the case of just one nodes, but better for a dynamic node list since you can use Children.Keys. (Yes this is getting into Looks territory.) Could make a simplified version of C.K for this purpose, e.g.: NodeList<?> createNodes(Project p); interface NodeList<T> { List<T> keys(); void addChangeListener(ChangeListener l); // change in keys() void removeChangeListener(ChangeListener l); Node/*List<Node>?*/ node(T key); } // ... public static NodeList<?> fixed(Node... nodes); Another comment: in the context of findPath we may want to finally do issue #7551 properly, which would give us a unified API we could use for the Projects tab and other stuff. I am thinking something like the following, to be optionally put into a Node's lookup: interface NodeFinder { /** first return should be "best", might be others, empty if not found */ Iterator<Node> find(Node root, Object target); } /** fallback impl - checks in own Lookup, else asks children in turn */ public static NodeFinder simpleFinder(); with DataNode and FolderNode having impls to search for FileObject/DataObject in the natural way, and various project nodes likewise.
My implementations all create a single node. I generalized the signature just because I saw no reason not to. I agree with Jesse that any factory which created a group of nodes is going to want to group those nodes together anyway (e.g. SourceGroups), and one wouldn't want to insert nodes in between. Jesse, I see your point about returning Children instead of a Node array or list of nodes, but I worry about making the API too obtuse. Why can't the logical view that's reading this stuff be resposible for maintaining the dynamic nature of the nodes? It seems to me that the logical view will be refreshing its keys when necessary, and can re-invoke createNodes() at that point.
"I see your point about returning Children instead of a Node array or list of nodes, but I worry about making the API too obtuse." - that's why I suggested the utility method, so you could just write e.g. public NodeList<?> createNodes(Project p) { return Something.fixed(new MyNode(p)); } "Why can't the logical view that's reading this stuff be resposible for maintaining the dynamic nature of the nodes? It seems to me that the logical view will be refreshing its keys when necessary, and can re-invoke createNodes() at that point." - the key point is "when necessary". The logical view cannot know when a node factory needs to refresh its nodes, since it has no idea where the node factory is getting these nodes from. E.g. if you have a factory adding one node to a web app project per registered server, you need to fire a change event in the NodeList whenever some servers are installed. But the web app project type module probably isn't going to know to refresh nodes in this particular circumstance.
Ok, I give up. The menu analogy is not 100% correct. In the project nodes I assume some "primary owner" which is the project type itself. You convinced me that it's probably correct if that one puts some constraints on where the additional providers can place their nodes and which sections are not to be separated. +1 on the NodeList return type. This definitely needs to be dynamic and only the NodeFactories know when it's time to update.
On the LogicalViewProvider implementor side it would be nice to have some support class/method, something along the lines of: public class MyLogicalViewProvider { public Node createLogicalView() { return new MyProjectRootNode(SOMETHING.createProjectCompositeChildren("Projects/Nodes/Web")); } I've looked at the DefaultLogicalViewProvider in attachment and idea of taking the created project node from a delegate provider and replacing it's children. The above mentioned snippet seems simpler to me.
Agreed, simple factory methods that do just the difficult part are to be preferred to base classes.
*** Issue 77766 has been marked as a duplicate of this issue. ***
It would be very nice to see this integrated; as long as it's impossible to do this, we're going to continue having very non-optimal UIs for things that significantly augment, enhance or decorate a project. A prime example is UML support in the enterprise pack - the choice to implement a UML model of a project as a separate project synchronized with the Java project is the right design choice; but exposing it to the user as a separate project is exposing an implementation detail - it's quite bizarre for one project to be, in practice, two projects. So UI-wise it leads to really weird stuff. Or consider the original packager module, or the JNLP module, or the html projects module - all of these are by far best implemented as separate projects, but from the user's point of view, they are new aspects of the java project they're working on - and that's what they should look like in the UI. We really should solve this ASAP, as we will continue to have either less maintainable implementations (i.e. the JNLP module as rewritten not to be a project type) or less usable UIs as long as this is an issue. This should be fixed before even more code is written and committed to that contains assorted workarounds for the inability to do this sort of thing, as it will be more work to maintain and fix such code later.
ok, let me try to rework the initial api and incorporate the comments expressed. Once done I will submit for api review again.
Created attachment 33311 [details] api changes to projectsuiapi project
Created attachment 33312 [details] reimplementation of j2se project using the new apis.
Created attachment 33313 [details] projectuiapi update javadoc
I've reimplemented the NodeFactory apis, taking the comments in this issue into account. please review I removed the node finding method from NodeFactory and the implementation of LogicalViewProvider in j2se project uses the old method of finding subnodes. It works fine for existing usecases. I agree with jesse that we need a more generic APIs for finding nodes. When done it should not influence this api in any way.
should be a fast review, adding the keyword
ok, thanks for the review, I will integrate into trunk.
closing as fixed.is integrated into trunk.