Lior Vernia has posted comments on this change. Change subject: webadmin: fluent UiAction ......................................................................
Patch Set 19: (18 comments) Haven't gone over the tests yet, as there were quite a few comments on the code itself (so tests might change). This should give you enough to work on for now... :) And I'll proceed to review SR-IOV patches next. https://gerrit.ovirt.org/#/c/36848/19/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, List<VdcReturnValueBase>> failedActionsMap, I still don't think this overload is required/helpful. See my comment in UiAction concerning storing errors in a map. Line 65: MessageFormatter messageFormatter) { Line 66: List<VdcActionType> actions = new ArrayList<>(); Line 67: List<VdcReturnValueBase> returnValues = new ArrayList<>(); Line 68: Line 69: for (Entry<VdcActionType, List<VdcReturnValueBase>> entry : failedActionsMap.entrySet()) { Line 70: for (int i = 0; i < entry.getValue().size(); ++i) { Line 71: actions.add(entry.getKey()); Line 72: } Line 73: returnValues.addAll(entry.getValue()); This looks wrong to me. I think you need to add the same key to actions several times, once for each of the values. The overload you're invoking later seems to assume correspondence between each item in the returnValues list to the same index item in the returnValues list. Line 74: } Line 75: Line 76: runMultipleActionsFailed(actions, returnValues, messageFormatter); Line 77: } https://gerrit.ovirt.org/#/c/36848/19/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/action/UiAction.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/action/UiAction.java: Line 52: Line 53: /** Line 54: * @param model Line 55: * the model on which to start and stop the progress. Line 56: * @param the This is not the parameter's name... Line 57: * action name Line 58: */ Line 59: public UiAction(Model model, String name) { Line 60: this.model = model; Line 82: runNextAction(); Line 83: } Line 84: } Line 85: Line 86: private final void runActionNoStateIncrease(ActionFlowState actionFlowState) { If runAction(ActionFlowState) is renamed, then this could be renamed to simply runAction() - which will better desbribe the method, as it it quite oblivious to the actual State object (it just sets it). Line 87: this.actionFlowState = actionFlowState; Line 88: if (finalAction != null) { Line 89: actionFlowState.setFinalAction(finalAction); Line 90: } Line 84: } Line 85: Line 86: private final void runActionNoStateIncrease(ActionFlowState actionFlowState) { Line 87: this.actionFlowState = actionFlowState; Line 88: if (finalAction != null) { This handling of finalAction is a little strange. Basically it is set on an arbitrary action in the flow (usually the one returned at the end of the line action.and().then()..., right?), and since you don't know which action that is, you need to check for each action you go through whether that was the one where the user set the final action. It also seems quite error prone, because similarly the user may set a different final action on different actions in a single flow. Then it is not well-defined which final action will actually run (I mean, it's deterministic, but the behavior is not predictable for the typical developer). To me, this seems to stem from the fact that the final actions shouldn't be a property of a single action in the flow, it's actually a property of the entire flow. Therefore there should probably be a "runner" or "coordinator" of some sort that will accept a constructed flow, and where the final action of the entire flow could be set. Then that runner will trigger runAction() of the first action in the flow. This will have the added benefit of there only being one final action, by definition. Line 89: actionFlowState.setFinalAction(finalAction); Line 90: } Line 91: runAction(); Line 92: } Line 98: * Line 99: * @param actionFlowState Line 100: * the state of the flow this action wants to join. Line 101: */ Line 102: public void runAction(ActionFlowState actionFlowState) { Shouldn't this be private? To my understanding it should only be called from one action on another. And only to run in parallel - if that is indeed the case, I'd call it runParallelAction(). This would be similar in function to runNextAction(), only it will be in charge of increasing the State counter (whereas runNextAction() is in charge of decreasing it). Line 103: actionFlowState.setValue(actionFlowState.getValue() + 1); Line 104: runActionNoStateIncrease(actionFlowState); Line 105: } Line 106: Line 132: * <code>actionFlowState</code> are completed. Line 133: * Line 134: * @param finalAction Line 135: */ Line 136: public void onAllExecutionsFinish(SimpleAction finalAction) { See long comment above - not sure this belongs in the UiAction class. Line 137: this.finalAction = finalAction; Line 138: } Line 139: Line 140: protected boolean shouldExecute() { Line 136: public void onAllExecutionsFinish(SimpleAction finalAction) { Line 137: this.finalAction = finalAction; Line 138: } Line 139: Line 140: protected boolean shouldExecute() { See comment in UiVdcMutlipleAction - not necessarily required. In fact, it's a little strange that the responsibility to run the next actions is split between this class and subclasses - if an action succeeds it is the subclass's responsibility, but if it shouldn't run then it's this class's responsibility? Line 141: return true; Line 142: } Line 143: Line 144: protected UiAction getNextAction() { Line 144: protected UiAction getNextAction() { Line 145: return nextAction; Line 146: } Line 147: Line 148: protected void runNextAction() { This and many other methods are protected though they could be declarted as private - is this on purpose? Please scan them and see if some should be changed to private. Line 149: if (getNextAction() != null) { Line 150: getNextAction().runActionNoStateIncrease(actionFlowState); Line 151: } else { Line 152: actionFlowState.setValue(actionFlowState.getValue() - 1); Line 161: public String getName() { Line 162: return name; Line 163: } Line 164: Line 165: void tryToFinalize(boolean handleErrors) { Should be private? Line 166: if (actionFlowState.isAllDone()) { Line 167: model.stopProgress(); Line 168: Line 169: if (handleErrors) { Line 163: } Line 164: Line 165: void tryToFinalize(boolean handleErrors) { Line 166: if (actionFlowState.isAllDone()) { Line 167: model.stopProgress(); Not necessarily - you need to make sure that "you" were the one who initiated startProgress() to begin with. If startProgress() was originally invoked from the "client" code, it should be their responsibility to invoke stopProgress(). Line 168: Line 169: if (handleErrors) { Line 170: handleErrors(); Line 171: } Line 165: void tryToFinalize(boolean handleErrors) { Line 166: if (actionFlowState.isAllDone()) { Line 167: model.stopProgress(); Line 168: Line 169: if (handleErrors) { See my comment in UiVdcAction - I'd likely simplify these classes and just drop the handleErrors boolean (and always handleErrors() when using this infrastructure). Line 170: handleErrors(); Line 171: } Line 172: Line 173: if (actionFlowState.getFinalAction() != null) { Line 195: return actionFlowState; Line 196: } Line 197: Line 198: protected static class ActionFlowState { Line 199: private int value; I think "value" is not a good name for this member, I'd name it according to exactly what it counts. So something like "branchCounter". Getter and setter as well. Line 200: private SimpleAction finalAction; Line 201: Map<VdcActionType, List<VdcReturnValueBase>> failedActionsMap = new HashMap<>(); Line 202: Line 203: public ActionFlowState(int value, SimpleAction finalAction) { Line 228: protected void addFailure(VdcActionType actionType, VdcReturnValueBase result) { Line 229: List<VdcReturnValueBase> actionTypeResults = failedActionsMap.get(actionType); Line 230: if (actionTypeResults == null) { Line 231: actionTypeResults = new LinkedList<>(); Line 232: failedActionsMap.put(actionType, actionTypeResults); Why a map? Is there real value in storing the errors according to the action type? Can't you just maintain two lists, one for the errors and one for the types? You eventually convert to that format anyway in FrontendEventsHandlerImpl (that's why you need a new overload there...). Line 233: } Line 234: Line 235: actionTypeResults.add(result); Line 236: } https://gerrit.ovirt.org/#/c/36848/19/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/action/UiVdcAction.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/action/UiVdcAction.java: Line 38: public class UiVdcAction extends UiAction { Line 39: Line 40: private VdcActionType actionType; Line 41: private VdcActionParametersBase parameters; Line 42: private boolean showErrorDialog; 1. Is this currently needed? If not, I'm not sure I'd add it. It's likely this infrastructure will only be used in case a complex flow is generated, so you would typically want this to be false. It'll simplify the code (which is complex enough as it is), so might be more valuable to remove the functionality. 2. If it is already needed, I'd name it more clearly, something like showErrorDialogOnFirstFailure ("showErrorDialog" might be clear if you know current Frontend code, but it's not that self-explanatory in this flow). Line 43: Line 44: /** Line 45: * @param actionType Line 46: * the <code>VdsActionType</code> Line 71: IFrontendActionAsyncCallback callback = createCallback(); Line 72: Frontend.getInstance().runAction(actionType, parameters, callback, showErrorDialog); Line 73: } Line 74: Line 75: IFrontendActionAsyncCallback createCallback() { Private? Line 76: return new IFrontendActionAsyncCallback() { Line 77: @Override Line 78: public void executed(FrontendActionAsyncResult result) { Line 79: VdcReturnValueBase returnValueBase = result.getReturnValue(); Line 80: if (returnValueBase != null && returnValueBase.getSucceeded()) { Line 81: onSuccess(); Line 82: runNextAction(); Line 83: } else { Line 84: getActionFlowState().setValue(getActionFlowState().getValue() - 1); Similar to my comment in patchset 5, I'm still not sure it's best to "cut the entire branch" if an action in the middle failed. This also isn't consistent with the implementation in UiVdcMultipleAction - there you allow to continue upon failures... But if it suffices for your current needs, then the ability to continue with subsequent command execution can be added by whomever needs it in the future. Line 85: onFailure(returnValueBase); Line 86: } Line 87: } Line 88: }; https://gerrit.ovirt.org/#/c/36848/19/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/action/UiVdcMultipleAction.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/action/UiVdcMultipleAction.java: Line 100: } Line 101: Line 102: @Override Line 103: protected boolean shouldExecute() { Line 104: return super.shouldExecute() && parametersList != null && !parametersList.isEmpty(); I think the isEmpty() check isn't required, Frontend.runMultipleAction() seems to be able to handle it. So consider removing it. I'd also consider just changing the Frontend.runMultipleAction() code to handle null parameters instead. It would simplify your code, as shouldExecute() would not be required at all. Line 105: } -- 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: 19 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alona Kaplan <[email protected]> Gerrit-Reviewer: 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
