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
>
>
