Greg Sheremeta has posted comments on this change. Change subject: engine,userportal,webadmin: enhance locale selection ......................................................................
Patch Set 7: (9 comments) http://gerrit.ovirt.org/#/c/27973/7//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-05-20 13:10:34 -0400 Line 4: Commit: Alexander Wels <aw...@redhat.com> Line 5: CommitDate: 2014-05-28 13:17:15 -0400 Line 6: Line 7: engine,userportal,webadmin: enhance locale selection suggest change to: made supported and displayed locales configurable Line 8: Line 9: - Added UnsupportedDisplayedLocales configuration variable to Line 10: control which unsupported locales are displayed. Any locales Line 11: in this list which are also in the UnsupportedLocaleFilter list Line 4: Commit: Alexander Wels <aw...@redhat.com> Line 5: CommitDate: 2014-05-28 13:17:15 -0400 Line 6: Line 7: engine,userportal,webadmin: enhance locale selection Line 8: please add some plain English that describes what got enhanced. suggest: 1. made the list of locales supported be configurable 2. added the ability to override the list of displayed locales -- that is, display specific locales even though they're unsupported PS -- why would you want to display locales even though they're unsupported? it's unclear to me after reviewing the patch. thanks! Line 9: - Added UnsupportedDisplayedLocales configuration variable to Line 10: control which unsupported locales are displayed. Any locales Line 11: in this list which are also in the UnsupportedLocaleFilter list Line 12: will still be displayed. http://gerrit.ovirt.org/#/c/27973/7/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 15: * @param allLocales List of all available locale. Cannot be null. Line 16: * @param unsupportedDisplayLocales The list of unsupported locales to display anyway. Cannot be null Line 17: * @param unsupportedLocalesFilter The list of unsupported locales. Cannot be null Line 18: * @return A {@code List} containing the intersection of the two lists. Line 19: */ This is a little hard to understand. Suggest overhauling this comment like so: /** * Determine the list of locales to display by subtracting unsupported * locales from the list of all locales, but then adding back in locales * we want to display anyway (locale overrides). * * Elements in the lists should be of format xx_YY, which is the standard Java Locale format. * @param allLocales List of all available locales. Cannot be null. * @param localeOverrides The list of unsupported locale * overrides (display them even though they're unsupported). Cannot be null * @param unsupportedLocales The list of unsupported locales. Cannot be null * @return The {@code List} of locales to display */ Line 20: public static List<String> getDisplayedLocales(List<String> allLocales, List<String> unsupportedDisplayLocales, Line 21: List<String> unsupportedLocalesFilter) { Line 22: List<String> result = allLocales; Line 23: //Remove any unsupported we do want to display. Line 16: * @param unsupportedDisplayLocales The list of unsupported locales to display anyway. Cannot be null Line 17: * @param unsupportedLocalesFilter The list of unsupported locales. Cannot be null Line 18: * @return A {@code List} containing the intersection of the two lists. Line 19: */ Line 20: public static List<String> getDisplayedLocales(List<String> allLocales, List<String> unsupportedDisplayLocales, suggest rename unsupportedDisplayLocales -> localeOverrides suggest rename unsupportedLocalesFilter -> unsupportedLocales Line 21: List<String> unsupportedLocalesFilter) { Line 22: List<String> result = allLocales; Line 23: //Remove any unsupported we do want to display. Line 24: unsupportedLocalesFilter.removeAll(unsupportedDisplayLocales); http://gerrit.ovirt.org/#/c/27973/7/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 185: node.put("value", value); //$NON-NLS-1$ Line 186: return node.toString(); Line 187: } Line 188: Line 189: private String getLocaleInformation() { suggest renaming method to something more specific, perhaps 'buildLocalesToDisplayString'. please add method comment. Line 190: ObjectNode node = mapper.createObjectNode(); Line 191: StringBuilder localeBuilder = new StringBuilder(); Line 192: localeBuilder.append(' '); Line 193: for (String localeString: UnsupportedLocaleHelper.getLocalesKeys(ConfigValues.UnsupportedDisplayedLocales)) { http://gerrit.ovirt.org/#/c/27973/7/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/utils/LocaleInfoData.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/utils/LocaleInfoData.java: Line 14: public static native LocaleInfoData instance() /*-{ Line 15: return $wnd.localeInfo; Line 16: }-*/; Line 17: Line 18: private native String getUnsupportedDisplayLocaleString() /*-{ suggest matching these up with my suggestions in UnsupportedLocaleHelper.java Line 19: return this.UnsupportedDisplayedLocales; Line 20: }-*/; Line 21: Line 22: private native String getUnsupportedLocalesFilterString() /*-{ Line 41: } Line 42: return result; Line 43: } Line 44: Line 45: /** suggest matching the comment and parameter names with my suggestions in UnsupportedLocaleHelper.java Line 46: * Determine the list of locales to display based on the result of subtracting all unsupported locales from Line 47: * the list of all locales. The list of unsupported locales can be changed by the list of locales to display Line 48: * anyway. Elements in the lists should be of format xx_YY, which is the standard Java Locale format. Line 49: * @param allLocaleNames List of all available locale. Cannot be null. http://gerrit.ovirt.org/#/c/27973/7/frontend/webadmin/modules/pom.xml File frontend/webadmin/modules/pom.xml: Line 203: <profiles> Line 204: <profile> Line 205: <id>all-langs</id> Line 206: <properties> Line 207: <gwt.locale>${gwt.availableLocales}</gwt.locale> +1 :) Line 208: </properties> Line 209: </profile> Line 210: <profile> Line 211: <id>gwtdraft</id> http://gerrit.ovirt.org/#/c/27973/7/packaging/etc/engine-config/engine-config.properties File packaging/etc/engine-config/engine-config.properties: Line 394: PMHealthCheckEnabled.type=Boolean Line 395: PMHealthCheckEnabled.description="Enable/Disable Power Management Health Check feature." Line 396: PMHealthCheckIntervalInSec.type=Integer Line 397: PMHealthCheckIntervalInSec.description="The interval in which the Power Management Health Check is running." Line 398: UnsupportedDisplayedLocales.description=A comma separated list of locale keys that you want to display, this allows you to display some unsupported locales. suggest change to: A comma separated list of locale keys to display even though they're unsupported -- 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: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@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: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
