Lior Vernia has posted comments on this change.
Change subject: webadmin: add profiles tab to add/edit network dialogs
......................................................................
Patch Set 11: Code-Review+1
(9 comments)
Please pay attention to comments, there's one bug among them.
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/EditNetworkModel.java
Line 133: @Override
Line 134: protected void performProfilesActions(final Guid networkGuid) {
Line 135:
Line 136: List<VnicProfileModel> profileModels =
(List<VnicProfileModel>) getProfiles().getItems();
Line 137: List<VnicProfileModel> profileModelsToRemove = new
ArrayList<VnicProfileModel>(profileModels);
I think you meant originalProfileModels and not profileModels, at the moment
profileModelsToRemove will always end up being an empty ArrayList.
Line 138: profileModelsToRemove.removeAll(profileModels);
Line 139:
Line 140: final List<VnicProfileModel> profileModelsToAdd = new
ArrayList<VnicProfileModel>();
Line 141:
Line 138: profileModelsToRemove.removeAll(profileModels);
Line 139:
Line 140: final List<VnicProfileModel> profileModelsToAdd = new
ArrayList<VnicProfileModel>();
Line 141:
Line 142: for (VnicProfileModel profileModel : profileModels) {
You could do this more elegantly using Linq.where().
Line 143: if (profileModel instanceof NewVnicProfileModel) {
Line 144: profileModelsToAdd.add(profileModel);
Line 145: }
Line 146: }
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/NewNetworkModel.java
Line 103: @Override
Line 104: protected void initProfiles() {
Line 105: List<VnicProfileModel> profiles = new
LinkedList<VnicProfileModel>();
Line 106:
Line 107: profiles.add(new NewVnicProfileModel(getSourceListModel(),
getSelectedDc().getcompatibility_version(), false));
I've seen that a default profile named the same as the network is always
created. If this is intentional, rather than a bug, then I think it should be
reflected in the initial list of profiles.
Line 108: getProfiles().setItems(profiles);
Line 109: }
Line 110:
Line 111: @Override
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/profiles/VnicProfileModel.java
Line 41: private final EntityModel sourceModel;
Line 42: private final Version dcCompatibilityVersion;
Line 43: private final boolean customPropertiesSupported;
Line 44: private ListModel network;
Line 45: private VnicProfile vnicProfile = null;
I don't actually care, but null is the default value anyway.
Line 46: private boolean customPropertiesVisible;
Line 47:
Line 48: public EntityModel getName()
Line 49: {
Line 42: private final Version dcCompatibilityVersion;
Line 43: private final boolean customPropertiesSupported;
Line 44: private ListModel network;
Line 45: private VnicProfile vnicProfile = null;
Line 46: private boolean customPropertiesVisible;
Consider getting rid of this boolean and encoding the same information in the
customPropertySheet's isAvailable property.
Line 47:
Line 48: public EntityModel getName()
Line 49: {
Line 50: return name;
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationConstants.java
Line 183:
Line 184: @DefaultStringValue("Export")
Line 185: String exportLabel();
Line 186:
Line 187: @DefaultStringValue("vNic Profiles")
vNIC or VNIC, as you decide for the other instances.
Line 188: String profilesLabel();
Line 189:
Line 190: @DefaultStringValue("Create on external provider")
Line 191: String exportCheckboxLabel();
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/vnicProfile/VnicProfilesEditor.java
Line 87:
Line 88: final HorizontalPanel profilePanel = new HorizontalPanel();
Line 89:
Line 90: PushButton addButton = new PushButton(new
Image(resources.increaseIcon()));
Line 91: PushButton remvoeButton = new PushButton(new
Image(resources.decreaseIcon()));
Typo, should be removeButton.
Line 92: addButton.addStyleName(style.addButtonStyle());
Line 93: remvoeButton.addStyleName(style.removeButtonStyle());
Line 94: profilePanel.add(vnicProfileWidget);
Line 95: profilePanel.add(addButton);
Line 92: addButton.addStyleName(style.addButtonStyle());
Line 93: remvoeButton.addStyleName(style.removeButtonStyle());
Line 94: profilePanel.add(vnicProfileWidget);
Line 95: profilePanel.add(addButton);
Line 96: profilePanel.add(remvoeButton);
Can't this last paragraph be achieved by implementation in
VnicProfileWidget.ui.xml?
Line 97:
Line 98: addButton.addClickHandler(new ClickHandler() {
Line 99: @Override
Line 100: public void onClick(ClickEvent event) {
Line 94: profilePanel.add(vnicProfileWidget);
Line 95: profilePanel.add(addButton);
Line 96: profilePanel.add(remvoeButton);
Line 97:
Line 98: addButton.addClickHandler(new ClickHandler() {
Isn't this behavior, and that of the remove button as well, generic for this
type of widget? Isn't this already implemented somewhere in a reusable manner?
If not, I think it should be implemented here in a more reusable manner, and
not just for VNICs.
Line 99: @Override
Line 100: public void onClick(ClickEvent event) {
Line 101: List models = (List<VnicProfileModel>)
getValue().getItems();
Line 102: VnicProfileModel existingProfileModel =
(VnicProfileModel) value;
--
To view, visit http://gerrit.ovirt.org/17499
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1b88f0458862e136e5ff035883cb044ef1e896b5
Gerrit-PatchSet: 11
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alona Kaplan <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Lior Vernia <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
