Alona Kaplan 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? Done 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 They are the same type. Please notice "asyncQuery.setModel(getViewFilterType());" was set before running the query. The query is async. It means the ui is not blocked while is waits for the result. The user can switch the selected view from "attached" to "unattached" or vise versa. This checks make sure the result is still relevant (the view is the same as was when the query was sent). 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 in After taking a second look on what I"ve done here, I decided not to abuse the label field and instead to add isInterfaceAttachedByLabel public method. 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 As I changed the implementation this comment is not relevant any-more. But still, the original reason for this was the I wanted to call setItems just after all the items are completely initialized. Meaning, the "abused" label field is initialized. If there are no vlan devices, you are correct, at this point we can call setItems, But if there are vlan device, at this point they are not initialized. 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)us The field is no longer (ab)used :) NetworkHostListModel.isInterfaceAttachedByLabel(..) is used instead. 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: 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
