anmolbabu 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 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 beginning 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 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 updateActionAvailability so the action would be disabled right 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 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 UIConstants 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 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. Can you please extract out the common portions of on<Actions> although I don't see any gain apart from the code size reduction. Same for the window displaying functions also. 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 replacement requirements 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. Can the similar actions - Delete and DeleteAll be grouped instead of seperate actions. 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: anmolbabu <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
