Re: [Trinidad][Skinning]Support @locale rules (TRINIDAD-1041)

[Jeanne Waldman]

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 <jeanne.wald...@oracle.com> 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