Lior Vernia has posted comments on this change. Change subject: webadmin: fluent UiAction ......................................................................
Patch Set 5: (12 comments) On top of the specific comments: 1. I'm not convinced you need the UiAction class. You already have the UICommand which has execution, validity check and callback methods... You only need a class that will supply you with the "fluent API", e.g. an ActionCoordinator that will have the and() and then() methods and will take care of the flow ordering. 2. You should add a test class to check commands are run in the correct order, etc. The actual execution can/should be empty/mocked of course. 3. Generally the code looks a little more complicated than it has to be - many small methods that seem to be performing very similar things. I tried to comment where I saw fit, but I would ask you to look again at the whole code critically and try to see if it can be further simplified... http://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 constructs the composite error message, i.e. embeds the inner errors in the outer message, so I'm not sure it's a "getter". As for the class name, it's similar - it doesn't exactly fit the definition of a wrapper. I would suggest Formatter, but there's already plenty of usage of the word in these flows... For your consideration! Line 31: } http://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. Line 65: MessageWrapper messageWrapper) { Line 66: List<VdcActionType> actions = new ArrayList<>(); Line 67: List<VdcReturnValueBase> returnValues = new ArrayList<>(); Line 68: http://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 decrease when such a branch ends? It should be simpler to increase the counter every time a an action is added to the flow (by calling then() or and()), and decrease every time execution ends (and maybe fails). i.e., track individual actions instead of aggregating to branches. This would render this method redundant and simplify a lot of other code... 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 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 the subclasses and it's invoked from a point which is different from where this "template method" is invoked from in this class. 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. 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 implementation of the State design pattern, so you might just call it that. 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 can only have one failure per VdcActionType (which is quite arbitrary). Line 204: Line 205: public ActionFlowMutex(int value, UiAction finalAction) { Line 206: setValue(value); Line 207: this.finalAction = finalAction; http://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 case you continue execution. I think this goes against the general concept of these classes. Line 99: } Line 100: Line 101: @Override Line 102: protected void onActionExecuted() { http://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? Line 90: } Line 91: } Line 92: } Line 93: UiVdcMultipleAction.super.internalRunAction(); http://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. 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? Line 431: String uiCommonRunActionPartitialyFailed(String reason); -- To view, visit http://gerrit.ovirt.org/36848 To unsubscribe, visit http://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 <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: [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
