Lior Vernia has posted comments on this change. Change subject: webadmin: Add warning to Setup networks dialog ......................................................................
Patch Set 7: (15 comments) http://gerrit.ovirt.org/#/c/27463/7/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 68: @Override Line 69: public boolean isDisplayNetworkAffected(NetworkItemModel<?> op1, NetworkItemModel<?> op2) { Line 70: Line 71: final BondNetworkInterfaceModel bondNetworkInterfaceModel = (BondNetworkInterfaceModel) op1; Line 72: return isDisplayNetworkAttached(bondNetworkInterfaceModel.getItems()); Here you could call the overload that accepts (or should accept, as commented) a NetworkInterfaceModel. Line 73: } Line 74: Line 75: }, Line 76: DETACH_NETWORK { Line 347: } Line 348: Line 349: @Override Line 350: public boolean isDisplayNetworkAffected(NetworkItemModel<?> op1, NetworkItemModel<?> op2) { Line 351: return DETACH_NETWORK.isDisplayNetworkAffected(op1, null); What's being dragged here is a NIC to be removed from a bond, so what should be called is isDisplayNetworkAttached(op1). Line 352: } Line 353: Line 354: }, Line 355: REMOVE_UNMANAGED_NETWORK { Line 669: Line 670: return false; Line 671: } Line 672: Line 673: private static boolean isDisplayNetworkAttached(NetworkItemModel<?> networkItemModel) { The parameter should be of type NetworkInterfaceModel, then casting isn't necessary here. It's especially strange because in isDisplayNetwork() you indeed chose to be type safe and cast prior to calling the method, and here you chose the opposite. Line 674: return isDisplayNetworkAttached(((NetworkInterfaceModel) networkItemModel).getItems()); Line 675: } Line 676: Line 677: private static boolean isDisplayNetworkAttached(NetworkItemModel<?> op1, NetworkItemModel<?> op2) { Line 673: private static boolean isDisplayNetworkAttached(NetworkItemModel<?> networkItemModel) { Line 674: return isDisplayNetworkAttached(((NetworkInterfaceModel) networkItemModel).getItems()); Line 675: } Line 676: Line 677: private static boolean isDisplayNetworkAttached(NetworkItemModel<?> op1, NetworkItemModel<?> op2) { Same. Line 678: return isDisplayNetworkAttached(op1) || Line 679: isDisplayNetworkAttached(op2); Line 680: } Line 681: http://gerrit.ovirt.org/#/c/27463/7/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 111: commitChangesInfo = new InfoIcon(templates.italicTwoLines(constants.commitChangesInfoPart1(), constants.commitChangesInfoPart2()), resources); Line 112: Line 113: initWidget(ViewUiBinder.uiBinder.createAndBindUi(this)); Line 114: Line 115: validStatusLine = new StatusLine(style.statusPanel(), style.statusLabel()); I find it weird to have three different objects that maintain some state, but are all called to update the same object. I think it'd be far more intuitive to have different methods to set the message with different styling, e.g. setValidStatus(text), setWarningStatus(text), setErrorStatus(text). Line 116: warningStatusLine = new StatusLine(style.warningPanel(), style.warningLabel()); Line 117: errorStatusLine = new StatusLine(style.errorPanel(), style.errorLabel()); Line 118: Line 119: checkConnectivity.setContentWidgetStyleName(style.checkCon()); http://gerrit.ovirt.org/#/c/27463/7/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 { I asked you in the past to move this near statusPanel and errorPanel to ease comparison. Line 214: background-color: #F4FA58; 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 statusLabel and errorLabel. Line 220: font-size: 15px; Line 221: font-weight: bold; Line 222: color: #753603; Line 223: padding-left: 20px; http://gerrit.ovirt.org/#/c/27463/7/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/footer/StatusLabel.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/footer/StatusLabel.java: Line 8: protected static final int DURATION = 200; Line 9: Line 10: private String pendingText = ""; //$NON-NLS-1$ Line 11: Line 12: private final Runnable onFadeInCompleteRunnable; Consider having this as a protected method instead of a member, that does nothing by default. This is nice in that it doesn't force you to deal with a null callback, and that it isn't required to pass it in the constructor. Line 13: Line 14: private final Animation fadeInAnimation = new Animation() { Line 15: Line 16: @Override http://gerrit.ovirt.org/#/c/27463/7/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/footer/StatusPanel.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/footer/StatusPanel.java: Line 6: public final class StatusPanel extends SimplePanel { Line 7: Line 8: private final Runnable onStatusUpdateCompleteRunnable = new OnCompleteStatusUpdateCallback(); Line 9: Line 10: private final StatusLabel statusLable; Typo. Line 11: Line 12: private String backgroundStyle; Line 13: Line 14: @UiConstructor Line 12: private String backgroundStyle; Line 13: Line 14: @UiConstructor Line 15: public StatusPanel(String text, String backgroundStyle, String forgroundStyle) { Line 16: super(); Why call this explicitly? Line 17: Line 18: this.backgroundStyle = backgroundStyle; Line 19: this.statusLable = new StatusLabel(forgroundStyle, text, onStatusUpdateCompleteRunnable); Line 20: add(statusLable); Line 19: this.statusLable = new StatusLabel(forgroundStyle, text, onStatusUpdateCompleteRunnable); Line 20: add(statusLable); Line 21: } Line 22: Line 23: public void setTextAndStyle(String text, String backgroundStyle, String forgroundStyle) { Spelling is foreground. Line 24: setStyle(backgroundStyle, forgroundStyle); Line 25: setFadeText(text); Line 26: } Line 27: Line 24: setStyle(backgroundStyle, forgroundStyle); Line 25: setFadeText(text); Line 26: } Line 27: Line 28: public void setStyle(String backgroundStyle, String forgroundStyle) { Same here. Line 29: this.backgroundStyle = backgroundStyle; Line 30: statusLable.setStylePrimaryName(forgroundStyle); Line 31: } Line 32: Line 26: } Line 27: Line 28: public void setStyle(String backgroundStyle, String forgroundStyle) { Line 29: this.backgroundStyle = backgroundStyle; Line 30: statusLable.setStylePrimaryName(forgroundStyle); This should behave similarly to backgroundStyle, i.e. save as member and only apply in StatusLabel's overridden template method (what would replace the callback). Line 31: } Line 32: Line 33: public void setFadeText(String text) { Line 34: statusLable.setFadeText(text); Line 33: public void setFadeText(String text) { Line 34: statusLable.setFadeText(text); Line 35: } Line 36: Line 37: private String getBackgroundStyle() { I don't see the need for this method. Line 38: return backgroundStyle; Line 39: } Line 40: Line 41: private final class OnCompleteStatusUpdateCallback implements Runnable { Line 37: private String getBackgroundStyle() { Line 38: return backgroundStyle; Line 39: } Line 40: Line 41: private final class OnCompleteStatusUpdateCallback implements Runnable { This could be implemented as an overridden method, see my comment in StatusLabel. Line 42: @Override Line 43: public void run() { Line 44: setStylePrimaryName(getBackgroundStyle()); Line 45: } -- 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: 7 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
