Hi Ken,
Thanks for your comments.
Ken Paulsen wrote:
> You can send your reviews to admin_at_glassfish.dev.java.net (I've cc'd
> them in my reply). Anissa at a minimum should also see the changes
> you are submitting.
Will do.
>
> Here are my comments / questions:
>
> 1) Around line #670 in clusterProfileTree.jsf did you intend to add
> this line:
>
> + rendered="#{showLoadBalancer}"
>
>
> Similarly in clusterGeneral.jsf around line #114 and clusterTable.jsf
> line #149 did you mean to add the showLoadBalancer reference?
>
> - <sun:property id="lb" labelAlign="left"
> noWrap="#{true}" overlapLabel="#{false}"
> label="$resource{i18n.inst.lbLabel}" >
> + <sun:property id="lb" labelAlign="left"
> noWrap="#{true}" overlapLabel="#{false}"
> + label="$resource{i18n.inst.lbLabel}"
> rendered="#{showLoadBalancer}">
>
> and:
>
> + rowHeader="$boolean{true}" id="lb"
> rendered="#{showLoadBalancer}">
>
>
> After looking at the code more closely I see you are adding this
> session variable in our code (w/ value=true) and in your code (w/
> value=false). Perhaps this is OK... however, I want to make sure
> Anissa and others in the admin console are aware of this change so
> they don't wonder why it's there. Anissa, perhaps you have other
> ideas on how this should work?
Yes thats exactly what I am doing. The HTTP Load balancer is replaced by
the Converged Load Balancer in Sailfin and hence it was a requirement
that the HTTP load balancer be hidden in the GUI. I achieve this by the
session variable showLoadBalancer.
>
> 2) In MBeanTreeAdaptor.java you set both the parent node and the
> children node's "rendered" property using the same value
> (getOption("rendered")). I do not think this is necessary. If the
> parent node is not rendered, nothing under it will be rendered. Also,
> the pattern we use in this file is to use "childRenderer" for child
> properties... not to re-use the parent properties. So I recommend
> either removing the child "rendered" property entirely, or to add a
> separate rendered property for children called "childRendered".
I will remove the rendered property for the child nodes.
>
> Also, in this file (and probably others), I noticed that your
> indenting is incorrect (as shown by your own diff). This appears to
> be because you have your editor set to interpret a TAB as 4 spaces --
> a TAB should always be 8 spaces. An "indent" (sometimes called a "tab
> stop" or "shift width" or other names) can be 4 spaces (or whatever
> you want). See if you can fix your editor to correctly indent
> spaces. Also, fixing this will make reading our code easier as you'll
> see the indentation correctly (I'm sure it looks awful in your editor
> right now! :) ). Here's an example where your diffs show the wrong
> spacing:
>
> setProperty(props, "actionListener",
> desc.getOption("actionListener"));
> setProperty(props, "expanded", desc.getOption("expanded"));
> + setProperty(props, "rendered", desc.getOption("rendered"));
I will do this and thanks for this tip.
--
Irfan Ahmed
9180-66927726, Sun Microsystems Inc., Bangalore, India.