Lior Vernia has posted comments on this change. Change subject: webadmin: adding labels column to Network->Hosts sub-tab ......................................................................
Patch Set 10: (5 comments) http://gerrit.ovirt.org/#/c/24992/10/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/networks/NetworkHostListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/networks/NetworkHostListModel.java: Line 77: for (Object obj : returnList) { Line 78: items.add(new PairQueryable<VdsNetworkInterface, VDS>(null, (VDS) obj)); Line 79: } Line 80: setItems(items); Line 81: } else if (NetworkHostFilter.attached.equals(getViewFilterType())) { Instead of the complicated logic following this, how about this? * Add a query for all interfaces containing the network's label (only run once, not inside the loop). From this you should construct a Set<String> of interface names (it's possible to have the query just return a Map<String, VdsNetworkInterface> to begin with). * When iterating over the pairs, for each interface just check if its non-VLAN prefix exists in the Set; if it is, mark it as attached by label. Simple logic. Line 82: for (Object obj : returnList) { Line 83: final PairQueryable<VdsNetworkInterface, VDS> ifaceHostPair = Line 84: (PairQueryable<VdsNetworkInterface, VDS>) obj; Line 85: // If the network has label and the iface is vlan- Line 97: asyncQuery.setModel(model); Line 98: asyncQuery.asyncCallback = new INewAsyncCallback() { Line 99: @Override Line 100: public void onSuccess(Object model, Object returnValueObj) { Line 101: if (!model.equals(getViewFilterType())) { I don't understand what this check attempts to accomplish, they're not even the same type. Line 102: return; Line 103: } Line 104: Line 105: List<VdsNetworkInterface> vdsIfaces = Line 110: for (VdsNetworkInterface vdsIface : vdsIfaces) { Line 111: if (vdsIface.getName().equals(physicalIfaceName)) { Line 112: if (vdsIface.getLabels() != null Line 113: && vdsIface.getLabels().contains(networkLabel)) { Line 114: iface.setLabels(Collections.singleton(networkLabel)); Please explain in comment that you're using the labels field to mark the interface as containing the network "by label", which is not this field's original meaning. Line 115: } Line 116: break; Line 117: } Line 118: } Line 136: } Line 137: items.add(ifaceHostPair); Line 138: } Line 139: } Line 140: tryToSetItems((List<PairQueryable<VdsNetworkInterface, VDS>>) returnList, items); I don't understand why you need this instead of just setItems()?... At this point you've already finished iterating over all interfaces, haven't you? Line 141: } Line 142: } Line 143: } Line 144: }; http://gerrit.ovirt.org/#/c/24992/10/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/network/SubTabNetworkHostView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/network/SubTabNetworkHostView.java: Line 112: @Override Line 113: public SafeHtml getValue(PairQueryable<VdsNetworkInterface, VDS> object) { Line 114: if (object.getFirst() != null) { Line 115: VdsNetworkInterface nic = object.getFirst(); Line 116: return nic.getLabels() == null || nic.getLabels().isEmpty() ? SafeHtmlUtils.fromTrustedString(nic.getName()) Please add a comment here documenting that the labels field had been (ab)used to mark the interface as containing the network "by label", rather than its original designation. Also, since it's used below as well, I would extract the check to a properly-named method. Line 117: : templates.imageTextLabels(labelImage, nic.getName()); Line 118: } Line 119: return null; Line 120: } -- To view, visit http://gerrit.ovirt.org/24992 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I610703df29dcae7ace390e0ebb109475fb202263 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: 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
