Lior Vernia has posted comments on this change. Change subject: webadmin: Add warning to Setup networks dialog ......................................................................
Patch Set 4: (12 comments) http://gerrit.ovirt.org/#/c/27463/4/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/network/NetworkOperation.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/network/NetworkOperation.java: Line 100: } Line 101: }; Line 102: } Line 103: Line 104: @SuppressWarnings("unchecked") Strangely, my eclipse marks this as unnecessary, even though I can see the "unsafe" cast. Isn't it marked as unnecessary on yours? Line 105: @Override Line 106: public boolean isDisplayNetworkToBeChanged(NetworkItemModel<?> op1, NetworkItemModel<?> op2) { Line 107: Line 108: final LogicalNetworkModel logicalNetworkModel = (LogicalNetworkModel) op1; Line 245: }; Line 246: } Line 247: Line 248: @Override Line 249: public boolean isDisplayNetworkToBeChanged(NetworkItemModel<?> op1, NetworkItemModel<?> op2) { You could delegate here to BOND_WITH.isDisplayNetworkToBeChanged(), as both operations are essentially the same in this respect (join two NICs, go over their networks - doesn't really matter if one of the NICs is a bond). Line 250: final BondNetworkInterfaceModel bondNetworkInterfaceModel1 = (BondNetworkInterfaceModel) op1; Line 251: final BondNetworkInterfaceModel bondNetworkInterfaceModel2 = (BondNetworkInterfaceModel) op2; Line 252: Line 253: return isDisplayNetworkAttached(bondNetworkInterfaceModel1.getItems()) || Line 291: }; Line 292: } Line 293: Line 294: @Override Line 295: public boolean isDisplayNetworkToBeChanged(NetworkItemModel<?> op1, NetworkItemModel<?> op2) { Same comment. Line 296: final NetworkInterfaceModel nicToBeAddedToBond = (NetworkInterfaceModel) op1; Line 297: final BondNetworkInterfaceModel bondNetworkInterfaceModel = (BondNetworkInterfaceModel) op2; Line 298: Line 299: return isDisplayNetworkAttached(nicToBeAddedToBond.getItems()) || Line 320: }; Line 321: } Line 322: Line 323: @Override Line 324: public boolean isDisplayNetworkToBeChanged(NetworkItemModel<?> op1, NetworkItemModel<?> op2) { Same here, I would delegate to BOND_WITH for uniformity. Line 325: return ADD_TO_BOND.isDisplayNetworkToBeChanged(op2, op1); Line 326: } Line 327: Line 328: }, Line 358: }; Line 359: } Line 360: Line 361: @Override Line 362: public boolean isDisplayNetworkToBeChanged(NetworkItemModel<?> op1, NetworkItemModel<?> op2) { It would be more correct to delegate here to DETACH_NETWORK, as that is the similar operation (here you drag a network off a bond). Line 363: return BREAK_BOND.isDisplayNetworkToBeChanged(op1, null); Line 364: } Line 365: Line 366: }, Line 659: public boolean isNullOperation(){ Line 660: return false; Line 661: } Line 662: Line 663: @SuppressWarnings("unused") What is this for? Line 664: public boolean isDisplayNetworkToBeChanged(NetworkItemModel<?> op1, NetworkItemModel<?> op2) { Line 665: return false; Line 666: } Line 667: Line 660: return false; Line 661: } Line 662: Line 663: @SuppressWarnings("unused") Line 664: public boolean isDisplayNetworkToBeChanged(NetworkItemModel<?> op1, NetworkItemModel<?> op2) { I would name this isDisplayNetworkAffected, or isDisplayNetworkMoved. "Changed" would be more appropriate if any properties of the network were being modified (e.g. VLAN). Line 665: return false; Line 666: } Line 667: Line 668: public boolean isErroneousOperation() { Line 673: if (logicalNetworkInterfaces == null) { Line 674: return false; Line 675: } Line 676: for (LogicalNetworkModel logicalNetworkModel : logicalNetworkInterfaces) { Line 677: Please remove this empty line, it doesn't seem to separate two different sections of logic. Line 678: if (isDisplayNetwork(logicalNetworkModel)) { Line 679: return true; Line 680: } Line 681: } http://gerrit.ovirt.org/#/c/27463/4/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationMessages.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationMessages.java: Line 101: Line 102: @DefaultMessage("{0} GB") Line 103: String rebalanceFileSizeGb(String size); Line 104: Line 105: @DefaultMessage("{0}. Moving the display network will drop VM console connectivity until they are restarted") Please add a dot in the end, grammatically it's a complete sentence (as opposed to e.g. a title with no verb in it). Line 106: String moveDisplayNetworkWarning(String networkOperationMessage); Line 107: http://gerrit.ovirt.org/#/c/27463/4/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/HostSetupNetworksPopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/HostSetupNetworksPopupView.java: Line 173: NetworkOperation candidate = evtArgs.getCandidate(); Line 174: NetworkItemModel<?> op1 = evtArgs.getOp1(); Line 175: NetworkItemModel<?> op2 = evtArgs.getOp2(); Line 176: Line 177: if (evtArgs.isDrop()) { This is exactly the opposite of what's supposed to happen (and what used to happen before your patch). Notice that the styling would only change to error styling when the user would drop the item. To summarise, if there's no drop, you only need to set the text and set the styling to a valid one. If there's a drop, you only need to set the styling. Line 178: setValidStatusStyle(); Line 179: } else { Line 180: if (candidate == null) { Line 181: status.setFadeText(constants.noValidActionSetupNetwork()); http://gerrit.ovirt.org/#/c/27463/4/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/HostSetupNetworksPopupView.ui.xml File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/HostSetupNetworksPopupView.ui.xml: Line 209: font-weight: bold; Line 210: padding-top: 10px; Line 211: } Line 212: Line 213: .warningPanel { Please put this higher up, next to errorPanel, to ease the comparison. Line 214: background-color: yellow; Line 215: height: 30px; Line 216: border-bottom: 1px solid #C5C5C5; Line 217: } Line 215: height: 30px; Line 216: border-bottom: 1px solid #C5C5C5; Line 217: } Line 218: Line 219: .warningLabel { Same here with errorLabel. Line 220: font-size: 15px; Line 221: font-weight: bold; Line 222: color: #FF8C00; Line 223: padding-left: 20px; -- To view, visit http://gerrit.ovirt.org/27463 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I35f0e019b6f77ab90fec89f614b49db589602353 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Yevgeny Zaspitsky <[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
