Lior Vernia has posted comments on this change. Change subject: webadmin: Add warning to Setup networks dialog ......................................................................
Patch Set 6: (4 comments) http://gerrit.ovirt.org/#/c/27463/6/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 154: Line 155: @Override Line 156: public boolean isDisplayNetworkAffected(NetworkItemModel<?> op1, NetworkItemModel<?> op2) { Line 157: LogicalNetworkModel networkToBeAttached = (LogicalNetworkModel) op1; Line 158: return isDisplayNetwork(networkToBeAttached); This just occurred to me - it might be better to return this only if op1 hadn't previously been attached to another NIC (so something like "return networkToBeAttached.hasInterface() && isDisplayNetwork(networkToBeAttached)). Note, however, that if execution happens before the event is raised in HostSetupNetworksPopupView (this happens when a drop occurs), then networkToBeAttached.hasInterface() will return the state AFTER execution. So you need to somehow mark some flag during execution as well. Or think of a more elegant solution :) Line 159: } Line 160: Line 161: }, Line 162: BOND_WITH { http://gerrit.ovirt.org/#/c/27463/6/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 154: // this is called after both networks and nics were retrieved Line 155: HostSetupNetworksModel model = (HostSetupNetworksModel) sender; Line 156: List<LogicalNetworkModel> networks = model.getNetworks(); Line 157: List<NetworkInterfaceModel> nics = model.getNics(); Line 158: lastStatusLine.setEmptyStatusText(); I'm suggesting to drop this, see my comment below. Line 159: updateNetworks(networks); Line 160: updateNics(nics); Line 161: // mark as rendered Line 162: rendered = true; Line 175: final String message; Line 176: Line 177: if (candidate == null) { Line 178: message = constants.noValidActionSetupNetwork(); Line 179: statusLine = errorStatusLine; I would instantiate the status lines here instead of above. Line 180: } else { Line 181: message = candidate.getMessage(op1, op2); Line 182: Line 183: if (candidate.isErroneousOperation()) { Line 242: Line 243: void setEmptyStatusText(); Line 244: } Line 245: Line 246: private abstract class AbstractStatusLine implements StatusLine { I don't see the need for a class hierarchy here. You could have one class of Status, and just use its constructor to instantiate the three possible states. And you wouldn't need to have the three states instantiated by default, you can only construct the one you need for each case. setStatusText() isn't needed because you're only manipulating the default message in the case of a warning. You already have if clauses to determine which case is that of a warning, that you can't drop. So you might as well just update the message in your existing if clauses, and then set the fade text following the if clauses. setEmptyStatusText(), which by default does nothing, seems to me like a non-intuitive overkill for what we're trying to achieve. Instead, I would set the empty status by default, and never set it again in the dialog. This is a slight change of behavior, because even valid messages will remain displayed after a redraw, but that's okay - it used to be like that anyway, it's a very small change of behavior and it's very arguable which behavior is better to start with. Line 247: private final String panelStyleName; Line 248: private final String labelStyleName; Line 249: Line 250: AbstractStatusLine(String panelStyleName, String labelStyleName) { -- 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: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
