Vojtech Szocs has posted comments on this change.
Change subject: core: i18n welcome page with branding
......................................................................
Patch Set 6: Looks good to me, but someone else must approve
(3 inline comments)
Just some minor comments, otherwise the patch looks good.
IIUC each branding theme (package) can provide its own template that will be
part of resulting HTML. The only downside I see is a branding theme unable to
control entire HTML content, since the entire HTML is a composition of
individual templates.
Second approach is to implement template overriding, i.e. template from
branding theme #2 completely overrides template from branding theme #1.
Maybe we could go with a combination of both approaches -> support current
behavior (composition) but also support new option in branding.properties to
control whether theme's template will completely override current HTML, or
whether it will just get appended to current HTML.
....................................................
File backend/manager/modules/root/src/main/webapp/WEB-INF/ovirt-engine.jsp
Line 33: <div class="welcome"><fmt:message key="welcome.text" /></div>
Line 34: <div class="welcome">
Line 35: <script type="text/JavaScript">
Line 36: <!--
Line 37: document.write('<fmt:message key="version"><fmt:param
value="myVersion" /> </fmt:message>')
Maybe I'm missing something, but how does "myVersion" work here? (why do we
need to declare fmt:param here?)
Line 38: //-->
Line 39: </script>
Line 40: </div>
Line 41:
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/branding/BrandingManager.java
Line 247: while (keyMatcher.find()) {
Line 248: String key = keyMatcher.group(1);
Line 249: replacedTemplate = replacedTemplate.replaceAll("\\{"
+ key + "\\}", messageMap.get(key));
Line 250: }
Line 251: replacedTemplate = replacedTemplate.replaceAll("\\{0\\}",
locale.toString());
In README.branding "WELCOME PAGE TEMPLATE" section, you could mention that
"{0}" translates to current user locale.
You can also consider replacing (quite generic) "{0}" with something more
sensible, i.e. "{userLocale}".
Line 252: templateBuilder.append(replacedTemplate);
Line 253: }
Line 254: return templateBuilder.toString();
Line 255: }
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/branding/BrandingTheme.java
Line 252: * If a line starts with '#' it is considered a comment and will
not end up in the output.
Line 253: * @param fileName The name of the file to read.
Line 254: * @return The contents of the file as a string.
Line 255: */
Line 256: private String readWelcomeTemplateFile(final String fileName) {
Maybe we can use Apache Commons IO [1] to simplify this method, for example:
StringBuilder templateBuilder = new StringBuilder();
File file = new File(fileName);
for (String currentLine : FileUtils.readLines(file, "UTF-8")) {
if (!currentLine.startsWith("#")) { // # is comment.
templateBuilder.append(currentLine).append("\n");
}
}
return templateBuilder.toString();
[1] http://commons.apache.org/proper/commons-io/description.html
I would also recommend that this method just "throws IOException" because it
deals with file system access.
Callers of this method, i.e. code in getWelcomePageSectionTemplate() method,
would just try { ... } catch (Exception e) { ... } in order to deal with both
checked (like IOException) and unchecked (like NullPointerException) exceptions.
Line 257: StringBuilder templateBuilder = new StringBuilder();
Line 258: BufferedReader bufferedReader = null;
Line 259:
Line 260: try {
--
To view, visit http://gerrit.ovirt.org/16359
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9987ed58c2e0ead9b25c5f46fb974a96bfd46d30
Gerrit-PatchSet: 6
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: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
