jsr372-experts@javaserverfaces-spec-public.java.net

[jsr372-experts] Re: [review] Tiny issue in search expression API class

From: Edward Burns <edward.burns_at_oracle.com>
Date: Tue, 31 Jan 2017 12:26:38 -0800

>>>>> On Sun, 29 Jan 2017 16:50:17 +0100, arjan tijms <arjan.tijms_at_gmail.com> said:

AT> I'm reviewing the search expression API classes a little. First of all, I'm
AT> *very* impressed by the quality of the work, this is really well done!

>>>>> On Sun, 29 Jan 2017 21:56:12 -0500, Leonardo Uribe <leonardo.uribe_at_irian.at> said:

LU> Yes, it took a lot of time and a lot of iterations to get it done.

Yes, that's what I thought. That argued in favor of accepting it even
though it was uncomfortably late.

AT> I did found a tiny tiny issue in the API class
AT> javax.faces.component.search.ComponentNotFoundException though. It's almost
AT> too small to mention, but for completeness: it doesn't declare a static
AT> final serialVersionUID field.

Fixed that, thanks.

AT> Another small issue that I noticed is in the Javadoc of:

AT> javax.faces.application.Application.addSearchKeywordResolver(SearchKeywordResolver)

AT> It currently says:

AT> /**
AT> * <p class="changed_added_2_3">Cause an the argument
AT> <code>resolver</code> to be added to the resolver chain.
AT> *
AT> * It is not possible to remove an {_at_link SearchKeywordResolver}
AT> * registered with this method, once it has been registered.</p>
AT> *
AT> * <p>The default implementation throws
AT> * <code>UnsupportedOperationException</code> and is provided
AT> * for the sole purpose of not breaking existing applications that
AT> extend
AT> * {_at_link Application}.</p>
AT> *
AT> * @throws IllegalStateException if called after the first
AT> * request to the {_at_link javax.faces.webapp.FacesServlet} has been
AT> serviced.
AT> * @throws NullPointerException when resolver is null.
AT> *
AT> * @param resolver the SearchKeywordResolver to add.
AT> * @since 2.3
AT> */

AT> There's a small typo -> "Cause an the ..."

AT> But more importantly, it doesn't say if the new SearchKeywordResolver is
AT> added in front of the chain or at the end.

AT> In the implementation it's very explicitly added to the front, so it can
AT> override existing resolvers. Perhaps a small clarification here would help?

LU> New SearchKeywordResolver instances must be put on top. The idea is if the
LU> developer wants to override an existing keyword, it should be able to do it
LU> just adding it to the chain, just like with EL resolvers.

Fixed that.

Note that I pushed it to the new MOJARRA_2_3X_ROLLING branch and used
git cherry-pick to get it on master. Manfred will have more to say
about branches shortly.

Ed

-- 
| edward.burns_at_oracle.com | office: +1 407 458 0017
| 25 business days until planned start of JSF 2.3 Final Approval Ballot
| 15 business days until DevNexus 2017
| 40 business days until JavaLand 2017