Ramesh N has posted comments on this change. Change subject: webadmin: add additional features support in cluster popup. ......................................................................
Patch Set 7: (9 comments) https://gerrit.ovirt.org/#/c/39757/7/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/dataprovider/AsyncDataProvider.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/dataprovider/AsyncDataProvider.java: Line 4109: new GetClusterFeaturesByVersionAndCategoryParameters(version, category), Line 4110: aQuery); Line 4111: } Line 4112: Line 4113: public void getClusterFeaturesByClusterId(AsyncQuery aQuery, Guid clusterId) { > how about overloading I mean function names same. Just a thought. Here the parameters, Query and ReturnType all are different. So I had put in a separate function to make it clear. Line 4114: aQuery.converterCallback = new IAsyncConverter() { Line 4115: @Override Line 4116: public Object Convert(Object source, AsyncQuery _asyncQuery) Line 4117: { https://gerrit.ovirt.org/#/c/39757/7/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterListModel.java: Line 754: cluster.setCompatibilityVersion(version); Line 755: cluster.setMigrateOnError(model.getMigrateOnErrorOption()); Line 756: cluster.setVirtService(model.getEnableOvirtService().getEntity()); Line 757: cluster.setGlusterService(model.getEnableGlusterService().getEntity()); Line 758: for (AdditionalClusterFeature feature : model.getAdditionalClusterFeatures().getSelectedItem()) { > why for loop ? getSelectedItem() is a single item right? Thats the trick with CheckBoxGroup. Entity model is defined as ListModel<List<AdditionalClusterFeature>> additionalClusterFeatures; So getItem() will return the list of features selected. Line 759: cluster.getAddtionalFeaturesSupported().add(new SupportedAdditionalClusterFeature(cluster.getId(), Line 760: true, Line 761: feature)); Line 762: } https://gerrit.ovirt.org/#/c/39757/7/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterModel.java: Line 928: setAdditionalClusterFeatures(new ListModel<List<AdditionalClusterFeature>>()); Line 929: ArrayList<List<AdditionalClusterFeature>> newList = new ArrayList<List<AdditionalClusterFeature>>(); Line 930: newList.add(Collections.<AdditionalClusterFeature> emptyList()); Line 931: getAdditionalClusterFeatures().setItems(newList, Line 932: new ArrayList<AdditionalClusterFeature>()); > please remove the second arguement here, this is really not the right way. Yes. It doesn't work without the empty list. Let me fix the widget. Line 933: setSpiceProxyEnabled(new EntityModel<Boolean>()); Line 934: getSpiceProxyEnabled().setEntity(false); Line 935: getSpiceProxyEnabled().getEntityChangedEvent().addListener(this); Line 936: Line 1300: } Line 1301: })); Line 1302: } Line 1303: })); Line 1304: refreshAdditionalClusterFeaturesList(); > is this required? wouldn't the default selected app mode trigger either fro No. Its not required. I will remove it. Line 1305: } Line 1306: Line 1307: boolean isClusterDetached() { Line 1308: if (detached == null) { Line 1722: } Line 1723: updateAddtionClusterFeatureList(features, featuresEnabled); Line 1724: } Line 1725: }; Line 1726: AsyncDataProvider.getInstance().getClusterFeaturesByClusterId(asyncQuery, getEntity().getId()); > can you please add a comment why nesting this query? Sorry for the confusion. Here the outer query is to get all the features available for the cluster(based on compatibility version, services selected). Inner query is to get the list of features already selected for the cluster. This is applicable only in case of EditCluster. I will add a Comment. Line 1727: } else { Line 1728: updateAddtionClusterFeatureList(features, Line 1729: Collections.<AdditionalClusterFeature> emptySet()); Line 1730: } Line 1738: List<AdditionalClusterFeature> features = new ArrayList<>(); Line 1739: List<AdditionalClusterFeature> selectedFeatures = new ArrayList<>(); Line 1740: for (AdditionalClusterFeature feature : featuresAvailable) { Line 1741: features.add(feature); Line 1742: if (featuresEnabled.contains(feature)) { > why this check? I assume featuresEnabled are the selected ones? and the sel Yes featuredEnabled are the ones which are already selected for this cluster and saved in engine. Consider the case of un checking gluster service, then all gluster related features will not be shown in the list, but the featuresEnabled may have those features. This check helps to remove those features which are selected previously but not applicable now. Line 1743: selectedFeatures.add(feature); Line 1744: } Line 1745: } Line 1746: List<List<AdditionalClusterFeature>> listOfFeatures = new ArrayList<>(); Line 1746: listOfFeatures > clusterFeatureList? Done https://gerrit.ovirt.org/#/c/39757/7/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationConstants.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationConstants.java: Line 4287: Line 4288: @DefaultStringValue("Warning : Recommendations for geo-replication not met -") Line 4289: String geoReplicationRecommendedConfigViolation(); Line 4290: Line 4291: @DefaultStringValue("Additional features supported in the Cluster") > how about moving this to UI Messages displaying the cluster name as the fea This is the label for the collapsible widget not the title of the popup Line 4292: String addtionalClusterFeaturesTitle(); Line 4293: Line 4294: @DefaultStringValue("Additional Cluster Features") Line 4295: String addtionalClusterFeaturesLabel(); https://gerrit.ovirt.org/#/c/39757/7/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/cluster/ClusterPopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/cluster/ClusterPopupView.java: Line 220: ListModelCheckBoxGroup<AdditionalClusterFeature> additionalFeaturesEditor; Line 221: Line 222: @UiField Line 223: @Ignore Line 224: AdvancedParametersExpander additionalFeaturesExpander; > I assume this is a kind of shrink and expand. So I have a question here are We can't select all of them by default. User has to select the feature which he wants to enable. But it can be expanded by default. But that would make the Cluster Pop up really cluttered with scroll bar by default. Enabling some feature in the cluster means all the nodes in the cluster should support the features otherwise node can't be added to the cluster. Line 225: Line 226: @UiField Line 227: @Ignore Line 228: FlowPanel additionalFeaturesExpanderContent; -- To view, visit https://gerrit.ovirt.org/39757 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic13dda67f3e9a7d7134030c3923470291c3c5aec Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@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
