Vojtech Szocs has posted comments on this change.
Change subject: userportal, webadmin: branding support.
......................................................................
Patch Set 19: (58 inline comments)
Reviewed patch set 19 so some comments might not be relevant, Alex - let me
know if you have any questions.
So the general strategy is to take default styles away from GWT application and
externalize them as a separate (default) branding package. On one side, this
goes against GWT design philosophy that GWT output should be as optimized and
performant as possible (i.e. inline default styles right into application
code). On the other side, I agree that having a separate (default) branding
package essentially communicates clear interface with branding package authors.
Given above mentioned pros/cons, I guess that in this particular case, having
default styles externalized is OK. However, be advised that this approach might
lead to "jumpy UI" effect when loading styles for the first time (application
code has been loaded, but styles are still loading).
Some general comments:
* please use config/engine-code-format.xml (Java code formatting rules for
Eclipse) and *always try to format new code* (ctrl-shift-F in Eclipse), I see
that you use default (80-char) limit whereas engine-code-format.xml defines
120-char limit
* please avoid "//comment" and use "// comment" according to formatting rules
mentioned above (i.e. format source code properly)
* please use newlines to separate different logical elements to improve code
readability
* using "final" for method arguments has little-to-none performance gain in
modern JVMs (server-side code) and *zero* performance gain in GWT (client-side
code), which makes "final" redundant, except for cases when it's required
according to Java language spec
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/branding/BrandingManager.java
Line 16: import org.codehaus.jackson.node.ObjectNode;
Line 17: import org.ovirt.engine.core.utils.EngineLocalConfig;
Line 18:
Line 19: /**
Line 20: * This class manages the available branding themes.
"... branding themes and localized messages."
Line 21: */
Line 22: public class BrandingManager {
Line 23: /**
Line 24: * The default branding path.
Line 39: * The regex pattern to use to determine if a directory should be
used
Line 40: * as a branding directory. The pattern is '^\d?\d\-.+' So any two
digits
Line 41: * followed by a dash will do.
Line 42: */
Line 43: private static final Pattern DIRECTORY_PATTERN =
Pattern.compile(".+\\.brand"); //$NON-NLS-1$
The pattern differes from Javadoc, why?
Line 44:
Line 45: private static final int CURRENT_BRANDING_VERSION = 1;
Line 46: /**
Line 47: * A list of available {@code BrandingTheme}s.
Line 41: * followed by a dash will do.
Line 42: */
Line 43: private static final Pattern DIRECTORY_PATTERN =
Pattern.compile(".+\\.brand"); //$NON-NLS-1$
Line 44:
Line 45: private static final int CURRENT_BRANDING_VERSION = 1;
Please add Javadoc here, this is important constant!
Additionally, newline after this would improve readability.
Line 46: /**
Line 47: * A list of available {@code BrandingTheme}s.
Line 48: */
Line 49: private List<BrandingTheme> themes = null;
Line 45: private static final int CURRENT_BRANDING_VERSION = 1;
Line 46: /**
Line 47: * A list of available {@code BrandingTheme}s.
Line 48: */
Line 49: private List<BrandingTheme> themes = null;
null assignment is redundant here.
Line 50:
Line 51: /**
Line 52: * The root path of the branding themes.
Line 53: */
Line 73: * @return A {@code List} of {@code BrandingTheme}s.
Line 74: */
Line 75: public List<BrandingTheme> getBrandingThemes() {
Line 76: if (themes == null && brandingRootPath.exists() &&
brandingRootPath.isDirectory()
Line 77: && brandingRootPath.canRead()) {
Why are we reloading branding themes only for the first time, i.e. when "themes
== null"?
Since "themes" is local state specific to BrandingManager class, you'd have to
restart Engine after you alter "brandingRootPath" directory content, is this
really what we want?
Line 78: themes = new ArrayList<BrandingTheme>();
Line 79: List<File> directories = Arrays.asList(
Line 80: brandingRootPath.listFiles(new FileFilter() {
Line 81: @Override
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;
Why is returning null on empty message object important?
In other words, empty JavaScript object can serve as "null object" indicating
there are no custom messages.
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 18: * <li>The path to the theme</li>
Line 19: * <li>The name of the style sheet associated with the theme</li>
Line 20: * </ul>
Line 21: */
Line 22: public class BrandingTheme {
Please use newline (below) to separate inner type members.
Line 23: /**
Line 24: * Enumeration specifying which application type to get a
Line 25: * style sheet for.
Line 26: */
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) {
Assuming load() is meant to be one-time operation, I suggest to remove load()
method parameters, make relevant fields final, and introduce constructor that
sets those fields.
Line 118: path = brandingPath.substring(
Line 119: brandingRootPath.getAbsolutePath().
Line 120: length());
Line 121: filePath = brandingPath;
Line 122: available = false;
Line 123: FileInputStream propertiesFile;
Line 124: try {
Line 125: propertiesFile = new FileInputStream(brandingPath
Line 126: + "/" + BRANDING_PROPERTIES_NAME); //$NON-NLS-1$
String brandingPropertiesFile = brandingPath + "/" + BRANDING_PROPERTIES_NAME;
(Try to avoid code duplication.)
Line 127: brandingProperties.load(propertiesFile);
Line 128: available = brandingVersion == getVersion();
Line 129: if (!available) {
Line 130: log.warn("Unable to load branding theme, mismatched
version: " //$NON-NLS-1$
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();
Design of this method looks strange to me ... first it initializes
"brandingProperties" field and then it calls method (getVersion) that assumes
"brandingProperties" has already been set.
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 134: // Unable to load properties file, disable theme.
Line 135: log.warn("Unable to load properties file for "
//$NON-NLS-1$
Line 136: + "theme located here:"//$NON-NLS-1$
Line 137: + brandingPath + "/" //$NON-NLS-1$
Line 138: + BRANDING_PROPERTIES_NAME);
See my comment above on "brandingPropertiesFile".
Maybe also include "e" (exception) as log.warn method argument?
Line 139: }
Line 140: return available;
Line 141: }
Line 142: /**
Line 150: /**
Line 151: * Getter for the webadmin style sheet name.
Line 152: * @return The {@code String} representing the style sheet.
Line 153: */
Line 154: public final String getWebadminStyleSheetName() {
Why is this method necessary? The caller should have appropriate
ApplicationType enum instance, we can just call getCssKey() on that instance.
Line 155: return
brandingProperties.getProperty(ApplicationType.WEBADMIN.getCssKey());
Line 156: }
Line 157:
Line 158: /**
Line 158: /**
Line 159: * Getter for the user portal style sheet name.
Line 160: * @return The {@code String} representing the style sheet.
Line 161: */
Line 162: public final String getUserPortalStyleSheetName() {
Why is this method necessary? The caller should have appropriate
ApplicationType enum instance, we can just call getCssKey() on that instance.
Line 163: return
brandingProperties.getProperty(ApplicationType.USER_PORTAL.getCssKey());
Line 164: }
Line 165:
Line 166: /**
Line 190: * Get the style sheet type based on the passed in {@code
ApplicationType}.
Line 191: * @param type The application type to get the style sheet string
for.
Line 192: * @return A {@code String} representation of the style sheet
name.
Line 193: */
Line 194: public final String getThemeStyleSheet(final ApplicationType
type) {
The implementation of this method looks weird to me.
You can just call "brandingProperties.getProperty(type.getCssKey())" - what's
the purpose of the switch statement?
(The "default" switch case is redundant, given that ApplicationType only has
two values.)
Line 195: String result = null;
Line 196: switch (type) {
Line 197: case USER_PORTAL:
Line 198: result = getUserPortalStyleSheetName();
Line 212: * @return A {@code ResourceBundle} containing messages.
Line 213: */
Line 214: public final ResourceBundle getMessagesBundle() {
Line 215: // Default to US Locale.
Line 216: return getMessagesBundle(Locale.US);
Locale.US should be externalized as constant into LocaleFilter class (try to
avoid code duplication).
Line 217: }
Line 218:
Line 219: /**
Line 220: * Get the theme messages resource bundle.
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$
We should use regex to remove ".properties" suffix, instead of using
replaceAll() method.
Line 234: locale,
Line 235: urlLoader);
Line 236: } catch (IOException e) {
Line 237: // Unable to load messages resource bundle.
Line 235: urlLoader);
Line 236: } catch (IOException e) {
Line 237: // Unable to load messages resource bundle.
Line 238: log.warn("Unable to read message resource " //$NON-NLS-1$
Line 239: + "bundle, returning null"); //$NON-NLS-1$
Maybe also include "e" (exception) as log.warn method argument?
Line 240: }
Line 241: return result;
Line 242: }
Line 243:
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/BrandingServlet.java
Line 56: String fullPath = getFullPath(path);
Line 57: if (fullPath != null) {
Line 58: File file = new File(fullPath);
Line 59: if (!file.exists() || !file.canRead() ||
file.isDirectory()) {
Line 60: log.error("Unable to locate file, will send a 404
error response."); //$NON-NLS-1$
I suggest to log requested file path as well, i.e. see PluginResourceServlet
for reference.
Line 61: response.sendError(HttpServletResponse.SC_NOT_FOUND);
Line 62: } else {
Line 63: String etag = generateEtag(file);
Line 64: if
(etag.equals(request.getHeader(GwtDynamicHostPageServlet.
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)) {
Why are we checking isSane for "path" (request.getPathInfo) instead of full
(resolved) path to requested resource?
In other words:
isSane(path)
vs.
isSane(brandingManager.getBrandingRootPath().getAbsolutePath() + path)
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/java/org/ovirt/engine/ui/frontend/server/gwt/GwtDynamicHostPageServlet.java
Line 30:
Line 31: /**
Line 32: * Renders the HTML host page of a GWT application.
Line 33: * <p>
Line 34: * Embeds additional data (JavaScript objects) into the host page.
config/engine-code-format.xml (Java code formatting rules for Eclipse)
specifies following:
<setting id="org.eclipse.jdt.core.formatter.lineSplit" value="120"/>
In general this means "maximum line width is 120 characters".
Please start using above mentioned code formatting rules, instead of
Eclipse-default 80 character limit.
Line 35: * By default, information about the currently logged-in
Line 36: * user is included via {@code userInfo} object.
Line 37: */
Line 38: public abstract class GwtDynamicHostPageServlet extends HttpServlet {
Line 35: * By default, information about the currently logged-in
Line 36: * user is included via {@code userInfo} object.
Line 37: */
Line 38: public abstract class GwtDynamicHostPageServlet extends HttpServlet {
Line 39: /**
Please use newline to separate inner class members... (better code readability,
whitespace really matters!)
Line 40: * The values of this enum are used by the MD5 sum calculation to
Line 41: * determine if the values have changed.
Line 42: */
Line 43: public enum MD5Attributes {
Line 112: request.setAttribute(MD5Attributes.ATTR_STYLES.getKey(),
brandingThemes);
Line 113: // Set the messages that need to be replaced.
Line 114: request.setAttribute(MD5Attributes.ATTR_MESSAGES.getKey(),
getBrandingMessages(getLocaleFromRequest(request)));
Line 115: // Set the default theme path.
Line 116: request.setAttribute(ATTR_THEME_PATH, "theme"); //$NON-NLS-1$
Do we really need to set request attribute in order to share common "theme"
constant across different components?
Line 117: // Set class of servlet
Line 118:
request.setAttribute(MD5Attributes.ATTR_APPLICATION_TYPE.getKey(),
getApplicationType());
Line 119: // Set attribute for userInfo object
Line 120: VdcUser loggedInUser =
getLoggedInUser(request.getSession().getId());
Line 161: private Locale getLocaleFromRequest(final ServletRequest request)
{
Line 162: Locale locale = (Locale)
request.getAttribute(LocaleFilter.LOCALE); //$NON-NLS-1$
Line 163: if (locale == null) {
Line 164: //If no locale defined, default back to US locale.
Line 165: locale = Locale.US;
Please add new constant, i.e. "public static final Locale defaultLocale =
Locale.US" into LocaleFilter, and replace "Locale.US" usage here and in other
related classes to use the "defaultLocale" constant.
Line 166: }
Line 167: return locale;
Line 168: }
Line 169:
Line 167: return locale;
Line 168: }
Line 169:
Line 170: /**
Line 171: * Get a JavaScript associated array string that define the
branding messages.
associated -> associative
Line 172: * @param locale {@code Locale} to use to look up the messages.
Line 173: * @return The messages as a {@code String}
Line 174: */
Line 175: private String getBrandingMessages(final Locale locale) {
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/UserPortalHostPageServlet.java
Line 21: }
Line 22:
Line 23: @Override
Line 24: public ApplicationType getApplicationType() {
Line 25: return BrandingTheme.ApplicationType.USER_PORTAL;
Why use "BrandingTheme.ApplicationType" when return type already uses
"ApplicationType" directly?
Line 26: }
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/WebAdminHostPageServlet.java
Line 41: }
Line 42:
Line 43: @Override
Line 44: public ApplicationType getApplicationType() {
Line 45: return BrandingTheme.ApplicationType.WEBADMIN;
Why use "BrandingTheme.ApplicationType" when return type already uses
"ApplicationType" directly?
Line 46: }
Line 47:
Line 48: @Override
Line 49: protected boolean filterQueries() {
....................................................
File
frontend/webadmin/modules/frontend/src/main/resources/META-INF/resources/GwtHostPage.jsp
Line 7: <meta http-equiv="X-UA-Compatible" content="IE=edge">
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}">
Put indentation here for better readability, this is nested under <c:forEach>
Line 12: <link rel="stylesheet" type="text/css"
href="${pageContext.request.contextPath}/${requestScope['theme']}${theme.path}/${theme.getThemeStyleSheet(requestScope['applicationType'])}">
Line 13: </c:if>
Line 14: </c:forEach>
Line 15: </c:if>
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'])}">
Again, wrong indentation, code readability matters!
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 8: /**
Line 9: * This class contains dynamic messages available to the application.
This
Line 10: * class is a singleton.
Line 11: */
Line 12: public class DynamicConstants {
Please add newline below, helps code readability.
Line 13: protected static final String LOGIN_HEADER_LABEL =
"login_header_label"; //$NON-NLS-1$
Line 14: protected static final String MAIN_HEADER_LABEL =
"main_header_label"; //$NON-NLS-1$
Line 15: protected static final String COPY_RIGHT_NOTICE =
"copy_right_notice"; //$NON-NLS-1$
Line 16: protected static final String APPLICATION_TITLE =
"application_title"; //$NON-NLS-1$
Line 13: protected static final String LOGIN_HEADER_LABEL =
"login_header_label"; //$NON-NLS-1$
Line 14: protected static final String MAIN_HEADER_LABEL =
"main_header_label"; //$NON-NLS-1$
Line 15: protected static final String COPY_RIGHT_NOTICE =
"copy_right_notice"; //$NON-NLS-1$
Line 16: protected static final String APPLICATION_TITLE =
"application_title"; //$NON-NLS-1$
Line 17: protected static final String VERSION_ABOUT = "version_about";
//$NON-NLS-1$
Consider adding enum to contain message keys (without the prefix).
Line 18:
Line 19: /**
Line 20: * The name under which the dictionary will appear in the host
page.
Line 21: */
Line 28:
Line 29: /**
Line 30: * Protected default constructor.
Line 31: */
Line 32: @Inject
@Inject is redundant here, its purpose is to inject any additional dependencies
(instances bound in Ginjector context).
For no-arg constructor, there are no dependencies so no need for @Inject,
Guice/GIN honors no-arg constructor when binding types, assuming there are no
other constructors.
Line 33: public DynamicConstants() {
Line 34: dictionary = null;
Line 35: try {
Line 36: dictionary =
Dictionary.getDictionary(MESSAGES_DICTIONARY_NAME);
Line 30: * Protected default constructor.
Line 31: */
Line 32: @Inject
Line 33: public DynamicConstants() {
Line 34: dictionary = null;
This statement is redundant.
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.
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.
I suggest to log this using GWT java.util.logging emulation.
Line 39: }
Line 40: }
Line 41:
Line 42: /**
Line 46: * @param fallback The fallback in case the dictionary cannot find
a string associated with the key.
Line 47: * @return A {@code String} looked up using the key from the
Line 48: * {@code Dictionary}.
Line 49: */
Line 50: protected String getString(final String key, final String
fallback) {
See my comment above - adding enum for possible keys, the enum would also
contain fallback (default) value, this would improve overall code
maintainability and remove need for subclassing DynamicConstants altogether.
(In addition, if DynamicConstants is really meant to be subclassed, it should
be marked as "abstract" anyway.)
Line 51: String result = fallback;
Line 52: try {
Line 53: if (dictionary != null) {
Line 54: result = dictionary.get(key);
Line 53: if (dictionary != null) {
Line 54: result = dictionary.get(key);
Line 55: }
Line 56: } catch (MissingResourceException mre) {
Line 57: // Do nothing, the key doesn't exist.
I suggest to log this using GWT java.util.logging emulation.
Line 58: }
Line 59: return result;
Line 60: }
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/dialog/SimpleDialogPanel.ui.xml
Line 108:
Line 109: <g:FlowPanel>
Line 110: <g:FlowPanel addStyleNames="{style.header}">
Line 111: <g:SimplePanel ui:field="logoPanel"
addStyleNames="{style.headerLeftPanel}">
Line 112: <g:Image
styleName='obrand_dialogLogoImage' url="clear.cache.gif" />
Please explain why is "clear.cache.gif" necessary here?
Line 113: </g:SimplePanel>
Line 114: <g:FlowPanel
addStyleNames="{style.obrand_headerCenterPanel}">
Line 115: <g:FlowPanel
ui:field="headerContainerPanel">
Line 116: <g:SimplePanel
ui:field="headerTitlePanel" addStyleNames="{style.headerTitle}" />
Line 122: </g:FlowPanel>
Line 123: </g:FlowPanel>
Line 124: <g:SimplePanel
addStyleNames="{style.headerRightPanel}">
Line 125: <g:FlowPanel>
Line 126: <g:Image
styleName='obrand_dialogHeaderImage' url="clear.cache.gif" />
Same as my comment above.
Line 127: <g:PushButton
ui:field='closeIconButton' addStyleNames="{style.closeIconButton}">
Line 128: <g:upFace
image='{resources.dialogIconClose}' />
Line 129: <g:downFace
image='{resources.dialogIconCloseDown}' />
Line 130: <g:upHoveringFace
image='{resources.dialogIconCloseRollover}' />
....................................................
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();
If we externalize "active" why leave "inactive" non-externalized? (for
consistency)
Line 35: }
Line 36:
Line 37: @UiField
Line 38: FocusPanel tabContainer;
....................................................
File
frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/ApplicationDynamicConstants.java
Line 11: * associated {@code ApplicationConstants} message.
Line 12: *
Line 13: * This is for the user portal
Line 14: */
Line 15: public class ApplicationDynamicConstants extends DynamicConstants {
Why do we need to subclass DynamicConstants?
It seems that UserPorta/WebAdmin-specific ApplicationConstants reference is the
only thing needed here.
So I suggest following:
- rename UserPortalDynamicConstants to something more appropriate, i.e.
ConstantWithDefault enum
- move ConstantWithDefault enum to DynamicConstants
- make DynamicConstants @Inject CommonApplicationConstants (available in
gwt-common)
- if necessary, move ApplicationConstants methods up into
CommonApplicationConstants
Result: no DynamicConstants subclassing necessary, less code to maintain.
Line 16: /**
Line 17: * This enum contains the keys for the {@code Dictionary} to look
up dynamic
Line 18: * messages from the host page. If the particular message is not
found, the
Line 19: * fall-back is used.
....................................................
File
frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/section/login/view/LoginPopupView.ui.xml
Line 114:
Line 115: <d:SimplePopupPanel ui:field="popup" width="480px">
Line 116: <d:header>
Line 117: <g:HorizontalPanel
styleName="{loginPopupStyle.loginPopupHeader}">
Line 118: <g:Image
styleName="obrand_loginPopupHeaderLogoImage" url="clear.cache.gif" />
Same as before, why is "clear.cache.gif" necessary here?
Line 119: <g:HTMLPanel
styleName="{loginPopupStyle.obrand_loginPopupHeaderCenter}">
Line 120: <g:Label ui:field="headerLabel"
addStyleNames="{loginPopupStyle.headerLabelStyle}"/>
Line 121: </g:HTMLPanel>
Line 122: <g:Image
styleName="obrand_loginPopupHeaderImage" url="clear.cache.gif" />
Line 118: <g:Image
styleName="obrand_loginPopupHeaderLogoImage" url="clear.cache.gif" />
Line 119: <g:HTMLPanel
styleName="{loginPopupStyle.obrand_loginPopupHeaderCenter}">
Line 120: <g:Label ui:field="headerLabel"
addStyleNames="{loginPopupStyle.headerLabelStyle}"/>
Line 121: </g:HTMLPanel>
Line 122: <g:Image
styleName="obrand_loginPopupHeaderImage" url="clear.cache.gif" />
Same question as above.
Line 123: </g:HorizontalPanel>
Line 124: </d:header>
Line 125:
Line 126: <d:content>
....................................................
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 -->
Please add newline above for better readability.
Also. please reformat this XML file, I saw that you use 4-space convention for
indentation, or just use TAB characters.
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>
Line 4: <display-name>oVirt UserPortal 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>
This definition should go into
frontend/src/main/resources/META-INF/web-fragment.xml
(The point of web fragment is to define common servlet/filter names for easy
use within specific web applications.)
Line 9: </filter>
Line 10: <filter-mapping>
Line 11: <filter-name>LocaleFilter</filter-name>
Line 12:
<url-pattern>/org.ovirt.engine.ui.userportal.UserPortal/*</url-pattern>
Line 27: <servlet-name>GenericApiServlet</servlet-name>
Line 28:
<url-pattern>/org.ovirt.engine.ui.userportal.UserPortal/GenericApiGWTService</url-pattern>
Line 29: </servlet-mapping>
Line 30:
Line 31: <servlet-mapping>
See my comment above, please reformat this XML file.
Line 32: <servlet-name>BrandingServlet</servlet-name>
Line 33: <url-pattern>/theme/*</url-pattern>
Line 34: </servlet-mapping>
Line 35:
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationDynamicConstants.java
Line 10: * associated {@code ApplicationConstants} message.
Line 11: *
Line 12: * This is for the web admin
Line 13: */
Line 14: public class ApplicationDynamicConstants extends DynamicConstants {
See my comment in userportal ApplicationDynamicConstants.java - we should
simplify the implementation, it also seems there's a lot of unnecessary code
duplicity between these two ApplicationDynamicConstants classes.
Line 15: /**
Line 16: * This enum contains the keys for the {@code Dictionary} to look
up dynamic
Line 17: * messages from the host page. If the particular message is not
found, the
Line 18: * fall-back is used.
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/login/view/LoginPopupView.ui.xml
Line 111:
Line 112: <d:SimplePopupPanel ui:field="popup" width="480px">
Line 113: <d:header>
Line 114: <g:HorizontalPanel
styleName="{loginPopupStyle.loginPopupHeader}">
Line 115: <g:Image
styleName="obrand_loginPopupHeaderLogoImage" url="clear.cache.gif" />
Same as before, why is "clear.cache.gif" necessary here?
Line 116: <g:HTMLPanel
styleName="{loginPopupStyle.obrand_loginPopupHeaderCenter}">
Line 117: <g:Label ui:field="headerLabel"
addStyleNames="{style.headerLabelStyle}"/>
Line 118: </g:HTMLPanel>
Line 119: <g:Image
styleName="obrand_loginPopupHeaderImage" url="clear.cache.gif" />
Line 115: <g:Image
styleName="obrand_loginPopupHeaderLogoImage" url="clear.cache.gif" />
Line 116: <g:HTMLPanel
styleName="{loginPopupStyle.obrand_loginPopupHeaderCenter}">
Line 117: <g:Label ui:field="headerLabel"
addStyleNames="{style.headerLabelStyle}"/>
Line 118: </g:HTMLPanel>
Line 119: <g:Image
styleName="obrand_loginPopupHeaderImage" url="clear.cache.gif" />
Same question as above.
Line 120: </g:HorizontalPanel>
Line 121: </d:header>
Line 122:
Line 123: <d:content>
....................................................
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
Are CSS definitions in "defines.css" used anywhere else, do we still need this
CSS file?
....................................................
File packaging/branding/ovirt.brand/branding.properties
Line 1: #OVIRT branding properties.
See my comment in README.branding - this file should document purpose and other
details on supported options.
Line 2:
Line 3: #style sheets.
Line 4: user_portal_css=ovirt_user_portal.css
Line 5: web_admin_css=ovirt_web_admin.css
....................................................
File packaging/branding/ovirt.brand/ovirt_messages.properties
Line 1: #Available properties:
See my comment in README.branding - this file should document individual
message keys.
Line 2: #common
Line 3: obrand.common.login_header_label=Open Virtualization Manager
Line 4: obrand.common.copy_right_notice=
Line 5: obrand.common.version_about=oVirt Engine Version:
....................................................
File packaging/branding/ovirt.brand/ovirt_user_portal.css
Line 1: @import url("ovirt_common.css");
See my comment in README.branding - this file should document individual CSS
classes.
Line 2:
Line 3: /* MainTabBasicView.ui.xml */
Line 4: .obrand_borderPanelStyle {
Line 5: border-color: #719823;
....................................................
File packaging/branding/ovirt.brand/ovirt_web_admin.css
Line 1: @import url("ovirt_common.css");
See my comment in README.branding - this file should document individual CSS
classes.
Line 2:
Line 3: /* HeaderView.ui.xml */
Line 4: .obrand_logo {
Line 5: float: left;
....................................................
File README.branding
Line 6:
Line 7: ovirt-engine supports pluggable branding that may effect the user
interface
Line 8: themes and selected strings. There may be several branding packages
Line 9: installed in specific order, each package overrides the previous ones
Line 10: for the resources it defines.
Overall, this README file is excellent! Helped me understand a lot about how
the branding works just by reading this file.
Line 11:
Line 12: PACKAGE FORMAT
Line 13: --------------
Line 14:
Line 11:
Line 12: PACKAGE FORMAT
Line 13: --------------
Line 14:
Line 15: branding.properties
Which properties in branding.properties file are mandatory and which are
optional?
Please specify this information here as well as within reference oVirt branding
package's branding.properties file, for posterity (see my comment below).
Line 16: This is the main properties file that defines where the branding
Line 17: resources can be found. Resources are relative to
branding.properties.
Line 18:
Line 19: user_portal_css - Css to inject into user portal.
Line 20: web_admin_css - Css to inject into web admin.
Line 21: messages - Standard java message bundle.
Line 22: version - The version of the branding in the package. Only versions
Line 23: that match the one defined in the engine will be loaded. The
current
Line 24: version is 1.
I suggest to put this information also inside reference oVirt branding
package's branding.properties file, using properties (#) comment syntax.
"The current version is" information deserves special attention, I suggest to
have a separate paragraph for it.
Also, if a branding package defines "version=2" and Engine supports "version=1"
shouldn't we aim for backwards compatibility, so that branding package authors
don't have to maintain multiple packages for each version?
(In other words, what's the reason behind exact version checking?)
Line 25:
Line 26: CSS INJECTION
Line 27:
Line 28: CSS are injected in the reverse order of the branding package order,
this
Line 25:
Line 26: CSS INJECTION
Line 27:
Line 28: CSS are injected in the reverse order of the branding package order,
this
Line 29: way the last css's styles overrides the previous ones.
I think this paragraph should be removed, as mentioned below we use standard
conf.d convention to impose order among multiple branding packages.
Assuming CSS style sheets are applied sequentially, the latter one always
overrides the former one.
Line 30:
Line 31: The oVirt UI is broken up into three distinct modules at this point.
Line 32: * User Portal
Line 33: * Web Admin
Line 126: EXAMPLE
Line 127: -------
Line 128: If you look in packaging/branding/ovirt.brand you will see a
functional
Line 129: example branding theme which just happens to be the default oVirt
theme.
Line 130: You will notice the following files:
If we already have a reference oVirt branding package that is "complete" in
terms of CSS classes and message keys, what's the point of providing full
listing of CSS classes + message keys in this file?
Wasn't the reference oVirt branding package supposed to serve as, well,
reference to these CSS classes + message keys, communicating clear (and
complete) interface for branding package authors?
(In other words, do we really want to maintain full CSS class + message key
list in this README file?)
I suggest to remove CSS class + message key listing in this README file (except
for examples, of course) and provide extra documentation (comments) right
inside reference oVirt branding package's CSS and properties files.
For CSS this means adding extra comments to .obrand_* classes in:
- ovirt_user_portal.css + any @import'ed css files
- ovirt_web_admin.css + any @import'ed css files
For messages this means adding extra comments to obrand.* message keys in:
- ovirt_messages.properties
(no need to do this for ovirt_messages_xx_YY.properties since this is just
locale-specific message value override)
(Additionally, in future, we can consider auto-generating the list of brandable
CSS classes and message keys. My point is to avoid maintaining same information
in different files by hand.)
Line 131:
Line 132: 1. branding.properties, This is the main properties file defining
Line 133: the branding properties.
Line 134: 2. user_portal.css, The user portal style sheet file.
Line 151: own directory.
Line 152:
Line 153: The branding directory is treated as a standard conf.d, in which
directories
Line 154: are sorted by name, each package is read by order and overrides
Line 155: the previous ones.
+1
Using standard conf.d convention to impose order among multiple branding
packages sounds reasonable here.
Just wondering, when you install branding package, will this package
automatically assume it has order N and put its files into say
${engine_syconfdir}/branding/N-PKGNAME.brand directory?
--
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: 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
