Shubhendu Tripathi has posted comments on this change. Change subject: webadmin: UI for gluster volume snapshot actions ......................................................................
Patch Set 1: (10 comments) https://gerrit.ovirt.org/#/c/38151/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/GlusterVolumeSnapshotListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/GlusterVolumeSnapshotListModel.java: Line 36: setTitle(ConstantsManager.getInstance().getConstants().snapshotsTitle()); Line 37: setHelpTag(HelpTag.volume_snapshots); Line 38: setHashName("volume_snapshots");//$NON-NLS-1$ Line 39: Line 40: setRestoreSnapshotCommand(new UICommand("Restore", this)); //$NON-NLS-1$ > Please make the first letter small this is just a convention although done Line 41: setDeleteSnapshotCommand(new UICommand("Delete", this)); //$NON-NLS-1$ Line 42: setDeleteAllSnapshotsCommand(new UICommand("DeleteAll", this)); //$NON-NLS-1$ Line 43: setActivateSnapshotCommand(new UICommand("Activate", this)); //$NON-NLS-1$ Line 44: setDeactivateSnapshotCommand(new UICommand("Deactivate", this)); //$NON-NLS-1$ Line 162: super.executeCommand(command); Line 163: if (command.equals(getRestoreSnapshotCommand())) { Line 164: restoreSnapshot(); Line 165: } else if (command.getName().equals("onRestoreSnapshot")) { //$NON-NLS-1$ Line 166: onRestoreSnapshot(); > kindly group all on<Actions> probably towards end and commands towards the done Line 167: } else if (command.equals(getDeleteSnapshotCommand())) { Line 168: deleteSnapshot(); Line 169: } else if (command.getName().equals("onDeleteSnapshot")) { //$NON-NLS-1$ Line 170: onDeleteSnapshot(); Line 204: } Line 205: Line 206: UICommand okCommand = UICommand.createDefaultOkUiCommand("onRestoreSnapshot", this); //$NON-NLS-1$ Line 207: model.getCommands().add(okCommand); Line 208: UICommand cancelCommand = UICommand.createCancelUiCommand("CancelConfirmation", this); //$NON-NLS-1$ > camelCase with small letter starting done. Just copied from somewhere else :) Line 209: model.getCommands().add(cancelCommand); Line 210: } Line 211: Line 212: private void onRestoreSnapshot() { Line 369: if (model.getProgress() != null) { Line 370: return; Line 371: } Line 372: Line 373: if (getSelectedItems().size() != 1) { > This appears to me to be a redundant check as it is already done in updateA correct. not required here Line 374: return; Line 375: } Line 376: Line 377: model.startProgress(null); Line 397: Line 398: ConfirmationModel model = new ConfirmationModel(); Line 399: setConfirmWindow(model); Line 400: model.setTitle(ConstantsManager.getInstance().getConstants().confirmDeactivateSnapshot() Line 401: + " - " + getEntity().getName()); //$NON-NLS-1$ > Kindly move this to UIMessages done Line 402: model.setHelpTag(HelpTag.volume_deactivate_snapshot_confirmation); Line 403: model.setHashName("volume_deactivate_snapshot_confirmation"); //$NON-NLS-1$ Line 404: model.setMessage(ConstantsManager.getInstance().getMessages().confirmVolumeSnapshotDeactivate()); Line 405: Line 400: model.setTitle(ConstantsManager.getInstance().getConstants().confirmDeactivateSnapshot() Line 401: + " - " + getEntity().getName()); //$NON-NLS-1$ Line 402: model.setHelpTag(HelpTag.volume_deactivate_snapshot_confirmation); Line 403: model.setHashName("volume_deactivate_snapshot_confirmation"); //$NON-NLS-1$ Line 404: model.setMessage(ConstantsManager.getInstance().getMessages().confirmVolumeSnapshotDeactivate()); > As the above message doesn't have any dynamic parts please move it to UICon done Line 405: Line 406: UICommand okCommand = UICommand.createDefaultOkUiCommand("onDeactivateSnapshot", this); //$NON-NLS-1$ Line 407: model.getCommands().add(okCommand); Line 408: UICommand cancelCommand = UICommand.createCancelUiCommand("CancelConfirmation", this); //$NON-NLS-1$ Line 404: model.setMessage(ConstantsManager.getInstance().getMessages().confirmVolumeSnapshotDeactivate()); Line 405: Line 406: UICommand okCommand = UICommand.createDefaultOkUiCommand("onDeactivateSnapshot", this); //$NON-NLS-1$ Line 407: model.getCommands().add(okCommand); Line 408: UICommand cancelCommand = UICommand.createCancelUiCommand("CancelConfirmation", this); //$NON-NLS-1$ > camel Case starting small done Line 409: model.getCommands().add(cancelCommand); Line 410: } Line 411: Line 412: private void onDeactivateSnapshot() { Line 433: @Override Line 434: public void executed(FrontendActionAsyncResult result) { Line 435: ConfirmationModel localModel = (ConfirmationModel) getConfirmWindow(); Line 436: localModel.stopProgress(); Line 437: setConfirmWindow(null); > Just a thought. may be. will give a try. Line 438: } Line 439: }); Line 440: } Line 441: https://gerrit.ovirt.org/#/c/38151/1/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 434: @DefaultMessage("The selected snapshot will be activated. Do you want to continue?") Line 435: String confirmVolumeSnapshotActivate(); Line 436: Line 437: @DefaultMessage("The selected snapshot will be deactivated.\n Do you want to continue?") Line 438: String confirmVolumeSnapshotDeactivate(); > Please move all these to UIConstants as none of them have any dynamic repla done https://gerrit.ovirt.org/#/c/38151/1/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/gluster/SubTabGlusterVolumeSnapshotView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/gluster/SubTabGlusterVolumeSnapshotView.java: Line 81: return getDetailModel().getRestoreSnapshotCommand(); Line 82: } Line 83: }); Line 84: Line 85: getTable().addActionButton(new WebAdminButtonDefinition<GlusterVolumeSnapshotEntity>(constants.deleteVolumeSnapshot()) { > Just a thought. Actually deleteAll does not need any snapshot to be selected from the list. Its enabled even if nothing is selected. Also while calling the bll only volumeId is passed and VDSM verb also expects only a volumeId for deleting all the snapshots. Line 86: @Override Line 87: protected UICommand resolveCommand() { Line 88: return getDetailModel().getDeleteSnapshotCommand(); Line 89: } -- To view, visit https://gerrit.ovirt.org/38151 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3d5b6162c6c6931cd0366db5ba600566ab8ac7c9 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Kanagaraj M <[email protected]> Gerrit-Reviewer: Omer Frenkel <[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
