Vojtech Szocs has posted comments on this change.
Change subject: userportal, webadmin: branding support.
......................................................................
Patch Set 19: (11 inline comments)
Thanks for addressing all of my comments, Alex.
There are still some open comments, please go over them and let me know what
you think.
> Guys, we must start to realize the the project is more than single
> technology, it is integration of multiple components and technologies.
> Writing that 'files are not in place' is just like writing that some java
> class, CSS or html are not in place.
Maybe it's just me, I need to switch my mind-set to accept the fact that
compiled GWT application isn't self-sufficient, i.e. might depend on external
resources. So I get your point, Alon.
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/branding/BrandingManager.java
Line 132: ObjectNode node = new ObjectMapper().createObjectNode();
Line 133: for (Map.Entry<String, String> entry : keyValues.entrySet()) {
Line 134: node.put(entry.getKey(), entry.getValue());
Line 135: }
Line 136: return node.size() > 0 ? node.toString() : null;
Thanks, I'm OK with current approach, assuming client-side code (dynamic
constants) always checks for presence of message object (not null).
Line 137: }
Line 138:
Line 139: /**
Line 140: * Get the root path of the branding files.
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/branding/BrandingTheme.java
Line 113: * @param brandingRootPath The root of the path to the branding
theme,
Line 114: * @param brandingVersion The version to load, if the version
don't match the load will fail.
Line 115: * @return {@code true} if successfully loaded, {@code false}
otherwise.
Line 116: */
Line 117: public boolean load(final String brandingPath, final File
brandingRootPath, final int brandingVersion) {
I still don't understand why it's better to have arguments in load() vs. final
fields set up inside constructor.
BrandingTheme class is meant to load branding resources like this:
- instantiate
- call load() method
- if successful, class contains loaded resources, store its reference somewhere
Calling load() is therefore one-time operation, load() isn't supposed to be
called more than once on given instance with different parameters. From class
perspective, this makes brandingPath, brandingRootPath and brandingVersion
conceptually immutable parameters. Also, from design perspective, It's always
good to reduce class mutability (state that can be modified) if possible.
(Currently, load() acts both as setter for these parameters + logic for actual
load operation.)
To summarize:
- conceptually immutable parameters should be implemented as final fields
- conceptually mutable parameters should be implemented as method parameters
Long story short: I suggest to use final fields instead of method parameters in
this particular case.
Line 118: path = brandingPath.substring(
Line 119: brandingRootPath.getAbsolutePath().
Line 120: length());
Line 121: filePath = brandingPath;
Line 124: try {
Line 125: propertiesFile = new FileInputStream(brandingPath
Line 126: + "/" + BRANDING_PROPERTIES_NAME); //$NON-NLS-1$
Line 127: brandingProperties.load(propertiesFile);
Line 128: available = brandingVersion == getVersion();
Well, I'd rather prefer following implementation:
- init brandingProperties via Properties.load
- modify getVersion to be private and accept Properties argument
Also, some questions:
- are there any callers for getVersion outside this class?
- are there any callers for isAvailable outside this class?
(I recommend having getters only when they are needed)
Line 129: if (!available) {
Line 130: log.warn("Unable to load branding theme, mismatched
version: " //$NON-NLS-1$
Line 131: + getVersion() + " wanted version: " +
brandingVersion); //$NON-NLS-1$
Line 132: }
Line 229: new URL[] {
Line 230: themeDirectory.toURI().toURL() });
Line 231: result = ResourceBundle.getBundle(
Line 232: brandingProperties.getProperty(MESSAGES_KEY).
Line 233: replaceAll("\\.properties", ""),
//$NON-NLS-1$ //$NON-NLS-2$
> What is the problem with replaceAll?
Example - branding.properties file contains:
messages=my.messages.properties.whatever.properties
After doing replaceAll, you would end up using "my.messages.whatever" instead
of "my.messages.properties.whatever" -> you should remove ".properties" suffix
only.
So instead of replaceAll, you should use String.lastIndexOf and do substring to
cut out the part without suffix.
Line 234: locale,
Line 235: urlLoader);
Line 236: } catch (IOException e) {
Line 237: // Unable to load messages resource bundle.
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/BrandingServlet.java
Line 104: * @return A full absolute path for the passed in path.
Line 105: */
Line 106: String getFullPath(final String path) {
Line 107: String result = null;
Line 108: if (path != null && ServletUtils.isSane(path)) {
OK, I see your point. I guess that for the purpose of "isSane" checking, it
doesn't matter if we pass only a portion of the path vs. full (real requested)
path, assuming the branding root path is safe.
Line 109: // Return a result relative to the branding root path.
Line 110: result =
brandingManager.getBrandingRootPath().getAbsolutePath() + path;
Line 111: } else {
Line 112: log.error("The path \"" + path + "\" is not sane");
//$NON-NLS-1$ //$NON-NLS-2$
....................................................
File
frontend/webadmin/modules/frontend/src/main/resources/META-INF/resources/GwtHostPage.jsp
Line 8: <meta name="gwt:property"
content="locale=${requestScope['locale']}">
Line 9: <c:if test="${requestScope['brandingStyle'] != null}">
Line 10: <c:forEach items="${requestScope['brandingStyle']}"
var="theme">
Line 11: <c:if
test="${theme.getThemeStyleSheet(requestScope['applicationType']) != null}">
Line 12: <link rel="stylesheet" type="text/css"
href="${pageContext.request.contextPath}/${requestScope['theme']}${theme.path}/${theme.getThemeStyleSheet(requestScope['applicationType'])}">
I guess output HTML indentation isn't that important in comparison to JSP code
readability, plus in future we might consider trimming JSP whitespace anyway.
Line 13: </c:if>
Line 14: </c:forEach>
Line 15: </c:if>
Line 16: <script type="text/javascript">
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/utils/DynamicConstants.java
Line 34: dictionary = null;
Line 35: try {
Line 36: dictionary =
Dictionary.getDictionary(MESSAGES_DICTIONARY_NAME);
Line 37: } catch (MissingResourceException mre) {
Line 38: // Do nothing, the dictionary doesn't exist.
You're right, in this case its normal for dictionary to be null.
Line 39: }
Line 40: }
Line 41:
Line 42: /**
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/dialog/tab/DialogTab.java
Line 30:
Line 31: interface Style extends CssResource {
Line 32: String obrand_active();
Line 33:
Line 34: String inactive();
OK, but just because downstream branding package doesn't override "inactive"
style, it doesn't mean other branding package wouldn't want to override it.
For me, "active" and "inactive" are conceptually closely related, so it makes
sense to externalize them both for consistency, but for now I'm OK with current
approach.
Line 35: }
Line 36:
Line 37: @UiField
Line 38: FocusPanel tabContainer;
....................................................
File frontend/webadmin/modules/userportal-gwtp/src/main/webapp/WEB-INF/web.xml
Line 1: <web-app xmlns="http://java.sun.com/xml/ns/javaee";
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
Line 2: xsi:schemaLocation="http://java.sun.com/xml/ns/javaee
http://java.sun.com/xml/ns/javaee/web-app_3_0.xsd"; version="3.0">
Line 3:
Line 4: <display-name>oVirt UserPortal UI</display-name>
Line 5: <!-- Filters -->
For Java source code, we should always use 4-space instead of TAB chars.
For XML, I'm not sure, most XMLs contain TAB chars. (engine-code-format.xml for
Eclipse affects only Java sources)
I'd say both 4-space and TABs are allowed for XMLs, so it's up to you.
Line 6: <filter>
Line 7: <filter-name>LocaleFilter</filter-name>
Line 8:
<filter-class>org.ovirt.engine.core.utils.servlet.LocaleFilter</filter-class>
Line 9: </filter>
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/MainSectionView.ui.xml
Line 1
Line 2
Line 3
Line 4
Line 5
OK, then we can also delete defines.css file.
....................................................
File frontend/webadmin/modules/webadmin/src/main/webapp/WEB-INF/web.xml
Line 4: <display-name>oVirt WebAdmin UI</display-name>
Line 5: <!-- Filters -->
Line 6: <filter>
Line 7: <filter-name>LocaleFilter</filter-name>
Line 8:
<filter-class>org.ovirt.engine.core.utils.servlet.LocaleFilter</filter-class>
I forgot about this in my first review, please remove this as this should go
into web-fragment.xml file.
Line 9: </filter>
Line 10: <filter-mapping>
Line 11: <filter-name>LocaleFilter</filter-name>
Line 12: <url-pattern>/webadmin/*</url-pattern>
--
To view, visit http://gerrit.ovirt.org/13181
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4a8a426ce7d688d33c5ae2b70632c836843106b2
Gerrit-PatchSet: 19
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alex Lourie <alou...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Eyal Edri <ee...@redhat.com>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com>
Gerrit-Reviewer: Moran Goldboim <mgold...@redhat.com>
Gerrit-Reviewer: Ofer Schreiber <oschr...@redhat.com>
Gerrit-Reviewer: Sahina Bose <sab...@redhat.com>
Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
