anmolbabu has posted comments on this change. Change subject: webadmin : GeoRep Config popup view ......................................................................
Patch Set 11: (7 comments) https://gerrit.ovirt.org/#/c/34217/11/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/GlusterConfigAwareCell.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/cell/GlusterConfigAwareCell.java: Line 31: delegate = new SelectionCell(allowedValues); Line 32: } Line 33: Line 34: @Override Line 35: public boolean handlesEvent(CellPreviewEvent<EntityModel> event) { > this can be simply "return BrowserEvents.CLICK.equals( event.getNativeEvent ok Line 36: NativeEvent nativeEvent = event.getNativeEvent(); Line 37: if (!BrowserEvents.CLICK.equals(nativeEvent.getType())) { Line 38: return false; Line 39: } Line 71: Line 72: @Override Line 73: public void render(Context context, GlusterGeoRepSessionConfiguration value, SafeHtmlBuilder sb) { Line 74: List<String> allowedValues = value.getAllowedValues(); Line 75: boolean isValuesConstrained = > Here the conditions looks exactly opposite to what is in onBrowserEvent(). I can make the conditions same but now, although they are opposite, they are handled appropriately meaning the if in onbrowserevent became else in render Line 76: allowedValues == null || allowedValues.isEmpty() || allowedValues.size() == 1 Line 77: && allowedValues.get(0).isEmpty(); Line 78: SafeHtmlBuilder sbDelegate = new SafeHtmlBuilder(); Line 79: if (isValuesConstrained) { https://gerrit.ovirt.org/#/c/34217/11/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/GlusterVolumeGeoReplicationSessionConfigModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/GlusterVolumeGeoReplicationSessionConfigModel.java: Line 71: public void addCancelCommand(UICommand cancelCommand) { Line 72: this.cancelCommand = cancelCommand; Line 73: this.getCommands().add(cancelCommand); Line 74: } Line 75: > This logic can go inside the setConfigsModel method. I didn't do that bcoz I wanted the setter to be merely a dummy setter. What do you suggest? Line 76: public void copyConfigsToMap(List<GlusterGeoRepSessionConfiguration> configsTocopy) { Line 77: for (GlusterGeoRepSessionConfiguration currentConfig : configsTocopy) { Line 78: configsMap.put(currentConfig.getKey(), currentConfig.getValue()); Line 79: } Line 98: sessionConfig.getKey(), Line 99: sessionConfig.getValue()); Line 100: } Line 101: Line 102: public void updateCommandExecutabilities(boolean queryExecutionStatus) { > Its better to rename the variable queryExecutionStatus to isExecutionAllowe Done Line 103: setAllConfigsCommand.setIsExecutionAllowed(queryExecutionStatus); Line 104: resetAllConfigsCommand.setIsExecutionAllowed(queryExecutionStatus); Line 105: } Line 106: https://gerrit.ovirt.org/#/c/34217/11/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/VolumeGeoRepListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/VolumeGeoRepListModel.java: Line 274: Line 275: addUICommandsToConfigWindow(configModel); Line 276: } Line 277: Line 278: private void fetchConfigForSession(GlusterGeoRepSession selectedItem) { > May be a better name for the variable? selectedItem How about selectedSession? Line 279: Frontend.getInstance().runQuery(VdcQueryType.GetGlusterVolumeGeoRepConfigList, new IdQueryParameters(selectedItem.getId()), new AsyncQuery(new INewAsyncCallback() { Line 280: @Override Line 281: public void onSuccess(Object model, Object returnValue) { Line 282: VdcQueryReturnValue vdcQueryReturnValue = (VdcQueryReturnValue) returnValue; https://gerrit.ovirt.org/#/c/34217/11/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIMessages.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIMessages.java: Line 415 Line 416 Line 417 Line 418 Line 419 > Move this file from the patch set. Done https://gerrit.ovirt.org/#/c/34217/11/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/gin/uicommon/VolumeModule.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/gin/uicommon/VolumeModule.java: Line 230: return result; Line 231: } Line 232: Line 233: @Provides Line 234: @Singleton > Can we format this line so that it becomes easy to read? Done Line 235: public SearchableDetailModelProvider<GlusterGeoRepSession, VolumeListModel, VolumeGeoRepListModel> getVolumeGeoRepListProvider(EventBus eventBus, final Provider<DefaultConfirmationPopupPresenterWidget> defaultConfirmPopupProvider, final Provider<GlusterVolumeGeoRepActionConfirmPopUpViewPresenterWidget> geoRepActionConfirmationPopupProvider, final Provider<GlusterVolumeGeoReplicationSessionConfigPopupPresenterWidget> geoRepConfigPopupProvider, final Provider<VolumeListModel> mainModelProvider, final Provider<VolumeGeoRepListModel> modelProvider) { Line 236: SearchableDetailTabModelProvider<GlusterGeoRepSession, VolumeListModel, VolumeGeoRepListModel> result = new SearchableDetailTabModelProvider<GlusterGeoRepSession, VolumeListModel, VolumeGeoRepListModel>(eventBus, defaultConfirmPopupProvider) { Line 237: @Override Line 238: public AbstractModelBoundPopupPresenterWidget<? extends Model, ?> getModelPopup(VolumeGeoRepListModel source, UICommand lastExecutedCommand, Model windowModel) { -- To view, visit https://gerrit.ovirt.org/34217 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I150946fe016d85cb378ed3d548eab5581321fbfe Gerrit-PatchSet: 11 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@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
