Kanagaraj M 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 VdcActionType of the geo rep actions. "StartGlusterVolumeGeoRep" and "StopGeoRepSession" doesn't look good to read. Either we should call them as - StartGlusterVolumeGeoRep - StopGlusterVolumeGeoRep or - StartGeoRepSession - StopGeoRepSession 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() - can verbName be called as 'action' like below? - okCommandLink can be called as commandName 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 have both ok and cancel commands in a single place, which will make the code clean and easier to read 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 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 here 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. It will be useful when the force also failed, then the error message will be shown in the dialog. 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 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 resumeed" for resume 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" 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? 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 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 <[email protected]> Gerrit-Reviewer: Kanagaraj M <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: anmolbabu <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
