Alexander Wels has posted comments on this change. Change subject: engine,userportal,webadmin: configurable unsupported locales ......................................................................
Patch Set 8: (9 comments) http://gerrit.ovirt.org/#/c/27973/8/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/servlet/UnsupportedLocaleHelper.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/servlet/UnsupportedLocaleHelper.java: Line 21: */ Line 22: public static List<String> getDisplayedLocales(List<String> allLocales, Line 23: List<String> unsupportedLocalesFilterOverrides, Line 24: List<String> unsupportedLocalesFilter) { Line 25: List<String> result = allLocales; > Small thing - I think it's better to do: I think we are okay with it, but doing an extra copy to be safer works as well, I will make the change. Line 26: //Override unsupported locales that we do want to display. Line 27: unsupportedLocalesFilter.removeAll(unsupportedLocalesFilterOverrides); Line 28: //Remove remaining unsupported locales from the result. Line 29: result.removeAll(unsupportedLocalesFilter); Line 23: List<String> unsupportedLocalesFilterOverrides, Line 24: List<String> unsupportedLocalesFilter) { Line 25: List<String> result = allLocales; Line 26: //Override unsupported locales that we do want to display. Line 27: unsupportedLocalesFilter.removeAll(unsupportedLocalesFilterOverrides); > Same question as above. Done Line 28: //Remove remaining unsupported locales from the result. Line 29: result.removeAll(unsupportedLocalesFilter); Line 30: Collections.sort(result); Line 31: return result; Line 33: Line 34: public static List<String> getLocalesKeys(ConfigValues configValues) { Line 35: if (!configValues.equals(ConfigValues.UnsupportedLocalesFilter) Line 36: && !configValues.equals(ConfigValues.UnsupportedLocalesFilterOverrides)) { Line 37: throw new IllegalArgumentException("Passed in config value not related to locales"); //$NON-NLS-1$ > Alternative approach is to make this method private and introduce two extra Done Line 38: } Line 39: List<String> locales = Config.<List<String>> getValue(configValues); Line 40: List<String> result = new ArrayList<String>(); Line 41: if (locales != null && !locales.isEmpty()) { Line 45: String underScoredLocaleKey = localeKey.replaceAll("-", "_"); Line 46: org.apache.commons.lang.LocaleUtils.toLocale(underScoredLocaleKey); Line 47: result.add(underScoredLocaleKey); Line 48: } catch (IllegalArgumentException iae) { Line 49: //The locale passed in is not valid, don't add it to the list. > Maybe we could log a warning here? Done Line 50: } Line 51: } Line 52: } Line 53: return result; http://gerrit.ovirt.org/#/c/27973/8/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/GwtDynamicHostPageServlet.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/GwtDynamicHostPageServlet.java: Line 195: * </ol> Line 196: * The result will be all locales - (unsupported locale filter - unsupported locale filter overrides). Line 197: * @return A {@code String} containing the Unsupported locale filter information in JSON format. Line 198: */ Line 199: private String getUnsupportedLocaleFilterInformation() { > Hm, why do we need to pass both UnsupportedLocalesFilter + UnsupportedLocal You make a good point, we really should just pass the list of ones we want to show, with the calculation happening on server side. I will fix. Line 200: ObjectNode node = mapper.createObjectNode(); Line 201: StringBuilder localeBuilder = new StringBuilder(); Line 202: localeBuilder.append(' '); Line 203: for (String localeString: UnsupportedLocaleHelper.getLocalesKeys( Line 197: * @return A {@code String} containing the Unsupported locale filter information in JSON format. Line 198: */ Line 199: private String getUnsupportedLocaleFilterInformation() { Line 200: ObjectNode node = mapper.createObjectNode(); Line 201: StringBuilder localeBuilder = new StringBuilder(); > Joining collection elements via StringBuilder is quite messy. Done Line 202: localeBuilder.append(' '); Line 203: for (String localeString: UnsupportedLocaleHelper.getLocalesKeys( Line 204: ConfigValues.UnsupportedLocalesFilterOverrides)) { Line 205: localeBuilder.append(localeString); http://gerrit.ovirt.org/#/c/27973/8/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/utils/UnsupportedLocalesFilterInfoData.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/utils/UnsupportedLocalesFilterInfoData.java: Line 52: * (display them even though they're unsupported). Cannot be null Line 53: * @param unsupportedLocalesFilter The list of unsupported locales. Cannot be null Line 54: * @return An array of locales that has been filtered. Line 55: */ Line 56: public static String[] getFilteredLocaleNames(List<String> allLocaleNames, > This method is basically the same thing as UnsupportedLocaleHelper.getDispl Done Line 57: List<String> unsupportedLocalesFilterOverrides, Line 58: List<String> unsupportedLocalesFilter) { Line 59: List<String> result = new ArrayList<String>(); Line 60: result.addAll(allLocaleNames); http://gerrit.ovirt.org/#/c/27973/8/frontend/webadmin/modules/userportal-gwtp/pom.xml File frontend/webadmin/modules/userportal-gwtp/pom.xml: Line 211: <version>0.2.5</version> Line 212: <configuration> Line 213: <property> Line 214: <name>gwt.availableLocales</name> Line 215: <value>{{ def bd=project.basedir; new File(bd + 'backend/manager/modules/utils/src/main/resources/languages.properties').withInputStream { def prop = new Properties();prop.load(it);prop.keySet().sort().join(',')} }}</value> > Nice, I assume this is Groovy :-) Yes this is groovy, very javascriptish isn't it. I will see if I can move the string into a variable like that. Line 216: </property> Line 217: </configuration> Line 218: </plugin> Line 219: </plugins> http://gerrit.ovirt.org/#/c/27973/8/frontend/webadmin/modules/webadmin/pom.xml File frontend/webadmin/modules/webadmin/pom.xml: Line 161: <version>0.2.5</version> Line 162: <configuration> Line 163: <property> Line 164: <name>gwt.availableLocales</name> Line 165: <value>{{ def bd=project.basedir; new File(bd + 'backend/manager/modules/utils/src/main/resources/languages.properties').withInputStream { def prop = new Properties();prop.load(it);prop.keySet().sort().join(',')} }}</value> > See my comment in userportal-gwtp/pom.xml. See the comment at the top of this section, I can't move it to a parent as the dynamic variable doesn't seem to be there if I do. But I can move the path into a variable. Line 166: </property> Line 167: </configuration> Line 168: </plugin> Line 169: <plugin> -- To view, visit http://gerrit.ovirt.org/27973 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5b816694d6c34549360b189eb85c840688957bdb Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Einav Cohen <[email protected]> Gerrit-Reviewer: Greg Sheremeta <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
