Marius,
Thanks for your comments. My replies are inline.
> 1. I don't think you should rely on log4j. None
> of the other taglibs do and it is an unnecessary
> dependency. You can use:
> pageContext.getServletContext().log(...)
>
> It may not be as versatile as log4j, but it is more
> than enough for this taglib.
The main problem with the servlet context logging is not that its not as
versatile as log4j, it's that it's not versatile at all! :-) You can't
decide whether or not you want to log requested but unavailable locales, or
how to handle beyond a simple log in a file the (IMHO critical) situation
where a language has not defined a key. For example, we set ours up to log
AND send email to the person in charge of translations.
There was some discussion about this a few weeks ago before my original
post... we already have dependencies on other apache components, e.g. xalan,
xerces, servlet.jar, oro, etc. so at that time nobody really came out
strongly against it, so I left it in.
That said, there are really only those two places where logging
configuration is needed (the other two logs are debug logs that can be
removed) so in the interests of not forcing people to use something they
don't want, I'll go ahead and make the switch to logging through the context
-- but with a specified prefix -- so we can implement something to watch the
log file for us and send the email when we see logs with the "key not found"
prefix.
It will be nice when JSR-047 is finalized and we have a standard
java.logging package - hopefully it will be based on log4j, or at least
provide most of its functionality. :-)
> 2. There should be other ways to set the locale.
> Relying on the browser settings should be only one
> of them.
>
> In many applications the user explicitly
> chooses a language/locale and this setting is
> stored in the session. You should be able to
> set the locale based on such a setting.
Fair enough. I'll add an session attribute to store the locale in. The page
which allows selecting the language/locale would just not use the
<i18n:localize/> tag -- or else it would read the browser's session. :-)
> 3. Both LocaleHelper and ResourceHelper are
> derived from BodyTagSupport. This dependency
> is not needed and these classes are not
> tags.
good catch. copy/paste error on my part when I was splitting those functions
out of the localize and message tags.
> 4. The bundle name can be either set explicitly
> or it will be retrieved from an attribute/environment.
>
> May be the attribute and the environment names
> should be set explicitly as well instead of using
> hard coded values.
> This will make the JSP page more readable as well.
umm... maybe I didn't understand the first half. how would allowing changing
the attribute/environments make jsp pages more readable? those values
wouldn't be seen inside the jsp pages. one would be seen inside a
load-on-startup servlet, and the other would be seen inside the deployment
descriptor.
I'll make changes 1,2, and 3, and wait to hear back from you on #4.
Tim
