Vojtech Szocs 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: List<String> result = new ArrayList<String>(allLocales); because with original code: List<String> result = allLocales; any modification of "result" will also modify "allLocales" input parameter. In other words, are we OK with modifying method input parameters? (I'd rather avoid this.) 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. 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 public methods, each one accepting UnsupportedLocalesFilter / UnsupportedLocalesFilterOverrides and delegating to the private method. Current approach is OK too, but please consider adding small Javadoc that only UnsupportedLocalesFilter / UnsupportedLocalesFilterOverrides is supported for "configValues" argument. 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? 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 + UnsupportedLocalesFilterOverrides to GWT client? Can't we simplify this to pass only "unsupported locale filter - unsupported locale filter overrides" or similar? 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. Instead, I'd suggest to simply use StringUtils.join from Apache Commons Lang: String joined = StringUtils.join(collection, separator); It should make the code more readable. 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.getDisplayedLocales method. Why do we need to duplicate this method? Can't we just do this kind of processing in GWT host page servlet (utilizing UnsupportedLocaleHelper) and pass resulting info to GWT client? As far as I can see, the client really needs just String[] containing filtered locale names - its computation can be done on server side. 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 :-) It would be nice to extract stuff like "backend/manager/modules/utils/src/main/ resources/languages.properties" as a separate Maven property. Isn't the following possible? <rawProperties> gwt.langFile = backend/manager/modules/utils/src/main/resources/languages.properties </rawProperties> <property> <name>gwt.availableLocales</name> <value>{{ compute value while referencing ${gwt.langFile} }}</value> </property> 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. Also, I guess we shouldn't duplicate same config across two different poms - we can place this into parent pom instead. 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
