Also, this code in SkinCSSDocumentHandler.java:
_locales = locales != null ? new HashSet<Locale>(locales)
: new HashSet<Locale>();
If we aren't going to modify the HashSet(), use Collections.emptySet().
When JIRA-1547 is applied, CollectionUtils will ahve a method that
optimizes this case and the single element cases.
SkinStyleSheetNode--it doesn't make sense to have a getLocales() that
converts the Set to an array just so that StyleSheetNode can convert the
array back into a set--we should chanage StyleSheetNode to take a
Set<Locale>.
-- Blake Sullivan
Jeanne Waldman said the following On 12/28/2009 3:50 PM PT:
Here is a code review that is based on formatting only. The code itself looks
fine, but I need to review it more thoroughly.
Also, you'll need to document this feature in the skinning.xml file.
*SkinCSSDocumentHandler:*
1. Keep the imports:
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Set;
This is convention. We don't use * in our imports
2. Check indentation, throughout the code you added, the indentation doesn't
match the rest of the file.
} else if (atRule.startsWith(_AT_LOCALE))
{
_parseCustomAtRule(_AT_LOCALE, atRule);
}
3. Check indentation:
else if (_AT_LOCALE.equals(type))
_locales = null;
else if (_AT_LOCALE.equals(type)) {
_locales = new HashSet<Locale>();
for (int i = 0; i < typeArray.length; i++) {
Locale locale = LocaleUtils
.getLocaleForIANAString(typeArray[i].replace('_',
'-').trim());
_locales.add(locale);
}
}
4. brackets '{' '}' need to be on their own line to fit the style we use in all
the files
5. add parens around (locales != null)
_locales = locales != null ? new HashSet<Locale>(locales)
: new HashSet<Locale>();
6. indentation
public Set<Locale> getLocales()
{
return _locales;
}
*SkinStyleSheetNode.java*
1. java.util imports, do not use *
2. indentation and brackets on their own line:
boolean localeMatch = _setsEqual(locales, _locales);
if (localeMatch) {
boolean accMatch = _setsEqual(accProperties, _accProperties);
if (accMatch)
return true;
}
*
SkinStyleSheetParserUtils.java*
1. remove the comment that says locales are not supported.
Marius Petoi wrote, On 12/9/2009 11:45 PM PT:
> Hi Jeanne,
>
> Everything that you mentioned here is done, so I think it will be ok. Please
> have a look over the code and tell me whether it is fine and then I shall send
> another email with the voting proposal for the public API.
>
> Marius
>
> On Thu, Dec 10, 2009 at 5:50 AM, Jeanne Waldman <[email protected]
> <mailto:[email protected]>> wrote:
>
> I'll take a look. We should get approval from the community for @locale,
> because this is a public API.
> Could you send out another email with the subject set so people know they
> should vote on the public API? Or you can wait until I review this first.
>
> +1 from me for @locale.
>
> In XSS we had this syntax:
> <styleSheet locales="en">
> <styleSheet browsers="ie">
>
> We converted the <styleSheet browsers..> syntax to @agent ie or @agent ie,
> gecko, so I think we should convert the locales to @locale with commas
> separating the locales. We should support the locales the same way we do
> for xss, so the code change should be pretty simple, since we already have
> the support for xss to follow.
> The locale should be included for the hashcode in the css filename
> (stylesheetdocumentid)
>
> Jeanne
>
> Marius Petoi wrote, On 12/9/2009 4:57 AM PT:
>
> Hello,
>
> I have added a patch that implements the support for skin selectors
> depending on the locale
> (https://issues.apache.org/jira/browse/TRINIDAD-1041). This is done in
> the CSS using @locale rules. The syntax of the @locale rules is
> similar to the syntax of the @agent and @platform rules:
>
> @locale en-us, fr
> {
> /* skin selectors definitions go here */
> }
>
> The set of supported locales is afterwards stored in each
> StyleSheetNode. I also implemented an example in the trinidad-demo.
> There is a skin (localeDemo.css) which contains @locale rules.
> Afterwards, the localeDemo.jspx page displays a textInput with a
> different color, depending on the locale. In order to configure the
> proper skin, a "LocaleDemoBean" is used, which is used for configuring
> the proper skin-family (localeDemo), in the same way in which the
> accessibility profile is configured for accessibilityProfileDemo.jspx.
>
> Could you please have a look over the patch and see if this resolves
> the issue.
>
> Regards,
> Marius
>
>
