[jira] Commented: (ADFFACES-15) Added XMLMenuModel

["Adam Winer (JIRA)" <ad...@incubator.apache.org>]

    [ http://issues.apache.org/jira/browse/ADFFACES-15?page=comments#action_12414203 ] 

Adam Winer commented on ADFFACES-15:
------------------------------------

Code review on the patch:

General:
- All code must have an Apache license, not an Oracle license;  patches
  with non-Apache licenses cannot be accepted.
- Code does not follow ADF Faces coding conventions:
     -opening braces on new lines
     -two-space indentation
     -private methods start with underscores
     - Import all classes (no java.io.Serializable, java.net.URL, etc. in code)
- Never call ex.printStackTrace().  _LOG.severe() is sufficient.
- Implementation details (like describing the set of hashmaps created
  to implement a menu model) should be left out of the Javadoc,
  and moved into "//"-style comments inside the class.
- Do not access any Servlet APIs when ExternalContext is sufficient
  (as it is in all usages herein)

XmlMenuModel:
- Comments like "our Xdk" aren't appropriate for Apache codebases
- Do not use code from sun.* (sun.misc.Service)
- What is the use of createNotVisible(), putCachedMenuList(), getCachedMenuList()?
- Don't call Map.get() and put() with Object.hashCode().  Just use the object directly.
- _rootModelMap is not a threadsafe useage, since it will be shared across all requests
   (and potentially multiple webapps).  I'm not sure what its purpose is in general;  there's
   likely a simpler way to achieve what you're going after (maybe a ThreadLocal?)
- For MenuContentHandler, please give "s1", "s2", "s3" more descriptive names.

CombinationMenuModel:
- Is this class useful on its own?  If not, maybe it could be merged into XMLMenuModel,
  or at least made package-private?
- Most of the public methods do not appear to need to be public, and could be private,
  package-private, or protected (use the most restrictive that works).

MenuContentHandlerImpl:
- _handlerId can be more simply (and reliably) computed with System.identityHashCode().
- loadBundle() is only going to get called once, with the Locale in place for the request
  when this file is parsed, so I don't think this will properly be retranslated on each request.
- BundleMap.get() shouldn't return "MISSING:", it should return "!!!" + key + "!!!" to follow the
  JSF 1.2 spec.

MenuUtils:
- Should be package-private

MenuNode:
- Rename to getIcon(), setIcon();  if the tree bug is still present, please file an issue.
- joinLabelAndAcessKey(), splitLabelAndAcessKey() are mispelled (AccessKey)

Could you address these issues and post a new patch?

> Added XMLMenuModel
> ------------------
>
>          Key: ADFFACES-15
>          URL: http://issues.apache.org/jira/browse/ADFFACES-15
>      Project: MyFaces ADF-Faces
>         Type: New Feature

>  Environment: software platform
>     Reporter: Gary Kind
>  Attachments: trunk.patch
>
> Additions to MyFaces ADF-Faces to allow easy creation of menus (navigation and tabbed navigation) for .jspx pages using ADF-Faces UI components.   Menus are specified in XML Metadata files.  In the .jspx page menu items are af:commandNavigationItems nodestamped inside of an af:page component.  Menu Model  bean that processes the metadata files, along with the navigation-rules/cases are specified in the faces-config.xml file.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira