Awesome, looking good and not a lot of code so far :)

With the filtering Stian has identified some usability issues plus the fact it doesn't account for system nodes. So in the end you shouldn't have to filter out the visible and system nodes like you are doing. There is a PortalRequest.userFilter() method that should handle this.

So it would look something like:
Node node = request.getNavigation().getNode(request.getNodePath(), Nodes.visitChildren());
for (Node child : node.filter(request.userFilter())) ....

However we're looking to see how we can make it easier, especially in regards to discovering it.

On 12/21/2012 09:25 AM, Julien Viet wrote:
I spent some time to write a navigation and a breadcrumb portlets using the new API.

Before doing any formal feedback, I want you to review the portlets and tell me because I may not have used it the right way, specially with the filtering of nodes in the nav portlet.

I pushed the two portlets here: https://github.com/vietj/gatein-apps

And here is a screenshot of what you should see :-)


On Dec 12, 2012, at 10:07 PM, Nick Scavelli <nscavell@redhat.com> wrote:


On 12/12/2012 05:28 AM, Julien Viet wrote:
TBH…

The API feel looks quite good.

There are some odd things but we discussed about it already:

- some common classes -> util like Describable etc…
Yes I don't like the package organization thus far. However I don't think Describable belongs in a util package. Maybe should just be in root package org.gatein.api ?
- what do you think of replacing org.gatein.api.portal.Group and org.gatein.api.portal.User by java.security.Principal ?
Haven't really thought about the user and group stuff yet. I have a feeling this will evolve, but I wanted to keep this pretty simple.
- is the interface org.gatein.portal.api.Formatted really necessary with respect to the java.util.Formattable interface ?
I had this at one point but changed. I think I was limited somewhat in the output but I even question the need for this "formatted" id stuff anyways.
- I would renamed Visibility.Flag to Visibility.Status
Sure.

I would organize differently the exception to make it simpler : one single exception ApiException but that uses error code enum instead of subtypes:

public enum ErrorCode {
   PAGE_NOT_FOUND, etc…
}
Not sure if we should only have one exception but I agree having these different NotFound exceptions are too much. EntityNotFoundException (which we have) should be enough. I think the enum solution seems a little but too custom (non-standard) for a public API. If we were to go down an error code route (which I think opens up a bag of worms) then possibly.



On Dec 11, 2012, at 1:24 AM, Nick Scavelli <nscavell@redhat.com> wrote:

All,

I wanted to send an email to discuss the public API and more specifically the current efforts around Navigation. I created a forum post here https://community.jboss.org/thread/215521 to show the information a little easier; however, since I think discussions are easier to follow in an email, I encourage people to reply to this email rather then the forum for further discussion.

Javadoc published here: http://gatein.github.com/gatein-api-java/ (I had problems with Chrome cache, so clear cache if you get 404 errors)

Since the navigation part of the public API is the part we've been focusing on, it's going to be the most complete in terms of design, javadoc, etc. So I am expecting more feedback in this area as apposed to things like pages and sites.

- Nick





_______________________________________________
gatein-dev mailing list
gatein-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/gatein-dev