anmolbabu 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. 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? or should it be getSelectedItems() ? 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. If we had some other thing to select other than the first item in list, then we could have used this overloaded function but here, I don't see such a need. If it doesn't work w/o this we might need to 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 from enableovirt entity changed or enable gluster entity changed. I am really not sure if it will be, so can you please check if this is really required 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? I see you are using same asyncQuery handler so I assume you are probably only filtering out some results from first list. If that's the case then why outer query is required? 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 selected ones are available. If not can you please add a small comment? Line 1743: selectedFeatures.add(feature); Line 1744: } Line 1745: } Line 1746: List<List<AdditionalClusterFeature>> listOfFeatures = new ArrayList<>(); Line 1746: listOfFeatures clusterFeatureList? 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 features are specific to this cluster(although not really "this" cluster but instead the version of essentially vdsm but also gluster). Assuming this to be the name at the top 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 all the features selected by default? If not I think we shouldn't be using this shrink-expand kind of a thing right? or atleast can be it expanded initially and let user shrink it if he doesn't want? Bcoz, there is no harm enabling these features as I assume only UI widgets get enabled and if he doesn't want them he could probably change them in edit or something but I really don't think the reverse is a fairer approach. Ignore if its already this way(I am a little too lazy to iterate back and forth to check it ;) ) else please consider it. 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: 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
