Alona Kaplan has posted comments on this change. Change subject: webadmin: fluent UiAction ......................................................................
Patch Set 5: (12 comments) Regarding to the general comment- 1. UiCommand has a lot of unrelated properties and logic. I want a clean start. I don't see any good reason reusing it. 2. This phase of comment fixes still doesn't contain tests. Will add it in the next phase (tomorrow I hope). 3. I hope now I addressed all your comments and also did some more small fixes the code is clearer. https://gerrit.ovirt.org/#/c/36848/5/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/IFrontendEventsHandler.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/IFrontendEventsHandler.java: Line 26: Line 27: void publicConnectionClosed(Exception ex); Line 28: Line 29: public static interface MessageWrapper { Line 30: public String getMessage(String innerMessage); > Consider the naming of the class and the method. The method actually constr Done Line 31: } https://gerrit.ovirt.org/#/c/36848/5/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/FrontendEventsHandlerImpl.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/FrontendEventsHandlerImpl.java: Line 60: runMultipleActionsFailed(actions, returnValues); Line 61: } Line 62: Line 63: @Override Line 64: public void runMultipleActionsFailed(Map<VdcActionType, VdcReturnValueBase> failedActionsMap, > See my comment in UiAction, this overload might not be needed. Changed to Map<VdcActionType, List<VdcReturnValueBase>>, the overload is still needed. Line 65: MessageWrapper messageWrapper) { Line 66: List<VdcActionType> actions = new ArrayList<>(); Line 67: List<VdcReturnValueBase> returnValues = new ArrayList<>(); Line 68: https://gerrit.ovirt.org/#/c/36848/5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/UiAction.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/UiAction.java: Line 77: runNextAction(); Line 78: } Line 79: } Line 80: Line 81: private final void runActionNoMutexIncrease(ActionFlowMutex actionFlowMutex) { > Why do you only increase count when a new "flow branch" is created and decr "decrease every time execution ends (and maybe fails)" will force the subclasses to maintain the state counter. Decreasing the counter at the end of each "branch' can be maintained in one centralized place (UiAction.runNextAction). "increase the counter every time a an action is added to the flow (by calling then() or and())"- is problematic since in case of an error if the flow the next action won't be executed, therefore the counter will never be decreased. It can cause the aggregated error message not to be displayed. So IMO, it is ok to leave the code as is. Line 82: this.actionFlowMutex = actionFlowMutex; Line 83: if (finalAction != null) { Line 84: actionFlowMutex.setFinalAction(finalAction); Line 85: } Line 92: * will be executed just after all the actions in the flow (including this one) will be finished. Line 93: * Line 94: * @param actionFlowMutex the mutex of the flow this action wants to join. Line 95: */ Line 96: public void runAction(ActionFlowMutex actionFlowMutex) { > Public? O.o You case see a use case example in-https://gerrit.ovirt.org/#/c/36976/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/VfsConfigAction.java (line 70) Line 97: actionFlowMutex.setValue(actionFlowMutex.getValue() + 1); Line 98: runActionNoMutexIncrease(actionFlowMutex); Line 99: } Line 100: Line 97: actionFlowMutex.setValue(actionFlowMutex.getValue() + 1); Line 98: runActionNoMutexIncrease(actionFlowMutex); Line 99: } Line 100: Line 101: void internalRunAction() { > This looks like it should be an abstract method, it's only used by one of t I've changed the method to be abstract and added a SyncUiAction class with the default implementation. The purpose of the method is to contain the inner implementation of invoking the command's 'business code' and invoking the next action. Asynchronous actions like UiVdcAction and UiVdcMultipleAction override this method to invoke the next action after getting the result of the current action from the server. Synchronous actions (for example https://gerrit.ovirt.org/#/c/36976/9/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/VfsConfigAction.java) should now extend SyncUiAction and only override 'onActionExecuted' to contain their 'business code'. The method is package private and should be overridden just by infrastructural action classes that want to change the inner flow of invoking the next action. (Please notice I moved the four action classes to package org.ovirt.engine.ui.uicommonweb.action). Line 102: onActionExecuted(); Line 103: runNextAction(); Line 104: } Line 105: Line 105: Line 106: /** Line 107: * This method contains the code that will run when the action is executed. Line 108: */ Line 109: protected abstract void onActionExecuted(); > This method doesn't seem necessary. I removed this method from this class and moved it to SyncUiAction as explained in the previous comment. I hope it makes things clearer... Line 110: Line 111: /** Line 112: * Specifying an action that will run immediately after <code>this</code> action has finished its execution. Line 113: * Line 196: protected ActionFlowMutex getActionFlowMutex() { Line 197: return actionFlowMutex; Line 198: } Line 199: Line 200: protected static class ActionFlowMutex { > This isn't a mutex, just a glorified counter. I think it's basically an imp Changed to ActionFlowState. Line 201: private int value; Line 202: private UiAction finalAction; Line 203: Map<VdcActionType, VdcReturnValueBase> failedActionsMap = new HashMap<>(); Line 204: Line 199: Line 200: protected static class ActionFlowMutex { Line 201: private int value; Line 202: private UiAction finalAction; Line 203: Map<VdcActionType, VdcReturnValueBase> failedActionsMap = new HashMap<>(); > It's not great to save failures according to VdcActionType - it means you c You are correct! Changed to Map<VdcActionType, List<VdcReturnValueBase>> Line 204: Line 205: public ActionFlowMutex(int value, UiAction finalAction) { Line 206: setValue(value); Line 207: this.finalAction = finalAction; https://gerrit.ovirt.org/#/c/36848/5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/UiVdcAction.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/UiVdcAction.java: Line 94: protected void onFailure(VdcReturnValueBase returnValueBase) { Line 95: if (!showErrorDialog) { Line 96: getActionFlowMutex().addFailure(actionType, returnValueBase); Line 97: } Line 98: tryToFinalize(!showErrorDialog); > This decision is quite strange, especially since in the multiple actions ca You're just trying to finalize, if it is not the last action in the flow you won't finalize. Multiple actions case have it own class and implementation, so I'm not sure how it is related to here. (any way, if you continue the execution, it means the 'State' counter is not zero, so eventually the finalize won't be invoked.) Line 99: } Line 100: Line 101: @Override Line 102: protected void onActionExecuted() { https://gerrit.ovirt.org/#/c/36848/5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/UiVdcMultipleAction.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/UiVdcMultipleAction.java: Line 85: if (!showErrorDialog) { Line 86: for (VdcReturnValueBase singleResult : result.getReturnValue()) { Line 87: if (!singleResult.getCanDoAction()) { Line 88: getActionFlowMutex().addFailure(actionType, singleResult); Line 89: continue; > Why is this needed? It is a leftover from before I've added the failures aggregation. Removed the continue, thanks! Line 90: } Line 91: } Line 92: } Line 93: UiVdcMultipleAction.super.internalRunAction(); https://gerrit.ovirt.org/#/c/36848/5/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIMessages.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIMessages.java: Line 2: Line 3: import java.util.Date; Line 4: import java.util.List; Line 5: Line 6: import com.google.gwt.i18n.client.Messages.DefaultMessage; > I think you didn't intend to add this. Done Line 7: Line 8: public interface UIMessages extends com.google.gwt.i18n.client.Messages { Line 9: Line 10: @DefaultMessage("One of the parameters isn''t supported (available parameter(s): {0})") Line 426: Line 427: @DefaultMessage("Non-Passthrough profile cannot be attached to a VM of type {0}") Line 428: String vnicTypeDoesntMatchNonPassthroughProfile(String type); Line 429: Line 430: @DefaultMessage("Error while executing some of the action tasks: {0}") > At least some of the actions? Done Line 431: String uiCommonRunActionPartitialyFailed(String reason); -- To view, visit https://gerrit.ovirt.org/36848 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I262282d00bbb16a65f36a2463d3ffe8dbf6594c6 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
