What Simon proposed makes a lot of sense to me.
On Wed, Jul 2, 2008 at 1:18 PM, [EMAIL PROTECTED] <[EMAIL PROTECTED]> wrote: > Andrew Robinson schrieb: >>> >>> You must have had a real use case that pushed you to write this >>> component. >>> Can you please describe it? >>> >> >> The same as all usages of <c:choose />. The index based or more than >> one are just added benefits I threw in. I can provide examples, but I >> shouldn't have to. > > I certainly think all new components should have to provide proper > use-cases. Having very rarely used components in Tomahawk: > * makes it hard for users to find what they want (steeper learning curve) > * increases the maintenance burden > * increases the jarfile size > > So components should only go in if they are useful to a reasonable number of > people. > >> Just because someone can't think of when it would >> be needed, doesn't mean it never would be useful, but I can appease >> you curiosity. > > It's not curiosity. There is a vast amount of crap in Tomahawk right now, to > the point where Tomahawk is close to dying. There hasn't been a release for > a year. The number of open bugs is vast. So everyone *should* be watching > carefully to see that we don't increase the problems. > > That doesn't mean that good components cannot be added. But new stuff does > need to be evaluated for usefulness. > > And the author of a component is often too close to the code to see whether > it can be improved (that applies equally to me). Having other people look > critically at the purpose and API is very useful. You are free to point out > any issues with components I write (eg Orchestra stuff). > >> I created the component so that people would stop using >> c:choose and c:if in JSF pages and complain that they don't work in >> tables and such. >> >> 1) default c:choose functionality (render the first match): >> >> <s:limitRendered> >> <h:outputText value="#{person.first} #{person.last}" >> rendered="#{prefs.firstThenLast}" /> >> <h:outputText value="#{person.last}, #{person.first}" >> rendered="#{prefs.firstThenLast}" /> >> </s:limitRendered> >> > > Yep, this is a good use case. As you demonstrate later in your email, > writing mutually-exclusive rendered expressions for a set of components can > get nasty. > > I'm not a JSTL user, so your reference to c:choose wasn't perhaps as clear > to me as it might be to others. I think this way: > > if (cond1) render component 1 > else if (cond2) render component 2 > else if (cond3) render component 3 > else render component 4 > > Expanding the javadoc for the component a bit would be good, mentioning that > it simplifies rendered expressions for mutually-exclusive components. The > current docs don't mention that the implicit condition associated with the > "choose clauses" is the rendered expression; it makes sense once I know what > the component is doing but wasn't obvious at first. > >> 2) render index based. This behaves much like the tr:switcher >> component. But instead of using facets and facet names, it uses >> >> <s:limitRendered value="#{wizard.currentStep}" type="index"> >> <h:panelGroup> >> <h:outputText value="This is wizard step 1" /> >> </h:panelGroup> >> <h:panelGroup> >> <h:outputText value="This is wizard step 2" /> >> </h:panelGroup> >> <h:panelGroup> >> <h:outputText value="This is wizard step 3" /> >> </h:panelGroup> >> </s:limitRendered> >> > > I'm not so sure about this. The tr:switcher makes sense to me; it chooses a > component to render by name which will not be easily broken by page changes, > and where the link between what the backing bean EL expression returns and > what facet is selected is clear (the name matches). > > Selecting by index has a far smaller set of use-cases I think. And it can > promote code fragility; coupling an index returned by the backing bean with > an array defined in the page has potential for trouble. But the wizard > use-case is an example of a valid use of this functionality. > >> 3) render multiple children. Can be used much like "-v" vs "-vvvv" can >> be used for command line verbosity >> >> <s:limitRendered value="#{verbosity}"> >> <h:outputText value="#{title}" /> >> <h:outputText value="#{usage}" /> >> <h:outputText value="#{description}" /> >> </s:limitRendered> >> > > Equivalent to this: > <h:outputText value="#{title}" rendered="#{verbosity >=1}"/> > <h:outputText value="#{usage}" rendered="#{verbosity >=2}"/> > <h:outputText value="#{description}" rendered="#{verbosity >=3}"/> > > Yes, the limitRendered approach is a little more efficient; only one EL > expression evaluated rather than 3. But any JSF developer understands the > latter, while no-one can understand the limitRendered approach without > looking up the docs first. And a pretty rare use case I would think. Worth > including perhaps if it didn't have any other negatives, but I think it > does: it forces the name of the component to be generic and the > documentation to be complex. > >> Now I cannot tell you all the reasons they may be useful, but I can >> say that many time in Trinidad authors chose to only provide >> functionality that they themselves could think of, making the >> component useless for every use case they could not think of. Perhaps >> I cannot think of great reasons to render more than one child at the >> moment, but who is to say no one will ever want that? >> > > Making functionality more generic can be good. But it also increases the > complexity of the learning curve. And it can force code to use generic > unhelpful names because there is no simple name that summarizes the > functionality. > > If just the "render only first rendered child" functionality is provided, > then the component could be called "t:choose" or "t:chooseOne" Wouldn't that > be easier for people to understand than "t:limitRendered"? > > If the "render child selected by index" is considered important to include, > then t:chooseOne could also be the name. It suggests similarity with > "c:choose", but with a little extra. > >> The main use case is to stop this code: >> >> <h:outputText value="a" rendered="#{value1 eq 'a'}" /> >> <h:outputText value="b" rendered="#{value1 ne 'a' and value2 eq 'b'}" /> >> <h:outputText value="c" rendered="#{value1 ne 'a' and value2 ne 'b' >> and value3 eq 'c'}" /> >> <h:outputText value="otherwise" rendered="#{value1 ne 'a' and value2 >> ne 'b' and value3 ne 'c'}" /> >> etc. >> >> Several times I have had the use case where I only want to render a >> component if the previous one was not rendered. To get that >> functionality, I always had to negate the test of the rendered of the >> previous components then include the test for the current component. >> >> This is much easier to write and definitely to maintain: >> >> <s:limitRendered> >> <h:outputText value="a" rendered="#{value1 eq 'a'}" /> >> <h:outputText value="b" rendered="#{value2 eq 'b'}" /> >> <h:outputText value="c" rendered="#{value3 eq 'c'}" /> >> <h:outputText value="otherwise" /> >> </s:limitRendered> >> >> So for the most part, every use case in JSP to use <c:choose /> is a >> use case to use limitRendered in JSF. As mentioned, the index based or >> multiple support was just added functionality for rare use cases. >> > > Yep, agreed. > > My suggestion would be to promote this component, but in modified form: > * ditch the ability to select the first N > * ditch the ability to select a set of arbitrary elements by index (1,4,6) > * rename to t:chooseOne > * In normal case, choose "first rendered child" > * If an "index" attribute is defined, select child by index: > <t:chooseOne index="#{elexpr}"> > > Why? > * chooseOne is clear to understand, both for people used to JSTL and XSLT, > and others. > * the selecting of the first N elements is only very rarely useful, and the > selecting of arbitrary elements by index is both rare and actively dangerous > as it promotes fragile code. But it makes the helpful name "chooseOne" > impossible. Removing this functionality also simplifies the component > implementation. > > Regards, > Simon > >