anmolbabu has posted comments on this change. Change subject: webadmin: UI for gluster volume snapshot creation ......................................................................
Patch Set 28: (22 comments) https://gerrit.ovirt.org/#/c/35082/28/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 177: getDeleteSnapshotCommand().setIsExecutionAllowed(allowDelete); Line 178: getDeleteAllSnapshotsCommand().setIsExecutionAllowed(allowDeleteAll); Line 179: getActivateSnapshotCommand().setIsExecutionAllowed(allowActivate); Line 180: getDeactivateSnapshotCommand().setIsExecutionAllowed(allowDeactivate); Line 181: getNewSnapshotCommand().setIsAvailable(allowCreateSnapshot); I assume this command only needs to be disabled then please use setIsExecutionAllowed Line 182: getEditSnapshotScheduleCommand().setIsAvailable(allowEditSnapshotSchedule); Line 183: } Line 184: Line 185: Line 178: getDeleteAllSnapshotsCommand().setIsExecutionAllowed(allowDeleteAll); Line 179: getActivateSnapshotCommand().setIsExecutionAllowed(allowActivate); Line 180: getDeactivateSnapshotCommand().setIsExecutionAllowed(allowDeactivate); Line 181: getNewSnapshotCommand().setIsAvailable(allowCreateSnapshot); Line 182: getEditSnapshotScheduleCommand().setIsAvailable(allowEditSnapshotSchedule); or is it that either edit or new can be (even) visible at a given point in time. Line 183: } Line 184: Line 185: Line 186: @Override Line 583: getWindow not used anywhere can you please remove it Line 612: snapshotModel.setHashName("edit_volume_snapshot_schedule"); //$NON-NLS-1$ Line 613: snapshotModel.setTitle(constants.editVolumeSnapshotScheduleTitle()); Line 614: setWindow(snapshotModel); Line 615: Line 616: AsyncDataProvider.getInstance().getVolumeSnapshotSchedule(new AsyncQuery(this, new INewAsyncCallback() { So, from the entry in asyncdateprovider I see that for some/any reason if the query returns null here you will get a new GlusterVolumeSnapshotSchedule() essentially a new instance I am not sure how this can be handled bcoz I think most fields would be null unless they have a default value in the entity. I feel it is better to return a null in your converter if you have a null.so, that it can be handled here easily The advantage of the converter is to avoid doing returnvalue.getreturnvalue..... of this nature and directly obtain the return value Line 617: Line 618: @Override Line 619: public void onSuccess(Object model, Object returnValue) { Line 620: GlusterVolumeSnapshotSchedule schedule = (GlusterVolumeSnapshotSchedule) returnValue; Line 680: } Line 681: } Line 682: Line 683: private Date getExecutionTimeValue(GlusterVolumeSnapshotSchedule schedule) { Line 684: Date dt = new Date(); I somewhere saw you using Time class for a similar requirement why not use the same here. Line 685: dt.setHours(schedule.getExecutionTime().getHours()); Line 686: dt.setMinutes(schedule.getExecutionTime().getMinutes()); Line 687: Line 688: return dt; Line 687: Line 688: return dt; Line 689: } Line 690: Line 691: private GlusterVolumeSnapshotScheduleRecurrence getRecurrenceType(org.ovirt.engine.core.common.businessentities.gluster.GlusterVolumeSnapshotScheduleRecurrence value) { why not utilizing the same backend class? Line 692: switch (value) { Line 693: case INTERVAL: Line 694: return GlusterVolumeSnapshotScheduleRecurrence.IntervalBased; Line 695: case HOURLY: https://gerrit.ovirt.org/#/c/35082/28/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/GlusterVolumeSnapshotModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/GlusterVolumeSnapshotModel.java: Line 38: private EntityModel<Date> executionTime; Line 39: private ListModel<List<DayOfWeek>> daysOfWeek; Line 40: private ListModel<String> daysOfMonth; Line 41: Line 42: public GlusterVolumeSnapshotModel(List<GlusterVolumeSnapshotScheduleRecurrence> recurrenceOptions, why is it required to be passed here? Line 43: boolean generalTabVisible, Line 44: boolean scheduleTabVisible) { Line 45: init(recurrenceOptions); Line 46: setAvailabilities(); Line 59: setEndByOptions(new ListModel<EndDateOptions>()); Line 60: setTimeZones(new ListModel<String>()); Line 61: setDaysOfMonth(new ListModel<String>()); Line 62: startAt = new EntityModel<>(); Line 63: startAt.setEntity(new Date()); you could do this in previous statement itself as setStartAt(new EntityModel<>(new Date())); Line 64: endDate = new EntityModel<>(); Line 65: endDate.setEntity(new Date()); Line 66: executionTime = new EntityModel<>(); Line 67: executionTime.setEntity(new Date()); Line 65: t same as above Line 63: startAt.setEntity(new Date()); Line 64: endDate = new EntityModel<>(); Line 65: endDate.setEntity(new Date()); Line 66: executionTime = new EntityModel<>(); Line 67: executionTime.setEntity(new Date()); same as above Line 68: initIntervals(); Line 69: initTimeZones(); Line 70: Line 71: recurrence.setItems(recurrenceOptions); Line 88: for (int nThMin = 1; mins < 55; nThMin++) { Line 89: mins = nThMin * 5; Line 90: intervals.add(String.valueOf(mins)); Line 91: } Line 92: getInterval().setItems(intervals, String.valueOf(5)); what is this? I mean the 2nd arguement Line 93: } Line 94: Line 95: private void initTimeZones() { Line 96: Set<String> timeZoneTypes = TimeZoneType.GENERAL_TIMEZONE.getTimeZoneList().keySet(); Line 96: Set<String> timeZoneTypes = TimeZoneType.GENERAL_TIMEZONE.getTimeZoneList().keySet(); Line 97: getTimeZones().setItems(timeZoneTypes); Line 98: } Line 99: Line 100: private void setAvailabilities() { might not be required I assume the default availabilities are true Line 101: getClusterName().setIsAvailable(true); Line 102: getVolumeName().setIsAvailable(true); Line 103: getSnapshotName().setIsAvailable(true); Line 104: getDescription().setIsAvailable(true); Line 252: contains why are you checking for position of ',' ? Line 260: setMessage(ConstantsManager.getInstance().getConstants().endDateBeforeStartDate()); Line 261: validEndDate = false; Line 262: } Line 263: Line 264: return getSnapshotName().getIsValid() && getDaysOfTheWeek().getIsValid() && getDaysOfMonth().getIsValid() doubt : how does this work? getSnapshotName().getIsValid() I understood but didn't see where the validation condition for the remaining is associated with the corresponding model's field Line 265: && validWeekDays && validMonthDays && validEndDate; Line 266: } Line 267: Line 268: public enum GlusterVolumeSnapshotScheduleRecurrence { Line 264: return getSnapshotName().getIsValid() && getDaysOfTheWeek().getIsValid() && getDaysOfMonth().getIsValid() Line 265: && validWeekDays && validMonthDays && validEndDate; Line 266: } Line 267: Line 268: public enum GlusterVolumeSnapshotScheduleRecurrence { Doubt : Why not utilize the backend enum itself. I had probably written this before only bcoz I was not depending on your backend patch but may be now that the backend patch is in, I feel you can reuse the same. Line 269: None(constants.notScheduledOption()), Line 270: IntervalBased(constants.intervalBasedSnapshot()), Line 271: HourlySnapshot(constants.hourlySnapshot()), Line 272: DailySnapshot(constants.dailySnapshot()), https://gerrit.ovirt.org/#/c/35082/28/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/volumes/VolumeListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/volumes/VolumeListModel.java: Line 571: GlusterVolumeEntity volumeEntity = list.get(0); Line 572: return (volumeEntity.getStatus() == GlusterStatus.UP); Line 573: } else { Line 574: return false; Line 575: } return ((list.size() == 0) && (list.get(0).getStatus() == GlusterStatus.UP)); Line 576: } Line 577: Line 578: private boolean isEditSnapshotScheduleAvailable(List<GlusterVolumeEntity> list) { Line 579: if (getSelectedItems().size() == 1) { Line 580: GlusterVolumeEntity volumeEntity = list.get(0); Line 581: return (volumeEntity.getStatus() == GlusterStatus.UP && volumeEntity.getSnapshotScheduled()); Line 582: } else { Line 583: return false; Line 584: } same as above Line 585: } Line 586: Line 587: private boolean isStopProfileAvailable(List<GlusterVolumeEntity> list) { Line 588: if (getSelectedItems().size() == 0) { https://gerrit.ovirt.org/#/c/35082/28/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 2600: @DefaultStringValue("Monthly") Line 2601: String monthlySnapshot(); Line 2602: Line 2603: @DefaultStringValue("AM") Line 2604: String snapshotScheduleMorning(); May no longer be used right? Line 2605: Line 2606: @DefaultStringValue("PM") Line 2607: String snapshotScheduleEvening(); Line 2608: https://gerrit.ovirt.org/#/c/35082/28/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterVolumeSnapshotCreatePopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterVolumeSnapshotCreatePopupView.java: Line 211: executionTimeEditor.setLabel(constants.executionTimeLabel()); Line 212: Line 213: criticalIntervalLabel.setText(constants.criticalSnapshotIntervalNote()); Line 214: criticalIntervalLabel.setVisible(false); Line 215: errorMsgLabel.setVisible(false); please move the above 2 lines to a new method and have it called from constructor just after localize Line 216: } Line 217: Line 218: @Override Line 219: public void edit(final GlusterVolumeSnapshotModel object) { Line 252: if (selectedItem == GlusterVolumeSnapshotScheduleRecurrence.DailySnapshot Line 253: || selectedItem == GlusterVolumeSnapshotScheduleRecurrence.WeeklySnapshot Line 254: || selectedItem == GlusterVolumeSnapshotScheduleRecurrence.MonthlySnapshot) { Line 255: executionTimeEditor.setVisible(true); Line 256: // executionTimeEditor.getContentWidget().showTimeOnly(); please remove the comment Line 257: timeZoneEditor.setVisible(true); Line 258: } else { Line 259: executionTimeEditor.setVisible(false); Line 260: timeZoneEditor.setVisible(false); Line 253: || selectedItem == GlusterVolumeSnapshotScheduleRecurrence.WeeklySnapshot Line 254: || selectedItem == GlusterVolumeSnapshotScheduleRecurrence.MonthlySnapshot) { Line 255: executionTimeEditor.setVisible(true); Line 256: // executionTimeEditor.getContentWidget().showTimeOnly(); Line 257: timeZoneEditor.setVisible(true); please move model-view interactions to presenter Line 258: } else { Line 259: executionTimeEditor.setVisible(false); Line 260: timeZoneEditor.setVisible(false); Line 261: } https://gerrit.ovirt.org/#/c/35082/28/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterVolumeSnapshotCreatePopupView.ui.xml File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterVolumeSnapshotCreatePopupView.ui.xml: Line 1: <?xml version="1.0" encoding="UTF-8"?> Line 2: <!DOCTYPE ui:UiBinder SYSTEM "http://dl.google.com/gwt/DTD/xhtml.ent";> Line 3: <ui:UiBinder xmlns:ui="urn:ui:com.google.gwt.uibinder" can you please remove the unused imports Line 4: xmlns:g="urn:import:com.google.gwt.user.client.ui" xmlns:d="urn:import:org.ovirt.engine.ui.common.widget.dialog" Line 5: xmlns:ge="urn:import:org.ovirt.engine.ui.common.widget.editor.generic" Line 6: xmlns:e="urn:import:org.ovirt.engine.ui.common.widget.editor" xmlns:w="urn:import:org.ovirt.engine.ui.common.widget" Line 7: xmlns:dp="urn:import:com.google.gwt.user.datepicker.client" xmlns:t="urn:import:org.ovirt.engine.ui.common.widget.dialog.tab"> -- To view, visit https://gerrit.ovirt.org/35082 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I823580ecb127e48e075c437668bfb19ff8ec4467 Gerrit-PatchSet: 28 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Einav Cohen <[email protected]> Gerrit-Reviewer: Kanagaraj M <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[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
