[jira] Commented: (TRINIDAD-1041) Support locale-specific styles

["Jeanne Waldman (JIRA)" <de...@myfaces.apache.org>]

    [ https://issues.apache.org/jira/browse/TRINIDAD-1041?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12795121#action_12795121 ] 

Jeanne Waldman commented on TRINIDAD-1041:
------------------------------------------

code review:

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

from Blake:
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>. 

--- more comments ---
_parseCustomAtRule -> change the comments from @agent since there are @platform and @locale since this comment was written.

Consider adding your locale demo to the current Skin demo instead of creating a new one.

add to the demo a test for multiple locales to show how you do this.

   * @param type type of the at rule. _AT_AGENT, _AT_PLATFORM, or _AT_ACC_PROFILE 
add _AT_LOCALE - in general sweep through your changes and make sure the comments are all up to date


> Support locale-specific styles
> ------------------------------
>
>                 Key: TRINIDAD-1041
>                 URL: https://issues.apache.org/jira/browse/TRINIDAD-1041
>             Project: MyFaces Trinidad
>          Issue Type: New Feature
>          Components: Skinning
>    Affects Versions: 1.0.6-core, 1.2.7-core
>            Reporter: Andy Schwartz
>            Assignee: Jeanne Waldman
>            Priority: Minor
>         Attachments: trinidad-examples-patch.patch, trinidad-impl-patch.patch
>
>
> Under the covers Trinidad still uses the legacy "XSS" style definition mechanism (eg. see base-desktop.xss).  It would be nice to finally port  these XSS files over to CSS, since the CSS is a far more familiar language.  However, before we can do that, we need to add a few remaining XSS features which our not present in our CSS skinning implementation.
> One feature that we support in XSS but not in CSS is the ability to define locale-specific styles.  In XSS, this is done via the "locales" attribute on the <styleSheet> element.
> Logging this issue to request that we add similar support to CSS skinning, with the goal of being able to convert our XSS files over to CSS.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.