anmolbabu has posted comments on this change. Change subject: webadmin : Geo-rep Actions UI ......................................................................
Patch Set 19: (11 comments) http://gerrit.ovirt.org/#/c/32538/19/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 230: onGeoRepSessionAction(VdcActionType.StopGeoRepSession, constants.stopVerb()); Line 231: } else if (command.getName().equalsIgnoreCase("onPauseGeoRepSession")) {//$NON-NLS-1$ Line 232: onGeoRepSessionAction(VdcActionType.PauseGlusterVolumeGeoRepSession, constants.pauseVerb()); Line 233: } else if (command.getName().equalsIgnoreCase("onResumeGeoRepSession")) {//$NON-NLS-1$ Line 234: onGeoRepSessionAction(VdcActionType.ResumeGeoRepSession, constants.resumeVerb()); > Not relevant to this patch. We need to keep the same convention for VdcAct Done. Can I do this as a part of another patch Line 235: } else if (command.getName().equalsIgnoreCase("closeWindow")) {//$NON-NLS-1$ Line 236: closeWindow(); Line 237: } Line 238: } Line 256: private void resumeGeoRepSession() { Line 257: geoRepAction("onResumeGeoRepSession", constants.geoReplicationResumeTitle(), HelpTag.volume_geo_rep_resume_confirmation, "volume_geo_rep_resume_confirmation", constants.resumeVerb(), VdcActionType.ResumeGeoRepSession);//$NON-NLS-1$//$NON-NLS-2$ Line 258: } Line 259: Line 260: private void geoRepAction(String okCommandLink, String confirmTitle, HelpTag helpTag, String hashName, String verbName, VdcActionType actionType) { > - this function can be renamed to performGeoRepAction() Done Line 261: GlusterGeoRepSession selectedSession = (GlusterGeoRepSession) getSelectedItem(); Line 262: if (selectedSession == null) { Line 263: return; Line 264: } Line 263: return; Line 264: } Line 265: UICommand okCommand = new UICommand(okCommandLink, this); Line 266: okCommand.setTitle(constants.ok()); Line 267: okCommand.setIsDefault(true); > okCommand initialization also can be moved to below function. So that you h Done Line 268: confirmGeoRepAction(confirmTitle, helpTag, hashName, messages.geoRepForceHelp(verbName), messages.geoRepForceTitle(verbName), okCommand); Line 269: onGeoRepSessionAction(actionType, verbName); Line 270: } Line 271: Line 268: confirmGeoRepAction(confirmTitle, helpTag, hashName, messages.geoRepForceHelp(verbName), messages.geoRepForceTitle(verbName), okCommand); Line 269: onGeoRepSessionAction(actionType, verbName); Line 270: } Line 271: Line 272: private void confirmGeoRepAction(String title, HelpTag helpTag, String hashName, String forceHelp, String forceLabelText, UICommand okCommand) { > this can be renamed to initializeGeoRepActionConfirmation Done Line 273: GlusterGeoRepSession selectedSession = (GlusterGeoRepSession) getSelectedItem(); Line 274: GlusterVolumeGeoRepActionConfirmationModel cModel = new GlusterVolumeGeoRepActionConfirmationModel(); Line 275: cModel.setTitle(title); Line 276: cModel.setHelpTag(helpTag); Line 309: } Line 310: Line 311: private void onGeoRepSessionAction(VdcActionType actionType, String action) { Line 312: final GlusterVolumeGeoRepActionConfirmationModel cModel = (GlusterVolumeGeoRepActionConfirmationModel) getWindow(); Line 313: cModel.startProgress(messages.geoRepActionProgressMessage(Character.toUpperCase(action.charAt(0)) + action.substring(1))); > You could just keep the constant itself initcap, instead of converting it h Done Line 314: boolean force = cModel.getForce().getEntity(); Line 315: if (force) { Line 316: closeWindow(); Line 317: } Line 311: private void onGeoRepSessionAction(VdcActionType actionType, String action) { Line 312: final GlusterVolumeGeoRepActionConfirmationModel cModel = (GlusterVolumeGeoRepActionConfirmationModel) getWindow(); Line 313: cModel.startProgress(messages.geoRepActionProgressMessage(Character.toUpperCase(action.charAt(0)) + action.substring(1))); Line 314: boolean force = cModel.getForce().getEntity(); Line 315: if (force) { > No need to close the window when user has selected force. Done Line 316: closeWindow(); Line 317: } Line 318: GlusterGeoRepSession selectedSession = (GlusterGeoRepSession)getSelectedItem(); Line 319: GlusterVolumeGeoRepSessionParameters sessionParamters = new GlusterVolumeGeoRepSessionParameters(selectedSession.getMasterVolumeId(), selectedSession.getId()); http://gerrit.ovirt.org/#/c/32538/19/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java: Line 313: @DefaultStringValue("pause") Line 314: String pauseVerb(); Line 315: Line 316: @DefaultStringValue("resume") Line 317: String resumeVerb(); > you could just call these as startGeoRep and respectively Done Line 318: Line 319: @DefaultStringValue("Rebalance Status") Line 320: String volumeRebalanceStatusTitle(); Line 321: http://gerrit.ovirt.org/#/c/32538/19/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 418: @DefaultMessage("This will force {0} geo-replication sessions on the nodes that are part of the master volume \n." Line 419: + "If it is unable to successfully {0} the geo-replication session on any node which is online and part of the master volume,\n " Line 420: + "the command will still {0} the geo-replication sessions on as many nodes as it can.\n" Line 421: + "This command can also be used to re-{0} geo-replication sessions on the nodes where the session has died, or has not {0}ed.") Line 422: String geoRepForceHelp(String action); > "has not {0}ed" will become "has not pauseed" for pause and "has not resume Done Line 423: Line 424: @DefaultMessage("Force {0} session") Line 425: String geoRepForceTitle(String action); Line 426: Line 424: @DefaultMessage("Force {0} session") Line 425: String geoRepForceTitle(String action); Line 426: Line 427: @DefaultMessage("{0} geo-rep in progress") Line 428: String geoRepActionProgressMessage(String action); > This should be much more readable like "Starting geo-replication session" Same problem as above Pauseing and Resumeing http://gerrit.ovirt.org/#/c/32538/19/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GeoRepActionConfirmPopUpView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GeoRepActionConfirmPopUpView.java: Line 115: driver.edit(object); Line 116: } Line 117: Line 118: @Override Line 119: public void setForceHelp(GlusterVolumeGeoRepActionConfirmationModel object) { > Why do you need to pass the whole model instead of just the error text? Done Line 120: geoRepForceHelpIcon.setText(templates.italicText(object.getForceHelp())); Line 121: } Line 122: Line 123: @Override Line 125: return driver.flush(); Line 126: } Line 127: Line 128: @Override Line 129: public void setErrorMessage(GlusterVolumeGeoRepActionConfirmationModel object) { > same here Done Line 130: String errorMessage = object.getMessage(); Line 131: errorMsg.setText(errorMessage); Line 132: errorMsg.setVisible(errorMessage != null); Line 133: } -- To view, visit http://gerrit.ovirt.org/32538 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I46fbd79f714175e3ad3ce76d858714c6fec3fdde Gerrit-PatchSet: 19 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@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
