Lior Vernia has posted comments on this change.
Change subject: webadmin: add profiles instead of networks to vnic and vm
dialogs
......................................................................
Patch Set 19: (18 inline comments)
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/CommonApplicationMessages.java
Line 104:
Line 105: @DefaultMessage("default: {0}")
Line 106: String defaultTimeZoneCaption(String currentDefault);
Line 107:
Line 108: @DefaultMessage("VM has {0} Nics. Assign them to the Profiles.")
I think you should drop the "the".
Line 109: String assignNicsToProfilesPlural(int numOfNics);
Line 110:
Line 111: @DefaultMessage("VM has 1 Nic. Assign it to a Profile.")
Line 112: String assignNicsToProfilesSingular();
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/network/LogicalNetworkEditor.java
Line 18: import com.google.gwt.user.client.TakesValue;
Line 19: import com.google.gwt.user.client.ui.Composite;
Line 20: import com.google.gwt.user.client.ui.Widget;
Line 21:
Line 22: public class LogicalNetworkEditor extends Composite implements
IsEditor<TakesValueEditor<VnicInstanceType>>, TakesValue<VnicInstanceType>,
HasElementId {
I think it should implement TakesConstrainedValue rather than TakesValue, as it
is a widget that offers a preset choice of values. Then if the backing model's
items change, the widget will be updated as well.
Line 23:
Line 24: interface WidgetUiBinder extends UiBinder<Widget,
LogicalNetworkEditor> {
Line 25: WidgetUiBinder uiBinder = GWT.create(WidgetUiBinder.class);
Line 26: }
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/networkinterface/NetworkInterfacePopupWidget.java
Line 65:
Line 66: @UiField(provided = true)
Line 67: @Path(value = "profile.selectedItem")
Line 68: @WithElementId("profile")
Line 69: public ListModelTypeAheadListBoxEditor<Object> profileEditor;
Shouldn't this just be something like LogicalNetworkEditor?
Line 70:
Line 71: @UiField(provided = true)
Line 72: @Path("nicType.selectedItem")
Line 73: @WithElementId("nicType")
Line 168: }
Line 169:
Line 170: @SuppressWarnings({ "rawtypes", "unchecked" })
Line 171: private void initManualWidgets() {
Line 172: profileEditor = new ListModelTypeAheadListBoxEditor<Object>(
Shouldn't this just be something like new LogicalNetworkEditor()? There seem to
be a lot of shared code.
Line 173: new
ListModelTypeAheadListBoxEditor.NullSafeSuggestBoxRenderer<Object>() {
Line 174:
Line 175: @Override
Line 176: public String getReplacementStringNullSafe(Object
data) {
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/EditProfileBehavior.java
Line 10:
Line 11: public class EditProfileBehavior extends ProfileBehavior {
Line 12:
Line 13: @Override
Line 14: public void initSelectedProfile(ListModel profileList,
VmNetworkInterface networkInterface) {
This method doesn't seem well-designed to me. There should just be a map that
returns a VnicProfileView by name instead of iterating through all of them
every time it's called. I know this kind of code was here before the current
feature, but this is a good opportunity to fix it.
Line 15: List<VnicProfileView> profiles = (List<VnicProfileView>)
profileList.getItems();
Line 16: profiles = profiles == null ? new ArrayList<VnicProfileView>()
: profiles;
Line 17:
Line 18: if (networkInterface.getVnicProfileId() == null) {
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/NewProfileBehavior.java
Line 18: public void initSelectedProfile(ListModel profileList,
VmNetworkInterface networkInterface) {
Line 19: List<VnicProfileView> profiles = (List<VnicProfileView>)
profileList.getItems();
Line 20: profiles = profiles == null ? new ArrayList<VnicProfileView>()
: profiles;
Line 21: for (VnicProfileView profile : profiles) {
Line 22: if (ENGINE_NETWORK_NAME != null && profile != null &&
profile.getNetworkName() != null
1. Again, if profiles were a map the implementation would be shorter and
smarter.
2. If not, I think the check whether profile.getNetworkName() != null isn't
necessary.
Line 23: &&
ENGINE_NETWORK_NAME.equals(profile.getNetworkName())) {
Line 24: profileList.setSelectedItem(profile);
Line 25: return;
Line 26: }
Line 19: List<VnicProfileView> profiles = (List<VnicProfileView>)
profileList.getItems();
Line 20: profiles = profiles == null ? new ArrayList<VnicProfileView>()
: profiles;
Line 21: for (VnicProfileView profile : profiles) {
Line 22: if (ENGINE_NETWORK_NAME != null && profile != null &&
profile.getNetworkName() != null
Line 23: &&
ENGINE_NETWORK_NAME.equals(profile.getNetworkName())) {
And we might want this check to be case-insensitive? So if this is turned into
a map, insert the String values after toLowerCase() operations.
Line 24: profileList.setSelectedItem(profile);
Line 25: return;
Line 26: }
Line 27: }
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ProfileBehavior.java
Line 28: profilesQuery.converterCallback = new IAsyncConverter() {
Line 29:
Line 30: @Override
Line 31: public Object Convert(Object returnValue, AsyncQuery
asyncQuery) {
Line 32: ArrayList<Network> clusterNetworks =
(ArrayList<Network>) asyncQuery.getModel();
This should be List rather than ArrayList.
Line 33:
Line 34: ProfileBehavior.this.clusterNetworks = clusterNetworks;
Line 35:
Line 36: ArrayList<VnicProfileView> vnicProfiles = new
ArrayList<VnicProfileView>();
Line 30: @Override
Line 31: public Object Convert(Object returnValue, AsyncQuery
asyncQuery) {
Line 32: ArrayList<Network> clusterNetworks =
(ArrayList<Network>) asyncQuery.getModel();
Line 33:
Line 34: ProfileBehavior.this.clusterNetworks = clusterNetworks;
Here is where the map would be constructed.
Line 35:
Line 36: ArrayList<VnicProfileView> vnicProfiles = new
ArrayList<VnicProfileView>();
Line 37:
Line 38: if (returnValue == null) {
Line 38: if (returnValue == null) {
Line 39: return vnicProfiles;
Line 40: }
Line 41:
Line 42: for (VnicProfileView vnicProfile :
(ArrayList<VnicProfileView>) returnValue) {
If you're only iterating over returnValue, you might as well cast it to
Iterable rather than ArrayList (or List, or Collection) and avoid possible
breakage in the future.
Line 43: Network network =
findNetworkById(vnicProfile.getNetworkId());
Line 44: if (network != null && network.isVmNetwork()) {
Line 45: vnicProfiles.add(vnicProfile);
Line 46: }
Line 39: return vnicProfiles;
Line 40: }
Line 41:
Line 42: for (VnicProfileView vnicProfile :
(ArrayList<VnicProfileView>) returnValue) {
Line 43: Network network =
findNetworkById(vnicProfile.getNetworkId());
And this would be just get().
Line 44: if (network != null && network.isVmNetwork()) {
Line 45: vnicProfiles.add(vnicProfile);
Line 46: }
Line 47:
Line 50: if (hotUpdateSupported) {
Line 51: vnicProfiles.add(null);
Line 52: }
Line 53:
Line 54: Collections.sort(vnicProfiles, new
Comparator<VnicProfileView>() {
You would have told me to put this as a named comparator in Linq! :)
Line 55:
Line 56: private LexoNumericComparator lexoNumeric = new
LexoNumericComparator();
Line 57:
Line 58: @Override
Line 89: }
Line 90:
Line 91: public abstract void initSelectedProfile(ListModel profileLists,
VmNetworkInterface networkInterface);
Line 92:
Line 93: public Network findNetworkById(Guid networkId) {
This kind of code could again be replaced by a map.
Line 94: for (Network network : clusterNetworks) {
Line 95: if (network.getId().equals(networkId)) {
Line 96: return network;
Line 97: }
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmInterfaceCreatingManager.java
Line 44:
Line 45: /**
Line 46: * Used mainly when the VM is created from the template. If the Vm
has been created but without NICs, new
Line 47: * ones are created according to the vnicInstanceTypes. If the VM
is created with nics - e.g. by copying from template update they're profiles
Line 48: * as edited by the user (again, according to the
vnicInstanceTypes).
This should be formatted. Also, they're --> their.
Line 49: *
Line 50: * @param vmId The ID of the VM
Line 51: * @param vnicInstanceTypes list of nics as edited in the window
Line 52: */
Line 65: // there are some vnics created - update according
to the setup in the window
Line 66: ArrayList<VdcActionParametersBase> parameters =
new ArrayList<VdcActionParametersBase>();
Line 67:
Line 68: for (VmNetworkInterface created : createdNics) {
Line 69: for (VnicInstanceType edited :
vnicInstanceTypes) {
The nested loops run in O(n*m) time. If you used a map it could be reduced to
O(n), where elsewhere you it would cost you O(m) to construct the edited
VnicInstance map, so O(n+m) in total.
Line 70: // can not use getId() because they have
different IDs - one is already created, one is not
Line 71: // yet
Line 72: boolean sameNic =
edited.getNetworkInterface().getName().equals(created.getName());
Line 73:
Line 68: for (VmNetworkInterface created : createdNics) {
Line 69: for (VnicInstanceType edited :
vnicInstanceTypes) {
Line 70: // can not use getId() because they have
different IDs - one is already created, one is not
Line 71: // yet
Line 72: boolean sameNic =
edited.getNetworkInterface().getName().equals(created.getName());
This should probably have been equalsIgnoreCase(), as the widget enables users
to type in arbitrary strings. But it's not necessary if you use a map.
Line 73:
Line 74: boolean bothProfilesNull =
created.getVnicProfileId() == null &&
edited.getNetworkInterface().getVnicProfileId() == null;
Line 75:
Line 76: boolean sameProfiles =
created.getVnicProfileId() != null &&
created.getVnicProfileId().equals(edited.getNetworkInterface().getVnicProfileId());
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VnicInstanceType.java
Line 2:
Line 3: import
org.ovirt.engine.core.common.businessentities.network.VmNetworkInterface;
Line 4: import org.ovirt.engine.ui.uicommonweb.models.ListModel;
Line 5:
Line 6: public class VnicInstanceType extends ListModel {
I'm not really sure what this class represents, so another name might be more
suitable. Is it supposed to be a VNIC with its possible profiles?
Line 7:
Line 8: private VmNetworkInterface networkInterface;
Line 9:
Line 10: public VnicInstanceType(VmNetworkInterface networkInterface) {
....................................................
File
frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIMessages.java
Line 275: // Vnic
Line 276: @DefaultMessage("Hot Plug is not supported on cluster version
{0}")
Line 277: String hotPlugNotSupported(String clusterVersion);
Line 278:
Line 279: @DefaultMessage("Updating 'profile' on a running virtual machine
while the NIC is plugged is not supported on cluster version {0}")
I think some constants in UIConstants need to be refined in the same manner,
off the top of my head the one having to do with the changeability for an
external network.
Line 280: String hotProfileUpdateNotSupported(String clusterVersion);
Line 281:
Line 282: @DefaultMessage("Custom({0})")
Line 283: String customSpmPriority(int priority);
--
To view, visit http://gerrit.ovirt.org/17322
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I00ea69609acf8a8bfeb0a5357489141f726b6323
Gerrit-PatchSet: 19
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alona Kaplan <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Livnat Peer <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
