Lior Vernia has posted comments on this change. Change subject: webadmin: [Labels] SetupNetwork emphasize unconfigured interface ......................................................................
Patch Set 5: (7 comments) http://gerrit.ovirt.org/#/c/24975/5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/HostSetupNetworksModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/HostSetupNetworksModel.java: Line 615: NetworkLabelModel labelModel = networkLabelMap.get(network.getLabel()); Line 616: if (labelModel == null) { Line 617: labelModel = new NetworkLabelModel(network.getLabel(), this); Line 618: networkLabelMap.put(network.getLabel(), labelModel); Line 619: } Could you add a comment about this adding of networks here that they're only candidates to be drawn as part of the label, and that this doesn't yet considers whether they actually exist on the interface? Line 620: labelModel.getNetworks().add(networkModel); Line 621: } Line 622: } Line 623: setNetworks(networkModels); Line 724: networkModel.attachViaLabel(); Line 725: } else { Line 726: // The network has the same label as the nic but not attached to the nic. Line 727: // It means this network couldn't be attached to the nic Line 728: // because it has a conflict with the other networks on the nic. Not necessarily. It's possible that everything is legal but the host was non-responsive when the configuration was set, isn't it? Line 729: iter.remove(); Line 730: networkModel.setErrorNicName(ifName); Line 731: } Line 732: } Line 726: // The network has the same label as the nic but not attached to the nic. Line 727: // It means this network couldn't be attached to the nic Line 728: // because it has a conflict with the other networks on the nic. Line 729: iter.remove(); Line 730: networkModel.setErrorNicName(ifName); Which means this isn't necessarily correct. Line 731: } Line 732: } Line 733: Line 734: // attach label itself to nic http://gerrit.ovirt.org/#/c/24975/5/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 103: Line 104: @DefaultMessage("{0} GB") Line 105: String rebalanceFileSizeGb(String size); Line 106: Line 107: @DefaultMessage("Network couldn''t be assigned to ''{0}'' via label ''{1}''. <br> It has a conflict with another network on this nic.") Similarly to the errors in the status bar, I would prefer to give the user the information on which other network caused a collision (if a collision in fact occurred). This is saved during network operations in the culpritNetwork field of LogicalNetworkModel, if I'm not mistaken. Maybe the NetworkOperation mechanism can be used as part of the initNicModels() flow to re-use that mechanism. Line 108: String networkLabelConflict(String nicName, String label); http://gerrit.ovirt.org/#/c/24975/5/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/panels/ItemInfoPopup.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/panels/ItemInfoPopup.java: Line 25: private final FlexTable contents = new FlexTable(); Line 26: private static final EnumRenderer<NetworkBootProtocol> RENDERER = new EnumRenderer<NetworkBootProtocol>(); Line 27: private final ApplicationConstants constants = ClientGinjectorProvider.getApplicationConstants(); Line 28: private final ApplicationTemplates templates = ClientGinjectorProvider.getApplicationTemplates(); Line 29: final ApplicationResources resources = ClientGinjectorProvider.getApplicationResources(); I know it's not related to this patch, but since the other fields are private, I'm not sure why this has default access... Care to fix this? Line 30: private final ApplicationMessages messages = ClientGinjectorProvider.getApplicationMessages(); Line 31: SafeHtml mgmtNetworkImage = SafeHtmlUtils.fromTrustedString(AbstractImagePrototype.create(resources.mgmtNetwork()) Line 32: .getHTML()); Line 33: SafeHtml vmImage = SafeHtmlUtils.fromTrustedString(AbstractImagePrototype.create(resources.networkVm()).getHTML()); http://gerrit.ovirt.org/#/c/24975/5/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/panels/NetworkPanel.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/panels/NetworkPanel.java: Line 55: vmImage = network.getEntity().isVmNetwork() ? new Image(resources.networkVm()) : null; Line 56: migrationImage = network.getEntity().getCluster().isMigration() ? Line 57: new Image(resources.migrationNetwork()) : null; Line 58: notSyncImage = !network.isInSync() ? new Image(resources.networkNotSyncImage()) : null; Line 59: alertImage = network.getErrorNicName()!=null ? new Image(resources.alertImage()) : null; A little spacing here? :) Line 60: Line 61: if (network.isManagement()) { Line 62: mgmtNetworkImage.setStylePrimaryName(style.networkImageBorder()); Line 63: } Line 91: Line 92: rowPanel.setWidget(0, 0, dragImage); Line 93: Line 94: Panel statusPanel = new HorizontalPanel(); Line 95: rowPanel.setWidget(0, 1, statusPanel); This probably pushes the network name to the right, taking it out of alignment with other networks' names, doesn't it? Maybe it would be better to add the alertImage to the right of the network name? Line 96: if (alertImage != null) { Line 97: statusPanel.add(alertImage); Line 98: } Line 99: -- To view, visit http://gerrit.ovirt.org/24975 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifa83ddfd2ceddc8f2227dcc8cc36c721baf8c7bc Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@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
