Alona Kaplan has posted comments on this change.
Change subject: webadmin, userportal: logical network editor
......................................................................
Patch Set 8: (13 inline comments)
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ExistingVmModelBehavior.java
Line 245: List<VmNetworkInterface> nics =
(List<VmNetworkInterface>) result;
Line 246: initNetworkInterfaces(networkBehavior, nics);
Line 247: }
Line 248: };
Line 249: AsyncDataProvider.getVmNicList(getVmNicsQuery, vm.getId());
I"m not sure why do you need to get the nics from the backend again. Changing
the dc/cluster shouldn't affect the nics. I think you can call the query just
once and not on each dc/cluster change.
Line 250: }
Line 251:
Line 252: @Override
Line 253: protected void changeDefualtHost()
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/NetworkBehavior.java
Line 19:
Line 20: query.converterCallback = new IAsyncConverter() {
Line 21:
Line 22: @Override
Line 23: public Object Convert(Object returnValue, AsyncQuery
asyncQuery) {
nice:)
Line 24: ArrayList<Network> networks = new ArrayList<Network>();
Line 25: for (Network a : (ArrayList<Network>) returnValue) {
Line 26: if (a.isVmNetwork()) {
Line 27: networks.add(a);
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/NetworkFrontendActionAsyncCallbacks.java
Line 5: import org.ovirt.engine.core.compat.Guid;
Line 6: import org.ovirt.engine.ui.uicompat.FrontendActionAsyncResult;
Line 7: import org.ovirt.engine.ui.uicompat.IFrontendActionAsyncCallback;
Line 8:
Line 9: public class NetworkFrontendActionAsyncCallbacks {
This class is relevant just for queries that are done to init the unit vm model.
Please change its name to reflect it.
Also, I think this whole class is redundant and makes the code very confusing.
It is very hard to understand a code line that looks like this-
Frontend.RunAction(VdcActionType.AddVmFromTemplate, param, new
NetworkFrontendActionAsyncCallbacks.NetworkCreateOrUpdateFrontendActionAsyncCallback(unitVmModel,
defaultNetworkCreatingManager), this);
Please consider removing this class and moving its logic to
VmNetworkCreatingManager.
VmNetworkCreatingManager can implement IFrontendActionAsyncCallback.
It can get an action type as a parameter and call the relevant method according
to the type.
(Also, I think it is more readable to have a different file for each class and
not having a class of classes. But if you"ll move all the code to the
VmNetworkCreatingManager and remove this class you can ignore this comment :) ).
Line 10:
Line 11: public static abstract class
BaseNetworkFrontendActionAsyncCallback implements IFrontendActionAsyncCallback {
Line 12:
Line 13: private final UnitVmModel unitVmModel;
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/NewVmModelBehavior.java
Line 199: }
Line 200:
Line 201: List<VmNetworkInterface> nics =
Line 202: (List<VmNetworkInterface>)
((VdcQueryReturnValue) returnValue).getReturnValue();
Line 203: if (nics.size() == 0) {
Shouldn't this code be part of initNetworkInterfaces?
Line 204: return;
Line 205: }
Line 206:
Line 207: initNetworkInterfaces(networkBehavior, nics);
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/NullNetwork.java
Line 1: package org.ovirt.engine.ui.uicommonweb.models.vms;
Line 2:
Line 3: import org.ovirt.engine.core.common.businessentities.network.Network;
Line 4:
Line 5: public class NullNetwork extends Network {
Why do you need this class? NetworkBehavior adds null to the network list. And
the code in VmModelBehaviorBase for adding null network is redundant.
Line 6:
Line 7: private static final long serialVersionUID = 1L;
Line 8:
Line 9: @Override
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/UnitVmModel.java
Line 103: private void
setDataCenterWithClustersList(NotChangableForVmInPoolListModel
dataCenterWithClustersList) {
Line 104: this.dataCenterWithClustersList = dataCenterWithClustersList;
Line 105: }
Line 106:
Line 107: private ListModel nicWithLogicalNetworks;
As I understand this list holds all the NicWithLogicalNetworks of the vm.
So its name should be nicsWithLogicalNetworks.
Line 108:
Line 109: public ListModel getNicWithLogicalNetworks() {
Line 110: return nicWithLogicalNetworks;
Line 111: }
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java
Line 2009: parameters.setSoundDeviceEnabled((Boolean)
model.getIsSoundcardEnabled().getEntity());
Line 2010:
Line 2011: setVmWatchdogToParams(model, parameters);
Line 2012: Frontend.RunAction(VdcActionType.AddVmFromScratch,
parameters,
Line 2013: new IFrontendActionAsyncCallback() {
Why don't you use
NetworkFrontendActionAsyncCallbacks.NetworkCreateFrontendAsyncCallback as you
did in this case in UserPortalListModel?
Line 2014: @Override
Line 2015: public void
executed(FrontendActionAsyncResult result) {
Line 2016: VdcReturnValueBase returnValueBase =
result.getReturnValue();
Line 2017: if (returnValueBase != null &&
returnValueBase.getSucceeded()) {
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmModelBehaviorBase.java
Line 907: @Override
Line 908: public void onSuccess(Object model, Object returnValue) {
Line 909:
getModel().getIsSoundcardEnabled().setEntity(returnValue);
Line 910: }
Line 911: }, getModel().getHash()), id);
How is this change related to the patch?
Line 912: }
Line 913:
Line 914: protected void initNetworkInterfaces(final NetworkBehavior
behavior, final List<VmNetworkInterface> nics) {
Line 915:
Line 940:
getModel().getNicWithLogicalNetworks().setSelectedItem(Linq.firstOrDefault(nicsWithLogicalNetwork));
Line 941:
Line 942: }
Line 943:
Line 944: private void addNullNetwork(List<Network> networks) {
This method is not specific to the vm and can be reused by all the network
models.
Please consider moving it to the NetworkBehavior.
Also, as I see adding the null network is part of the networkBehavior.convert,
why do you also need it here?
Line 945: if (!isNullNetworkSupported(cluster)) {
Line 946: return;
Line 947: }
Line 948: networks.add(new NullNetwork());
Line 948: networks.add(new NullNetwork());
Line 949:
Line 950: }
Line 951:
Line 952: private boolean isNullNetworkSupported(VDSGroup cluster) {
same
Line 953: if (cluster == null ||
cluster.getcompatibility_version() == null) {
Line 954: return false;
Line 955: }
Line 956:
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmNetworkCreatingManager.java
Line 21: public VmNetworkCreatingManager(PostNetworkCreatedCallback
callback) {
Line 22: this.callback = callback;
Line 23: }
Line 24:
Line 25: public void createOrUpdateNetworks(final Guid vmId,
createOrUpdate is a confusing name. It is hard to understand when to use
create, when update and when createOrUpdate (and there is no documentation...).
Please change the method name to reflect it is used when you add vm from
template. And please add documentation to the methods.
Or maybe if you want this method to be used not just for creating vm from
template, change the name to updateOrCreateIfNothingToUpdate (or something like
this). And remove the explanation about templates from the comments and the
variable names.
Line 26: final List<NicWithLogicalNetworks>
nicsWithLogicalNetworks) {
Line 27: AsyncQuery getVmNicsQuery = new AsyncQuery();
Line 28: getVmNicsQuery.asyncCallback = new INewAsyncCallback() {
Line 29: @Override
Line 88:
Line 89: public void updateNetworks(final Guid vmId, final
List<NicWithLogicalNetworks> nicsWithLogicalNetworks) {
Line 90: ArrayList<VdcActionParametersBase> parameters =
createAddVmInterfaceParams(vmId, nicsWithLogicalNetworks);
Line 91:
Line 92: if (parameters.size() == 0) {
updateNetworksFromParams() has the same code. Why do you also need it here?
Line 93: callback.networkCreated();
Line 94: return;
Line 95: }
Line 96:
Line 138:
Line 139: public static interface PostNetworkCreatedCallback {
Line 140: void networkCreated();
Line 141:
Line 142: void queryFailed(Object target);
I don't really understand why queryFailed needs the target parameter. If
networkCreated can live without it I guess queryFailed can be implemented
without it.
Line 143: }
--
To view, visit http://gerrit.ovirt.org/15102
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a5b488236ba5a08fc9050ed13a6ed49802769da
Gerrit-PatchSet: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: Alona Kaplan <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Einav Cohen <[email protected]>
Gerrit-Reviewer: Frank Kobzik <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
