Vojtech Szocs has posted comments on this change.

Change subject: userportal,webadmin: Fix behavior unsupported locale
......................................................................


Patch Set 4: (6 inline comments)

Sorry for posting comments on older patch set, some of them might be resolved 
already, please take a look.

....................................................
File 
backend/manager/modules/root/src/main/java/org/ovirt/engine/core/LocaleFilter.java
Line 64:      * @return The determined {@code Locale}
Line 65:      */
Line 66:     private Locale determineLocale(final HttpServletRequest request) {
Line 67:         // Step 1.
Line 68:         Locale locale = 
LocaleUtils.getLocaleFromString(request.getParameter(LOCALE));
Please ensure that LocaleUtils.getLocaleFromString(String,boolean) uses 
LocaleFilter.DEFAULT_LOCALE instead of Locale.US directly.
Line 69:         // Step 2.
Line 70:         if (locale == null) { // No locale parameter.
Line 71:             locale = getLocaleFromCookies(request.getCookies());
Line 72:         }


Line 75:             locale = request.getLocale();
Line 76:         }
Line 77:         // Step 4.
Line 78:         if (locale == null) { // No accept headers.
Line 79:             locale = Locale.US;
Please use LocaleFilter.DEFAULT_LOCALE instead of Locale.US directly.
Line 80:         }
Line 81:         Locale resolvedLocale = lookupSupportedLocale(locale, 
getLocaleKeys());
Line 82:         log.debug(String.format("Incoming locale: %s. Filter 
determined locale to be: %s",
Line 83:                 locale.toLanguageTag(), 
resolvedLocale.toLanguageTag()));


Line 106:      * Get a list of available locales.
Line 107:      * @return A {@code List} of Locale strings.
Line 108:      */
Line 109:     public static List<String> getLocaleKeys() {
Line 110:         ResourceBundle bundle = 
ResourceBundle.getBundle(LocaleFilter.class.getPackage().getName() + LANGUAGES,
I'd prefer using Properties instead of ResourceBundle, since 
languages.properties file isn't meant to be localized anyway.
Line 111:                 Locale.US);
Line 112:         List<String> keys = Collections.list(bundle.getKeys());
Line 113:         // Can't just return the unsorted list, it won't match the 
GWT output.
Line 114:         // Apparently GWT does a sort before showing the list as 
well, which


Line 124:      * @return The supported locale based on the passed in 
filteredLocale and list of supported locales.
Line 125:      */
Line 126:     private Locale lookupSupportedLocale(final Locale filteredLocale, 
final List<String> supportedLocales) {
Line 127:         for (String localeString: supportedLocales) {
Line 128:             Locale locale = 
LocaleUtils.getLocaleFromString(localeString);
Small thing: I'd prefer renaming this variable to "supportedLocale"
Line 129:             @SuppressWarnings("unchecked")
Line 130:             List<Locale> localeLookupList = 
org.apache.commons.lang.LocaleUtils.localeLookupList(locale);
Line 131:             for (Locale lookupLocale: localeLookupList) {
Line 132:                 if (lookupLocale.equals(filteredLocale)) {


Line 126:     private Locale lookupSupportedLocale(final Locale filteredLocale, 
final List<String> supportedLocales) {
Line 127:         for (String localeString: supportedLocales) {
Line 128:             Locale locale = 
LocaleUtils.getLocaleFromString(localeString);
Line 129:             @SuppressWarnings("unchecked")
Line 130:             List<Locale> localeLookupList = 
org.apache.commons.lang.LocaleUtils.localeLookupList(locale);
+1 using commons LocaleUtils.localeLookupList
Line 131:             for (Locale lookupLocale: localeLookupList) {
Line 132:                 if (lookupLocale.equals(filteredLocale)) {
Line 133:                     // Matched on one of the locales. Find the 
supported one in the list.
Line 134:                     return locale;


Line 135:                 }
Line 136:             }
Line 137:         }
Line 138:         // TODO: When branding patch is merged update me to say 
LocaleFilter.defaultLocale
Line 139:         return Locale.US;
Please use LocaleFilter.DEFAULT_LOCALE instead of Locale.US directly.
Line 140:     }
Line 141: 
Line 142:     @Override
Line 143:     public void init(final FilterConfig filterConfig) throws 
ServletException {


-- 
To view, visit http://gerrit.ovirt.org/15520
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5beceaf7f4ce2333e83cd72c3fa8721103cf805
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to