Lior Vernia has posted comments on this change.
Change subject: core+webadmin: reordering VM NICs
......................................................................
Patch Set 5:
(4 comments)
Generally looks good, just a couple of suggestions how to make the change less
intrusive to other pieces of code.
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/Frontend.java
Line 598: * @param successCallback The callback to be executed when all
actions have succeeded.
Line 599: * @param state State object
Line 600: * @param runCallbacksOnEmptyRun Whether to run the callback or
not even if no commands were run
Line 601: */
Line 602: public void runMultipleActions(final VdcActionType actionType,
Maybe add another override with the old signature of this one, which passes
true as runCallbacksOnEmptyRun by default? It would obviate the need to change
the call in userportal, and might be convenient for others in the future.
True is actually the more reasonable default choice, and it wouldn't hurt the
call from userportal because it can never be called with a empty parameters.
Line 603: final List<VdcActionParametersBase> parameters,
Line 604: final IFrontendActionAsyncCallback successCallback,
Line 605: final Object state,
Line 606: final boolean runCallbacksOnEmptyRun) {
Line 606: final boolean runCallbacksOnEmptyRun) {
Line 607: if (parameters == null || parameters.isEmpty()) {
Line 608: if (runCallbacksOnEmptyRun && successCallback != null) {
Line 609: VdcReturnValueBase emptyResult = new
VdcReturnValueBase();
Line 610: VdcActionParametersBase emptyParams = new
VdcActionParametersBase();
I'm thinking maybe it's best to put null in those instead of creating empty
objects, because it would make it easier to see that nothing was run (either by
checking or by seeing that an exception is thrown).
What do you think?
Line 611: successCallback.executed(new
FrontendActionAsyncResult(actionType,
Line 612: emptyParams, emptyResult, state));
Line 613: }
Line 614: return;
Line 631: */
Line 632: public void runMultipleActions(final VdcActionType actionType,
Line 633: final List<VdcActionParametersBase> parameters,
Line 634: final IFrontendActionAsyncCallback successCallback) {
Line 635: runMultipleActions(actionType, parameters, successCallback,
null, false);
I think it would be better to pass true here by default. I checked the 3 usages
of this method, for 2 of them it would do no harm and for the third it would in
fact solve a non-reported bug (looking at the code, it appears that when
importing a network from a provider, if no clusters exist in the DC then a vNIC
profile won't be created for the network).
Line 636: }
Line 637:
Line 638: /**
Line 639: * A convenience method that calls
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/UnitVmModelNetworkAsyncCallback.java
Line 9:
Line 10: private final UnitVmModel unitVmModel;
Line 11: private final VmInterfaceCreatingManager networkCreatingManager;
Line 12: private final Guid idToUpdate;
Line 13: private final boolean isAddingNewVm;
I think this parameter might not be necessary, take a look at
UnitVmModel.getIsNew() - doesn't it return true iff a new VM is created?
I am not absolutely certain that the cases where it is set to true are exactly
the cases where you pass true here, needs checking.
Line 14:
Line 15: public UnitVmModelNetworkAsyncCallback(final UnitVmModel
unitVmModel,
Line 16: final VmInterfaceCreatingManager networkCreatingManager,
boolean isAddingNewVm) {
Line 17:
--
To view, visit http://gerrit.ovirt.org/22512
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d02e3eff9466d9094bc16cd489503d25c9f4bef
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Oved Ourfali <[email protected]>
Gerrit-Reviewer: Lior Vernia <[email protected]>
Gerrit-Reviewer: Livnat Peer <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: Oved Ourfali <[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
