You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@myfaces.apache.org by Marius Petoi <ma...@codebeat.ro> on 2009/12/09 13:57:28 UTC
[Trinidad][Skinning]Support @locale rules (TRINIDAD-1041)
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
Re: [Trinidad][Skinning]Support @locale rules (TRINIDAD-1041)
Posted by Matthias Wessendorf <ma...@apache.org>.
On Thu, Dec 10, 2009 at 8:45 AM, Marius Petoi <ma...@codebeat.ro> wrote:
> 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.
I also think that is email (to dev@) is right to ping the community.
In a little while,
we need a vote, yes that's correct as well
-Matthias
>
> Marius
>
> On Thu, Dec 10, 2009 at 5:50 AM, Jeanne Waldman <je...@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
>
>
--
Matthias Wessendorf
blog: http://matthiaswessendorf.wordpress.com/
sessions: http://www.slideshare.net/mwessendorf
twitter: http://twitter.com/mwessendorf
Re: [Trinidad][Skinning]Support @locale rules (TRINIDAD-1041)
Posted by Jeanne Waldman <je...@oracle.com>.
I copied all these comments into the JIRA issue and added a couple more.
Marius, let me know when you have the new patch up.
Thanks!
Jeane
Blake Sullivan wrote, On 12/28/2009 5:11 PM PT:
> 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 > <ma...@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
>> >
>> >
>>
>
>
Re: [Trinidad][Skinning]Support @locale rules (TRINIDAD-1041)
Posted by Blake Sullivan <bl...@oracle.com>.
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
> > <ma...@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
> >
> >
>
Re: [Trinidad][Skinning]Support @locale rules (TRINIDAD-1041)
Posted by Marius Petoi <ma...@codebeat.ro>.
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
<je...@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
>>
>
Re: [Trinidad][Skinning]Support @locale rules (TRINIDAD-1041)
Posted by Jeanne Waldman <je...@oracle.com>.
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