Lior Vernia has posted comments on this change.
Change subject: webadmin: adding Affinity Groups views
......................................................................
Patch Set 8:
(46 comments)
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/configure/scheduling/affinity_groups/VmSelectionModel.java
Line 5: import org.ovirt.engine.ui.uicommonweb.models.ListModel;
Line 6: import org.ovirt.engine.ui.uicommonweb.models.Model;
Line 7:
Line 8: public class VmSelectionModel extends Model {
Line 9: ListModel<Pair<String, VM>> vms;
The use of pairs seems unnecessary to me. You could use strings instead, that
would only hold the name to display, and in VmsSelectionModel keep a
Map<String, Guid> that would return the selected VM GUIDs when flushed.
Line 10:
Line 11: public VmSelectionModel() {
Line 12: setVms(new ListModel<Pair<String, VM>>());
Line 13: }
Line 15: public ListModel<Pair<String, VM>> getVms() {
Line 16: return vms;
Line 17: }
Line 18:
Line 19: public void setVms(ListModel<Pair<String, VM>> vms) {
I think this could be private.
Line 20: this.vms = vms;
Line 21: }
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/configure/scheduling/affinity_groups/VmsSelectionModel.java
Line 13: import org.ovirt.engine.ui.uicompat.Event;
Line 14: import org.ovirt.engine.ui.uicompat.EventArgs;
Line 15: import org.ovirt.engine.ui.uicompat.IEventListener;
Line 16:
Line 17: public class VmsSelectionModel extends ListModel<VmSelectionModel> {
In general this class does pretty much what KeyValueModel does - implement a
unique and exclusive key extension of AddRemoveRowWidget. The differences, as
far as I can see, are that here you don't have value fields, and that the value
backing each line model is different.
I would definitely have this class extend the other, or ideally extend
AddRemoveRowWidget for a specific flavor (ExclusiveKeyAddRemoveRowWidget) which
would be used by these two widgets.
Line 18:
Line 19: public final static Pair<String, VM> SELECT_KEY = new Pair<String,
VM>("select a VM", null); //$NON-NLS-1$
Line 20: public final static Pair<String, VM> NO_KEYS = new Pair<String,
VM>("no available vms", null); //$NON-NLS-1$
//ConstantsManager.getInstance().getConstants().noKeyAvailable();
Line 21:
Line 15: import org.ovirt.engine.ui.uicompat.IEventListener;
Line 16:
Line 17: public class VmsSelectionModel extends ListModel<VmSelectionModel> {
Line 18:
Line 19: public final static Pair<String, VM> SELECT_KEY = new Pair<String,
VM>("select a VM", null); //$NON-NLS-1$
Probably want to externalize this, capitalize the S and possibly add "..." in
the end.
Line 20: public final static Pair<String, VM> NO_KEYS = new Pair<String,
VM>("no available vms", null); //$NON-NLS-1$
//ConstantsManager.getInstance().getConstants().noKeyAvailable();
Line 21:
Line 22: Map<Guid, VM> allVmMap;
Line 23: private final Map<Guid, VM> usedVmMap = new HashMap<Guid, VM>();
Line 16:
Line 17: public class VmsSelectionModel extends ListModel<VmSelectionModel> {
Line 18:
Line 19: public final static Pair<String, VM> SELECT_KEY = new Pair<String,
VM>("select a VM", null); //$NON-NLS-1$
Line 20: public final static Pair<String, VM> NO_KEYS = new Pair<String,
VM>("no available vms", null); //$NON-NLS-1$
//ConstantsManager.getInstance().getConstants().noKeyAvailable();
The constant isn't capitalized and isn't externalized, and I'm not sure I
understand the comment.
Line 21:
Line 22: Map<Guid, VM> allVmMap;
Line 23: private final Map<Guid, VM> usedVmMap = new HashMap<Guid, VM>();
Line 24: boolean disableEvent = false;
Line 98:
Line 99: public List<Pair<String, VM>> getAvailableKeys() {
Line 100: Map<Guid, Pair<String, VM>> map = new HashMap<Guid,
Pair<String, VM>>();
Line 101:
Line 102: if (allVmMap != null) {
It's inefficient that you always recompute the available keys. You could
maintain a collection of available keys instead, and update it when changes
occur, e.g. another key was chosen in one of the line model would lead to the
old key being reinstated and the new key to be removed.
This collection should also be a set and not a list, to make contains() queries
(which happen) efficient.
Line 103: for (VM vm : allVmMap.values()) {
Line 104: if (!usedVmMap.containsKey(vm.getId())) {
Line 105: map.put(vm.getId(), new Pair<String,
VM>(vm.getName(), vm));
Line 106: }
Line 116:
Line 117: return list;
Line 118: }
Line 119:
Line 120: public void updateKeys() {
This method could also be more efficient. You could add a parameter that states
which line model was changed, and only update the rest according to the changes
(rather than clearing usedVmMap and rebuilding everything).
Line 121: if (getItems() != null && usedVmMap != null) {
Line 122: disableEvent = true;
Line 123: usedVmMap.clear();
Line 124: for (VmSelectionModel vmSelectionModel : getItems()) {
Line 146: }
Line 147: return list;
Line 148: }
Line 149:
Line 150: public boolean validate() {
If this does nothing (and seems to not be called anywhere), I would just drop
it. Model's getIsValid() will by default be true and can be called instead (but
again, this model is currently not validated anywhere as far as I can tell). I
would normally add this method only if I knew I had to update whether the model
was valid, and only then return a result.
Line 151: boolean isValid = true;
Line 152:
Line 153: return isValid;
Line 154: }
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/configure/scheduling/affinity_groups/list/AffinityGroupListModel.java
Line 25: import org.ovirt.engine.ui.uicompat.ConstantsManager;
Line 26: import org.ovirt.engine.ui.uicompat.FrontendMultipleActionAsyncResult;
Line 27: import
org.ovirt.engine.ui.uicompat.IFrontendMultipleActionAsyncCallback;
Line 28:
Line 29: public abstract class AffinityGroupListModel<T extends
BusinessEntity<Guid>> extends SearchableListModel<AffinityGroup> {
Smart to only implement once and reuse in both subtabs, I liked it.
Line 30: private UICommand newCommand;
Line 31: private UICommand editCommand;
Line 32: private UICommand removeCommand;
Line 33:
Line 33:
Line 34: private T parentEntity;
Line 35:
Line 36: public AffinityGroupListModel() {
Line 37:
setTitle(ConstantsManager.getInstance().getConstants().affinityGroupsTitle());
Pretty sure this title isn't used for anything, you can drop it and the
associated constant.
Line 38: setHashName("affinity_groups"); // $//$NON-NLS-1$
Line 39: setAvailableInModes(ApplicationMode.VirtOnly);
Line 40:
Line 41: setNewCommand(new UICommand("New", this)); //$NON-NLS-1$
Line 35:
Line 36: public AffinityGroupListModel() {
Line 37:
setTitle(ConstantsManager.getInstance().getConstants().affinityGroupsTitle());
Line 38: setHashName("affinity_groups"); // $//$NON-NLS-1$
Line 39: setAvailableInModes(ApplicationMode.VirtOnly);
I genuinely don't know, but is this required if you're already hiding the
subtab under cluster when it doesn't support Virt? Because if running only
Gluster, the VMs tab should probably be invisible anyway, and the cluster tab
is taken care of, so...
Also, if it is needed, this should probably be either Virt or Both, shouldn't
it?
Line 40:
Line 41: setNewCommand(new UICommand("New", this)); //$NON-NLS-1$
Line 42: setEditCommand(new UICommand("Edit", this)); //$NON-NLS-1$
Line 43: setRemoveCommand(new UICommand("Remove", this)); //$NON-NLS-1$
Line 66: getRemoveCommand().setIsExecutionAllowed(hasSelectedItems);
Line 67: }
Line 68:
Line 69: @Override
Line 70: protected void syncSearch() {
Is there any good reason to implement this rather than just call
super.syncSearch() with getQueryType() as argument?
Line 71: if (getParentEntity() != null) {
Line 72: super.syncSearch();
Line 73: AsyncQuery asyncQuery = new AsyncQuery(this, new
INewAsyncCallback() {
Line 74:
Line 76: public void onSuccess(Object model, Object
returnValue) {
Line 77: AffinityGroupListModel<?> affinityGroupListModel =
(AffinityGroupListModel<?>) model;
Line 78: ArrayList<AffinityGroup> list =
Line 79: (ArrayList<AffinityGroup>)
((VdcQueryReturnValue) returnValue).getReturnValue();
Line 80: affinityGroupListModel.setItems(list);
If this implementation remains, you could lose the variable and call
AffinityGroupListModel.this.setItems().
Line 81: }
Line 82: });
Line 83: VdcQueryParametersBase parameters = new
IdQueryParameters(getParentEntity().getId());
Line 84: parameters.setRefresh(getIsQueryFirstTime());
Line 86: setIsQueryFirstTime(false);
Line 87: }
Line 88: }
Line 89:
Line 90: protected abstract VdcQueryType getQueryType();
This could be replaced by setting a member from the concrete subtab models
rather than overriding an abstract method, since the query type is only set
once and doesn't depend on the state (there's no computation involved).
But it's a matter of style...
Line 91:
Line 92: protected abstract ClusterResolver getClusterResolver();
Line 93:
Line 94: public UICommand getNewCommand() {
Line 88: }
Line 89:
Line 90: protected abstract VdcQueryType getQueryType();
Line 91:
Line 92: protected abstract ClusterResolver getClusterResolver();
Similar comment.
Line 93:
Line 94: public UICommand getNewCommand() {
Line 95: return newCommand;
Line 96: }
Line 94: public UICommand getNewCommand() {
Line 95: return newCommand;
Line 96: }
Line 97:
Line 98: public void setNewCommand(UICommand newCommand) {
This and other setters don't have to be public, could be private.
Line 99: this.newCommand = newCommand;
Line 100: }
Line 101:
Line 102: @Override
Line 129: return;
Line 130: }
Line 131:
Line 132: AffinityGroupModel model = getNewAffinityGroupModel();
Line 133: model.init();
I don't think there's any reason to separate this from the constructor. It
could be a different method within AffinityGroupModel, but it could be private
and called from within the constructor.
Line 134: setWindow(model);
Line 135: }
Line 136:
Line 137: protected AffinityGroupModel getNewAffinityGroupModel() {
Line 137: protected AffinityGroupModel getNewAffinityGroupModel() {
Line 138: return new NewAffinityGroupModel(this, getClusterResolver()) {
Line 139:
Line 140: @Override
Line 141: protected AffinityGroup getAffinityGroup() {
This is a strange design choice in my opinion, was there a special reason you
went for this? It seems to me that instead of having the abstract method
getAffinityGroup(), you could more simply pass the affinityGroup member in the
constructor of AffinityGroupModel (as you do for EditAffinityGroupModel). This
is something that only needs to be set once, not called repeatedly (similar to
comments on the abstract methods in this class).
Line 142: return new AffinityGroup();
Line 143: }
Line 144: };
Line 145: }
Line 151: AffinityGroup affinityGroup = getSelectedItem();
Line 152: if (affinityGroup == null) {
Line 153: return;
Line 154: }
Line 155: sortVms(affinityGroup);
Generally you don't sort the VMs, and in the one case where you do - you don't
really sort them either, but just move one to the top of the list. So firstly,
consider dropping this method, making edit() protected and add the logic only
in VmAffinityGroupListModel. Secondly, you could drop the comparator there and
just iterate over the items to find the specific ID and move it to the top of
the list, the comparator thing isn't necessary.
Line 156: AffinityGroupModel model = new
EditAffinityGroupModel(affinityGroup, this, getClusterResolver());
Line 157: model.init();
Line 158: setWindow(model);
Line 159: }
Line 161: protected void sortVms(AffinityGroup affinityGroup) {
Line 162:
Line 163: }
Line 164:
Line 165: private void remove() {
It's a matter of style, but consider putting this logic in a
RemoveAffinityGroupModel. Encapsulation is generally good, and it would make
this class simpler.
Line 166: if (getWindow() != null) {
Line 167: return;
Line 168: }
Line 169:
Line 187: command.setIsCancel(true);
Line 188: model.getCommands().add(command);
Line 189: }
Line 190:
Line 191: private void onRemove() {
This too, of course, if you decide to create that model.
Line 192: ConfirmationModel model = (ConfirmationModel)
getConfirmWindow();
Line 193:
Line 194: if (model.getProgress() != null) {
Line 195: return;
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/configure/scheduling/affinity_groups/list/ClusterResolver.java
Line 4:
Line 5: /**
Line 6: * used to fetch cluster info for affinity groups models VM and cluster
Line 7: */
Line 8: public interface ClusterResolver {
And this wouldn't be needed if these things are just set from the concrete
subtab models.
Line 9: Guid getClusterId();
Line 10:
Line 11: String getClusterName();
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/configure/scheduling/affinity_groups/list/VmAffinityGroupListModel.java
Line 49: }
Line 50:
Line 51: @Override
Line 52: protected void sortVms(AffinityGroup affinityGroup) {
Line 53: Collections.sort(affinityGroup.getEntityIds(), new
Comparator<Guid>() {
As commented earlier, I think a comparator for this logic is an overkill :)
Line 54:
Line 55: @Override
Line 56: public int compare(Guid o1, Guid o2) {
Line 57: if (o1.equals(getParentEntity().getId())) {
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/configure/scheduling/affinity_groups/model/AffinityGroupModel.java
Line 35: private final ClusterResolver clusterResolver;
Line 36:
Line 37: private EntityModel<String> name;
Line 38: private EntityModel<String> description;
Line 39: private ListModel<AffinityPolarity> polarity;
AffinityPolarity is quite an overkill for a boolean. I also don't see it ever
have more values than positive or negative. Any reason why this enum exists?
Line 40: private ListModel<AffinityEnforcementType> enforcementType;
Line 41: private VmsSelectionModel vmsSelectionModel;
Line 42:
Line 43:
Line 36:
Line 37: private EntityModel<String> name;
Line 38: private EntityModel<String> description;
Line 39: private ListModel<AffinityPolarity> polarity;
Line 40: private ListModel<AffinityEnforcementType> enforcementType;
Similar comment... if this is even extended, I suspect it would become an int
value rather than an enum. As an enum, I don't see that it would be extended
much (what would be the next value? Medium? Somewhat Hard?).
Line 41: private VmsSelectionModel vmsSelectionModel;
Line 42:
Line 43:
Line 44: public AffinityGroupModel(ListModel<?> sourceListModel,
ClusterResolver clusterResolver) {
Line 55: setVmsSelectionModel(new VmsSelectionModel());
Line 56: startProgress(null);
Line 57:
Line 58: //TODO: should be by cluster id and remove clusterName method
from resolver.
Line 59: AsyncDataProvider.getVmListByClusterName(new AsyncQuery(this,
new INewAsyncCallback() {
I would be careful about putting a query in the constructor, I think if it
fails then the dialog might not be open yet and the error might not show.
Line 60:
Line 61: @Override
Line 62: public void onSuccess(Object model, Object returnValue) {
Line 63: ArrayList<VM> vmList = (ArrayList<VM>) returnValue;
Line 70: }
Line 71:
Line 72: public abstract void init();
Line 73:
Line 74: protected abstract AffinityGroup getAffinityGroup();
This could be just a member that's set from the concrete tab models.
Line 75:
Line 76: protected abstract VdcActionType getSaveActionType();
Line 77:
Line 78: protected void addCommands() {
Line 72: public abstract void init();
Line 73:
Line 74: protected abstract AffinityGroup getAffinityGroup();
Line 75:
Line 76: protected abstract VdcActionType getSaveActionType();
This could be just a member that's set in the concrete subclasses.
Line 77:
Line 78: protected void addCommands() {
Line 79: UICommand command = new UICommand("OnSave", this);
//$NON-NLS-1$
Line 80:
command.setTitle(ConstantsManager.getInstance().getConstants().ok());
Line 89: public EntityModel<String> getName() {
Line 90: return name;
Line 91: }
Line 92:
Line 93: public void setName(EntityModel<String> name) {
No reason for setters to be public.
Line 94: this.name = name;
Line 95: }
Line 96:
Line 97: public EntityModel<String> getDescription() {
Line 135: if (!validate()) {
Line 136: return;
Line 137: }
Line 138:
Line 139: if (getProgress() != null) {
Is this something that could actually happen?
Line 140: return;
Line 141: }
Line 142: AffinityGroup group = getAffinityGroup();
Line 143: group.setName(getName().getEntity());
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/UnitVmModel.java
Line 1817: updateWatchdogModels(osType);
Line 1818: }
Line 1819:
Line 1820: private void updateWatchdogModels() {
Line 1821: updateWatchdogModels(getOSType().getSelectedItem());
Are these changes relevant to this patch?
Line 1822: }
Line 1823:
Line 1824: private void updateWatchdogModels(Integer osType) {
Line 1825: VDSGroup cluster = getSelectedCluster();
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java
Line 368: }
Line 369:
Line 370: private ErrorPopupManager errorPopupManager;
Line 371:
Line 372: private VmAffinityGroupListModel affinityGroupListModel;
Don't see a reason to make this a member with setter and getter, you could just
instantiate it inline - list.add(new VmAffinityGroupListModel()).
Line 373:
Line 374: public VmAffinityGroupListModel getAffinityGroupListModel() {
Line 375: return affinityGroupListModel;
Line 376: }
....................................................
File
frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java
Line 2096: @DefaultStringValue("Not available when Templates are not
configured.")
Line 2097: String notAvailableWithNoTemplates();
Line 2098:
Line 2099: @DefaultStringValue("Affinity Groups")
Line 2100: String affinityGroupsTitle();
This can be removed, following comment in AffinityGroupListModel.
Line 2101:
Line 2102: @DefaultStringValue("New Affinity Group")
Line 2103: String newAffinityGroupsTitle();
Line 2104:
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationConstants.java
Line 3455: @DefaultStringValue("Name")
Line 3456: String affinityGroupNameLabel();
Line 3457:
Line 3458: @DefaultStringValue("Polarity")
Line 3459: String affinityGroupPolarityLabel();
Consider reusing polarityAffinityGroup(), if one of them changes then the other
should probably change too.
Line 3460:
Line 3461: @DefaultStringValue("Enforcing")
Line 3462: String affinityGroupEnforceTypeLabel();
Line 3463:
Line 3458: @DefaultStringValue("Polarity")
Line 3459: String affinityGroupPolarityLabel();
Line 3460:
Line 3461: @DefaultStringValue("Enforcing")
Line 3462: String affinityGroupEnforceTypeLabel();
Same.
Line 3463:
Line 3464: @DefaultStringValue("Description")
Line 3465: String affinityDescriptionLabel();
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/scheduling/AffinityGroupPopupView.ui.xml
Line 1: <?xml version="1.0" encoding="UTF-8"?>
Line 2: <!DOCTYPE ui:UiBinder SYSTEM "http://dl.google.com/gwt/DTD/xhtml.ent";>
Line 3: <ui:UiBinder
Trailing whitespace.
Line 4: xmlns:ui="urn:ui:com.google.gwt.uibinder"
Line 5: xmlns:g="urn:import:com.google.gwt.user.client.ui"
Line 6: xmlns:d="urn:import:org.ovirt.engine.ui.common.widget.dialog"
Line 7: xmlns:e="urn:import:org.ovirt.engine.ui.common.widget.editor"
Line 1: <?xml version="1.0" encoding="UTF-8"?>
Line 2: <!DOCTYPE ui:UiBinder SYSTEM "http://dl.google.com/gwt/DTD/xhtml.ent";>
Line 3: <ui:UiBinder
Line 4: xmlns:ui="urn:ui:com.google.gwt.uibinder"
Line 5: xmlns:g="urn:import:com.google.gwt.user.client.ui"
Trailing whitespace.
Line 6: xmlns:d="urn:import:org.ovirt.engine.ui.common.widget.dialog"
Line 7: xmlns:e="urn:import:org.ovirt.engine.ui.common.widget.editor"
Line 8:
xmlns:ge="urn:import:org.ovirt.engine.ui.common.widget.editor.generic"
Line 9:
xmlns:ag="urn:import:org.ovirt.engine.ui.webadmin.section.main.view.popup.scheduling.widgets">
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/scheduling/widgets/AddRemoveVmWidget.java
Line 16: WidgetUiBinder uiBinder = GWT.create(WidgetUiBinder.class);
Line 17: }
Line 18:
Line 19: private VmsSelectionModel model;
Line 20: private final LinkedList<VmRowWidget> widgets = new
LinkedList<VmRowWidget>();
If enabled is dropped - is widgets still required? I don't think flush() is
called.
Line 21: private boolean enabled = true;
Line 22:
Line 23: AddRemoveVmWidget() {
Line 24: initWidget(WidgetUiBinder.uiBinder.createAndBindUi(this));
Line 17: }
Line 18:
Line 19: private VmsSelectionModel model;
Line 20: private final LinkedList<VmRowWidget> widgets = new
LinkedList<VmRowWidget>();
Line 21: private boolean enabled = true;
I don't see that anywhere you need this widget this disabled - you can drop
this member and related code.
Line 22:
Line 23: AddRemoveVmWidget() {
Line 24: initWidget(WidgetUiBinder.uiBinder.createAndBindUi(this));
Line 25: }
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/scheduling/widgets/AddRemoveVmWidget.ui.xml
Line 1: <?xml version="1.0" encoding="UTF-8"?>
Line 2: <!DOCTYPE ui:UiBinder SYSTEM "http://dl.google.com/gwt/DTD/xhtml.ent";>
Line 3: <ui:UiBinder xmlns:ui="urn:ui:com.google.gwt.uibinder"
Trailing whitespace.
Line 4: xmlns:g="urn:import:com.google.gwt.user.client.ui"
Line 5: xmlns:e="urn:import:org.ovirt.engine.ui.common.widget.editor">
Line 6:
Line 7: <ui:style
type="org.ovirt.engine.ui.common.widget.AddRemoveRowWidget.WidgetStyle">
Line 1: <?xml version="1.0" encoding="UTF-8"?>
Line 2: <!DOCTYPE ui:UiBinder SYSTEM "http://dl.google.com/gwt/DTD/xhtml.ent";>
Line 3: <ui:UiBinder xmlns:ui="urn:ui:com.google.gwt.uibinder"
Line 4: xmlns:g="urn:import:com.google.gwt.user.client.ui"
Line 5: xmlns:e="urn:import:org.ovirt.engine.ui.common.widget.editor">
This doesn't seem to be used.
Line 6:
Line 7: <ui:style
type="org.ovirt.engine.ui.common.widget.AddRemoveRowWidget.WidgetStyle">
Line 8: .margin {
Line 9: margin-left: 20px;
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/scheduling/widgets/VmRowWidget.java
Line 22: import com.google.gwt.user.client.ui.Composite;
Line 23: import com.google.gwt.user.client.ui.HasEnabled;
Line 24: import com.google.gwt.user.client.ui.Widget;
Line 25:
Line 26: public class VmRowWidget extends Composite implements
HasValueChangeHandlers<VmSelectionModel>, HasEditorDriver<VmSelectionModel>,
HasEnabled {
Don't think you're using the HasEnabled methods anywhere, so this interface and
associated methods and member can be removed.
Line 27:
Line 28: interface WidgetUiBinder extends UiBinder<Widget, VmRowWidget> {
Line 29: WidgetUiBinder uiBinder = GWT.create(WidgetUiBinder.class);
Line 30: }
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/scheduling/widgets/VmRowWidget.ui.xml
Line 1: <?xml version="1.0" encoding="UTF-8"?>
Line 2: <!DOCTYPE ui:UiBinder SYSTEM "http://dl.google.com/gwt/DTD/xhtml.ent";>
Line 3: <ui:UiBinder xmlns:ui="urn:ui:com.google.gwt.uibinder"
Trailing whitespace.
Line 4: xmlns:g="urn:import:com.google.gwt.user.client.ui"
Line 5: xmlns:e="urn:import:org.ovirt.engine.ui.common.widget.editor">
Line 6:
Line 7: <ui:with field='resources'
type='org.ovirt.engine.ui.common.CommonApplicationResources' />
Line 3: <ui:UiBinder xmlns:ui="urn:ui:com.google.gwt.uibinder"
Line 4: xmlns:g="urn:import:com.google.gwt.user.client.ui"
Line 5: xmlns:e="urn:import:org.ovirt.engine.ui.common.widget.editor">
Line 6:
Line 7: <ui:with field='resources'
type='org.ovirt.engine.ui.common.CommonApplicationResources' />
Don't think this is used.
Line 8:
Line 9: <ui:style
type="org.ovirt.engine.ui.webadmin.section.main.view.popup.scheduling.widgets.VmRowWidget.WidgetStyle">
Line 10: .fieldWidth {
Line 11: width: 180px;
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/AbstractSubTabAffinityGroupsView.java
Line 85: }
Line 86: });
Line 87: }
Line 88:
Line 89: protected abstract void addMembersColumn(ApplicationConstants
constants);
The implementation in the two subclasses is practically identical. I would put
it here, and in the case of the VM subtab I would take out the one value from
the list beforehand.
Line 90:
Line 91: protected String joinMembers(List<String> strings, String
separator, String skipString, ApplicationConstants constants) {
Line 92: if (strings == null || strings.isEmpty()) {
Line 93: return constants.noMembersAffinityGroup();
Line 87: }
Line 88:
Line 89: protected abstract void addMembersColumn(ApplicationConstants
constants);
Line 90:
Line 91: protected String joinMembers(List<String> strings, String
separator, String skipString, ApplicationConstants constants) {
I haven't looked at the backend side so I have no idea, but this sort of
serialization is possibly also needed when persisting to the DB. If so, I would
put it in one central place and not here.
Line 92: if (strings == null || strings.isEmpty()) {
Line 93: return constants.noMembersAffinityGroup();
Line 94: }
Line 95: StringBuffer result = new StringBuffer();
--
To view, visit http://gerrit.ovirt.org/22716
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe5d6f6efe2b13297b653e1622a26c7b2b44cba8
Gerrit-PatchSet: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Lior Vernia <lver...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
