Alona Kaplan has posted comments on this change. Change subject: webadmin: fluent UiAction ......................................................................
Patch Set 19: (17 comments) 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 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 sev I'm adding the same key to the actions several times- please refer to lines 70-72 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... Done 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 sim Done 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 Added an UnsupportedOperationException in case a finalAction it set twice. Updated the finalAction's documentation to reflect better its behavior. 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 fro That's what I wrote you as a reply to your comment on patch set 5- You can see a use case example in-https://gerrit.ovirt.org/#/c/36976/12/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/VfsConfigAction.java (line 71) My need is to create an action which contains a new action flow which extends (parallel to) the original one. That is the original actions flow- setupNetworks.then(getCommitNetworkChangesAction()).then(getVfsConfigAction()).onAllExecutionsFinish(closeAction); In this case, getVfsConfigAction() contains another action flow (for updating the vfs config, adding and removing the networks/labels). Lets call the this new flow- vfsConfig action flow. Just when "getVfsConfigAction()" from the original flow is executed ("onActionExecute()"), we're executing the new flow. It means that in this stage we can't add parallel or next actions to the original flow, it can be done (and take effect) only before running the original flow. So, the way to add the new 'vfsConfig action flow' to the original flow it to use the same flow state. Done- changing the name to runParallelAction(..). 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. Please refer to my response to the long comment:) 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. Please my response to your comment in UiVdcMutlipleAction. 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 This method cannot be private, it is executed by the sub classes. I changed it to pachage protected, I don't see any reason for non-infra action classes to call it. For example https://gerrit.ovirt.org/#/c/36848/19/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/action/UiVdcAction.java (line 82). What other methods? 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? The method cannot be private. It is called from the subclasses. For example- https://gerrit.ovirt.org/#/c/36848/19/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/action/UiVdcAction.java (line 98). It is package protected since I didn't see a reason for non-infra action classes to call it. 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 initiat Done 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 Please refer to my response in UiVdcAction. 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 t Done 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 actio I eventually convert the map into two lists because it was the original code and I didn't want to refactor all of it. But the correct data structure to store the errors is a map (instead of maintaining two lists that should have the same size and order). I prefer to write the new code with the correct data structure and later convert it, over writing a new code with a wrong data structure from the beginning. 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 t Renamed it to showErrorDialogOnFirstFailure. 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? Done 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 t I suffices my current needs. so as you wrote, it anyone will need it he can add it:) 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() se 1. I don't want to refer on Frontend.runMultipleAction() implementation. I prefer to leave it as is. 2. shouldExecute will still be required, it is a core method in the UiAction flow. The intention of 'shouldExecute()' is to be able to create on permutation of a flow (althogh that is some use cases different actions in the flow wouldn't run). Please see an example in- https://gerrit.ovirt.org/#/c/36976/12/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/HostSetupNetworksModel.java (line 1069) 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
