Vojtech Szocs has posted comments on this change.
Change subject: userportal, webadmin: branding support.
......................................................................
Patch Set 19: (3 inline comments)
Replying to Alon's comments, I also have one more general comment:
Before this patch, default CSS styles were part of compiled GWT application.
After this patch, if a developer doesn't have default oVirt branding package
installed, he will get messed up UI due to missing styles that should have been
provided by this package.
I think there should also be some script for developers (under
packaging/branding/ovirt.brand) that takes default oVirt branding package files
and copies them to relevant directory. This way, developers can just modify
oVirt branding package CSS files and re-run this script to apply changes at
runtime.
In other words, please don't force developers to build default oVirt branding
RPM just for sake of having default styles at runtime.
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/branding/BrandingManager.java
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()) {
Well, in this particular case, getBrandingThemes() is called on each request
for WebAdmin/UserPortal HTML page.
Regarding performance, we're just listing directories and parsing
branding.properties files, no big deal :-)
For example, UI Plugin feature allows reloading plugin metadata/configuration
from local filesystem on each WebAdmin/UserPortal HTML page request. This is
convenient for rapid plugin development or installing extra plugins without any
real reason to disrupt (restart) Engine service.
I'm OK with current approach, I was just curious about real "why" -
performance, runtime complexity or stuff like that aren't too relevant in this
particular case.
Line 78: themes = new ArrayList<BrandingTheme>();
Line 79: List<File> directories = Arrays.asList(
Line 80: brandingRootPath.listFiles(new FileFilter() {
Line 81: @Override
....................................................
File README.branding
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.
Thanks Alon, I agree that we should do some version checking here to preserve
consistency with current Engine UI.
However, my question was related to part that says: "Only versions that match
the one defined in the engine will be loaded".
This implies exact (==) version checking, i.e. "engineBrandingVersion ==
pluginBrandingVersion".
I'm OK with this, but we could also consider having interval-like notation for
version checking, for example:
* [2] --> engineBrandingVersion == 2
* [2,) --> 2 <= engineBrandingVersion
* (1,3] --> 1 < engineBrandingVersion <= 3
It's just an idea, for now I'm OK with exact version checking. My point was,
given different engineBrandingVersion's X and X+1, assuming X+1 preserves
compatibility (CSS class names, message keys) for X, the version check could be
based on interval, instead of exact (==) version comparison.
Line 25:
Line 26: CSS INJECTION
Line 27:
Line 28: CSS are injected in the reverse order of the branding package order,
this
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.
OK, so it's the packager's responsibility to properly install branding package
files into appropriately-named 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 <[email protected]>
Gerrit-Reviewer: Alex Lourie <[email protected]>
Gerrit-Reviewer: Alexander Wels <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Einav Cohen <[email protected]>
Gerrit-Reviewer: Eyal Edri <[email protected]>
Gerrit-Reviewer: Itamar Heim <[email protected]>
Gerrit-Reviewer: Kanagaraj M <[email protected]>
Gerrit-Reviewer: Moran Goldboim <[email protected]>
Gerrit-Reviewer: Ofer Schreiber <[email protected]>
Gerrit-Reviewer: Sahina Bose <[email protected]>
Gerrit-Reviewer: Sandro Bonazzola <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
