myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Blake Sullivan <blake.sulli...@oracle.com>
Subject Re: [Trinidad][Skinning]Support @locale rules (TRINIDAD-1041)
Date Tue, 29 Dec 2009 01:11:01 GMT
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 <jeanne.waldman@oracle.com 
> > <mailto:jeanne.waldman@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
> >
> >
>   


Mime
View raw message