Bug 72441 - API for inserting nodes into project ui
API for inserting nodes into project ui
Status: RESOLVED FIXED
Product: projects
Classification: Unclassified
Component: Generic Infrastructure
5.x
All All
: P1 (vote)
: 6.x
Assigned To: Milos Kleint
issues@projects
: API, API_REVIEW_FAST
: 77766 (view as bug list)
Depends on:
Blocks: 72091 93470
  Show dependency treegraph
 
Reported: 2006-02-09 01:31 UTC by Rich Unger
Modified: 2007-01-30 17:49 UTC (History)
7 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
contains a diff file and 2 new files (NodeFactory.java and DefaultLogicalViewProvider.java) (6.46 KB, application/x-compressed)
2006-02-09 01:51 UTC, Rich Unger
Details
api changes to projectsuiapi project (20.96 KB, patch)
2006-08-28 09:22 UTC, Milos Kleint
Details | Diff
reimplementation of j2se project using the new apis. (50.89 KB, patch)
2006-08-28 09:24 UTC, Milos Kleint
Details | Diff
projectuiapi update javadoc (187.25 KB, application/octet-stream)
2006-08-28 09:41 UTC, Milos Kleint
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Rich Unger 2006-02-09 01:31:35 UTC
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.
Comment 1 Rich Unger 2006-02-09 01:51:48 UTC
Created attachment 28804 [details]
contains a diff file and 2 new files (NodeFactory.java and DefaultLogicalViewProvider.java)
Comment 2 misterm 2006-02-09 12:00:21 UTC
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.
Comment 3 Marco Walther 2006-02-15 22:06:23 UTC
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 
 
Comment 4 Rich Unger 2006-02-15 22:33:45 UTC
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.

Comment 5 Milos Kleint 2006-03-20 13:00:37 UTC
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.
Comment 6 Jesse Glick 2006-03-20 16:22:49 UTC
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.
Comment 7 Milos Kleint 2006-03-20 20:07:23 UTC
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).
Comment 8 Jesse Glick 2006-03-21 00:38:31 UTC
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.
Comment 9 Milos Kleint 2006-03-21 07:34:04 UTC
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".
Comment 10 Jesse Glick 2006-03-21 21:48:31 UTC
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.
Comment 11 Rich Unger 2006-03-21 21:59:27 UTC
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.

Comment 12 Jesse Glick 2006-03-21 22:13:12 UTC
"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.
Comment 13 Milos Kleint 2006-03-22 10:28:12 UTC
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.
Comment 14 Milos Kleint 2006-03-24 10:26:18 UTC
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.
Comment 15 Jesse Glick 2006-03-24 14:27:07 UTC
Agreed, simple factory methods that do just the difficult part are to be
preferred to base classes.
Comment 16 Jesse Glick 2006-06-13 17:16:26 UTC
*** Issue 77766 has been marked as a duplicate of this issue. ***
Comment 17 _ tboudreau 2006-08-04 21:44:23 UTC
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.
Comment 18 Milos Kleint 2006-08-24 08:46:03 UTC
ok, let me try to rework the initial api and incorporate the comments expressed.
Once done I will submit for api review again.

Comment 19 Milos Kleint 2006-08-28 09:22:33 UTC
Created attachment 33311 [details]
api changes to projectsuiapi project
Comment 20 Milos Kleint 2006-08-28 09:24:37 UTC
Created attachment 33312 [details]
reimplementation of j2se project using the new apis.
Comment 21 Milos Kleint 2006-08-28 09:41:21 UTC
Created attachment 33313 [details]
projectuiapi update javadoc
Comment 22 Milos Kleint 2006-08-28 09:48:56 UTC
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.



Comment 23 Milos Kleint 2006-08-31 07:53:47 UTC
should be a fast review, adding the keyword
Comment 24 Milos Kleint 2006-09-18 10:26:27 UTC
ok, thanks for the review, I will integrate into trunk.
Comment 25 Milos Kleint 2006-10-05 12:19:29 UTC
closing as fixed.is integrated into trunk.


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