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.