Lior Vernia has posted comments on this change. Change subject: webadmin: edit vfsConfig- labels ......................................................................
Patch Set 36: (8 comments) https://gerrit.ovirt.org/#/c/36422/36/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 363: isBondSalve ? null : getFreeLabels(), Line 364: isBondSalve ? null : labelToIface, Line 365: nicToVfsConfig.get(entity.getId()), Line 366: allNetworks, Line 367: dcLabels); You don't actually need this extra argument - you only use it to extract the unused labels, which are already passed as another argument by invoking getFreeLabels() (you just wrongly swap it for null in the bond slave case, which you should change as part of what we discussed relating a previous patchset). Line 368: editPopup = interfacePopupModel; Line 369: Line 370: // OK Target Line 371: okTarget = new BaseCommandTarget() { https://gerrit.ovirt.org/#/c/36422/36/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/NicLabelModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/NicLabelModel.java: Line 31 Line 32 Line 33 Line 34 Line 35 See related comments - I think the constructor should mostly stay the same here, the only difference would be to take the original labels logic out into a template method. Line 77 Line 78 Line 79 Line 80 Line 81 This validation is still needed in both PF and VF cases - there's no reason to type the same label twice. Line 13: public abstract class NicLabelModel extends ListModel<ListModel<String>> { Line 14: private List<String> originalLabels; Line 15: private Collection<String> suggestedLabels; Line 16: Line 17: protected void initLabelModels() { This should always be called at the end of the constructor, in both subclasses. Why not keep the original constructor code here, but move the PF original labels computation to an abstract protected setOriginalLabels() method? It would be trivial in the VF case and have the logic in the PF case, but the constructor would otherwise be the same for both subclasses. Line 18: Collections.sort(originalLabels, new LexoNumericComparator()); Line 19: LinkedList<ListModel<String>> items = new LinkedList<ListModel<String>>(); Line 20: for (String label : originalLabels) { Line 21: ListModel<String> labelModel = new ListModel<String>(); Line 29: public Collection<String> getSuggestedLabels() { Line 30: return suggestedLabels; Line 31: } Line 32: Line 33: public void setSuggestedLabels(Collection<String> suggestedLabels) { This method isn't needed - you still do the same in both subclasses, so original code should stay in this base class's constructor. Line 34: this.suggestedLabels = suggestedLabels; Line 35: } Line 36: Line 37: public List<String> getOriginalLabels() { https://gerrit.ovirt.org/#/c/36422/36/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/VfsConfigModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/VfsConfigModel.java: Line 25: public VfsConfigModel() { Line 26: this(new HostNicVfsConfig(), Collections.<Network> emptyList(), new TreeSet<String>()); Line 27: } Line 28: Line 29: public VfsConfigModel(HostNicVfsConfig vfsConfig, List<Network> allClusterNetworks, SortedSet<String> dcLabels) { You should change this to suggestedLabels and have it passed from HostNicModel (which already has this data). Line 30: setEntity(vfsConfig); Line 31: maxNumOfVfs.setEntity(vfsConfig.getMaxNumOfVfs()); Line 32: numOfVfs.setEntity(vfsConfig.getNumOfVfs()); Line 33: allNetworksAllowed.setItems(Arrays.asList(AllNetworksSelector.values())); Line 34: allNetworksAllowed.setSelectedItem(vfsConfig.isAllNetworksAllowed() ? AllNetworksSelector.allNetworkAllowed Line 35: : AllNetworksSelector.specificNetworks); Line 36: initNetworks(allClusterNetworks); Line 37: Line 38: dcLabels.removeAll(vfsConfig.getNetworkLabels()); Unneeded. Line 39: labelsModel = new VfsNicLabelModel(new ArrayList<>(vfsConfig.getNetworkLabels()), dcLabels); Line 40: } Line 41: Line 42: public EntityModel<Integer> getNumOfVfs() { https://gerrit.ovirt.org/#/c/36422/36/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/HostNicPopupView.ui.xml File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/HostNicPopupView.ui.xml: Line 12: width: auto !important; Line 13: } Line 14: </ui:style> Line 15: Line 16: <d:SimpleDialogPanel ui:field="mainPanel" width="680px" height="600px" > This doesn't add up - in the VF-only case you added 115px, why when both tabs appear do you need to increase by 175px? Line 17: <d:content> Line 18: <g:SimplePanel ui:field="contentPanel"> Line 19: <t:DialogTabPanel ui:field="tabPanel" height="100%"> Line 20: <t:tab> -- To view, visit https://gerrit.ovirt.org/36422 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icae8ed1d9b951e36593f38f9390d8d410f22d5cf Gerrit-PatchSet: 36 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
