Vojtech Szocs has posted comments on this change.
Change subject: userportal, webadmin: branding support[WIP].
......................................................................
Patch Set 1: (39 inline comments)
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/branding/BrandingManager.java
Line 20: /**
Line 21: * The default style sheet name.
Line 22: */
Line 23: public static final String DEFAULT_STYLE_FILENAME =
Line 24: "default.css"; //$NON-NLS-1$
Minor comment: engine-code-format.xml (Eclipse IDE / Java Code Style /
Formatter) defines max. line width = 120
Can you please use Engine code formatting rules in such places?
Line 25: /**
Line 26: * The default branding path.
Line 27: */
Line 28: private static final String BRANDING_PATH = "/branding";
//$NON-NLS-1$
Line 30: /**
Line 31: * The prefix used for common messages.
Line 32: */
Line 33: private static final String COMMON_PREFIX = "common"; //$NON-NLS-1$
Line 34: /**
Minor comment: please use newline to delimit field declarations.
Line 35: * A list of available {@code BrandingTheme}s.
Line 36: */
Line 37: private List<BrandingTheme> themes = null;
Line 38: /**
Line 37: private List<BrandingTheme> themes = null;
Line 38: /**
Line 39: * The root path of the branding themes.
Line 40: */
Line 41: private final File brandingRootPath = new
File(LocalConfig.getInstance().
Why not have an explicit constructor that takes etcDir String as argument?
>From testability point of view, final fields should always be initialized in a
>way that allows easy customization (e.g. passing mocked dependencies).
Line 42: getEtcDir() + BRANDING_PATH);
Line 43:
Line 44: /**
Line 45: * Get a list of available {@code BrandingTheme}s.
Line 44: /**
Line 45: * Get a list of available {@code BrandingTheme}s.
Line 46: * @return A {@code List} of {@code BrandingTheme}s.
Line 47: */
Line 48: public final List<BrandingTheme> getBrandingThemes() {
Why is this method final? Is it expected for users to create subclasses of
BrandingManager? If not, I wouldn't bother with "final" keyword at all.
Line 49: if (themes == null) {
Line 50: if (brandingRootPath.exists() &&
brandingRootPath.isDirectory()
Line 51: && brandingRootPath.canRead()) {
Line 52: themes = new ArrayList<BrandingTheme>();
Line 46: * @return A {@code List} of {@code BrandingTheme}s.
Line 47: */
Line 48: public final List<BrandingTheme> getBrandingThemes() {
Line 49: if (themes == null) {
Line 50: if (brandingRootPath.exists() &&
brandingRootPath.isDirectory()
Minor comment: nested "if" statements can be joined with "&&".
Line 51: && brandingRootPath.canRead()) {
Line 52: themes = new ArrayList<BrandingTheme>();
Line 53: List<File> directories = Arrays.asList(
Line 54: brandingRootPath.listFiles(new
DirectoryFilter()));
Line 75: */
Line 76: public final String getMessages(final String prefix) {
Line 77: List<BrandingTheme> messageThemes = getBrandingThemes();
Line 78: //We need this map to remove potential duplicate strings from
the
Line 79: //resource bundles.
Minor comment: please add space after "//".
Line 80: Map<String, String> keyValues = new HashMap<String, String>();
Line 81: for (BrandingTheme theme: messageThemes) {
Line 82: ResourceBundle messagesBundle = theme.getMessagesBundle();
Line 83: for (String key: messagesBundle.keySet()) {
Line 83: for (String key: messagesBundle.keySet()) {
Line 84: if (key.startsWith(prefix) ||
key.startsWith(COMMON_PREFIX)) {
Line 85: //We can potentially override existing values here
Line 86: //but this is fine as the themes are sorted in
order
Line 87: //And later messages should override earlier ones.
Minor comment: please add space after "//".
Line 88: keyValues.put(key.replaceFirst(prefix + "\\.",
""). //$NON-NLS-1$
Line 89: replaceFirst(COMMON_PREFIX + "\\.", ""),
//$NON-NLS-1$
Line 90: messagesBundle.getString(key));
Line 91: }
Line 91: }
Line 92: }
Line 93: }
Line 94: //Turn the map into a string with the format:
Line 95: //{'key':'value',...}
Minor comment: please add space after "//".
Line 96: return getMessagesFromMap(keyValues);
Line 97: }
Line 98:
Line 99: /**
Line 99: /**
Line 100: * @param keyValues The map to turn into the string.
Line 101: * @return A string of format {'key':'value',...}
Line 102: */
Line 103: private String getMessagesFromMap(final Map<String, String>
keyValues) {
I think it's better to use Jackson (JSON) library here, instead of constructing
JSON by hand.
E.g. similar to GwtDynamicHostPageServlet.getUserInfoObject() method, and
calling toString() on resulting ObjectNode.
Line 104: boolean first = true;
Line 105: StringBuilder builder = new StringBuilder();
Line 106: builder.append('{'); //Open bracket //$NON-NLS-1$
Line 107: for (Map.Entry<String, String> entry : keyValues.entrySet()) {
Line 102: */
Line 103: private String getMessagesFromMap(final Map<String, String>
keyValues) {
Line 104: boolean first = true;
Line 105: StringBuilder builder = new StringBuilder();
Line 106: builder.append('{'); //Open bracket //$NON-NLS-1$
Minor comment: please add space after "//".
Line 107: for (Map.Entry<String, String> entry : keyValues.entrySet()) {
Line 108: if (!first) {
Line 109: builder.append(','); //$NON-NLS-1$
Line 110: }
Line 114: builder.append("':'"); //$NON-NLS-1$
Line 115: builder.append(entry.getValue());
Line 116: builder.append('\''); //$NON-NLS-1$
Line 117: }
Line 118: builder.append('}'); //Close bracket //$NON-NLS-1$
Minor comment: please add space after "//".
Line 119: return builder.toString();
Line 120: }
Line 121:
Line 122: /**
Line 129:
Line 130: /**
Line 131: * Comparator that sorts themes in their .
Line 132: */
Line 133: private class BrandingComparator implements
Comparator<BrandingTheme> {
What's the purpose of this comparator? It essentially reverts natural ordering
(imposed by Comparable interface) by -1 * compareTo().
Line 134: @Override
Line 135: public int compare(final BrandingTheme theme1,
Line 136: final BrandingTheme theme2) {
Line 137: return -1 * theme1.compareTo(theme2);
Line 140:
Line 141: /**
Line 142: * This class implements a {@code FileFilter} for directories.
Line 143: */
Line 144: private class DirectoryFilter implements FileFilter {
Why not inline this class directly where it's used? (nobody else uses it)
Line 145: @Override
Line 146: public boolean accept(final File file) {
Line 147: return file.isDirectory();
Line 148: }
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/branding/BrandingTheme.java
Line 145: propertiesFile = new FileInputStream(brandingPath
Line 146: + "/" + BRANDING_PROPERTIES_NAME); //$NON-NLS-1$
Line 147: brandingProperties.load(propertiesFile);
Line 148: } catch (IOException e) {
Line 149: //Unable to load properties file, disable theme.
Minor comment: please add space after "//".
Line 150: log.warn("Unable to load properties file for "
//$NON-NLS-1$
Line 151: + "theme located here:"//$NON-NLS-1$
Line 152: + brandingPath + "/" //$NON-NLS-1$
Line 153: + BRANDING_PROPERTIES_NAME);
Line 214: brandingProperties.getProperty(THEME_ORDER_KEY,
Line 215: "0")); //$NON-NLS-1$
Line 216: }
Line 217:
Line 218: @Override
Please put @Override close to method declaration.
Line 219: /**
Line 220: * Compares two branding themes to each other using the following:
Line 221: * <ol>
Line 222: * <li>type, OEM comes first, then END_USER</li>
Line 218: @Override
Line 219: /**
Line 220: * Compares two branding themes to each other using the following:
Line 221: * <ol>
Line 222: * <li>type, OEM comes first, then END_USER</li>
This implicitly means the order of BrandingThemeType enum members is
significant, shouldn't it be mentioned in enum Javadoc?
Line 223: * <li>order, if exists, as a regular integer</li>
Line 224: * </ol>
Line 225: */
Line 226: public final int compareTo(final BrandingTheme other) {
Line 265: * Get the theme messages resouce bundle for the US locale.
Line 266: * @return A {@code ResourceBundle} containing messages.
Line 267: */
Line 268: public final ResourceBundle getMessagesBundle() {
Line 269: //Default to US Locale.
Minor comment: please add space after "//".
Line 270: return getMessagesBundle(Locale.US);
Line 271: }
Line 272:
Line 273: /**
Line 278: public final ResourceBundle getMessagesBundle(final Locale
locale) {
Line 279: ResourceBundle result = null;
Line 280: try {
Line 281: File themeDirectory = new File(filePath);
Line 282: URLClassLoader urlLoader = new URLClassLoader(
Why is URLClassLoader necessary here? (maybe I am missing something)
Line 283: new URL[]{themeDirectory.toURI().toURL()});
Line 284: result = ResourceBundle.getBundle(
Line 285: brandingProperties.getProperty(MESSAGES_KEY).
Line 286: replaceAll("\\.properties", ""), //$NON-NLS-1$
//$NON-NLS-2$
Line 285: brandingProperties.getProperty(MESSAGES_KEY).
Line 286: replaceAll("\\.properties", ""), //$NON-NLS-1$
//$NON-NLS-2$
Line 287: locale, urlLoader);
Line 288: } catch (IOException e) {
Line 289: //Unable to load messages resource bundle.
Minor comment: please add space after "//".
Line 290: log.warn("Unable to read message resource " //$NON-NLS-1$
Line 291: + "bundle, returning null"); //$NON-NLS-1$
Line 292: }
Line 293: return result;
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/BrandingServlet.java
Line 19: * This class serves files from the branding themes to the browser.
This
Line 20: * includes images, style sheets and other files. It provides ETags so
Line 21: * browsers can cache the output of the servlet.
Line 22: */
Line 23: @WebServlet(name = "Branding", value = "/theme/*")
Wouldn't it be better to register BrandingServlet via web-fragment.xml and let
web applications (WebAdmin & UserPortal) use it via web.xml?
This way, each application will have BrandingServlet unconditionally registered
and applied to given URL mapping.
Disclaimer: I don't think servlet 3.0 annotations are that useful, I think
web-fragment.xml + web.xml is much better alternative.
Line 24: public class BrandingServlet extends HttpServlet {
Line 25:
Line 26: private static final long serialVersionUID = 8687185074759812924L;
Line 27:
Line 39: /**
Line 40: * Default constructor.
Line 41: */
Line 42: public BrandingServlet() {
Line 43: setBrandingManager(new BrandingManager());
You could also entirely omit default constructor, and have:
@Override public void init() { code_that_sets_manager }
In unit tests, you could set (mock) manager instance without default
constructor setting the real manager first.
Line 44: }
Line 45:
Line 46: /**
Line 47: * Set the branding manager.
Line 61: // Locate the requested file:
Line 62: String fullPath = getFullPath(path);
Line 63: if (fullPath != null) {
Line 64: File file = new File(fullPath);
Line 65: if (file == null || !file.exists() || !file.canRead()
Why "file == null" condition?
Line 66: || file.isDirectory()) {
Line 67: log.error("Unable to locate file, will send a "
//$NON-NLS-1$
Line 68: + "404 error response."); //$NON-NLS-1$
Line 69: response.sendError(HttpServletResponse.SC_NOT_FOUND);
Line 116: if (ServletUtils.isSane(path)) {
Line 117: List<BrandingTheme> brandingPaths = brandingManager.
Line 118: getBrandingThemes();
Line 119: for (BrandingTheme brandingTheme: brandingPaths) {
Line 120: if (path.startsWith(brandingTheme.getPath())) {
Instead of iterating over List<BrandingTheme> on every request, you can use
Map<String, BrandingTheme> - mapping brandingTheme.getPath() to corresponding
BrandingTheme instance.
This is to avoid O(n) access due to iteration, in favor of O(1) access due to
map value access, with regard to appropriate BrandingTheme instance.
Line 121: result = brandingManager.getBrandingRootPath().
Line 122: getAbsolutePath() + path;
Line 123: }
Line 124: }
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/GwtDynamicHostPageServlet.java
Line 55: /**
Line 56: * Get the value of the attribute key.
Line 57: * @return A {@code String} containing the key.
Line 58: */
Line 59: public String getAttributeKey() {
Very minor comment: I'd shorten this to just getKey(), it's obvious that this
returns an attribute key :-)
Line 60: return attributeKey;
Line 61: }
Line 62: }
Line 63: private static final long serialVersionUID = 3946034162721073929L;
Line 72: private static final String UTF_CONTENT_TYPE = "text/html;
charset=UTF-8"; //$NON-NLS-1$
Line 73:
Line 74: private BackendLocal backend;
Line 75: private ObjectMapper mapper;
Line 76: private final BrandingManager brandingManager = new
BrandingManager();
Why not initialize BrandingManager inside init(), just like ObjectMapper?
I don't think "final" keyword has any significant improvement here, and this
also hurts testability.
Line 77:
Line 78: @EJB(beanInterface = BackendLocal.class,
Line 79: mappedName =
"java:global/engine/bll/Backend!org.ovirt.engine.core.common.interfaces.BackendLocal")
Line 80: public void setBackend(BackendLocal backend) {
Line 91: // Set attribute for selector script
Line 92:
request.setAttribute(MD5Attributes.ATTR_SELECTOR_SCRIPT.getAttributeKey(),
getSelectorScriptName());
Line 93: // Set attribute for themes.
Line 94:
request.setAttribute(MD5Attributes.ATTR_STYLES.getAttributeKey(),
brandingManager.getBrandingThemes());
Line 95: //Set the messages that need to be replaced.
Minor comment: please add space after "//".
Line 96:
request.setAttribute(MD5Attributes.ATTR_MESSAGES.getAttributeKey(),
getBrandingMessages());
Line 97: //Set the default theme path.
Line 98: request.setAttribute(ATTR_THEME_PATH, ATTR_THEME_PATH);
Line 99: // Set attribute for default style sheet location.
Line 93: // Set attribute for themes.
Line 94:
request.setAttribute(MD5Attributes.ATTR_STYLES.getAttributeKey(),
brandingManager.getBrandingThemes());
Line 95: //Set the messages that need to be replaced.
Line 96:
request.setAttribute(MD5Attributes.ATTR_MESSAGES.getAttributeKey(),
getBrandingMessages());
Line 97: //Set the default theme path.
Minor comment: please add space after "//".
Line 98: request.setAttribute(ATTR_THEME_PATH, ATTR_THEME_PATH);
Line 99: // Set attribute for default style sheet location.
Line 100: request.setAttribute(ATTR_DEFAULT_STYLE, "default.css");
//$NON-NLS-1$
Line 101: // Set class of servlet
Line 94:
request.setAttribute(MD5Attributes.ATTR_STYLES.getAttributeKey(),
brandingManager.getBrandingThemes());
Line 95: //Set the messages that need to be replaced.
Line 96:
request.setAttribute(MD5Attributes.ATTR_MESSAGES.getAttributeKey(),
getBrandingMessages());
Line 97: //Set the default theme path.
Line 98: request.setAttribute(ATTR_THEME_PATH, ATTR_THEME_PATH);
Isn't it dangerous to use same value as key, just because the value is same
string? :-)
In other words, it could be also: request.setAttribute(ATTR_THEME_PATH,
"theme"); //$NON-NLS-1$
Line 99: // Set attribute for default style sheet location.
Line 100: request.setAttribute(ATTR_DEFAULT_STYLE, "default.css");
//$NON-NLS-1$
Line 101: // Set class of servlet
Line 102:
request.setAttribute(MD5Attributes.ATTR_APPLICATION_TYPE.getAttributeKey(),
getApplicationType());
Line 212: * string representation of the MD5 sum.
Line 213: * @throws NoSuchAlgorithmException If the method cannot create
the digest
Line 214: * object.
Line 215: */
Line 216: protected MessageDigest getMd5Digest(final HttpServletRequest
request)
Excellent change! +1
Line 217: throws NoSuchAlgorithmException {
Line 218: MessageDigest digest = createMd5Digest();
Line 219: for (MD5Attributes attribute: MD5Attributes.values()) {
Line 220: if (request.getAttribute(attribute.getAttributeKey()) !=
null) {
....................................................
File
frontend/webadmin/modules/frontend/src/main/resources/META-INF/resources/GwtHostPage.jsp
Line 5: <head>
Line 6: <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
Line 7: <meta http-equiv="X-UA-Compatible" content="IE=edge">
Line 8: <c:if test="${requestScope['defaultStyle'] != null}">
Line 9: <link rel="stylesheet" type="text/css"
href="${requestScope['defaultStyle']}">
Minor comment: please indent child elements properly.
Line 10: </c:if>
Line 11: <c:if test="${requestScope['brandingStyle'] != null}">
Line 12: <c:forEach items="${requestScope['brandingStyle']}"
var="theme">
Line 13: <link rel="stylesheet" type="text/css"
href="${pageContext.request.contextPath}/${requestScope['theme']}${theme.path}/${theme.commonStyleSheetName}">
....................................................
File
frontend/webadmin/modules/frontend/src/main/resources/META-INF/web-fragment.xml
Line 12: <!-- * ClientBundle composite images: <md5>.cache.png |
<md5>.cache.gif -->
Line 13: <!-- * split points: deferredjs/<md5>/<n>.cache.js -->
Line 14: <init-param>
Line 15: <param-name>cache</param-name>
Line 16:
<param-value>.*\.cache\.html|.*\.cache\.png|.*\.cache\.gif|.*\.cache\.js|.*\.css</param-value>
Please update comment as well, e.g.:
* CSS style sheets
Line 17: </init-param>
Line 18:
Line 19: <!-- Resources which always need to be checked for
changes: -->
Line 20: <!-- * application host page: WebAdmin.html |
UserPortal.html -->
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/utils/DynamicConstants.java
Line 43: * Protected default constructor.
Line 44: * @param gwtConstants The fall back {@code
CommonApplicationConstants}.
Line 45: */
Line 46: @Inject
Line 47: protected DynamicConstants(final CommonApplicationConstants
gwtConstants) {
Why is this constructor protected?
Line 48: this.constants = gwtConstants;
Line 49: dictionary = null;
Line 50: try {
Line 51: dictionary =
Dictionary.getDictionary(MESSAGES_DICTIONARY_NAME);
Line 49: dictionary = null;
Line 50: try {
Line 51: dictionary =
Dictionary.getDictionary(MESSAGES_DICTIONARY_NAME);
Line 52: } catch (MissingResourceException mre) {
Line 53: //Do nothing, the dictionary doesn't exist.
Minor comment: please add space after "//".
Line 54: mre.printStackTrace();
Line 55: }
Line 56: }
Line 57:
Line 50: try {
Line 51: dictionary =
Dictionary.getDictionary(MESSAGES_DICTIONARY_NAME);
Line 52: } catch (MissingResourceException mre) {
Line 53: //Do nothing, the dictionary doesn't exist.
Line 54: mre.printStackTrace();
Please avoid this, just use client-side logger to log an ERROR message.
Line 55: }
Line 56: }
Line 57:
Line 58: /**
Line 68: if (dictionary != null) {
Line 69: result = dictionary.get(key);
Line 70: }
Line 71: } catch (MissingResourceException mre) {
Line 72: //Do nothing, the key doesn't exist.
Minor comment: please add space after "//".
Line 73: }
Line 74: return result;
Line 75: }
....................................................
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='dialogLogoImage'
url="clear.cache.gif" />
What's the reason behind "clear.cache.gif" ? Why isn't it used consistently in
other *.ui.xml files?
Line 113: </g:SimplePanel>
Line 114: <g:FlowPanel
addStyleNames="{style.headerCenterPanel}">
Line 115: <g:FlowPanel
ui:field="headerContainerPanel">
Line 116: <g:SimplePanel
ui:field="headerTitlePanel" addStyleNames="{style.headerTitle}" />
....................................................
File
frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/ApplicationDynamicConstants.java
Line 12: /**
Line 13: * Default constructor for UiBinder.
Line 14: */
Line 15: public ApplicationDynamicConstants() {
Line 16: this((CommonApplicationConstants)
I think the cast is redundant here.
Line 17: GWT.create(CommonApplicationConstants.class));
Line 18: }
Line 19:
Line 20: /**
Line 35: */
Line 36: public final String applicationTitle() {
Line 37: String result = getString(APPLICATION_TITLE);
Line 38: if (result == null) {
Line 39: result = ((ApplicationConstants)
constants).applicationTitle();
I think there should be a better way to do this, e.g. new enum for each
constant, that has both key (APPLICATION_TITLE) and method to return the
default value (constants.applicationTitle).
Line 40: }
Line 41: return result;
Line 42: }
Line 43:
....................................................
File
frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/webadmin/WebAdmin.gwt.xml
Line 1
Line 2
Line 3
Line 4
Line 5
What's the reason behind this change?
--
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: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Einav Cohen <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
