Alona Kaplan 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 th I can't use the freeLabels since it calculates the dc labels that are not use by any physical nic. I need to calculate for each vfs config the dc labels that are not uses by this specific config. In case of a bond slave, I'm not wrongly swapping free labels to null. Bond's slaves don't have PF labels tab, therefore there is no need to calculate the free labels for nothing. 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 Please see the reply in the related comments. 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 It is. Please see- https://gerrit.ovirt.org/#/c/36842/ 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 subclas Calling an overridden method from the constructor of a parent class is not recommended. In this case particularly it will cause NPE since the calculation of originalLabels in PfNicLabelModel using 'srcIfaces" which are not initialized when the NicLabelModel ctor is executed. 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 ori See the previous comment 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 HostNicMo As answered in the previous related comment the suggestedLabels contain different data, related to PF only. 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. Please see the previous comment. 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 ta When both of the tabs are shown there is a left side tabs bar that has a with of 60px. 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: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
